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

Add support for feature selection #108

Merged
merged 12 commits into from
Sep 3, 2021
Merged

Add support for feature selection #108

merged 12 commits into from
Sep 3, 2021

Conversation

tronical
Copy link
Contributor

@tronical tronical commented Sep 2, 2021

This merge requests is based on the commit in #99 but changes the code to use a target property.

Brord van Wierst and others added 5 commits September 2, 2021 13:27
This patch builds on the parent commit, but changes the API
to use a target property.

Since this approach requires CMake >= 3.19, the rust2cpp test that received features
was copied into the new features test and reverted from the old test, so that
it can still be run with older cmake versions.
* Make sure to use a space separator for the cargo command line
* Join the target property list to the expected comma delimiter for the generator
And $<IF> without the else part can be rewritten as plain $<>
Copy link
Collaborator

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

Looks good.

I think it might still make sense to be able to do

corrosion_import_crate(MANIFEST_PATH rust/Cargo.toml FEATURES myfeature)

in the common case, don't you think?

README.md Outdated
@@ -152,6 +152,10 @@ The pairs can also be generator expressions, as they will be evaluated using $<G
For example this may be useful if the build scripts of crates look for environment variables.
This feature requires CMake >= 3.19.0.

Cargo crates sometimes offer features, which traditionally can be specified with `cargo build` on the
command line as opt-in. You can select the features to use for a crate imported with Corrosion by
setting the `CORROSION_FEATURES` target list property on the targets created by `corrosion_import_crate`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe the README would benefit from an example

@tronical
Copy link
Contributor Author

tronical commented Sep 2, 2021

Looks good.

I think it might still make sense to be able to do

corrosion_import_crate(MANIFEST_PATH rust/Cargo.toml FEATURES myfeature)

in the common case, don't you think?

Hm, that's true. So FEATURES will apply to all the imported crates equally?

Reference the second function that's guarded by the second feature, otherwise we don't
know if both were passed.
@ogoffart
Copy link
Collaborator

ogoffart commented Sep 2, 2021

Hm, that's true. So FEATURES will apply to all the imported crates equally?

yeah

cmake/Corrosion.cmake Outdated Show resolved Hide resolved
The target property is empty, if it does not exist. If it is not empty, then
map it to the correct value (--foo) or the empty string.
@ogoffart ogoffart merged commit 9ea8f6b into corrosion-rs:master Sep 3, 2021
@tronical tronical mentioned this pull request Sep 3, 2021
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