Stabilize Cargo's new feature resolver #2957

ehuss · 2020-07-16T23:29:47Z

This RFC proposes to stabilize Cargo's new feature resolver, and the new command-line behavior for feature flags.

ehuss · 2020-07-16T23:30:01Z

cc @rust-lang/cargo

CryZe · 2020-07-17T01:52:36Z

At the moment

my-feature = ["my-optional-dependency/their-feature"]

also enables an optional dependency when my-feature is activated. I've encountered this for a std feature that I have, where I delegate std to all my dependencies, even optional ones, but that now forces them to be built, even if the user of my crate never wanted them. The only alternative is to add a feature for something like i-want-the-dependency-but-also-std, but that's annoying to use and starts a combinatorial explosion.

So I feel like the new resolver should also not automatically enable optional dependencies just because a feature got delegated. Otherwise I feel like this may be something that would go into an eventual resolver version 3 in some way.

ehuss · 2020-07-17T04:37:15Z

@CryZe I recognize that the inability to conditionally enable features on optional dependencies makes it difficult or impossible to express certain things. That request is tracked in rust-lang/cargo#3494.

There are likely several ways that can be added without breaking current behavior. Possibly through new syntax, something like namespaced features, or automatic features.

Part of the goal is to avoid highly disruptive changes, and I suspect changing the meaning of pkg/feat would break a large number of packages. Also, the resolver field is a global change, whereas a change to pkg/feat would be package-specific (that is, each package would need to decide which meaning it is using).

Finally, we recognize that there are a large number of items on the wish list for changes to features. However, I would prefer to avoid blocking progress for changes that are not on the horizon.

Eh2406

Thanks to @ehuss for all the work to make this happen! Overall I love the improvements!

text/0000-cargo-features2.md

Eh2406 · 2020-07-17T14:30:51Z

text/0000-cargo-features2.md

+
+Testing has been performed on various projects that shows sometimes
+dependencies are written to assume that features from another part of the
+graph are enabled, and fail to compile with the new resolver. Because the new


It took me a few tries to parse this sentence. Maybe it can be broken up something like "Testing has been performed on various projects. Some whur found to fail to compile with the new resolver. Witch shows that sometimes dependencies are written to assume that features from another part of the graph are enabled."

nagisa · 2020-07-17T23:58:46Z

text/0000-cargo-features2.md

