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 infrastructure for nightly features and flags #4433

Merged
merged 2 commits into from
Aug 25, 2017

Conversation

alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Aug 24, 2017

This PR starts adding infrastructure in Cargo for nightly features and nightly flags. The current design looks like:

  • There's a new package.cargo-features manifest key which accepts an array of strings. This array of strings is the list of enabled Cargo features for that crate.
  • A new suite of flags behind -Z, like the compiler, are accepted on the command line for all commands.
  • Features and unstable flags in Cargo are required to be used on Cargo's nightly channel, which is the same as Rust's nightly channel.
  • Features and unstable flags cannot be used on the stable/beta channels of Rust/Cargo.
  • Crates which enable features in their manifest are disallowed from being published to crates.io

The motivation behind this support is unblock a number of efforts in Cargo by allowing them to safely get implemented behind a nightly feature gate. Once behind a feature gate they can iterate in-tree without having to worry about "insta stability" and we can also get valuable usage feedback about upstream users.

Closes #4409

@rust-highfive
Copy link

r? @matklad

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

@alexcrichton
Copy link
Member Author

cc @rust-lang/cargo

Copy link
Member

@matklad matklad left a comment

Choose a reason for hiding this comment

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

Looks good to me, only cosmetic comments!

@@ -72,6 +73,7 @@ Options:
--no-fail-fast Run all benchmarks regardless of failure
--frozen Require Cargo.lock and cache are up to date
--locked Require Cargo.lock is up to date
-Z FLAG ... Unstable (nightly-only) flags to Cargo
Copy link
Member

Choose a reason for hiding this comment

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

It's possible that it's better to hide this from help, just to avoid having the unstable word in the help for the main Rust tool, but looks like this can be difficult to achieve with docopt, so we can just skip it altogether.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that's how I felt, and another use case for not using docopt eventually!

src/bin/cargo.rs Outdated
@@ -1,3 +1,5 @@
#![allow(non_snake_case)]
Copy link
Member

Choose a reason for hiding this comment

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

Hm, let's allow directly on flag_z fields perhaps? Alternatively, docopt uses serde now, so perhaps we can have flag_z and rename it to flag_Z on serde level?

//!
//! And hopefully that's it! Bear with us though that this is, at the time of
//! this writing, a very new feature in Cargo. If the process differs from this
//! we'll be sure to update this documentation!
Copy link
Member

Choose a reason for hiding this comment

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

Super great comment! 💯 💯 💯 👍

}

pub fn require(&self, feature: &Feature) -> CargoResult<()> {
if *(feature.get)(self) {
Copy link
Member

Choose a reason for hiding this comment

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

*(feature.get)(self) looks cryptic. Why do we return &bool? :)

I would try to write it like this probably:

if feature.is_enabled(self) {
    return Ok(())
}
// bail here

@@ -644,6 +648,9 @@ impl TomlManifest {
};
let profiles = build_profiles(&me.profile);
let publish = project.publish.unwrap_or(true);
let empty = Vec::new();
let cargo_features = project.cargo_features.as_ref().unwrap_or(&empty);
Copy link
Member

Choose a reason for hiding this comment

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

Would unwrap_or(&[]) work here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh that just required munging around with Option<&Vec<T>> -> Option<&[T]> which was just a pain.

}

#[test]
fn nightly_feature_requires_nightly() {
Copy link
Member

@matklad matklad Aug 24, 2017

Choose a reason for hiding this comment

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

Let's add a similar test, when a feature is activated in a dependency, and not in in the current crate. We should reject unstable dependencies for stable crates.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah right yeah meant to add a test for that

@matklad
Copy link
Member

matklad commented Aug 24, 2017

cc @carols10cents. I hope you'll reach consensus about crates.io side of the things :)

@alexcrichton alexcrichton force-pushed the unstable-features branch 2 times, most recently from 7ff2fbb to 88b2f42 Compare August 25, 2017 15:58
@alexcrichton
Copy link
Member Author

Addressed review comments

@matklad
Copy link
Member

matklad commented Aug 25, 2017

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 25, 2017

📌 Commit f26fc37 has been approved by matklad

@bors
Copy link
Collaborator

bors commented Aug 25, 2017

⌛ Testing commit f26fc37 with merge 3da1443...

bors added a commit that referenced this pull request Aug 25, 2017
Add infrastructure for nightly features and flags

This PR starts adding infrastructure in Cargo for nightly features and nightly flags. The current design looks like:

* There's a new `package.cargo-features` manifest key which accepts an array of strings. This array of strings is the list of enabled Cargo features for that crate.
* A new suite of flags behind `-Z`, like the compiler, are accepted on the command line for all commands.
* Features and unstable flags in Cargo are required to be used on Cargo's nightly channel, which is the same as Rust's nightly channel.
* Features and unstable flags cannot be used on the stable/beta channels of Rust/Cargo.
* Crates which enable features in their manifest are disallowed from being published to crates.io

The motivation behind this support is unblock a number of efforts in Cargo by allowing them to safely get implemented behind a nightly feature gate. Once behind a feature gate they can iterate in-tree without having to worry about "insta stability" and we can also get valuable usage feedback about upstream users.

Closes #4409
@bors
Copy link
Collaborator

bors commented Aug 25, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing 3da1443 to master...

@bors bors merged commit f26fc37 into rust-lang:master Aug 25, 2017
@alexcrichton alexcrichton deleted the unstable-features branch August 25, 2017 20:16
@Ericson2314
Copy link
Contributor

Yay this landed!! @aturon sorry I never found time to patch in with the planning of this.

@matklad
Copy link
Member

matklad commented Sep 17, 2017

@alexcrichton I've tried to use nightly features to experiment with "optimized dependencies", but faced a problem: virtual manifests do not support cargo features, and it's not trivial to add support for them, because currently cargo-features live under [project] section, and virtual manifest lacks one.

What do you think about moving cargo-features to the top-level?

@alexcrichton
Copy link
Member Author

@matklad seems reasonable to me!

@matklad
Copy link
Member

matklad commented Sep 17, 2017

Ok, then are we bound by some backwards compat concerns here? This shipped 24 days ago, have it hit beta?

@alexcrichton
Copy link
Member Author

Nah nothing AFAIK is using this yet, so we can change it as we like

@ehuss ehuss added this to the 1.21.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support nightly cargo features
6 participants