Auto merge of #5353 - matklad:features, r=alexcrichton

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`.

r? @alexcrichton

Author: bors

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),
         }
 

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();
 

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),
+    );
+}