-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Conversation
r? @matklad (rust_highfive has picked a reviewer for you, use r? to override) |
cc @rust-lang/cargo |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)] |
There was a problem hiding this comment.
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super great comment! 💯 💯 💯 👍
src/cargo/core/features.rs
Outdated
} | ||
|
||
pub fn require(&self, feature: &Feature) -> CargoResult<()> { | ||
if *(feature.get)(self) { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
cc @carols10cents. I hope you'll reach consensus about crates.io side of the things :) |
7ff2fbb
to
88b2f42
Compare
88b2f42
to
f26fc37
Compare
Addressed review comments |
@bors r+ |
📌 Commit f26fc37 has been approved by |
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
☀️ Test successful - status-appveyor, status-travis |
Yay this landed!! @aturon sorry I never found time to patch in with the planning of this. |
@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 What do you think about moving |
@matklad seems reasonable to me! |
Ok, then are we bound by some backwards compat concerns here? This shipped 24 days ago, have it hit beta? |
Nah nothing AFAIK is using this yet, so we can change it as we like |
This PR starts adding infrastructure in Cargo for nightly features and nightly flags. The current design looks like:
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.-Z
, like the compiler, are accepted on the command line for all commands.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