Extend -Zpackage-features with more capabilities. #8074

ehuss · 2020-04-05T21:23:05Z

This is a proposal to extend -Zpackage-features with new abilities to change how features are selected on the command-line. See unstable.md for documentation on what it does.

I've contemplated a variety of ways we could transition this to stable. I tried a few experiments trying to make a "transition with warnings" mode, but I'm just too concerned about breaking backwards compatibility. The current way is just fundamentally different from the new way, and I think it would be a bumpy ride to try to push it.

The stabilization story is that the parts of this that add new functionality (feature flags in virtual worskpaces, and member/feat syntax) can be stabilized at any time. The change for cargo build -p member --features feat in a different member's directory can maybe be part of -Zfeatures stabilization, which will need to be opt-in. I've been trying to come up with some transition plan, and I can't think of a way without making it opt-in, and making it part of -Zfeatures is an opportunity to simplify things. One concern is that this might be confusing (--features flag could behave differently in different workspaces, and documenting the differences), but that seems hard to avoid.

Closes #6195
Closes #4753
Closes #5015
Closes #4106
Closes #5362

rust-highfive · 2020-04-05T21:23:09Z

r? @Eh2406

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

alexcrichton

Nice!

So as we go further down this path I'm thinking more about the roll-out story for this. What would you think of removing all the various -Z flags we have today and instead roll everything into something like -Zfeatures2 for a "features 2.0 feature". Or, perhaps better yet, move it into the manifest as a cargo-features-gated field in [package]?

I'm basically curious if we can move closer to "all we need to do is flip a switch" territory with something we're comfortable rolling out. I'm worried now that to understand the full story of how we envision enabling all this you've gotta understand a lot of various -Z flags and how they interact

alexcrichton · 2020-04-06T15:03:45Z

src/doc/src/reference/unstable.md

+ enables matching features. It is still an error if none of the packages
+ define a given feature.
+* `--features` and `--no-default-features` are now allowed in the root of a
+ virtual workspace.


To confirm, these flags, when applied in this scenario, would be applied to all workspace members, right?

alexcrichton · 2020-04-06T15:04:27Z

src/cargo/core/features.rs

