Auto merge of #5430 - matklad:bring-old-features-back, r=alexcrichton

Revert "Enable new behavior of `--feature`"

This reverts commit 038eec5.

As discussed at #5364, the new behavior unfortunately causes real-life breakage, so we have to revert it.

This is kinda sad, this is a part of the larger issue with feature selection, which, at the moment, has a behavior which I would classify (loosely speaking) as unsound:

* `cargo build -p foo` and `cargo build -p foo -p bar` might produce different artifacts for `foo` ([repro](https://github.com/matklad/workspace-vs-feaures))
* `cargo build -p foo` might produce different artifacts, depending on cwd ([repro](https://github.com/matklad/features-cwd))

The new feature behavior specifically addressed the second point.

It is unclear what we could do with this... One option, instead of flatly erroring out, as the revreted commit does, is to print a warning, but change the set of activated features. It will still be a breaking change, but it at least has  a chance of working by accident.

r? @alexcrichton

Author: bors

src/cargo/core/features.rs

@@ -299,6 +299,7 @@ pub struct CliUnstable {
     pub no_index_update: bool,
     pub avoid_dev_deps: bool,
     pub minimal_versions: bool,
+    pub package_features: bool,
 }
 
 impl CliUnstable {
@@ -332,6 +333,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/core/resolver/types.rs

@@ -155,7 +155,7 @@ impl<'a> RegistryQueryer<'a> {
     }
 }
 
-#[derive(Clone, Copy, Eq, PartialEq)]
+#[derive(Clone, Copy)]
 pub enum Method<'a> {
     Everything, // equivalent to Required { dev_deps: true, all_features: true, .. }
     Required {

src/cargo/core/workspace.rs

@@ -118,7 +118,6 @@ pub struct WorkspaceRootConfig {
 
 /// An iterator over the member packages of a workspace, returned by
 /// `Workspace::members`
-#[derive(Clone)]
 pub struct Members<'a, 'cfg: 'a> {
     ws: &'a Workspace<'cfg>,
     iter: slice::Iter<'a, PathBuf>,

src/cargo/ops/resolve.rs

@@ -217,43 +217,86 @@ pub fn resolve_with_previous<'a, 'cfg>(
         registry.add_sources(&[member.package_id().source_id().clone()])?;
     }
 
-    let method = match method {
-        Method::Everything => Method::Everything,
-        Method::Required {
-            features,
-            all_features,
-            uses_default_features,
-            ..
-        } => {
-            if specs.len() > 1 && !features.is_empty() {
-                bail!("cannot specify features for more than one package");
-            }
-            let members_requested = ws.members()
-                .filter(|m| specs.iter().any(|spec| spec.matches(m.package_id())))
-                .count();
-            if members_requested == 0 {
+    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. Resolve whole workspace to get `foo` into the
-                // resolution graph.
-                if !(features.is_empty() && !all_features && uses_default_features) {
-                    bail!("cannot specify features for packages outside of workspace");
+                // 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());
                 }
-                Method::Everything
-            } else {
-                method
             }
         }
-    };
+        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 summaries = ws.members()
-        .filter(|m| {
-            method == Method::Everything || specs.iter().any(|spec| spec.matches(m.package_id()))
-        })
-        .map(|member| {
             let summary = registry.lock(member.summary().clone());
-            (summary, method)
-        })
-        .collect::<Vec<_>>();
+            summaries.push((summary, method_to_resolve));
+        }
+    };
 
     let root_replace = ws.root_replace();
 

tests/testsuite/features.rs

@@ -1674,42 +1674,46 @@ fn combining_features_and_package() {
         .build();
 
     assert_that(
-        p.cargo("build --all --features main")
+        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"),
+        execs().with_status(101).with_stderr_contains(
+            "\
+             [ERROR] cannot specify features for more than one package",
+        ),
     );
 
     assert_that(
-        p.cargo("build --package dep --features main")
+        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",
+            "\
+             [ERROR] cannot specify features for packages outside of workspace",
         ),
     );
     assert_that(
-        p.cargo("build --package dep --all-features")
+        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",
+            "\
+             [ERROR] cannot specify features for packages outside of workspace",
         ),
     );
     assert_that(
-        p.cargo("build --package dep --no-default-features")
+        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",
+            "\
+             [ERROR] cannot specify features for packages outside of workspace",
         ),
     );
 
     assert_that(
-        p.cargo("build --all --all-features")
+        p.cargo("build -Z package-features --all --all-features")
             .masquerade_as_nightly_cargo(),
         execs().with_status(0),
     );
     assert_that(
-        p.cargo("run --package foo --features main")
+        p.cargo("run -Z package-features --package foo --features main")
             .masquerade_as_nightly_cargo(),
         execs().with_status(0),
     );

tests/testsuite/test.rs

@@ -2863,7 +2863,13 @@ fn selective_test_optional_dep() {
     let p = p.build();
 
     assert_that(
-        p.cargo("test -v --no-run --package a"),
+        p.cargo("test")
+            .arg("-v")
+            .arg("--no-run")
+            .arg("--features")
+            .arg("a")
+            .arg("-p")
+            .arg("a"),
         execs().with_status(0).with_stderr(
             "\
 [COMPILING] a v0.0.1 ([..])
@@ -3050,8 +3056,6 @@ fn pass_correct_cfgs_flags_to_rustdoc() {
             version = "0.1.0"
             authors = []
 
-            [workspace]
-
             [features]
             default = ["feature_a/default"]
             nightly = ["feature_a/nightly"]
@@ -3129,7 +3133,10 @@ fn pass_correct_cfgs_flags_to_rustdoc() {
     let p = p.build();
 
     assert_that(
-        p.cargo("test --package feature_a --verbose"),
+        p.cargo("test")
+            .arg("--package")
+            .arg("feature_a")
+            .arg("--verbose"),
         execs().with_status(0).with_stderr_contains(
             "\
 [DOCTEST] feature_a
@@ -3138,7 +3145,7 @@ fn pass_correct_cfgs_flags_to_rustdoc() {
     );
 
     assert_that(
-        p.cargo("test --verbose"),
+        p.cargo("test").arg("--verbose"),
         execs().with_status(0).with_stderr_contains(
             "\
 [DOCTEST] foo
@@ -3188,6 +3195,45 @@ fn test_release_ignore_panic() {
     assert_that(p.cargo("bench").arg("-v"), execs().with_status(0));
 }
 
+#[test]
+fn test_many_with_features() {
+    let p = project("foo")
+        .file(
+            "Cargo.toml",
+            r#"
+            [package]
+            name = "foo"
+            version = "0.0.1"
+            authors = []
+
+            [dependencies]
+            a = { path = "a" }
+
+            [features]
+            foo = []
+
+            [workspace]
+        "#,
+        )
+        .file("src/lib.rs", "")
+        .file(
+            "a/Cargo.toml",
+            r#"
+            [package]
+            name = "a"
+            version = "0.0.1"
+            authors = []
+        "#,
+        )
+        .file("a/src/lib.rs", "")
+        .build();
+
+    assert_that(
+        p.cargo("test -v -p a -p foo --features foo"),
+        execs().with_status(0),
+    );
+}
+
 #[test]
 fn test_all_workspace() {
     let p = project("foo")