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 crate features to check cxx version command #598

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FabianHummel
Copy link

Adds the supplied crate features to the cxx version check command so it does not fail when the cxx dependency is made optional and locked behind a feature flag.

Fixes #597

@jschwe
Copy link
Collaborator

jschwe commented Jan 3, 2025

Is this fix sufficient? In particular are the generated bindings correct, even without us passing any features to the actual cxxbridge command? cxxbridge just has a --cfg flag and not a feature flag, so I'm wondering if it internally uses something like --all-features, or if we would need to manually set the corresponding feature flags via --cfg.

If cxxbridge uses --all-features, then we might also want to just use --all-features here too (which should be fine as long as features are really additive)

@FabianHummel
Copy link
Author

FabianHummel commented Jan 3, 2025

I noticed that, too. The cxxbridge command would also need to be able to accept a list of features, but I have not really looked into this yet as this is not corrosion specific.

The goal would be to achieve being able to add a conditional flag above cxx::bridge like in the example below, which is not yet possible with the changes I have made and requires an additional PR on cxx's side:

#[cfg(feature = "interop")]
#[cxx::bridge]
mod ffi {
   // ...
}

For me, this PR to corrosion was "enough" to solve my issue because I added #[cfg(not(feature = "wasm"))] above the cxx bridge as my crate only either compiles to web assembly or C++, but this is not an ideal solution to this problem.

@jschwe
Copy link
Collaborator

jschwe commented Jan 4, 2025

The cxxbridge command would also need to be able to accept a list of features, but I have not really looked into this yet as this is not corrosion specific.

In theory, the --cfg option is all that is needed - it's just not super convenient - especially if we want to support --all-features

@jschwe jschwe enabled auto-merge (rebase) January 4, 2025 10:36
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.

Upon second thought, I realized we can't merge this as is.
The reason is that Corrosion explicitly allows the features to contain CMake Generator expressions. Since execute_process runs at configure time, we can't use generator expressions here (at least not without more changes).

As a quick fix, I think we could just pass --all-features unconditionally.
What do you think?

@FabianHummel
Copy link
Author

Yes, passing --all-features is fine.

However, how does the --cfg flag even work? It expects <name="value" | name[=true] | name=false> but I have not gotten anything to work after trying several options such as --cfg features="interop", --cfg --features="interop" or --cfg all-features=true

@jschwe
Copy link
Collaborator

jschwe commented Jan 4, 2025

However, how does the --cfg flag even work?

it basically maps to #[cfg(feature="xxx-feature")], so it needs to be singular, and I think the quotes are important too (but its been a while since I last used --cfg. There is also no such thing as --cfg all-features, we would need to parse the output of cargo metadata to get a list of all available features.

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_add_cxxbridge does not respect crate features
2 participants