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

Optional dependency when a feature is *not* enabled. #1839

Closed
SimonSapin opened this issue Jul 27, 2015 · 17 comments
Closed

Optional dependency when a feature is *not* enabled. #1839

SimonSapin opened this issue Jul 27, 2015 · 17 comments
Labels
A-features Area: features — conditional compilation A-target-dependencies Area: [target.'cfg(foo)'.dependencies] S-needs-team-input Status: Needs input from team on whether/how to proceed.

Comments

@SimonSapin
Copy link
Contributor

Some libraries can run on stable Rust with degraded performance (e.g. without using NonZero) or limited capabilities (some APIs disabled). The emerging practice seems to be having an unstable or nightly Cargo feature to enable user to opt into using unstable Rust features when they are available.

I’m publishing rust-rc as a work around for std::rc::Weak not being stable yet. At the moment, there doesn’t seem to be a way to have this crate as a dependency by default, but not when the unstable feature is enabled. Could this be added to Cargo?

Note that reversing the default and having a stable feature only moves the problem to dependencies that can/should only be used on unstable Rust.

@alexcrichton alexcrichton added the A-features Area: features — conditional compilation label Jul 27, 2015
@Boddlnagg
Copy link
Contributor

I'd also like to have this. In midir, there are multiple backends (ALSA and/or JACK on Linux, CoreMIDI and/or JACK on OSX, WinMM on Windows), and my plan is to have a jack feature flag which enables JACK and disables the other (native) one. I also would like to remove the dependency on ALSA/CoreMIDI in the case of compiling for JACK, so it could even be used if ALSA is not installed on the system.

My first idea was what is requested above, i.e. to simply disable alsa-sys dependency if jack feature is selected.

An alternative to this is to have a default feature (e.g. native), which could be disabled if jack is enabled, and then have platform-specific dependencies for this feature (namely alsa-sys on Linux and CoreMIDI on OSX), using something like [target.x86_64-unknown-linux-gnu.features]. It also seems to be currently impossible to have target platform specific default-feature-sets, which would also solve the problem.

So far I was not able to find a workaround that works without defining platform-dependent dependencies/features in dependent crates, but I may have overlooked something.

@eternaleye
Copy link

With the [target.'cfg(...)'.dependencies] syntax, this could be supported simply by allowing cfg(feature = "...") to behave as it does in Rust code:

[target.'cfg(not(feature = "std"))'.dependencies]
hashmap_core = "0.1.2"

Currently, such a section is treated as enabled regardless of feature status, which honestly seems like a bug to me.

@lschmierer
Copy link

Same case for me:

[target.'cfg(not(feature = "std"))'.dependencies]
heapless = "0.2.7"

[features]
default = ["std"]
std = []

I only want the heapless dependency if feature std is not enabled.
heapless works with nightly only, but I want my library to be compatible with stable if std is enabled.

Is the current behaviour a bug or is it intended?

@niyue
Copy link

niyue commented Nov 14, 2022

With the [target.'cfg(...)'.dependencies] syntax, this could be supported simply by allowing cfg(feature = "...") to behave as it does in Rust code

This doesn't seem to be a valid solution any more. According to here, https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#platform-specific-dependencies

Unlike in your Rust source code, you cannot use [target.'cfg(feature = "fancy-feature")'.dependencies] to add dependencies based on optional features. Use the [features] section instead:

[dependencies]
foo = { version = "1.0", optional = true }
bar = { version = "1.0", optional = true }

[features]
fancy-feature = ["foo", "bar"]

@weihanglo weihanglo added S-needs-team-input Status: Needs input from team on whether/how to proceed. A-target-dependencies Area: [target.'cfg(foo)'.dependencies] labels May 14, 2023
grim7reaper added a commit to HydroniumLabs/h3o that referenced this issue Jan 28, 2024
How it was done
===============

First step was to replace the references to the `std` crate by `core` or
`alloc` when possible.
Which was always the case, except for the `Error` trait which isn't
available in `core` on stable[1].

Another change, that may impact the performance a bit, was to replace
the usage of Hash{Map,Set} by the B-Tree variant, since the hash-based
one aren't available in core (due to a lack of a secure random number
generator).
There should be no visible impact on default build, since we're still
using hashtable when `std` is enabled (which is the default behavior).
Maybe I should give a shot to `hashbrown` to bring back the hashtable
version into `no-std` as well.

