Tracking Issue for weak dependency features
ehuss opened this issue · 39 comments
Summary
Original issue: #3494
Implementation: #8818
Documentation: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#weak-dependency-features
-Z weak-dep-features will enable "weak" dependency features, whereby dep_name?/feature_name syntax in the [features] table will not automatically enable the dep_name dependency (unlike the dep_name/feature_name syntax which always enables dep_name). This just indicates that the feature on that dependency should be enabled if something else enables that dependency on this package.
Unresolved issues
- Bike shed the syntax. We've discussed various variants:
dep_name?/feat_name?dep_name/feat_namedep_name?feat_name- Or maybe something more verbose like
{dep="dep_name", feature="feat_name", enable-dep=false} - Or a more verbose string format, like
if-dep:dep_name/feat_name(read this as "if dependency foo then enable feature bar"). - Concerns:
- Too verbose, and people may be less likely to use it.
- Terse
?format means people will have to look up what it means, and searching for punctuation is difficult. - Interaction with
org_name/pkg_namesyntax, which could contribute to the punctuation soup.
- Possibly make this the default behavior for
dep_name/feat_namesyntax on an edition boundary. A concern is that this is not an (easily) rustfix-able change. - Figure out how to deal with breakage of Cargo versions older than 1.19. Resolved in #9161.
About tracking issues
Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.
Possibly make this the default behavior for dep_name/feat_name syntax on an edition boundary [...]
I think is the best option if we can make it work.
foo = [ "bar/baz" ] # just /baz if bar is activated
foo = [ "bar", "bar/baz" ] # bar and bar/baz@mehcode I think the issue is that due to the way it's built, rustfix knows nothing about Cargo.toml, thus cargo fix does neither. I guess the solution would be to extend cargo fix to also change Cargo.toml. That would work but is non-trivial. I think https://github.com/matklad/tom might be useful here.
In my opinion we shouldn't have a syntax for this feature, instead a programmer could use "foo/bar" directly. There is a compatibility concern by doing so, but it could be dealt with by providing a warning when a dependency feature is used without depending on a dependency itself.
For example, in this case there would be no warning as feature b depends on lib.
a = ["lib"]
b = ["a", "lib/feature"]However, without depending directly or indirectly on a dependency there would be a warning.
b = ["lib/feature"]Then in a future edition of Rust the behaviour would change so that having "lib/feature" feature wouldn't enable "lib" feature.
@xfix FWIW we wanted to avoid that because it means the meaning of a/b differs depending on whose lockfile you're looking at. We thought that would be too confusing because a/b has been a feature of Cargo for so long. That's what pushed us to find a different syntax.
(although what you describe I believe is technically feasible, our hesitation was just about the confusion of having the same syntax mean two different things depending on the context)
differs depending on whose lockfile you're looking at.
@alexcrichton you mean Cargo.toml? lockfiles can be shared by crates of different editions and they also don't have the a/b syntax anywhere.
I think having the same syntax could be balanced by having a warning when using the syntax in older editions - so there would be only a single behavior that doesn't cause a warning. Also, I think the behaviour of a/b feature enabling a optional dependency is somewhat surprising.
Add to that we can implement a warning now, we don't have to wait for Edition 2021 or something like that - a warning could be dealt with by replacing "a/b" with "a", "a/b" - this works even in Rust 1.0.
From a list of 30 most popular dependencies (by recent downloads) on crates.io I found that the 2 crates unintentionally enable optional features.
-
randunintentionally enablesrand_chachawithstdfeaturestd = ["rand_core/std", "rand_chacha/std", "alloc", "getrandom", "libc"]
-
synunintentionally enablesquotewithproc-macrofeatureproc-macro = ["proc-macro2/proc-macro", "quote/proc-macro"]
On the other hand log has a feature called kv_unstable_sval which enables sval dependency by saying sval/fmt.
kv_unstable_sval = ["kv_unstable", "sval/fmt"]However log could easily deal with a suggested warning by doing the following instead.
kv_unstable_sval = ["kv_unstable", "sval", "sval/fmt"]In fact, rand_core is already doing something similar. It technically doesn't need to specify "getrandom", but does it anyway.
std = ["alloc", "getrandom", "getrandom/std"]Er yes I meant the manifest, not the lock file. @xfix that's perhaps plausible to simply warn, but that's also a massive task to force the entire Rust community to relearn a feature they already know. There's a very real cost to that which isn't present if we pick a new syntax.
Personally I think that warning or not, switching it without an opt-in of either an edition or special syntax is a violation of Rust's stability guarantees. I support the idea of changing the syntax on a edition boundary, but I think this feature is too useful to wait for a new edition, or be even blocked by a discussion about the syntax :). First it can be introduced with the new custom syntax, then later one can decide whether the new edition should switch to it by default. Also there is the question of interactions between the RFC to do workspace sharing of information and having heterogenous edition choices inside a workspace.
@alexcrichton From my experience, the current behavior is not really an expected one, but an unfortunate gotcha that I discovered independently. I'm strongly in favor of changing dep_name/feature syntax behavior in edition 2021.
I think we need to have a distinct syntax for this, even in a new edition. I don't think we should go directly from one meaning of abc/xyz to another.
Now that the new feature resolver has gone in, are there any blockers preventing us from stabilizing this once we're confident it works as intended?
I think we bikeshedded the syntax fairly extensively in the Cargo meetings. abc?/xyz feels like it has the right degree of orthogonality, such that / keeps the same meaning and the addition of ? means "optional".
I also think we need to give people some guidance about how soon after stabilization they can safely start using this in popular crates. For instance, I'd really like to use this in git2, but that's in the dependency trees of many crates; how long after release would it be reasonable to start using it?
With regard to how long before using, why would this be any different than any other MSRV change? I just checked the code for the time crate, and the main branch could use this (not any current release, though). I'd just hold out six months per my MSRV policy.
Features go into the index, will this cause any issues with older cargo's loading older versions of a crate which has started using it in a new version?
@Nemo157 In general the Resolver is pretty good at not picking versions that contain things it does not understand. But now that you mention it we should definitely test in this case.
are there any blockers preventing us from stabilizing
The path to stabilization is:
- Testing and feedback.
- Decide on the syntax.
- Decide if the index format change is OK.
- Update crates.io to accept the syntax.
- Stabilize and document.
The testing phase is important, because enabling this causes Cargo to use the new feature resolver in "classic" mode when resolver = "1". I've tested it a fair bit, and I am unaware of any issues, but it is a big change. Anyone can test it by setting the environment variable __CARGO_FORCE_NEW_FEATURES=compare, which will compare the results of the old and new resolver, and panic if they are not identical. I did that on a bunch of projects, but the edge cases normally arise when using unusual combinations of flags that I did not test (aside from Cargo's own test suite of about 2,000 tests).
we need to give people some guidance about how soon after stabilization
I'm not sure how feasible or useful that guidance could be. Projects have widely different MSRV policies, and it's a choice they have to make based on the level of support they want to and can feasibly provide. Whatever they choose is valid, and I'm not sure we can imply with a recommendation that their choice isn't a good one. If I only want to support the latest stable, that shouldn't be shunned. Just like if someone wants to support 2 year old releases, and they need to reject PRs because it violates that choice, I think that choice is valid and shouldn't be discouraged.
For git2, I think Alex is relatively aggressive on using newer releases than most projects (stable-1 I think).
will this cause any issues with older cargo's loading older versions of a crate which has started using it in a new version?
Those packages are mostly ignored by older cargos. There's a test for this here (although that is not directly testing older cargos, the concept is the same). I just checked, and it works starting with version 1.19 (released 2017-07-20). Prior versions fail with a parse error. We don't have an explicit timeframe in our compatibility policy (per https://doc.crates.io/contrib/design.html#forwards-compatibility). I think a minimum of 2 years isn't bad, but we haven't discussed it much.
@jhpratt wrote:
With regard to how long before using, why would this be any different than any other MSRV change?
@Nemo157 wrote:
Features go into the index, will this cause any issues with older cargo's loading older versions of a crate which has started using it in a new version?
This was the exact concern that motivated the suggestion to give people guidance. I'm not suggesting that such guidance would tell people what their MSRV (or MSCV) policy should be. Rather, I think we need guidance (supported by testing) along the lines of "There are no special compatibility considerations here, you can follow your normal MSRV policy". I want to make sure people know that if they release a new version of their crate that uses this feature, they won't break people who are using older versions of their crate.
@ehuss wrote:
Those packages are mostly ignored by older cargos. There's a test for this here (although that is not directly testing older cargos, the concept is the same). I just checked, and it works starting with version 1.19 (released 2017-07-20).
Thank you for testing that! That's exactly the kind of guidance I think people will need to be able to use this feature confidently.
Decide on the syntax.
We had a couple of fairly detailed discussions about syntax and orthogonality, which seemed to reach a consensus on the ?/ syntax. Is this still an open question?
Decide if the index format change is OK.
Update crates.io to accept the syntax.
Makes sense. Given that people can use this feature on nightly, this seems like it should happen in parallel with testing and feedback.
I've also submitted #9015 as draft documentation for the features section of the reference.
I just checked, and it works starting with version 1.19 (released 2017-07-20). Prior versions fail with a parse error. We don't have an explicit timeframe in our compatibility policy (per https://doc.crates.io/contrib/design.html#forwards-compatibility). I think a minimum of 2 years isn't bad, but we haven't discussed it much.
I think that's a problem. It's one thing to have crates on crates.io break support with older cargos, it's another thing to have those older cargos be broken even if they only use an older version of the crates. Is there a way to make the feature fail in a better way for those older cargos?
Is this still an open question?
I think it is still open to be changed. There has been almost no feedback from users about this change, other than the 3 people who have commented here not liking the syntax.
Is there a way to make the feature fail in a better way for those older cargos?
I don't immediately have any ideas on how to make that work.
I realize that it can be painful to break old versions of software. I'm not sure if 1.19 is old enough to draw the line. Older versions could still be used with --frozen, careful vendoring, or using an older copy of the index. I do feel that 4 years might not be enough time. I don't think the index can remain compatible forever, but we haven't decided on a reasonable timeframe.
How hard would it be to implement a separate "features-weak" map in the index serialization? Somehow keeping the new kind of features in a new table? Ether just the weak items, or the elements activate the weak items, or the entire thing (if there are any weak items then the hole map is call "features-weak" instead of "features").
There are two issues with that. One is that older cargos would ignore the field, and possibly end up resolving the features incorrectly (which may or may not work in practice). The other is that the features would still be listed in the [features] table of Cargo.toml, and the older Cargo's would fail to parse the manifest (though that won't be a problem if the project is locked to an older version).
If we want to have the rule that older Cargos will work as long as they have a Cargo.lock file, that may be a sufficient fix.
There has been almost no feedback from users about this change, other than the 3 people who have commented here not liking the syntax.
Put me down as a user who does like the abc?/xyz syntax.
I'd say the ? syntax is acceptable as a short-term solution, but it should become the default behavior in some future edition or so. Everyone is surprised by the current default behavior.
@ehuss Both of those are only problems if old Cargo selects a version that uses the new functionality, and I am ok with old Cargos braking if it trys to build a new crate. My (and I think @est31) concern is that and old Cargo that has a dep on and old version will stop working because Cargo can not read the index where a new unselected version uses the new sintaks. Maybe we have misunderstood your comment "Prior versions fail with a parse error."
Here is the test case I am worried about:
s = 1.0 does not use weak.
s = 2.0 uses weak.
root depends on s="1.0", can it be built with Cargo pre 1.19?
Yea, after thinking about it more, that is probably a good solution. Some things I would consider doing:
- Test it out, with various versions of Cargo from 1.0 to 1.50, and see how they react.
- Figure out what changed in 1.19.
- Probably name the field something more generic, like
features-extfor an "extension", so that other things like #5565 could take advantage of it. - Ratchet up the syntax restrictions which are currently a warning (at least for this extra field).
- Consider adding a version field to the index to better deal with this in the distant future.
Figure out what changed in 1.19.
Commit 3d6de41 allowed ignoring malformed manifests from 1.20. Since the beginning (1.0?) cargo was always throwing errors if it encountered malformed manifests.
From what I understand, if we just add this syntax directly to features, any cargo version before 1.20 will fail when working with these deps, right? Couldn't we be okay with that since we are doing a 2021 edition soon?
@pksunkara We don't want to permanently break older cargos. I'm not sure how that is related to the edition, but there wouldn't be a way to retroactively change older versions.
Sorry, I should have posted a followup here. #9161 implemented the recommendation of adding a new features2 field, which should partially solve the problem. One drawback is that versions of cargo older than 1.51 may resolve packages that have new feature syntax, but behave as if those features don't exist. This generally shouldn't be a problem for weak features, but there are some edge cases for namespaced features.
There is now a developer-only set of tests at https://github.com/ehuss/cargo/blob/master/tests/testsuite/old_cargos.rs for validating compatibility with older versions of cargo.
Reviewed that, looks great. With that in place, what is stopping us from stabilising this? Would love to get this feature released on stable.
Well, I think there is another syntax to support this without breaking anything, by turning a feature name into a table, as we do for dependencies:
[dependencies.rand]
version = "0.8.3"
default-features = false
optional = true
[features]
use_rand = ["rand"]
std = { features = ["rand/std"], dependencies = [] }
# or
[features.std]
features = ["rand/std"] # features that this feature depends on
dependencies = [] # dependency crates that this feature depends on, empty means nothingSome notes:
- Choice of keys for
[features.<name>]is flexible, e.g.[features.<name>.depends]or[features.<name>.uses]or[features.<name>.crates](or anything, really.) - Accepting above keys for the sake of argument, current format will be a syntactic sugar, and would mean these two forms are equivalent in the
featurestable:std = ["rand/std"] std = { features = ["rand/std"], dependencies = ["rand"] }
- The only downside that I can think of is added verbosity, although choice of keys can play a significant role here.
Adding support for this alternative feature syntax would also enable #4956 quite easily.
[features.std]
description = "Some descriptive text here..."
features = ["rand/std"]
dependencies = ["rand"]Maybe an alternative syntax for features should be implemented first:
[features.std]
dependencies = ["rand"]Which would be identical to this:
[features]
std = ["rand"]And from that point add new features like a description field (#4956), this feature and maybe even #5565. This is all related and would be enabled by features represented as a table rather than a simple array of strings.
Also, considering mutually exclusive featues (#2980):
[features]
first = []
[features.second]
conflicts = ["first"]I like the idea of allowing a table for features, and that mechanism should absolutely support weak dependency features. I don't think that should be a blocker for introducing weak dependency features, for which we'll definitely want a shorthand syntax.
I have posted a proposal to stabilize this in rust-lang/rfcs#3143.
Anything holding up actually stabilizing this now that the RFC was merged?
There's a fair lot of work to update crates.io, and perform testing. I may work on it this week, but it may be a long while before it hits stable.
It is just waiting on rust-lang/crates.io#3867.