Introduction of namespaced features (see #1286) #5300

djc · 2018-04-05T15:20:05Z

I think this basically covers all of the plans from #1286, although it still needs a bunch of tests and documentation updates. Submitting this PR to get some early feedback on the direction.

alexcrichton · 2018-04-06T19:51:22Z

Thanks! I'm have a bit of a hard time following this though, mind adding some comments on the various bail! statements in the updated feature resolution loop?

Additionally, to recap, the idea here is that packages with this flag only activate optional dependencies through crate:foo, right?

djc · 2018-04-16T07:36:32Z

@alexcrichton I've added a whole bunch of comments.

To recap: the idea here is that features live in a separate namespace from dependencies. As such, when specifying dependencies in a feature value list (in the features table), you have to prefix dependencies with crate:. However, even after this PR, you do not have to specify a feature explicitly if you just have an optional dependency: for optional dependencies where no feature is explicitly defined, an implicit feature will be created -- this should help avoid boilerplate. Then again, if you do specify a feature of the same name as an optional dependency, it must include the crate:feature dependency.

Hope that makes sense.

alexcrichton

Ok thanks so much for the comments, they're definitely quite helpful! I think this is definitely the right direction to go in, and with some tests this is likely good to go!

alexcrichton · 2018-04-17T15:54:12Z

src/cargo/core/summary.rs

                    dep
                ),
-                (&CrateFeature(_, _), Some(_)) | (&Feature(_), _) => {}
+                (&CrateFeature(_, _), _, _) => {}


Could this explicitly list (&CrateFeature(_, _), true, _) for exhaustiveness?

bors · 2018-04-17T16:24:43Z

☔ The latest upstream changes (presumably #5374) made this pull request unmergeable. Please resolve the merge conflicts.

djc · 2018-04-18T11:46:30Z

Added a number of test cases that also did a good job flushing a number of logic issues out of the implementation. What is required in terms of documentation?

One thing to consider: should namespaced-features default to true for Edition2018?

alexcrichton · 2018-04-18T15:18:17Z

Great, thanks! Mind adding documentation to unstable.md in the src/doc directory about this feature?

I think we've still got some more design work to do on this before enabling it by default. Features enabled in 2018 must be warned against if they're incompatible with the previous edition for one thing, and we've also got the registry to worry about as we'll need to communicate this information to the registry and older versions of Cargo as well (as features affect dependency resolution).

It's fine to punt those questions to later, though, as this is primarily just a new nightly feature that's "in the works"

alexcrichton · 2018-04-18T15:21:18Z

src/cargo/core/summary.rs

            }
            values.push(val);
        }