The last change was a bit more annoying (and surprising): most of the
math functions aren't available in core[2].
So I've implemented a fallback using `libm` when `std` isn't enabled.
Note that due to Cargo limitation[3] `libm` is always pulled as a
dependency, but it won't be linked unless necessary thanks to
conditional compilation.

Known limitations
================

Cannot use the `geo` feature without `std`, mainly because the `geo`
crate itself isn't `no-std` (as well as a bunch of its dependencies I
guess).

--------

[1]: rust-lang/rust#103765
[2]: rust-lang/rfcs#2505
[3]: rust-lang/cargo#1839

Closes: #19
grim7reaper added a commit to HydroniumLabs/h3o that referenced this issue Jan 28, 2024
How it was done
===============

First step was to replace the references to the `std` crate by `core` or
`alloc` when possible.
Which was always the case, except for the `Error` trait which isn't
available in `core` on stable[1].

Another change, that may impact the performance a bit, was to replace
the usage of Hash{Map,Set} by the B-Tree variant, since the hash-based
one aren't available in core (due to a lack of a secure random number
generator).
There should be no visible impact on default build, since we're still
using hashtable when `std` is enabled (which is the default behavior).
Maybe I should give a shot to `hashbrown` to bring back the hashtable
version into `no-std` as well.

The last change was a bit more annoying (and surprising): most of the
math functions aren't available in core[2].
So I've implemented a fallback using `libm` when `std` isn't enabled.
Note that due to Cargo limitation[3] `libm` is always pulled as a
dependency, but it won't be linked unless necessary thanks to
conditional compilation.

Known limitations
================

Cannot use the `geo` feature without `std`, mainly because the `geo`
crate itself isn't `no-std` (as well as a bunch of its dependencies I
guess).

--------

[1]: rust-lang/rust#103765
[2]: rust-lang/rfcs#2505
[3]: rust-lang/cargo#1839

Closes: #19
@a-rustacean
Copy link

Any workaround?

@epage
Copy link
Contributor

epage commented Feb 9, 2024

cfg(features) is tracked in #8170.

For the original use case, cfg_accessible support in cargo would help out (or cfg(rust-version) for non-nightly cases). I don't think there is an issue for tracking the cargo side of that work.

For the surface reading of this issue, this is a special case of #2980 (see also https://internals.rust-lang.org/t/pre-rfc-mutually-excusive-global-features/19618).
I'm going to close in favor of that (and #8170). I'm going to close in favor of those issues. If there is a reason we should leave this open, let us know! If there isn't an issue for cfg_accessible / cfg(rust-version), I welcome someone to create it.

@epage
Copy link
Contributor

epage commented Jul 4, 2024

@Kixunil
Copy link

Kixunil commented Jul 4, 2024

@epage I disagree that global mutually exclusive features solve this because this is neither global nor mutually-exclusive. There is a case when a library depends on a more powerful option to build. And yes, I do co-maintain such crate. Forcing users to set a global feature would be annoying.

So I think either this one or #14188 should be reopened. And while maybe global mutually exclusive features could have a special case to resolve this I think it'd be just too complex.

@epage
Copy link
Contributor

epage commented Jul 12, 2024

While I didn't notice that this was closed when I closed #14188, particularly that the reason it was closed was for the specific users need and not for the broader topic, if I open this, I'd either be re-closing it or proposing it to be closed, looking for a second on the Cargo team, because of the discussion in that This Development Cycle in Cargo section I linked to.

@Kixunil
Copy link

Kixunil commented Jul 12, 2024

I didn't notice that this was closed when I closed #14188

No problem, such things happen.

particularly that the reason it was closed was for the specific users need and not for the broader topic

Why is that a problem?

I'd either be re-closing it or proposing it to be closed

Do I understand correctly that your argument against it is pretty much what the document you linked says? Specifically:

This falls apart because these new dependencies would not have feature resolution performed. We would instead need to loop over running the feature resolving, checking not(features), and adding them to the set we evaluate next time. This is complex to implement, algorithmically complex, and may run into cycles with dev-dependencies.

First, I don't see how it would run into a cycle. Once a dependency is added it's not removed (cycles in crates themselves are already forbidden) so in the worst case scenario all features of all crates would get enabled. And there's not an infinite amount of features or crates. Therefore the loop must terminate once there are no more features to enable.

