-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Document that "features should be additive" #4328
Comments
Hey! This is surprising to me as I see a lot of crypto crates that let you choose your backend via mutually exclusive features. Is there any plan to actually enforce this? And what are the issues with mutually exclusive features? |
How would that be enforced? Cargo takes the union of all features that are enabled for a crate throughout the dependency graph. If two crates end up enabling two mutually exclusive features of another crate, then they'll both be enabled when building it. The result of that would depend on the implementation of the crate. |
An easy way would be to actually build the crate with --all-features and notice that it fails, but I can see how that would be too much overhead :) It doesn't seem to be a problem only for mutually exclusive crates. For example imagine that:
(you can imagine the verification being skipped with |
When the following dependencies and their features were specified: ```toml tracing-core = { version = "0.1", default-features = false, features = ["std"] } tracing = { version = "0.1", default-features = false } ``` The build would fail with the following error: The build will fail with the following failure: ``` error[E0412]: cannot find type `Once` in crate `tracing_core` --> tracing/src/lib.rs:840:35 | 840 | pub type Once = tracing_core::Once<()>; | ^^^^ not found in `tracing_core` | ``` This happened because `tracing-core` exports `Once` only if its `std` feature is disabled. And the depending `tracing` crate assumed that if its `std` feature was disabled, so would be the `std` feature in `tracing-core`. This is a violation of the [undocumented "features must be additive" guideline][ag]. In this commit tracing-core is adjusted to export `Once` regardless of whether `std` is disabled or not. [ag]: rust-lang/cargo#4328
When the following dependencies and their features were specified: ```toml tracing-core = { version = "0.1", default-features = false, features = ["std"] } tracing = { version = "0.1", default-features = false } ``` The build would fail with the following error: ``` error[E0412]: cannot find type `Once` in crate `tracing_core` --> tracing/src/lib.rs:840:35 | 840 | pub type Once = tracing_core::Once<()>; | ^^^^ not found in `tracing_core` | ``` This happened because `tracing-core` exports `Once` only if its `std` feature is disabled. And the depending `tracing` crate assumed that if its `std` feature was disabled, so would be the `std` feature in `tracing-core`. This is a violation of the [undocumented "features must be additive" guideline][ag]. In this commit tracing-core is adjusted to export `Once` regardless of whether `std` is disabled or not. [ag]: rust-lang/cargo#4328
When the following dependencies and their features were specified: ```toml tracing-core = { version = "0.1", default-features = false, features = ["std"] } tracing = { version = "0.1", default-features = false } ``` The build would fail with the following error: ``` error[E0412]: cannot find type `Once` in crate `tracing_core` --> tracing/src/lib.rs:840:35 | 840 | pub type Once = tracing_core::Once<()>; | ^^^^ not found in `tracing_core` | ``` This happened because `tracing-core` exports `Once` only if its `std` feature is disabled. And the depending `tracing` crate assumed that if its `std` feature was disabled, so would be the `std` feature in `tracing-core`. This is a violation of the [undocumented "features must be additive" guideline][ag]. In this commit tracing-core is adjusted to export `Once` regardless of whether `std` is disabled or not. [ag]: rust-lang/cargo#4328
I wrote a blog post on this topic. Parts of it can probably be adapted for the docs. Full post: https://dev.to/rimutaka/cargo-features-explained-with-examples-194g. The relevant part is copied here. I'm happy to make a PR if this description is acceptable. It probably needs to be much shorter to go into the reference. Cargo takes the union of all features enabled for a crate throughout the dependency graph. If multiple crates enable mutually exclusive features of another crate, then all those features will be enabled at build time. The result of that would depend on the implementation of the crate and may produce a compiler error if mutually exclusive crates or features are enabled. An example of this type of dependency would be Crate X that depends on Crates A and Crate B, while both A and B depend on Crate awesome.
In the following example both
Consider a more complicated example with three possible configurations for
The following combination will make crate
Removing
The following combination will also result in the same compilation error because package
|
@ehuss Does #8997 resolve this? I've been reading over the new https://doc.rust-lang.org/nightly/cargo/reference/features.html, but I can't find anything which discourages of forbids non-additive features. I think ideally this should be explicitly documented in the features documentation. Is there something I'm missing or can we potentially reopen this? |
@daboross It takes time for cargo updates to be published on the nightly channel (usually about a week). Until then, the new chapter can be found in the source tree, with the discussion of features being additive starting around https://github.com/rust-lang/cargo/blob/master/src/doc/src/reference/features.md#feature-unification. |
Although it seems to be well-known, I find it surprising that the requirement "features should be additive" is not documented at all on docs.crates.io. Disrespecting this leads to tricky errors when merging features like chyh1990/yaml-rust#44 and rust-bakery/nom#544 (= #4323).
rust-lang/rfcs#1841 mentioned some documentation changes which should better be added here.
The text was updated successfully, but these errors were encountered: