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

Mutually exclusive features #2980

Open
retep998 opened this issue Aug 9, 2016 · 39 comments
Open

Mutually exclusive features #2980

retep998 opened this issue Aug 9, 2016 · 39 comments
Labels
A-features Area: features — conditional compilation C-enhancement Category: enhancement S-needs-rfc Status: Needs an RFC to make progress.

Comments

@retep998
Copy link
Member

retep998 commented Aug 9, 2016

In Windows API there is the notion of a family and partitions. Your choice of family is basically which platform you are targeting and there is a choice between WINAPI_FAMILY_DESKTOP_APP WINAPI_FAMILY_PC_APP WINAPI_FAMILY_PHONE_APP WINAPI_FAMILY_SYSTEM and WINAPI_FAMILY_SERVER. Your choice of family in turn enables certain partitions which enable access to certain functionality. At the moment I cannot use cargo features for this because they are not mutually exclusive, so two downstream dependencies could specify different families which is completely wrong. Even if I use a build script to ensure only one such feature is selected, it would be a nightmare to ensure that every single downstream dependency chooses the same family, threading the choice of family feature through long chains of transitive dependencies that shouldn't have to care about that choice.

What I think would work well here is some sort of feature selection that has to choose exactly one option from a closed set, and that choice is controlled by the root crate. All dependencies that are affected by that choice will then need some way to be informed of the choice so they can adjust which functionality to use depending on that choice.

At the moment the only workaround I see is for the user to pass an environment variable while building the project so winapi's build script can read that to choose the family followed by emitting key=value stuff which downstream dependencies can read via build scripts through the DEP_WINAPI_key=value environment variables to emit cfgs to control their own code.

Also, this can also apply to other choices, such as which version of Windows you want to target, newer versions have more functionality that dependencies might want to be able to take advantage of, but the user might also want them to target an older version instead so it can run on more systems.

@Boddlnagg
Copy link
Contributor

I just had this idea, but haven't thought it through yet: To get mutually exclusive features couldn't you just add some piece of code to the crate that is gated on the features that should not be used together and make rustc output some error message that actually points to the problem (do we have something like #error in C, probably a macro?).

This could be considered an ugly workaround, but maybe it allows the greatest flexibility.

@retep998
Copy link
Member Author

retep998 commented Aug 26, 2016

@Boddlnagg Suppose the user is working on their project apple which depends on banana which in turn depends on cat which in turn depends on winapi. Now cat is capable of supporting two different winapi families, so how does it deal with this? Suppose it decides to provide two features for each of these options so the user can decide, but wait, the user is not the one depending on cat, it is banana. So banana is now responsible for re-exporting those features, even though it does not use winapi directly, and should not care at all which winapi family is being used. But now realize that winapi has a ton of reverse dependencies and the poor user's apple project ends up depending on a whole messy dependency graph with winapi up many of those branches. Do you really expect every single crate in between apple and winapi to diligently re-export features so that the user can choose which family they want?

Right now the way features work is that foo specifies which features it requires from bar and the user only has any say in this if foo provides its own features to control this, and every single transitive dependency downstream re-exports these features. What I really think we need instead is a way for foo to specify which features it could use from bar, but not necessarily require. Then the user can choose which of those features it wants and foo will be notified in some manner of which features it actually got so it can adjust its code appropriately. Imagine something like hyper where right now if you want to disable the ssl feature (due to that annoying openssl dependency) you have to hope every single crate which depends on hyper, even transitively, re-exports the option to not depend on the ssl feature. If instead each of the crates that directly depends on hyper said it merely could use the ssl feature if it was available, then the user could choose whether or not to enable ssl from a single point and all the reverse dependencies of hyper would be aware of this and adjust accordingly. Without a system like this, I don't think it would be possible to sanely have mutually exclusive features.

@mat8913
Copy link

mat8913 commented Jan 5, 2017

Maybe allow specifying feature conflicts in Cargo.toml, such as:

[dependencies.awesome]
version = "1.3.5"
features = ["!secure-password", "civet"]

Which means this package depends on "awesome" but requires the "secure-password" feature to be disabled.

Also,

[features]
go-faster = []
secure-password = ["!go-faster", "bcrypt"]

means that this package provides features "go-faster" and "secure-password". But if "secure-password" is enabled, "go-faster" must be disabled.

@luser
Copy link
Contributor

luser commented Jan 18, 2018

I've run into this in practice before with the flate2 crate, which has features to select between using zlib and miniz as the implementation. It's an either-or choice, but nothing in cargo actually ensures that.

Your use case here seems like it's not well-served by features at all, honestly. cargo features as-implemented are allowed to be required by arbitrary crates in the dependency graph, so you'd have to have some way to completely override that. It's more similar to openssl-sys's OPENSSL_STATIC option, which is processed by its build script from the environment, or -Ctarget-feature=+crt-static. That is, it seems like a configuration option that can only be chosen by the crate at the top of the dependency tree which is producing a native binary output.

@stale
Copy link

stale bot commented Sep 17, 2018

As there hasn't been any activity here in over 6 months I've marked this as stale and if no further activity happens for 7 days I will close it.

I'm a bot so this may be in error! If this issue should remain open, could someone (the author, a team member, or any interested party) please comment to that effect?

The team would be especially grateful if such a comment included details such as:

  • Is this still relevant?
  • If so, what is blocking it?
  • Is it known what could be done to help move this forward?

Thank you for contributing!

(The cargo team is currently evaluating the use of Stale bot, and using #6035 as the tracking issue to gather feedback.)

If you're reading this comment from the distant future, fear not if this was closed automatically. If you believe it's still an issue please leave a comment and a team member can reopen this issue. Opening a new issue is also acceptable!

@stale stale bot added the stale label Sep 17, 2018
@retep998
Copy link
Member Author

Is this still relevant?

Yes.

If so, what is blocking it?

Someone needs to come up with a good design for a better feature system that supports mutually exclusive features among other things.

Is it known what could be done to help move this forward?

Convince someone to work on such a design.

@stale stale bot removed the stale label Sep 17, 2018
@thomcc
Copy link
Member

thomcc commented Oct 19, 2018

Someone needs to come up with a good design for a better feature system that supports mutually exclusive features among other things.

What's wrong with the design suggested by @mat8913? #2980 (comment)

It doesn't cover "among other things", but seems to address the issue while being flexible. (Some consideration would need to be given for the --all-features flag, but an error is seems sufficient)

@TheDan64
Copy link

TheDan64 commented Sep 21, 2019

In inkwell, we currently support 9 different versions of LLVM. This is represented by feature flags which correspond to different versions of our dependency, the llvm-sys crate. We enforce that they are used exclusively though some macro trickery. But cargo has no understanding of this, and so cannot enforce it appropriately. So currently we have multiple branches which just have the slight difference of changing the llvm-sys version number, and version feature flag.

Mutually exclusive feature flags would allow us to deploy (also to crates.io) one single release where you specify a feature flag, and cargo builds a crate graph that is able to use different dependency versions depending on the flag.

Some previous discussions:
#5653 (comment)
https://internals.rust-lang.org/t/mutually-exclusive-feature-flags/8601

What's wrong with the design suggested by @mat8913? #2980 (comment)

The second example in particular can end up being pretty verbose I think. As mentioned previously, in inkwell we have 9 features that we use exclusively for different LLVM versions. So it would look something like this:

[features]
llvm3-6 = ["!llvm3-7", "!llvm3-8", "!llvm3-9", ..., "!llvm8-0"]
llvm3-7 = ["!llvm3-6", "!llvm3-8", "!llvm3-9", ..., "!llvm8-0"]
...
llvm8-0 = ["!llvm3-6", "!llvm3-7", "!llvm3-8", ..., "!llvm7-0"]

I suppose this could be simplified a bit, ie if 3-6 has 3-7 excluded, then 3-7 doesn't need to list 3-6 explicitly. But this seems to be too implicit and potentially a maintenance headache.

It would be clearer to collect exclusions into named sets like so:

[mutually_exclusive_features]
llvm_versions = ["llvm3-6", "llvm3-7", "llvm3-8", ..., "llvm8-0"]

That way, we can keep verbosity down and the compiler can reference the set name in error messages. IE "llvm3-6 and llvm3-7 features cannot be combined as defined in the llvm_versions exclusion set"

To reformat @mat8913's examples:

[features]
go-faster = []
secure-password = ["bcrypt"]

[mutually_exclusive_features]
speed_or_security = ["go-faster", "secure-password"]

[dependencies.awesome]
version = "1.3.5"
features = ["civet"]
mutually_exclusive_features = ["secure-password"]

Note: Instead of mutually_exclusive_features we could just call it mutex_features for brevity since mutex is commonly known to mean "mutually exclusive"

Some consideration would need to be given for the --all-features flag, but an error is seems sufficient

Agreed. --all-features flag should straight up error when mutually exclusive features are specified

@seanyoung
Copy link

Another way of doing this is to provide "values" for features.

[features]
llvm="9"
flate2="zlib"
winapi_family="desktop_app"

Features could default to true or 1.

@GopherJ
Copy link

GopherJ commented May 12, 2020

hello guys, I've been waiting this for more than one year, any progress? It's open at: Aug 9, 2016 ~

@tuxxy
Copy link

tuxxy commented Jul 8, 2020

Still waiting on this, it would be very nice to have. :)

@FallenWarrior2k
Copy link

FallenWarrior2k commented Sep 30, 2021 via email

@photino
Copy link

photino commented Oct 6, 2021

If we have an ORM lib based on sqlx, which provides both async-std-rt and tokio-rt features. Since only one of the runtimes should be used, they are mutually exclusive. How can we map these features to sqlx's features runtime-async-std-native-tls and runtime-tokio-native-tls?

@Kixunil
Copy link

Kixunil commented Oct 6, 2021

For the time being one can use cfg(my_custom_feature) instead of cfg(feature = "my_feature") to enforce that libraries don't depend on particular feature. The binary can then specify feature via command line, which is not as great as having native support but better than nothing.

@FallenWarrior2k
Copy link

@photino Unfortunately, sqlx requires a single runtime to be selected through features. If you want to implement a similar requirement for your crate, you can probably take inspiration from this file.

Though, since sqlx is compiled before your crate, I doubt your checks could ever be hit, as the compiler error in sqlx-rt would stop compilation before your crate even gets to compile to be able to emit its own errors.

@alex
Copy link
Member

alex commented Mar 30, 2022

Another motivating use case here is that in the absence of a way for cargo to know that two features are mutually exclusive, there's no way to express the multiple-dependency-versions pattern (e.g. https://github.com/TrueLayer/reqwest-middleware/blob/main/reqwest-tracing/Cargo.toml) with a package that specifies the links field in Cargo.toml.

tetsuharuohzeki added a commit to tetsuharuohzeki/fastly-compute-template that referenced this issue Sep 27, 2022
…s will be `true`

This fixes the bug that is `cfg!(feature = "production")` always will be evaluated as `true`.
For example, the following code in `c_at_e_main` will always returns `"this is production"`
even if we run `make build_debug -j RELEASE_CHANNEL=canary`.

```rust
fn main() {
    let message = if cfg!(feature = "production") {
        "production"
    } else if cfg!(feature = "canary") {
        "canary"
    } else {
        "others"
    };
    println!("this is {}", message);
}
```

This bug is caused accidentally by our build system.

We set `default` feature for `c_at_e_main` to run
many rust toolchain that are not integrated for our build system
easily.

But our _release channel_ model is not mutually exclusive.
[cargo does not support it](rust-lang/cargo#2980).
So when we invoke `cargo build --feature canary`, then cargo will
enables both of `production` and `canary` feature because cargo enables
`default` feature set implicitly too.

Therefore, he above code will be expanded to:

```rust
fn main() {
    let message = if (true) {
        "production"
    } else if (true) {
        "canary"
    } else {
        "others"
    };
    println!("This is {}", message);
}
```

This causes the bug.

This change set `--no-default-features` CLI flags to cargo.
By this change, cargo will not enable `default` feature implicitly
and the bug will be fixed.

This change does not remove `default` feature from
`c_at_e_main`'s Cargo.toml
to still allow to invoke rust-analyzer without zero-configuration.

Note
-----

Can we set cargo's default CLI arguments without our build system?
=============

No.
[`.cargo/config.toml`](https://doc.rust-lang.org/cargo/reference/config.html)
has no way to disable default features without passing CLI flags.

Can we configure rust-analyzer to disable all default features?
==========

Yes, we can config it by vscode's setting.
But rust-analyzer does not have a way to editor agnostic settings.

- rust-lang/rust-analyzer#11010

There are rust-project.json but it's for non-cargo based project.
We would not like to use it.
https://rust-analyzer.github.io/manual.html#non-cargo-based-projects
@epage
Copy link
Contributor

epage commented May 17, 2023

Discussion Summary

Use cases

  • In Windows API there is the notion of a family and partitions. Your choice of family is basically which platform you are targeting and there is a choice between WINAPI_FAMILY_DESKTOP_APP WINAPI_FAMILY_PC_APP WINAPI_FAMILY_PHONE_APP WINAPI_FAMILY_SYSTEM and WINAPI_FAMILY_SERVER. Y
  • Controlling vendored or system library
  • Rendering backend
  • Classic C implementation vs Rust rewrite
  • Backend version (LLVM, Lua, etc)
  • Async runtimes
  • As an alternative to module level generics.
    • inlining size
    • Some use cases like this would be better handled by allowing multiple instances of a crate, rather than unifying them (comment)
    • See also spin

Other discussions

Related prior art

Proposals

Conflicting features

#2980 (comment)

[dependencies.awesome]
version = "1.3.5"
features = ["!secure-password", "civet"]

Feature values

#2980 (comment)

[features]
llvm="9"
flate2="zlib"
winapi_family="desktop_app"

Provides

#2980 (comment)

[features]
foo = []
bar = []

[features.foo]
provides = ["impl"]

[features.bar]
provides = ["impl"]

Only one provides=impl must be active at a time. Everything else would be a build failure.

See also https://github.com/ctron/rfcs/blob/feature/cargo_feature_caps_1/text/0000-cargo-mutex-features.md

@epage
Copy link
Contributor

epage commented May 17, 2023

Did a recent brainstorming discussion on this.

A key aspect we narrowed in on is that most of the use cases are really meant to be decided by the final binary and it is not ergonomic to bubble these up to the final binary. I've experienced this myself where cargo release had to add expose a vendored-openssl feature because a dependency used git2 with openssl and users of the binary need to control vendoring (or not). See also #9094.

We had the idea of "global features".

Care abouts

  • Mutually exclusive feature
  • Not piping up features through the entire stack
  • Coarse and fine-grained control (vendor all vs vendor specific)

High level

  • Defaulting so as not require each target to set it
    • Ideal: don't require app author to set the value
    • Ideal: don't require the user to set the value

Questions

  • How do we do coarse grain support?
  • Are mutually exclusive features open set (provides, requires) or closed set (variants)
  • What degree of error reporting (undefined features, undefined variants) do we support?
  • Distributed definition or a crate specifically defines a global feature?
    • If distributed, how does default variant work?
    • If crate-specific, how do we handle migrations across breaking changes?
    • If crate-specific, how much of a pain will it be to handle multiple crate versions within the dependency graph

... I feel like there were more points but I'm not finding it in my notes.

@Kixunil
Copy link

Kixunil commented May 18, 2023

For vendoring/system libs choice, I think the use case deserves a first-class feature. This is partially solved by being able to override build scripts. The missing part is that build scripts may need to also generate code (bindgen), so disabling them entirely doesn't help.

@Caellian
Copy link

Caellian commented Sep 15, 2023

Keep in mind that currently feature values are Vec<InternedString>, so feature values approach would prevent feature dependencies from being used AFAICT and be prone to user error:

my_feature = "dep:bytemuck" # unintended wrong use
my_feature = ["dep:bytemuck"] # what was intended

And this would effectively be the same as passing rustc-cfg from a build script except maybe being more trustworthy?

I think provides covers most current and future use cases assuming feature values would be:

#[derive(Default)] // default is the current `[]`
struct FeatureDetails {
  /// list of crate/feature dependencies
  dependencies: Vec<FeatureValue>,
  /// Name for mutual exclusion
  provides: Option<InternedString>,
  
  // ... future details ...
  
  /// List of features and dependencies disabled when this feature is active
  // disable: Vec<FeatureValue>,
}

And constructor could take arguments currently passed to build_feature_map in a form of fn new(deps: impl AsRef<Dependencies>, value: toml::value::Value) and then match for backwards compatibility.

@epage
Copy link
Contributor

epage commented Sep 25, 2023

I've created a Pre-RFC for mutually-exclusive, global features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-features Area: features — conditional compilation C-enhancement Category: enhancement S-needs-rfc Status: Needs an RFC to make progress.
Projects
None yet
Development

No branches or pull requests