Regarding algorithmic complexity, the loop would repeat at most as many times as there are dependencies that are using this. And while multiple crates would benefit from this I don't think that there are so many of them in most projects. I'd be surprised if it had to loop more than a few times on an average project. That loop will take much less time than recompiling a whole crate that's not needed or having a human figure out which of the exclusive features of a crate needs to be turned on.

I can't comment on implementation complexity as I haven't seen the resolution algorithm yet. What I can say is that I believe this feature to be quite valuable. It has potential to reduce compile times which is the topic everyone complains about and does so without making something else annoying. Thus the complexity might be worth it.

Just in case: if some part of my comment sounds like a strawman or otherwise dishonest I really don't mean to. I had hard time understanding your comment (I feel a bit tired) and it's quite possible I messed up something.

@epage
Copy link
Contributor

epage commented Jul 12, 2024

Why is that a problem?

Just calling out that I closed your general issue for one closed for more specific reasons, so it wasn't a full replacement of it as-is.

Once a dependency is added it's not removed (cycles in crates themselves are already forbidden)

Take the example from #14188

[package]
name = "foo"

[features]
extra = ["dep:powerful"]
"!extra" = ["dep:minimal"]

[dev-dependencies]
foo = { path = ".", features = ["extra"] }

Note: this is a legal cycle.

We don't know what dependencies exist on foo until we've processed all of its dependencies. However, to walk all of the dependencies, we need to know whether extra is enabled or not.

This is a simple case where we directly depend on foo. It doesn't have to be that way.

Regarding algorithmic complexity,

I can't speak to that. @Eh2406 would be the best person to.

I can't comment on implementation complexity as I haven't seen the resolution algorithm yet

The current resolve is a mess and is nearly feature frozen. We've made minor tweaks here or there but have been avoiding major ones. Work on a replacement resolver has picked back up again lately which improves the situation by giving us less control through abstracting and generalizing the resolver which makes it easier to reason about each of the pieces. I'm assuming the new resolver is still a ways out and @Eh2406 would also be the one to speak to in terms of implementation complexity.

@Kixunil
Copy link

Kixunil commented Jul 12, 2024

Just calling out that I closed your general issue for one closed for more specific reasons, so it wasn't a full replacement of it as-is.

Ah, OK, so looks like that one should be reopened?

We don't know what dependencies exist on foo until we've processed all of its dependencies. However, to walk all of the dependencies, we need to know whether extra is enabled or not.

Yes, to process it we need to repeat the processing multiple times. When building a non-dev build dev-dependencies aren't considered so we can ignore that but for dev build it'd just cause the feature to turn on and stay on. We could even decide to not disable the minimal dependency in this case since the situation is pathological.

Work on a replacement resolver has picked back up again lately which improves the situation

Oh, OK, understood that it may take a while. But it'd be great to have it available one day.

@epage
Copy link
Contributor

epage commented Jul 12, 2024

None of this is new ground and, last the last time this was discussed, the general consensus I got from the conversation was "this wasn't happening".

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 15, 2024

First off sorry for the slow response here. This is been in my inbox to reply to for... Way too long. Every time I figure out something clear to say I realize it's a massive oversimplification, and decided to try again the next day. This is one of the fascinating consequences of living in a P != NP world, any explanation that can be processed in linear time is ignoring an exponential amount of important detail. Nonetheless I need to get over procrastinating and actually participate in this conversation.

Just in case: if some part of my comment sounds like a strawman or otherwise dishonest I really don't mean to. I had hard time understanding your comment (I feel a bit tired) and it's quite possible I messed up something.

I absolutely feel the sentiment. Simple statements end up just being strawman, and technically correct statements end up to in the weeds to be understandable as a whole. We will all endeavor to be forgiving and charitable and understanding, so that we can continue to talk about such fascinating topics.

A problem with an imperative outer loop is one of error messages. If I depend on minimal = "=1.0.0" and bar = "*" and every version of bar depends on deep = "1.0.1", then there is no resolution. Ideally in error message would synthesize all of this information into a statement similar to that description. The current resolver does not, it will fail with an error message having to do with the last iteration of its outer loop "bar @ 0.0.1 could not satisfy its requirement on deep because of a previously selected version deep @ 1.0.0 which was selected to fulfill the requirements of root" (if we're lucky it's that coherent.) There is no way for it to talk about generalized patterns like "all versions of bar" because it's just a loop that tried them all. (Some details alighted.) The new resolver will give a complete error message, one that actually explains the problem with every single version of bar. Optimizing that error message to be short and understandable will be a lot of work, but it will be possible.

