Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add -Zallow-features to match rustc's -Z #9283

Merged
merged 15 commits into from
Apr 2, 2021

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Mar 17, 2021

This PR implements the -Zallow-features permanently-unstable feature flag that explicitly enumerates which unstable features are allowed (assuming unstable features are permitted in the first place). This mirrors the -Zallow-features flag of rustc which serves the same purpose for rustc features:

https://github.com/rust-lang/rust/blob/5fe790e3c40710ecb95ddaadb98b59a3bb4f8326/compiler/rustc_session/src/options.rs#L856-L857

This flag makes it easier to beta-test unstable features "safely" by ensuring that only a single unstable feature is used if you only have control over build system, and not the source code that developers may end up using, as discussed in this internals thread.

@rust-highfive
Copy link

r? @Eh2406

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 17, 2021
@ehuss
Copy link
Contributor

ehuss commented Mar 23, 2021

@jonhoo I just wanted to double check, I didn't follow the discussion on internals too closely. Is this still the direction you want to go?

@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 23, 2021

Yes, I think this feature is going to be part of the eventual solution at least. I think I'd also want a way to enable this through an environment variable, but that's sort of secondary. For now, this is a way to get feature-parity with rustc for unstable features, and gives users a way to to opt into only a subset of unstable features for testing purposes.

@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 24, 2021

85f7c80 makes it so that -Zallow-features is also forwarded to rustc. This makes it much easier to enforce a list of allowed unstable features across cargo and rustc, though let me know if this is not how we want to support that.

cc @joshtriplett

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be worthwhile to add this to the fingerprint? If, for example, you build a project, and then remove a feature from allow-features, and then build again to see what breaks, nothing happens. The fingerprint code is in fingerprint.rs, and you can search for rustdoc_map for an example. It just needs to add the hash to the config value.

One downside is that it will rebuild everything, so I'm uncertain if that is worth it. WDYT?

src/cargo/core/features.rs Show resolved Hide resolved
src/cargo/core/compiler/compilation.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/compilation.rs Outdated Show resolved Hide resolved
src/cargo/core/features.rs Outdated Show resolved Hide resolved
src/cargo/core/features.rs Outdated Show resolved Hide resolved
src/cargo/core/features.rs Show resolved Hide resolved
tests/testsuite/cargo_features.rs Outdated Show resolved Hide resolved
tests/testsuite/cargo_features.rs Outdated Show resolved Hide resolved
Jon Gjengset added 10 commits March 31, 2021 12:33
If this weren't the case, a user that compiled, and then re-ran cargo
with a particular set of allowed features would not see any breakage
even if other unstable features were used.

The downside of this is that changing allow-features causes a full
re-compile, but that still _seems_ like the right thing to do.
@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 31, 2021

Err, I'm not sure what's causing the CI error — it seems unrelated to this change?

@ehuss ehuss added the T-cargo Team: Cargo label Apr 1, 2021
@ehuss
Copy link
Contributor

ehuss commented Apr 1, 2021

Looks good, thanks!

I just want to double check with the team that they are OK with this change.

@rust-lang/cargo This adds the -Zallow-features flag which matches the same in rustc. The docs should explain exactly how it works.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 1, 2021

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.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Apr 1, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 2, 2021

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

@rfcbot rfcbot added the final-comment-period FCP — a period for last comments before action is taken label Apr 2, 2021
@rfcbot rfcbot removed the proposed-final-comment-period An FCP proposal has started, but not yet signed off. label Apr 2, 2021
@alexcrichton
Copy link
Member

@bors: r=ehuss

@bors
Copy link
Contributor

bors commented Apr 2, 2021

📌 Commit 8d79a75 has been approved by ehuss

@bors 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 2, 2021
@bors
Copy link
Contributor

bors commented Apr 2, 2021

⌛ Testing commit 8d79a75 with merge 10ef854...

@bors
Copy link
Contributor

bors commented Apr 2, 2021

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 10ef854 to master...

@bors bors merged commit 10ef854 into rust-lang:master Apr 2, 2021
@jonhoo jonhoo deleted the z-allowed-features branch April 2, 2021 16:13
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 6, 2021
Update cargo

3 commits in 3c44c3c4b7900b8b13c85ead25ccaa8abb7d8989..65d57e6f384c2317f76626eac116f683e2b63665
2021-03-31 21:21:15 +0000 to 2021-04-04 15:07:52 +0000
- Fix typo in contrib docs. (rust-lang/cargo#9328)
- fix clippy warnings (rust-lang/cargo#9323)
- Add -Zallow-features to match rustc's -Z (rust-lang/cargo#9283)
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 6, 2021
Update cargo

3 commits in 3c44c3c4b7900b8b13c85ead25ccaa8abb7d8989..65d57e6f384c2317f76626eac116f683e2b63665
2021-03-31 21:21:15 +0000 to 2021-04-04 15:07:52 +0000
- Fix typo in contrib docs. (rust-lang/cargo#9328)
- fix clippy warnings (rust-lang/cargo#9323)
- Add -Zallow-features to match rustc's -Z (rust-lang/cargo#9283)
@rfcbot rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Apr 12, 2021
@ehuss ehuss added this to the 1.53.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants