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

Allow Ninja Multi-config for Cmake >= 3.20 #137

Merged
merged 1 commit into from
Feb 24, 2022
Merged

Conversation

jschwe
Copy link
Collaborator

@jschwe jschwe commented Feb 6, 2022

This commit is not extensively tested but at a first glance,
Ninja Multiconfig seems to work. I'll test this a bit more thoroughly at work tomorrow.

Closes #50

This commit is not extensively tested, but at a first glance,
Ninja Multiconfig seems to work.

Signed-off-by: Jonathan Schwender <[email protected]>
@jschwe jschwe force-pushed the jschwe/multi-config branch from eab763f to 2948aa0 Compare February 14, 2022 20:09
@jschwe jschwe requested a review from ogoffart February 14, 2022 20:10
@jschwe
Copy link
Collaborator Author

jschwe commented Feb 14, 2022

Update: I tested this at work and Ninja Multi-Config seemed to work fine for our project.

@@ -240,9 +239,9 @@ function(_add_cargo_build)
set(byproducts)
foreach(byproduct_file ${ACB_BYPRODUCTS})
list(APPEND build_byproducts "${cargo_build_dir}/${byproduct_file}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about this one?
There is also something similar line 154

Copy link
Collaborator Author

@jschwe jschwe Feb 15, 2022

Choose a reason for hiding this comment

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

cargo_build_dir depends on both build_dir (which depends on $<CONFIG> for Multi-Config Generator) and on build_type_dir, which is either the cargo profile (if specified) or debug for debug builds or release for all other builds.

So the current behaviour is probably overly pessimistic and builds more seperately then it needs to.

@jschwe
Copy link
Collaborator Author

jschwe commented Feb 24, 2022

I'll go ahead and merge this to master. There could of course still be some hidden issues, but since Ninja-Multiconfig is currently just disabled, I don't think this MR can really break anything.

@jschwe jschwe merged commit 904fefb into master Feb 24, 2022
@jschwe jschwe deleted the jschwe/multi-config branch April 3, 2022 06:17
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.

Corrosion is incompatible with Ninja Multi-Config
2 participants