@@ -330,6 +330,7 @@ pub struct CliUnstable {
 pub avoid_dev_deps: bool,
 pub minimal_versions: bool,
 pub package_features: bool,
+ pub package_features2: bool,


Are you worried about breakage if we change the existing unstable feature package_features? I'd be ok with simply updating that since we're more likely to stabilize this than that.

I'm fine with merging them (will do that soonish). I wanted to get feedback first, and keeping them separate was a way to highlight how they differ.

alexcrichton · 2020-04-06T16:17:32Z

Oh I should also mention that these proposed semantics for the -p and --features flags sound great to me 👍

ehuss · 2020-04-06T16:19:08Z

"all we need to do is flip a switch"

Yea, that's the eventual plan. I could accelerate that, maybe after merging this? I was keeping them separate because each option had different implementation concerns and trade-offs.

I think we discussed earlier of making it a flag in the [workspace] table, since feature resolution is done workspace-wide. The problem with [package] is that it would be ignored in a workspace (can't "unify" for a single package), and there needs to be somewhere to specify it in a virtual manifest.

However, [workspace] isn't exactly ideal. For example, Cargo itself wouldn't be able to use it because of the lack of nested workspaces in rust-lang/rust. I think it would also be interesting if some point in the future cargo new defaulted to the new mode, but being part of [workspace] makes that more complicated. Every new project would have additional [workspace] boilerplate, and cargo new would need to check if it is already in a workspace.

Those concerns may not be dealbreakers. I think we should sooner or later support nested workspaces. And I think cargo new already checks for workspaces, though it is a little awkward.

alexcrichton · 2020-04-06T20:20:36Z

Oh sure yeah no need to conflate it here. I'm largely just worried that it's difficult to get real-world impact data/testing given the changing set of flags over time, but hey that's to be expected while things are worked on :)

Perhaps we should start a dedicated issue for how to roll out the new features features?

ehuss · 2020-04-08T21:11:47Z

I pushed an update removing the new -Z flag and just using the old one.

I also added a comment on the old tracking issue to try to solicit feedback.

Filed #8088 for a meta-tracking issue.

alexcrichton · 2020-04-09T15:41:29Z

@bors: r+

👍

bors · 2020-04-09T15:41:31Z

📌 Commit 54ace8a has been approved by alexcrichton

bors · 2020-04-09T15:43:12Z

⌛ Testing commit 54ace8a with merge 3a976c1...

bors · 2020-04-09T16:00:49Z

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 3a976c1 to master...

jethrogb · 2020-04-09T16:05:14Z

Did you really mean to close all of these? #6195 #4753 #5015 #4106 #5362

ehuss · 2020-04-09T16:19:51Z

Yes. This is the solution to all of them, unless you see something that is not covered?

jethrogb · 2020-04-09T16:21:01Z

I might be mistaken, but I believe normally issues are not closed until the feature is stabilized?

ehuss · 2020-04-09T16:25:37Z

Normally the issues are closed and a tracking issue is opened for stabilization (#5364 in this case). (In the case of an RFC, we reuse the original RFC tracking issue.) When it is stabilized, #5364 will be closed.

Update cargo 12 commits in 390e8f245ef2cd7ac698b8a76abf029f9abcab0d..74e3a7d5b756d7c0e94399fc29fcd154e792c22a 2020-04-07 17:46:45 +0000 to 2020-04-13 20:41:52 +0000 - Update dependencies to support illumos target (rust-lang/cargo#8093) - Whitelist another known spurious curl error (rust-lang/cargo#8102) - Fix nightly test matching rustc "warning" output. (rust-lang/cargo#8098) - Update default for codegen-units. (rust-lang/cargo#8096) - Fix freshness when linking is interrupted. (rust-lang/cargo#8087) - Add `cargo tree` command. (rust-lang/cargo#8062) - Add "build-finished" JSON message. (rust-lang/cargo#8069) - Extend -Zpackage-features with more capabilities. (rust-lang/cargo#8074) - Disallow invalid dependency names through crate renaming (rust-lang/cargo#8090) - Use the same filename hash for pre-release channels. (rust-lang/cargo#8073) - Index the commands section (rust-lang/cargo#8081) - Upgrade to mdBook v0.3.7 (rust-lang/cargo#8083)

Implement -Zpackage-features2

c17bfcb

rust-highfive assigned Eh2406 Apr 5, 2020

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

alexcrichton reviewed Apr 6, 2020

View reviewed changes

Merge -Zpackage-features2 and -Zpackage-features.

54ace8a

ehuss changed the title ~~Implement -Zpackage-features2~~ Implement -Zpackage-features Apr 8, 2020

ehuss changed the title ~~Implement -Zpackage-features~~ Extend -Zpackage-features with more capabilities. Apr 8, 2020

ehuss mentioned this pull request Apr 8, 2020

New behavior of --feature + --package combination #5364

Closed

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 Apr 9, 2020

bors merged commit 3a976c1 into rust-lang:master Apr 9, 2020

ehuss mentioned this pull request Apr 14, 2020

Update cargo rust-lang/rust#71138

Merged

foresterre mentioned this pull request May 9, 2020

imageproc support: include feature/dependency for a the sic_image_crate package foresterre/sic#395

Closed

ehuss added this to the 1.44.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend -Zpackage-features with more capabilities. #8074

Extend -Zpackage-features with more capabilities. #8074

ehuss commented Apr 5, 2020 •

edited

Loading

rust-highfive commented Apr 5, 2020

alexcrichton left a comment

alexcrichton Apr 6, 2020

ehuss Apr 6, 2020

alexcrichton Apr 6, 2020

ehuss Apr 6, 2020

alexcrichton commented Apr 6, 2020

ehuss commented Apr 6, 2020

alexcrichton commented Apr 6, 2020

ehuss commented Apr 8, 2020

alexcrichton commented Apr 9, 2020

bors commented Apr 9, 2020

bors commented Apr 9, 2020

bors commented Apr 9, 2020

jethrogb commented Apr 9, 2020

ehuss commented Apr 9, 2020

jethrogb commented Apr 9, 2020

ehuss commented Apr 9, 2020

Extend -Zpackage-features with more capabilities. #8074

Extend -Zpackage-features with more capabilities. #8074

Conversation

ehuss commented Apr 5, 2020 • edited Loading

rust-highfive commented Apr 5, 2020

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Apr 6, 2020

Choose a reason for hiding this comment

ehuss Apr 6, 2020

Choose a reason for hiding this comment

alexcrichton Apr 6, 2020

Choose a reason for hiding this comment

ehuss Apr 6, 2020

Choose a reason for hiding this comment

alexcrichton commented Apr 6, 2020

ehuss commented Apr 6, 2020

alexcrichton commented Apr 6, 2020

ehuss commented Apr 8, 2020

alexcrichton commented Apr 9, 2020

bors commented Apr 9, 2020

bors commented Apr 9, 2020

bors commented Apr 9, 2020

jethrogb commented Apr 9, 2020

ehuss commented Apr 9, 2020

jethrogb commented Apr 9, 2020

ehuss commented Apr 9, 2020

ehuss commented Apr 5, 2020 •

edited

Loading