The quality of the error message is a direct analog for the rigor of the internal algorithms. If there's a bug in the loop that skips over versions it should not, then a complete error message will have a logical fallacy and it. This logical fallacy can be spotted by users, or even checked by tests. If the error messages are not complete, it is incredibly difficult to even identify the bug occurred.

To change the subject little bit a more fundamental question I have about the proposed outer loop is... Say we have hundreds of versions of foo that all look like:

[package]
name = "foo"

[dependencies]
minimal = "=1.0.0" // `=` versions are rare but "things that cause conflicts" are not, and this is an easy example to construct and explain.

[features]
extra = ["dep:powerful"]
"!extra" = ["dep:minimal"]

in my root package looks like:

[package]
name = "root"

[dependencies]
minimal = "*"
foo = "*"

so if I understand the proposed loop correctly it first does the resolution that ignores all "! features. So it successfully discovers that root, minimal = 1.999.0 and foo = 1.999.0 successfully work. Then it scans the result for "! features that need to be activated. So it re-resolves adding the dependency on minimal. Which leads to a conflict. The fundamental question is what happens next?

We could:

  • Show the error to the user. From an implementation side, this says that by adding a "! feature we cannot change any previous version decisions. This definitely prevents the loop from being exponential, but this can lead to a very odd user experience. You could incrementally add dependencies to generate a valid lock file, then delete the lock file and get an error message about there not being a valid lock file available. (In this case starting without dependencies and cargo add foo followed by cargo add minimal will work just fine, but the opposite order will not.)
  • We could allow dependency resolution to choose other versions to avoid the conflict of "! being active, which will probably work correctly in this case giving us root, minimal = 1.0.0 and foo = 1.999.0. But we still have an odd order dependency here in that we can't consider other versions of foo because we don't in advance no if they need "! to be activated. Furthermore, in a slightly more complex example, one of the newly selected version of some other dependency might have a foo/extra, which could lead to really weird things.
  • We could tell resolution to try again to give us different versions without "! being active. We would need to somehow encode what the problem is with the current selection, otherwise it would just return the same resolution output. And, this outer loop is going to need to try again and exponential number of times. In this case it will need to try the intersection of all versions of minimal and foo a mere 1000000 iterations.
  • Something else? If so what?

I am sorry if I left out important details of my understanding, or miss important details of your understanding. Similarly, it is possible I will wake up tomorrow morning with a clear understanding of something that seems intractable at the moment.

@Kixunil
Copy link

Kixunil commented Jul 15, 2024

@Eh2406 thanks for thoughtful response!

Firstly I don't really know what you mean by minimal = "*" since asterisk means "any version works" therefore by definition anything matched before should match, so there's never a conflict. But I get that if you put there something else (higher or lower major version than the other crate picked) it's a conflicting situation.

I don't see any issue picking a different version and just have the crate twice in case of a conflict. Yes, it defeats the original purpose and a warning could be warranted but at least it works. This mechanism should be mainly used for private dependencies anyway.

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 15, 2024

Firstly I don't really know what you mean by minimal = "*" since asterisk means "any version works" therefore by definition anything matched before should match, so there's never a conflict.

Yes, if we see minimal = "=1.0.0" first then we can easily pick 1.0.0 and when we get to "any version works" we are satisfied. Unfortunately the requirement minimal = "=1.0.0" is going to be loaded by your outer loop later, the inside resolution it only sees minimal = "*" and therefore is going to pick the highest available version 1.999.0. The resolution and the outer loop may disagree about these versions and their consequences was the point I was trying to explore.

I don't see any issue picking a different version and just have the crate twice in case of a conflict. Yes, it defeats the original purpose and a warning could be warranted but at least it works. This mechanism should be mainly used for private dependencies anyway.

Many parts of cargo are built around the long-standing assumption that we do not build more than one semver compatible version of each crate. If this feature requires expanding the build model... that's a very different conversation.

@Kixunil
Copy link

Kixunil commented Jul 15, 2024

Many parts of cargo are built around the long-standing assumption that we do not build more than one semver compatible version of each crate.

Oh, that's weird, I thought it can compile any combination of crates and it just chooses not to. OK, you have good point to which I don't have a solution, at least not now.

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 A-target-dependencies Area: [target.'cfg(foo)'.dependencies] S-needs-team-input Status: Needs input from team on whether/how to proceed.
Projects
None yet
Development

No branches or pull requests