+Setting the resolver to `"2"` switches Cargo to use the new feature resolver.
+It also enables backwards-incompatible behavior detailed in [New command-line
+behavior](#new-command-line-behavior). `"2"` is the only valid option, if
+`resolver` is not specified then the old behavior is used.


Is there any reason why resolver = "1" is not something we are going to accept, even if just for reduced surprise factor? (As a prior art --edition accepts both 2018 and 2015 as a value)

There isn't a strong reason, just a personal preference of not wanting to see resolver = "1" being used, but it can be easily added.

I think we should add this, so that if we want to change the default for resolver at a later date (maybe a new rust edition etc..)

@pksunkara That assumes we want to support using a new edition but opting for the old resolver, which I don't know that we want to support. We could minimize the matrix of simultaneously supported features by not supporting the old resolver on a new edition that implies the new resolver.

jonas-schievink

Great work @ehuss! Can't wait to see this happen, this will help immensely in the embedded ecosystem ❤️

text/0000-cargo-features2.md

jonas-schievink · 2020-07-17T23:44:58Z

text/0000-cargo-features2.md

+
+Note that this is a global decision. So a command like `cargo build
+--all-targets` will include examples and tests, and thus features from
+dev-dependencies will be enabled.


Does this mean that doing cargo build --lib followed by cargo build --all-targets will potentially recompile a lot of the dependency graph if a new feature is turned on?

Yes, that is possible.

jonas-schievink · 2020-07-17T23:48:22Z

text/0000-cargo-features2.md

+[package]
+name = "my-package"
+version = "1.0.0"
+resolver = "2"


Incredibly nitpicky, but why the string "2" instead of the number 2?

The intent is to keep it a free-form string for future additions, like enabling different options. Hopefully that won't be necessary, but I wanted to leave it flexible.

nagisa · 2020-07-18T00:23:50Z

text/0000-cargo-features2.md

+* This adds complexity to Cargo, and adds boilerplate to `Cargo.toml`. It can
+  also be confusing when switching between projects that use different
+  settings. It is intended in the future that new resolver will become the
+  default via the "edition" declaration. This will remove the extra


edition is a non-global choice and therefore workspaces generally do not have the edition annotation. Does this mean that workspaces are doomed to forever using resolver annotation and/or using the old resolver?

We are planning to add the edition field to the [workspace] table. That is still a bit up in the air, since we haven't really defined what that will do (would it be the default for all workspace members?). Issue rust-lang/cargo#5784 is tracking that. One of my concerns is that with RFC 2906, that inheriting package metadata will require specifying { workspace = true } and that ends up creating more boilerplate in each package. But making it implicitly inherit the edition would be inconsistent with the other fields.

matklad

Excellent work!

I think scoping this to dev/target/host deps, and deferring workspace unification is a good call.

matklad · 2020-07-19T15:19:05Z

text/0000-cargo-features2.md

+[dev-dependencies](#dev-dependencies), the general rule is, if a dependency is
+not built, it does not affect feature resolution. For [host
+dependencies](#host-dependencies), the general rule is that packages used for
+building (like proc-macros) do not affect the packages being built.


Might be a good idea again to explicitly mention that workspace feature unification is deffered to future work, it took me a couple of reads to find out if this is in scope for the RFC.

text/0000-cargo-features2.md

djc · 2020-07-22T08:36:52Z

This looks great! Minor nit: I wonder about the choice to use a string for the resolver value in the manifest. Couldn't that be an integer instead? What ideas went into deciding it should be a string?

ehuss · 2020-07-22T13:38:26Z

@djc That is answered above here: #2957 (comment)

Lonami · 2020-07-22T15:45:34Z

@ehuss because multiple people have asked the same, maybe it should be written explicitly in the RFC under "Future possibilities":

By making the value a free-form string, future additions would be able to enable different options, even if this is not necessary at first.

pksunkara · 2020-09-04T23:48:01Z

text/0000-cargo-features2.md

+
+* Features for individual packages can be enabled by using
+  `member_name/feature_name` syntax. For example, `cargo build --workspace
+  --feature member_name/feature_name` will build all packages in a workspace,


I am assuming that we can do cargo build -p member1 -p member2 --features member1/foo. Would be a good idea expanding how usage in this item interacts with the usage in the above item.

I'm not sure what kind of expansion you are looking for. It enables the feature foo on the package member1. You can also experiment with -Zpackage-features on nightly to see how it behaves.

pksunkara · 2020-09-04T23:52:36Z

text/0000-cargo-features2.md

+Feedback on desired use cases for feature information will help define the
+solution. A possible alternative is to stabilize the [`--unit-graph`] flag,
+which exposes Cargo's internal graph structure, which accurately indicates the
+actual dependency relationships and uses the new feature resolver.


I am not sure how unit-graph's output looks like, but I want to say that cargo metadata and https://lib.rs/crates/cargo_metadata is used by a lot of projects in the ecosystem. I reckon we need to look into fixing this when this new resolver is implemented.

IIRC even rust-analyzer depends on it to understand the dependency graph. /cc @matklad

pksunkara · 2020-09-05T00:30:44Z

This looks awesome! Great work @ehuss.

I have a major concern that is not addressed in the RFC though. How will this interact with #2887 (artifact dependencies) or other future plans like that? I can kinda understand how the resolver works and can extrapolate the behaviour, but I would still like to see this specifically talked about.

ehuss · 2020-11-29T16:58:41Z

How will this interact with #2887 (artifact dependencies) or other future plans like that?

The RFC explicitly calls out the future concerns in the Future possibilities section, and in Drawbackes section, which mentions that future changes may require opting in to new behavior with the resolver setting.

I don't think it is feasible to design every potential enhancement that may come in the future and understand how they might interact. Some work along those lines has been done (see rust-lang/cargo#8799 and rust-lang/cargo#8818), but if this RFC is delayed until everything is considered, it will never be stabilized. The onus is on those future enhancements to consider how they integrate into the existing behaviors and environment.

ehuss · 2020-11-29T16:59:08Z

I've pushed some small updates to clarify some things. I'd like to nudge this along, so:

@rfcbot fcp merge

@rust-lang/cargo I believe this is in a state to be stabilized. The only open issue I am concerned about is rust-lang/cargo#8312 (I added a summary in the RFC), but there is no clear solution to me. I am uncertain how likely it is to cause problems (there haven't been any real-world reports so far). It may be possible that we can tweak the behavior later if it becomes evident that it is a real problem.

rfcbot · 2020-11-29T16:59:09Z

Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

Eh2406 · 2020-11-29T19:50:05Z

This is a clear improvement and the nightly implementation has been widely appreciated. So:
@rfcbot reviewed

At some point we should think if a future rewrite of the dependency resolver (pubgrub-rs) can become aware of and incorporate some of this logic. But the current feature resolver is working and will work with a rewrite, so there is no reason to hold up this on that.

rfcbot · 2020-11-29T19:50:12Z

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot · 2020-12-09T19:50:27Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

rust-lang/cargo#8857

ehuss · 2020-12-18T17:21:54Z

Thanks everyone for the comments! I have posted rust-lang/cargo#8997 to follow through with the stabilization.

Stabilize -Zfeatures and -Zpackage-features. This follows through with [RFC 2957](rust-lang/rfcs#2957) to stabilize the new feature resolver, and `-Zpackage-features` command-line changes. This also rewrites the "Features" chapter to try to expand it a little. I decided to leave the `-Zfeatures` flag in for now for testing, but it can be removed at a later date. There is a code change related to the `package-name/feature-name` syntax for the `--features` flag. I wanted to stabilize that for `resolver = "1"`, but I previously neglected to separate that behavior out, so it required change to `Workspace::members_with_features` to make that work (see the `resolver1_member_features` test). Closes #4328 Closes #5364 Closes #7914 Closes #7915 Closes #7916 Closes #8088 Closes #8431

Eh2406 reviewed Jul 17, 2020

View reviewed changes

jonas-schievink added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Jul 17, 2020

nagisa reviewed Jul 17, 2020

View reviewed changes

jonas-schievink approved these changes Jul 18, 2020

View reviewed changes

nagisa reviewed Jul 18, 2020

View reviewed changes

matklad reviewed Jul 19, 2020

View reviewed changes

Blisto91 mentioned this pull request Jul 19, 2020

WIP: Attempt to speed up CI by caching cargo dependencies linebender/druid#1086

Closed

matu3ba reviewed Jul 20, 2020

View reviewed changes

text/0000-cargo-features2.md Outdated Show resolved Hide resolved

jonas-schievink mentioned this pull request Jul 24, 2020

Can't specify features for individual crates in a virtual workspace rust-lang/rust-analyzer#5383

Closed

pksunkara reviewed Sep 4, 2020

View reviewed changes

rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Nov 29, 2020

rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Nov 29, 2020

rfcbot removed the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Nov 29, 2020

Blisto91 mentioned this pull request Dec 4, 2020

Autoselect Graphics Backend (no more --feature vulkan/metal/empty) amethyst/amethyst#2525

Merged

jplatte mentioned this pull request Dec 7, 2020

Building SQLx with -Z features fails launchbadge/sqlx#526

Closed

rfcbot added the finished-final-comment-period The final comment period is finished for this RFC. label Dec 9, 2020

rfcbot removed the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Dec 9, 2020

rfcbot added the to-announce label Dec 9, 2020

ehuss mentioned this pull request Dec 18, 2020

Stabilize -Zfeatures and -Zpackage-features. rust-lang/cargo#8997

Merged

ehuss added 8 commits December 18, 2020 09:16

Stabilize Cargo's new feature resolver

ecafa8c

Update for review feedback.

ffa4f33

"host compiler" -> "host platform"

5fa1791

Tweak wording to clarify.

e4dea70

Indicate that resolver="1" is now allowed.

b753ec1

rust-lang/cargo#8857

Clarify why resolver is a string.

1a81fbd

Add some notes about potentially surprising behaviors.

170bc93

Merge RFC 2957

66905a8

ehuss force-pushed the cargo-features2 branch from 219d2c2 to 66905a8 Compare December 18, 2020 17:21

ehuss merged commit 301da04 into rust-lang:master Dec 18, 2020

Progdrasil mentioned this pull request Apr 21, 2021

Seperate Types submodule into own crate rusqlite/rusqlite#941

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stabilize Cargo's new feature resolver #2957

Stabilize Cargo's new feature resolver #2957

ehuss commented Jul 16, 2020 •

edited

Loading

ehuss commented Jul 16, 2020

CryZe commented Jul 17, 2020 •

edited

Loading

ehuss commented Jul 17, 2020

Eh2406 left a comment

Eh2406 Jul 17, 2020

nagisa Jul 17, 2020

ehuss Jul 18, 2020

pksunkara Sep 4, 2020

joshtriplett Nov 30, 2020

jonas-schievink left a comment

jonas-schievink Jul 17, 2020

ehuss Jul 18, 2020

jonas-schievink Jul 17, 2020

ehuss Jul 18, 2020

nagisa Jul 18, 2020 •

edited

Loading

ehuss Jul 18, 2020

matklad left a comment

matklad Jul 19, 2020

djc commented Jul 22, 2020

ehuss commented Jul 22, 2020

Lonami commented Jul 22, 2020

pksunkara Sep 4, 2020

ehuss Nov 29, 2020

pksunkara Sep 4, 2020

pksunkara commented Sep 5, 2020 •

edited

Loading

ehuss commented Nov 29, 2020

ehuss commented Nov 29, 2020

rfcbot commented Nov 29, 2020 •

edited by joshtriplett

Loading

Eh2406 commented Nov 29, 2020

rfcbot commented Nov 29, 2020

rfcbot commented Dec 9, 2020

ehuss commented Dec 18, 2020

Stabilize Cargo's new feature resolver #2957

Stabilize Cargo's new feature resolver #2957

Conversation

ehuss commented Jul 16, 2020 • edited Loading

ehuss commented Jul 16, 2020

CryZe commented Jul 17, 2020 • edited Loading

ehuss commented Jul 17, 2020

Eh2406 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas-schievink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa Jul 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djc commented Jul 22, 2020

ehuss commented Jul 22, 2020

Lonami commented Jul 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pksunkara commented Sep 5, 2020 • edited Loading

ehuss commented Nov 29, 2020

ehuss commented Nov 29, 2020

rfcbot commented Nov 29, 2020 • edited by joshtriplett Loading

Eh2406 commented Nov 29, 2020

rfcbot commented Nov 29, 2020

rfcbot commented Dec 9, 2020

ehuss commented Dec 18, 2020

ehuss commented Jul 16, 2020 •

edited

Loading

CryZe commented Jul 17, 2020 •

edited

Loading

nagisa Jul 18, 2020 •

edited

Loading

pksunkara commented Sep 5, 2020 •

edited

Loading

rfcbot commented Nov 29, 2020 •

edited by joshtriplett

Loading