New namespaced features implementation. #8799

ehuss · 2020-10-20T22:57:09Z

This is a new implementation for namespaced features (#5565). See the unstable.md docs for a description of the new behavior. This is intended to fix several issues with the existing design, and to make it backwards compatible so that it does not require an opt-in flag.

This also includes tangentially-related changes to the new feature resolver. The changes are:

crate_name/feat_name syntax will now always enable the feature crate_name, even if it is an inactive optional dependency (such as a dependency for another platform). The intent here is to have a certain amount of consistency whereby "features" are always activated, but individual crates will still not be activated.
--all-features will now enable features for inactive optional dependencies. This is more consistent with --features foo enabling the foo feature, even when the foo dep is not activated.

I'm still very conflicted on how that should work, but I think it is better from a simplicity/consistency perspective. I still think it may be confusing if someone has a cfg(some_dep) in their code, and some_dep isn't built, it will error. The intent is that cfg(accessible(some_dep)) will be the preferred method in the future, or using platform cfg expression like cfg(windows) to match whatever is in Cargo.toml.

Closes #8044
Closes #8046
Closes #8047
Closes #8316

Questions

For various reasons, I changed the way dependency conflict errors are collected. One slightly negative consequence is that it will raise an error for the first problem it detects (like a "missing feature"). Previously it would collect a list of all missing features and display all of them in the error message. Let me know if I should retain the old behavior. I think it will make the code more complicated and brittle, but it shouldn't be too hard (essentially Requirements will need to collect a list of errors, and then resolve_features would need to check if the list is non-empty, and then aggregate the errors).
Should cargo metadata show the implicit features in the "features" table? Currently it does not, and I think that is probably best (it mirrors what is in Cargo.toml), but I could potentially see an argument to show how cargo sees the implicit features.

rust-highfive · 2020-10-20T22:57:13Z

r? @Eh2406

(rust_highfive has picked a reviewer for you, use r? to override)

Eh2406 · 2020-10-21T01:38:46Z

This was clearly a lot of work. Thank you!

pksunkara

Great work! 🎉

Just one suggestion.

pksunkara · 2020-10-22T09:01:42Z

src/doc/src/reference/unstable.md

-a feature of the same name as a dependency is defined, that feature must
-include the dependency as a requirement, as `foo = ["crate:foo"]`.
+[features]
+serde = ["crate:serde", "bigdecimal/serde", "chrono/serde", "num-bigint/serde"]


As per what you mentioned in the test crate_feature_with_explicit, I think it's a little bit ambiguous on what happens if I define a feature bigdecimal = []. Can we disallow bigdecimal/serde in that case and complain about it? And then we can suggest users to use crate:bigdecimal/serde.

I'm not entirely clear about what is ambiguous. bigdecimal = [] defines a feature named bigdecimal. If you specify bigdecimal/serde, it enables the bigdecimal crate (if it is optional), enables the serde feature on bigdecimal, and enables the bigdecimal feature. I don't think we can disallow that since that is the current behavior.

alexcrichton

This is looking great, thanks again for working on this! I've left a few nits here and there but otherwise I think this is basically ready to go

alexcrichton · 2020-10-23T17:24:30Z

src/cargo/core/resolver/features.rs

-                            let fv = FeatureValue::Crate(*dep_name);
+                            let fv = FeatureValue::Crate {
+                                dep_name: *dep_name,
+                                // explicit: *explicit,


alexcrichton · 2020-10-23T17:31:54Z

src/cargo/core/summary.rs

+        dep_feature: InternedString,
+        /// If this is true, then the feature used the `crate:` prefix, which
+        /// prevents enabling the feature named `dep_name`.
+        explicit: bool,


The term explicit here seemed a bit confusing to me given this documentation of what it means, perhaps something like crate_prefix or something like that?

Sounds better, thanks!

alexcrichton · 2020-10-23T17:32:20Z

src/cargo/core/summary.rs

-            Crate(ref c) => {
-                if s.namespaced_features() {
-                    format!("crate:{}", &c)
+                let (dep, explicit) = if dep.starts_with("crate:") {


I think strip_prefix is a stable thing to use now?

alexcrichton · 2020-10-23T17:36:24Z

tests/testsuite/features_namespaced.rs

+    Package::new("baz", "1.0.0").publish();
+    Package::new("bar", "1.0.0")
+        .add_dep(Dependency::new("baz", "1.0").optional(true))
+        .feature("feat", &["crate:baz"])


As a reminder to us, crates.io probably rejects this syntax today in the frontend so we'll likely need to update that to publish packages like this.

Yep. I'm actually not sure how to roll this out, since we'll need to be pretty confident on the design since it affects the index. At what point do we decide to turn it on?

To avoid confusion with the...other thing called "features".

I'm not sure why the original code partitioned the errors in the way that it did. I think it is better to exhaustively match all the reasons, so that when new reasons are added, it will be clear that this code needs to be updated. I also think it simplifies the code a little.

ehuss · 2020-10-25T19:52:11Z

I'm moved by the argument from @est31 that crate: is dangerously close in syntax to crate:: in Rust, and they mean significantly different things. I'm concerned that crate: would perpetuate more misunderstanding about what the term "crate" means. I think it is an uphill battle that cannot be won, but I don't want to push things in the direction of more confusion. I can revert this if people don't agree.

I have pushed a commit that renames it to dep:. I decided on dep: instead of pkg: because the name is not necessarily the package name (it is the "name in toml"), and I think it more clearly emphasizes that it is referring to a dependency.

alexcrichton · 2020-10-27T19:58:29Z

@bors: r+

I think it's a good point that crate: can be easily confused, and I also agree that crate/pkg is best to not dive into and dep: feels good here to me!

I'm going to go ahead and r+ since this is all unstable anyway, and we can continue to tweak it. Otherwise what's here looks great, thanks @ehuss!

bors · 2020-10-27T19:58:30Z

📌 Commit f4ac82a has been approved by alexcrichton

bors · 2020-10-27T19:58:39Z

⌛ Testing commit f4ac82a with merge 963bfe4...

bors · 2020-10-27T20:25:46Z

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 963bfe4 to master...

djc · 2020-10-27T20:50:26Z

Thanks for fixing this stuff up, sorry I wasn't able to get to it sooner.

Fix publishing with optional dependencies. In #8799, I neglected to update the `publish` code to use the correct features when generating the JSON to upload to the registry. The `Cargo.toml` file was correctly updated, but the JSON was not. This caused Cargo to send the implicit `dep:` feature syntax in the JSON blob, which crates.io rejects. The solution here is to use the original feature map before the implicit features have been added.

Fix publishing with optional dependencies. In rust-lang#8799, I neglected to update the `publish` code to use the correct features when generating the JSON to upload to the registry. The `Cargo.toml` file was correctly updated, but the JSON was not. This caused Cargo to send the implicit `dep:` feature syntax in the JSON blob, which crates.io rejects. The solution here is to use the original feature map before the implicit features have been added.

rust-highfive assigned Eh2406 Oct 20, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 20, 2020

ehuss mentioned this pull request Oct 20, 2020

Tracking issue for namespaced-features #5565

Closed

1 task

ehuss force-pushed the new-namespaced branch from 269f0a3 to babc25f Compare October 20, 2020 23:40

ehuss force-pushed the new-namespaced branch from babc25f to 1fc0a16 Compare October 21, 2020 02:07

pksunkara approved these changes Oct 22, 2020

View reviewed changes

alexcrichton reviewed Oct 23, 2020

View reviewed changes

ehuss added 8 commits October 23, 2020 16:04

Move namespaced features tests to a separate file.

2611f5c

Rename features() to unstable_features().

36c69a1

To avoid confusion with the...other thing called "features".

New namespaced features implementation.

bcfdf9f

Expose implicit features in cargo metadata.

b2cceaf

Some minor cleanup.

1149f68

Minor cleanup.

c8a3db8

Rename "explicit" to "crate_prefix".

8ff130b

ehuss force-pushed the new-namespaced branch from f999a5e to 8ff130b Compare October 23, 2020 23:42

taiki-e mentioned this pull request Oct 24, 2020

Should --each-feature and --feature-powerset run --all-features? taiki-e/cargo-hack#42

Closed

Rename crate: to dep:

f4ac82a

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 27, 2020

bors merged commit 963bfe4 into rust-lang:master Oct 27, 2020

This was referenced Oct 27, 2020

Finish implementation of -Zextra-link-arg. #8441

Merged

Add member table to cargo config #8797

Closed

ehuss mentioned this pull request Oct 28, 2020

Update cargo rust-lang/rust#78493

Merged

ehuss mentioned this pull request Nov 12, 2020

Fix publishing with optional dependencies. #8853

Merged

ehuss mentioned this pull request Nov 29, 2020

Stabilize Cargo's new feature resolver rust-lang/rfcs#2957

Merged

ehuss added this to the 1.49.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New namespaced features implementation. #8799

New namespaced features implementation. #8799

ehuss commented Oct 20, 2020

rust-highfive commented Oct 20, 2020

Eh2406 commented Oct 21, 2020

pksunkara left a comment

pksunkara Oct 22, 2020 •

edited

Loading

ehuss Oct 23, 2020

alexcrichton left a comment

alexcrichton Oct 23, 2020

alexcrichton Oct 23, 2020

ehuss Oct 23, 2020

alexcrichton Oct 23, 2020

alexcrichton Oct 23, 2020

ehuss Oct 23, 2020

ehuss commented Oct 25, 2020

alexcrichton commented Oct 27, 2020

bors commented Oct 27, 2020

bors commented Oct 27, 2020

bors commented Oct 27, 2020

djc commented Oct 27, 2020

New namespaced features implementation. #8799

New namespaced features implementation. #8799

Conversation

ehuss commented Oct 20, 2020

Questions

rust-highfive commented Oct 20, 2020

Eh2406 commented Oct 21, 2020

pksunkara left a comment

Choose a reason for hiding this comment

pksunkara Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

ehuss Oct 23, 2020

Choose a reason for hiding this comment

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Oct 23, 2020

Choose a reason for hiding this comment

alexcrichton Oct 23, 2020

Choose a reason for hiding this comment

ehuss Oct 23, 2020

Choose a reason for hiding this comment

alexcrichton Oct 23, 2020

Choose a reason for hiding this comment

alexcrichton Oct 23, 2020

Choose a reason for hiding this comment

ehuss Oct 23, 2020

Choose a reason for hiding this comment

ehuss commented Oct 25, 2020

alexcrichton commented Oct 27, 2020

bors commented Oct 27, 2020

bors commented Oct 27, 2020

bors commented Oct 27, 2020

djc commented Oct 27, 2020

pksunkara Oct 22, 2020 •

edited

Loading