Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix output directory for copied byproducts #568

Merged
merged 4 commits into from
Dec 11, 2024

Conversation

obsgolem
Copy link
Contributor

@obsgolem obsgolem commented Oct 8, 2024

CMake doesn't automatically append the config type to the output dir when using a custom command.

@obsgolem obsgolem marked this pull request as draft October 8, 2024 04:09
@jschwe
Copy link
Collaborator

jschwe commented Oct 8, 2024

I'm curious what the background of this PR is. Did you experience an issue with a multi-config generator?

@obsgolem
Copy link
Contributor Author

obsgolem commented Oct 8, 2024

Yes, I am having an issue where an executable is being copied to my build directory, not my build/Debug (or whatever) directory. This is with the VS generator. Some tests are failing, so I need to see if that is due to it assuming the behavior I was seeing, or if the behavior I was seeing is due to something complex I haven't found yet.

@obsgolem obsgolem force-pushed the josiah/fix_output_copy branch 2 times, most recently from ae9aa5b to 89a6f2d Compare October 10, 2024 17:58
@obsgolem obsgolem marked this pull request as ready for review October 10, 2024 19:10
@obsgolem
Copy link
Contributor Author

Apologies for the lockfile changes, don't know what is up with them. Want me to revert them?

Copy link
Collaborator

@jschwe jschwe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long delay in reviewing this.

I am having an issue where an executable is being copied to my build directory, not my build/Debug (or whatever) directory. This is with the VS generator. Some tests are failing, so I need to see if that is due to it assuming the behavior I was seeing, or if the behavior I was seeing is due to something complex I haven't found yet.

What kind of tests are failing? Corrosions tests or tests from your project?
I guess changing the default location the artifact is copied to in Multi-Config mode has the potential to break user code, so we would need to document it in the release notes and also give a short reason what this change fixes.

Apologies for the lockfile changes, don't know what is up with them. Want me to revert them?

That would be nice.

@obsgolem
Copy link
Contributor Author

obsgolem commented Nov 1, 2024

What kind of tests are failing? Corrosions tests or tests from your project?
I guess changing the default location the artifact is copied to in Multi-Config mode has the potential to break user code, so we would need to document it in the release notes and also give a short reason what this change fixes.

The output directory tests were failing, but after a couple rounds of revision I managed to fix that.

@obsgolem
Copy link
Contributor Author

Failing test is not due to me I think

Copy link
Collaborator

@jschwe jschwe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this again, I noticed that we unconditionally append the config_type now, when the expected behavior is to only append the config_type if the OUTPUT_DIRECTORY variable is not a generator expression

@jschwe jschwe force-pushed the josiah/fix_output_copy branch from 7e81465 to a3c3975 Compare December 11, 2024 05:56
@jschwe jschwe merged commit 4c0d9af into corrosion-rs:master Dec 11, 2024
40 checks passed
@jschwe
Copy link
Collaborator

jschwe commented Dec 11, 2024

Thanks! And sorry for the long review times

@obsgolem
Copy link
Contributor Author

Hey, if you can put up with long times between me responding to review, I can put up with long review times, thanks!

jschwe added a commit that referenced this pull request Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants