From 8d0b31b975e8dd168f71fa9a5d61100ac0b3ac74 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Fri, 13 Apr 2018 17:36:09 +0300 Subject: [PATCH] New semantics for `--features` flag Historically, feature-related flags like `--all-features`, `--no-default-features` and `--features` operated on the *current* package. That is, `cargo --package foo --feature feat` would activate `feat` for the package at the current working directory, and not for the `foo` package. `-Z package-features` flag implements the more obvious semantics for this combination of flags. This changes behavior, and that is why we want to start with an unstable opt-in. The changes are: * `--feature` flag affects the selected package. It is an error to specify `--feature` with more than a single `-p`, or with `-p` outside workspace (the latter could work in theory, but would be hard to implement). * `--all-features` and `--no-default-features` affect all selected packages, and not the one at cwd. * The package in `cwd` is not implicitly enabled when doing feature selection. That is, `cargo build -Z package-features -p foo` could select *less* features for various packages than `cargo build -p foo`. --- src/cargo/core/features.rs | 2 + src/cargo/ops/resolve.rs | 115 +++++++++++++++++++++++------------- tests/testsuite/features.rs | 82 +++++++++++++++++++++++++ 3 files changed, 158 insertions(+), 41 deletions(-) diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index c9658a8aaed..cac15214f53 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -292,6 +292,7 @@ pub struct CliUnstable { pub no_index_update: bool, pub avoid_dev_deps: bool, pub minimal_versions: bool, + pub package_features: bool, } impl CliUnstable { @@ -325,6 +326,7 @@ impl CliUnstable { "no-index-update" => self.no_index_update = true, "avoid-dev-deps" => self.avoid_dev_deps = true, "minimal-versions" => self.minimal_versions = true, + "package-features" => self.package_features = true, _ => bail!("unknown `-Z` flag specified: {}", k), } diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index 6d4e6d2dbf5..cf1af42e1a2 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -213,53 +213,86 @@ pub fn resolve_with_previous<'a, 'cfg>( registry.lock_patches(); } - let mut summaries = Vec::new(); + for member in ws.members() { registry.add_sources(&[member.package_id().source_id().clone()])?; - let method_to_resolve = match method { - // When everything for a workspace we want to be sure to resolve all - // members in the workspace, so propagate the `Method::Everything`. - Method::Everything => Method::Everything, - - // If we're not resolving everything though then we're constructing the - // exact crate graph we're going to build. Here we don't necessarily - // want to keep around all workspace crates as they may not all be - // built/tested. - // - // Additionally, the `method` specified represents command line - // flags, which really only matters for the current package - // (determined by the cwd). If other packages are specified (via - // `-p`) then the command line flags like features don't apply to - // them. - // - // As a result, if this `member` is the current member of the - // workspace, then we use `method` specified. Otherwise we use a - // base method with no features specified but using default features - // for any other packages specified with `-p`. - Method::Required { dev_deps, .. } => { - let base = Method::Required { - dev_deps, - features: &[], - all_features: false, - uses_default_features: true, - }; - let member_id = member.package_id(); - match ws.current_opt() { - Some(current) if member_id == current.package_id() => method, - _ => { - if specs.iter().any(|spec| spec.matches(member_id)) { - base - } else { - continue; - } + } + + let mut summaries = Vec::new(); + if ws.config().cli_unstable().package_features { + let mut members = Vec::new(); + match method { + Method::Everything => members.extend(ws.members()), + Method::Required { features, all_features, uses_default_features, .. } => { + if specs.len() > 1 && !features.is_empty() { + bail!("cannot specify features for more than one package"); + } + members.extend( + ws.members().filter(|m| specs.iter().any(|spec| spec.matches(m.package_id()))) + ); + // Edge case: running `cargo build -p foo`, where `foo` is not a member + // of current workspace. Add all packages from workspace to get `foo` + // into the resolution graph. + if members.is_empty() { + if !(features.is_empty() && !all_features && uses_default_features) { + bail!("cannot specify features for packages outside of workspace"); } + members.extend(ws.members()); } } - }; + } + for member in members { + let summary = registry.lock(member.summary().clone()); + summaries.push((summary, method)) + } + } else { + for member in ws.members() { + let method_to_resolve = match method { + // When everything for a workspace we want to be sure to resolve all + // members in the workspace, so propagate the `Method::Everything`. + Method::Everything => Method::Everything, + + // If we're not resolving everything though then we're constructing the + // exact crate graph we're going to build. Here we don't necessarily + // want to keep around all workspace crates as they may not all be + // built/tested. + // + // Additionally, the `method` specified represents command line + // flags, which really only matters for the current package + // (determined by the cwd). If other packages are specified (via + // `-p`) then the command line flags like features don't apply to + // them. + // + // As a result, if this `member` is the current member of the + // workspace, then we use `method` specified. Otherwise we use a + // base method with no features specified but using default features + // for any other packages specified with `-p`. + Method::Required { dev_deps, .. } => { + let base = Method::Required { + dev_deps, + features: &[], + all_features: false, + uses_default_features: true, + }; + let member_id = member.package_id(); + match ws.current_opt() { + Some(current) if member_id == current.package_id() => method, + _ => { + if specs.iter().any(|spec| spec.matches(member_id)) { + base + } else { + continue; + } + } + } + } + }; + + let summary = registry.lock(member.summary().clone()); + summaries.push((summary, method_to_resolve)); + } + }; - let summary = registry.lock(member.summary().clone()); - summaries.push((summary, method_to_resolve)); - } let root_replace = ws.root_replace(); diff --git a/tests/testsuite/features.rs b/tests/testsuite/features.rs index f877d3ea561..ffa741310e2 100644 --- a/tests/testsuite/features.rs +++ b/tests/testsuite/features.rs @@ -3,7 +3,9 @@ use std::io::prelude::*; use cargotest::support::paths::CargoPathExt; use cargotest::support::{execs, project}; +use cargotest::ChannelChanger; use hamcrest::assert_that; +use cargotest::support::registry::Package; #[test] fn invalid1() { @@ -1629,3 +1631,83 @@ fn many_cli_features_comma_and_space_delimited() { )), ); } + +#[test] +fn combining_features_and_package() { + Package::new("dep", "1.0.0").publish(); + + let p = project("ws") + .file( + "Cargo.toml", + r#" + [project] + name = "ws" + version = "0.0.1" + authors = [] + + [workspace] + members = ["foo"] + + [dependencies] + dep = "1" + "#, + ) + .file("src/lib.rs", "") + .file( + "foo/Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + authors = [] + [features] + main = [] + "#, + ) + .file("foo/src/main.rs", r#" + #[cfg(feature = "main")] + fn main() {} + "#) + .build(); + + assert_that( + p.cargo("build -Z package-features --all --features main") + .masquerade_as_nightly_cargo(), + execs().with_status(101).with_stderr_contains("\ +[ERROR] cannot specify features for more than one package" + ), + ); + + assert_that( + p.cargo("build -Z package-features --package dep --features main") + .masquerade_as_nightly_cargo(), + execs().with_status(101).with_stderr_contains("\ +[ERROR] cannot specify features for packages outside of workspace" + ), + ); + assert_that( + p.cargo("build -Z package-features --package dep --all-features") + .masquerade_as_nightly_cargo(), + execs().with_status(101).with_stderr_contains("\ +[ERROR] cannot specify features for packages outside of workspace" + ), + ); + assert_that( + p.cargo("build -Z package-features --package dep --no-default-features") + .masquerade_as_nightly_cargo(), + execs().with_status(101).with_stderr_contains("\ +[ERROR] cannot specify features for packages outside of workspace" + ), + ); + + assert_that( + p.cargo("build -Z package-features --all --all-features") + .masquerade_as_nightly_cargo(), + execs().with_status(0), + ); + assert_that( + p.cargo("run -Z package-features --package foo --features main") + .masquerade_as_nightly_cargo(), + execs().with_status(0), + ); +}