+
+        if !dependency_found {


I keep having a tough time following this dependency_found logic, but is there a reason it needs to span across the match statement above? Could this error be moved up before the match?

I added a whole bunch of comments, does that help? It spans across the for-loop (which includes the match) as a performance optimization: this way, we don't have to loop over the feature values twice. That's entirely premature (in the sense of not having done any performance measurements), so if the comments don't help enough, I could instead rewrite to check the values list after the loop.

bors · 2018-04-21T16:33:23Z

☔ The latest upstream changes (presumably #5335) made this pull request unmergeable. Please resolve the merge conflicts.

djc · 2018-04-23T07:29:11Z

I've rebased on latest master and added some documentation to the unstable section.

alexcrichton · 2018-04-24T14:59:43Z

src/cargo/util/toml/mod.rs

@@ -763,6 +764,7 @@ impl TomlManifest {
            deps,
            me.features.clone().unwrap_or_else(BTreeMap::new),
            project.links.clone(),
+            project.namespaced_features.unwrap_or(false),


Ah I just remembered, can this be gated behind a feature as well? (setting this key at all)

Sure - not sure what kind of gate you had in mind?

Oh something like cargo-features = ["namespaced-features"] in Cargo.toml, you can see some other examples of this via src/cargo/core/features.rs I think

@alexcrichton in that case, do we still need the separate namespaced-features in the manifest? It seems like those would basically be duplicate values.

For now yeah we'll want that (the Cargo feature basically unlocks the namespaced-features config value).

Long term we may not have both but for the near-term I think we'll want that.

alexcrichton · 2018-04-24T15:00:45Z

Ok thanks for the new comments, they look great! I think this is good to go with a feature gate and fixed tests!

djc · 2018-04-25T08:30:02Z

Huh, those test failures got introduced by merging a conflict wrong. Fixed now, and rebased on current master. Just let me know what kind of feature gate you want and this can (finally!) be merged.

bors · 2018-04-27T23:49:15Z

☔ The latest upstream changes (presumably #5384) made this pull request unmergeable. Please resolve the merge conflicts.

For now, all Summaries from a registry have it set to false.

…t-lang#1286) Here's an attempt at a table to cover the different cases: Feature Old (must be in features table) Continue Namespaced (might be stray value) In features table: Check that Crate dependency is in the list -> Non-optional dependency: Bail [PREVIOUSLY: bailed for non-optional dependency] -> Optional dependency: Insert feature of this name -> Else: Bail [PREVIOUSLY: bailed for unknown dependency or feature] Crate Old (might be stray value) Non-optional dependency: Bail No dependency found: Bail Namespaced Non-optional dependency: Bail No dependency found: Bail CrateFeature Old No dependency found: Bail Namespaced No dependency found: Bail

djc · 2018-04-30T08:44:24Z

Rebased on master and added the feature gate. Hope we can merge this now!

alexcrichton · 2018-04-30T14:57:32Z

Hm looks like CI may be failing?

djc · 2018-04-30T17:22:36Z

Don't know how I managed to break that -- I had at least some tests passing before. Anyway, it works now.

alexcrichton · 2018-04-30T17:52:32Z

@bors: r+

bors · 2018-04-30T17:52:33Z

📌 Commit 0b6f420 has been approved by alexcrichton

bors · 2018-04-30T17:52:41Z

⌛ Testing commit 0b6f420 with merge 4963d88...

Introduction of namespaced features (see #1286) I think this basically covers all of the plans from #1286, although it still needs a bunch of tests and documentation updates. Submitting this PR to get some early feedback on the direction.

bors · 2018-04-30T18:19:44Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 4963d88 to master...

This fixes an accidental bug introduced in rust-lang#5300 by ensuring a local map keeps track of the fact that there can be multiple dependencies for one name Closes rust-lang#5453

Fix optional dependencies and required dev-deps This fixes an accidental bug introduced in #5300 by ensuring a local map keeps track of the fact that there can be multiple dependencies for one name Closes #5453

rust-highfive assigned alexcrichton Apr 5, 2018

djc force-pushed the namespaced-features branch from fe285e1 to 9dca40d Compare April 16, 2018 07:32

alexcrichton reviewed Apr 17, 2018

View reviewed changes

djc force-pushed the namespaced-features branch 3 times, most recently from 8c4caa5 to 8dc1761 Compare April 18, 2018 11:43

djc changed the title ~~[WIP] Introduction of namespaced features (see #1286)~~ Introduction of namespaced features (see #1286) Apr 18, 2018

alexcrichton reviewed Apr 18, 2018

View reviewed changes

djc force-pushed the namespaced-features branch from 8dc1761 to 2483ac6 Compare April 23, 2018 07:18

alexcrichton reviewed Apr 24, 2018

View reviewed changes

djc force-pushed the namespaced-features branch from 2483ac6 to 3719d31 Compare April 25, 2018 08:29

djc added 7 commits April 28, 2018 13:41

Simplify retrieval of dependency data

e357d17

Make a dependencies map while building feature map

4966f53

Add namespaced-features option for manifest [project] section

797d453

Keep track of namespaced-features flag in Summary objects

a9f163e

For now, all Summaries from a registry have it set to false.

Add tests for namespaced features

dc5d023

Add some documentation about unstable namespaced-features feature

f5a4282

djc force-pushed the namespaced-features branch from 3719d31 to dad55a3 Compare April 30, 2018 08:43

Put namespaced features behind a feature gate

0b6f420

djc force-pushed the namespaced-features branch from dad55a3 to 0b6f420 Compare April 30, 2018 15:33

bors merged commit 0b6f420 into rust-lang:master Apr 30, 2018

ehuss mentioned this pull request May 1, 2018

Can't build project with serde with latest cargo #5453

Closed

alexcrichton mentioned this pull request May 2, 2018

Fix optional dependencies and required dev-deps #5454

Merged

tomaka mentioned this pull request Jan 2, 2020

Support for wasm32-unknown-unknown async-rs/futures-timer#51

Merged

djc mentioned this pull request Mar 27, 2020

Tracking issue for namespaced-features #5565

Closed

1 task

ehuss added this to the 1.27.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduction of namespaced features (see #1286) #5300

Introduction of namespaced features (see #1286) #5300

djc commented Apr 5, 2018

alexcrichton commented Apr 6, 2018

djc commented Apr 16, 2018

alexcrichton left a comment

alexcrichton Apr 17, 2018

djc Apr 18, 2018

bors commented Apr 17, 2018

djc commented Apr 18, 2018

alexcrichton commented Apr 18, 2018

alexcrichton Apr 18, 2018

djc Apr 23, 2018

bors commented Apr 21, 2018

djc commented Apr 23, 2018

alexcrichton Apr 24, 2018

djc Apr 24, 2018

alexcrichton Apr 25, 2018

djc Apr 25, 2018

alexcrichton Apr 25, 2018

alexcrichton commented Apr 24, 2018

djc commented Apr 25, 2018

bors commented Apr 27, 2018

djc commented Apr 30, 2018

alexcrichton commented Apr 30, 2018

djc commented Apr 30, 2018

alexcrichton commented Apr 30, 2018

bors commented Apr 30, 2018

bors commented Apr 30, 2018

bors commented Apr 30, 2018

Introduction of namespaced features (see #1286) #5300

Introduction of namespaced features (see #1286) #5300

Conversation

djc commented Apr 5, 2018

alexcrichton commented Apr 6, 2018

djc commented Apr 16, 2018

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Apr 17, 2018

djc commented Apr 18, 2018

alexcrichton commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Apr 21, 2018

djc commented Apr 23, 2018

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

alexcrichton commented Apr 24, 2018

djc commented Apr 25, 2018

bors commented Apr 27, 2018

djc commented Apr 30, 2018

alexcrichton commented Apr 30, 2018

djc commented Apr 30, 2018

alexcrichton commented Apr 30, 2018

bors commented Apr 30, 2018

bors commented Apr 30, 2018

bors commented Apr 30, 2018