Auto merge of #8074 - ehuss:package-features2, r=alexcrichton

bors · bors · commit 3a976c17670a · 2020-04-09T15:43:07.000Z
Extend -Zpackage-features with more capabilities. This is a proposal to extend `-Zpackage-features` with new abilities to change how features are selected on the command-line. See `unstable.md` for documentation on what it does. I've contemplated a variety of ways we could transition this to stable. I tried a few experiments trying to make a "transition with warnings" mode, but I'm just too concerned about breaking backwards compatibility. The current way is just fundamentally different from the new way, and I think it would be a bumpy ride to try to push it. The stabilization story is that the parts of this that add new functionality (feature flags in virtual worskpaces, and `member/feat` syntax) can be stabilized at any time. The change for `cargo build -p member --features feat` in a different member's directory can maybe be part of `-Zfeatures` stabilization, which will need to be opt-in. I've been trying to come up with some transition plan, and I can't think of a way without making it opt-in, and making it part of `-Zfeatures` is an opportunity to simplify things. One concern is that this might be confusing (`--features` flag could behave differently in different workspaces, and documenting the differences), but that seems hard to avoid. Closes #6195 Closes #4753 Closes #5015 Closes #4106 Closes #5362
diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs
@@ -1,7 +1,8 @@
 use std::cell::RefCell;
 use std::collections::hash_map::{Entry, HashMap};
-use std::collections::{BTreeMap, HashSet};
+use std::collections::{BTreeMap, BTreeSet, HashSet};
 use std::path::{Path, PathBuf};
+use std::rc::Rc;
 use std::slice;
 
 use glob::glob;
@@ -11,7 +12,7 @@ use url::Url;
 use crate::core::features::Features;
 use crate::core::registry::PackageRegistry;
 use crate::core::resolver::features::RequestedFeatures;
-use crate::core::{Dependency, PackageId, PackageIdSpec};
+use crate::core::{Dependency, InternedString, PackageId, PackageIdSpec};
 use crate::core::{EitherManifest, Package, SourceId, VirtualManifest};
 use crate::ops;
 use crate::sources::PathSource;
@@ -878,59 +879,143 @@ impl<'cfg> Workspace<'cfg> {
                 .collect());
         }
         if self.config().cli_unstable().package_features {
-            if specs.len() > 1 && !requested_features.features.is_empty() {
-                anyhow::bail!("cannot specify features for more than one package");
+            self.members_with_features_pf(specs, requested_features)
+        } else {
+            self.members_with_features_stable(specs, requested_features)
+        }
+    }
+
+    /// New command-line feature selection with -Zpackage-features.
+    fn members_with_features_pf(
+        &self,
+        specs: &[PackageIdSpec],
+        requested_features: &RequestedFeatures,
+    ) -> CargoResult<Vec<(&Package, RequestedFeatures)>> {
+        // Keep track of which features matched *any* member, to produce an error
+        // if any of them did not match anywhere.
+        let mut found: BTreeSet<InternedString> = BTreeSet::new();
+
+        // Returns the requested features for the given member.
+        // This filters out any named features that the member does not have.
+        let mut matching_features = |member: &Package| -> RequestedFeatures {
+            if requested_features.features.is_empty() || requested_features.all_features {
+                return requested_features.clone();
+            }
+            // Only include features this member defines.
+            let summary = member.summary();
+            let member_features = summary.features();
+            let mut features = BTreeSet::new();
+
+            // Checks if a member contains the given feature.
+            let contains = |feature: InternedString| -> bool {
+                member_features.contains_key(&feature)
+                    || summary
+                        .dependencies()
+                        .iter()
+                        .any(|dep| dep.is_optional() && dep.name_in_toml() == feature)
+            };
+
+            for feature in requested_features.features.iter() {
+                let mut split = feature.splitn(2, '/');
+                let split = (split.next().unwrap(), split.next());
+                if let (pkg, Some(pkg_feature)) = split {
+                    let pkg = InternedString::new(pkg);
+                    let pkg_feature = InternedString::new(pkg_feature);
+                    if summary
+                        .dependencies()
+                        .iter()
+                        .any(|dep| dep.name_in_toml() == pkg)
+                    {
+                        // pkg/feat for a dependency.
+                        // Will rely on the dependency resolver to validate `feat`.
+                        features.insert(*feature);
+                        found.insert(*feature);
+                    } else if pkg == member.name() && contains(pkg_feature) {
+                        // member/feat where "feat" is a feature in member.
+                        features.insert(pkg_feature);
+                        found.insert(*feature);
+                    }
+                } else if contains(*feature) {
+                    // feature exists in this member.
+                    features.insert(*feature);
+                    found.insert(*feature);
+                }
+            }
+            RequestedFeatures {
+                features: Rc::new(features),
+                all_features: false,
+                uses_default_features: requested_features.uses_default_features,
+            }
+        };
+
+        let members: Vec<(&Package, RequestedFeatures)> = self
+            .members()
+            .filter(|m| specs.iter().any(|spec| spec.matches(m.package_id())))
+            .map(|m| (m, matching_features(m)))
+            .collect();
+        if members.is_empty() {
+            // `cargo build -p foo`, where `foo` is not a member.
+            // Do not allow any command-line flags (defaults only).
+            if !(requested_features.features.is_empty()
+                && !requested_features.all_features
+                && requested_features.uses_default_features)
+            {
+                anyhow::bail!("cannot specify features for packages outside of workspace");
             }
-            let members: Vec<(&Package, RequestedFeatures)> = self
+            // Add all members from the workspace so we can ensure `-p nonmember`
+            // is in the resolve graph.
+            return Ok(self
                 .members()
-                .filter(|m| specs.iter().any(|spec| spec.matches(m.package_id())))
-                .map(|m| (m, requested_features.clone()))
+                .map(|m| (m, RequestedFeatures::new_all(false)))
+                .collect());
+        }
+        if *requested_features.features != found {
+            let missing: Vec<_> = requested_features
+                .features
+                .difference(&found)
+                .copied()
                 .collect();
-            if members.is_empty() {
-                // `cargo build -p foo`, where `foo` is not a member.
-                // Do not allow any command-line flags (defaults only).
-                if !(requested_features.features.is_empty()
-                    && !requested_features.all_features
-                    && requested_features.uses_default_features)
-                {
-                    anyhow::bail!("cannot specify features for packages outside of workspace");
+            // TODO: typo suggestions would be good here.
+            anyhow::bail!(
+                "none of the selected packages contains these features: {}",
+                missing.join(", ")
+            );
+        }
+        Ok(members)
+    }
+
+    /// This is the current "stable" behavior for command-line feature selection.
+    fn members_with_features_stable(
+        &self,
+        specs: &[PackageIdSpec],
+        requested_features: &RequestedFeatures,
+    ) -> CargoResult<Vec<(&Package, RequestedFeatures)>> {
+        let ms = self.members().filter_map(|member| {
+            let member_id = member.package_id();
+            match self.current_opt() {
+                // The features passed on the command-line only apply to
+                // the "current" package (determined by the cwd).
+                Some(current) if member_id == current.package_id() => {
+                    Some((member, requested_features.clone()))
                 }
-                // Add all members from the workspace so we can ensure `-p nonmember`
-                // is in the resolve graph.
-                return Ok(self
-                    .members()
-                    .map(|m| (m, RequestedFeatures::new_all(false)))
-                    .collect());
-            }
-            Ok(members)
-        } else {
-            let ms = self.members().filter_map(|member| {
-                let member_id = member.package_id();
-                match self.current_opt() {
-                    // The features passed on the command-line only apply to
-                    // the "current" package (determined by the cwd).
-                    Some(current) if member_id == current.package_id() => {
-                        Some((member, requested_features.clone()))
-                    }
-                    _ => {
-                        // Ignore members that are not enabled on the command-line.
-                        if specs.iter().any(|spec| spec.matches(member_id)) {
-                            // -p for a workspace member that is not the
-                            // "current" one, don't use the local
-                            // `--features`, only allow `--all-features`.
-                            Some((
-                                member,
-                                RequestedFeatures::new_all(requested_features.all_features),
-                            ))
-                        } else {
-                            // This member was not requested on the command-line, skip.
-                            None
-                        }
+                _ => {
+                    // Ignore members that are not enabled on the command-line.
+                    if specs.iter().any(|spec| spec.matches(member_id)) {
+                        // -p for a workspace member that is not the
+                        // "current" one, don't use the local
+                        // `--features`, only allow `--all-features`.
+                        Some((
+                            member,
+                            RequestedFeatures::new_all(requested_features.all_features),
+                        ))
+                    } else {
+                        // This member was not requested on the command-line, skip.
+                        None
                     }
                 }
-            });
-            Ok(ms.collect())
-        }
+            }
+        });
+        Ok(ms.collect())
     }
 }
 
diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs
@@ -62,7 +62,14 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> {
     let mut build_config = BuildConfig::new(config, Some(1), &opts.target, CompileMode::Build)?;
     build_config.requested_profile = opts.requested_profile;
     let target_data = RustcTargetData::new(ws, build_config.requested_kind)?;
-    let resolve_opts = ResolveOpts::everything();
+    // Resolve for default features. In the future, `cargo clean` should be rewritten
+    // so that it doesn't need to guess filename hashes.
+    let resolve_opts = ResolveOpts::new(
+        /*dev_deps*/ true,
+        &[],
+        /*all features*/ false,
+        /*default*/ true,
+    );
     let specs = opts
         .spec
         .iter()
diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md
@@ -474,7 +474,7 @@ cargo +nightly -Zunstable-options -Zconfig-include --config somefile.toml build
 
 CLI paths are relative to the current working directory.
 
-## Features
+### Features
 * Tracking Issues:
   * [itarget #7914](https://github.com/rust-lang/cargo/issues/7914)
   * [build_dep #7915](https://github.com/rust-lang/cargo/issues/7915)
@@ -549,6 +549,31 @@ The available options are:
 * `compare` — This option compares the resolved features to the old resolver,
   and will print any differences.
 
+### package-features
+* Tracking Issue: [#5364](https://github.com/rust-lang/cargo/issues/5364)
+
+The `-Zpackage-features` flag changes the way features can be passed on the
+command-line for a workspace. The normal behavior can be confusing, as the
+features passed are always enabled on the package in the current directory,
+even if that package is not selected with a `-p` flag. Feature flags also do
+not work in the root of a virtual workspace. `-Zpackage-features` tries to
+make feature flags behave in a more intuitive manner.
+
+* `cargo build -p other_member --features …` — This now only enables the given
+  features as defined in `other_member` (ignores whatever is in the current
+  directory).
+* `cargo build -p a -p b --features …` — This now enables the given features
+  on both `a` and `b`. Not all packages need to define every feature, it only
+  enables matching features. It is still an error if none of the packages
+  define a given feature.
+* `--features` and `--no-default-features` are now allowed in the root of a
+  virtual workspace.
+* `member_name/feature_name` syntax may now be used on the command-line to
+  enable features for a specific member.
+
+The ability to set features for non-workspace members is no longer allowed, as
+the resolver fundamentally does not support that ability.
+
 ### crate-versions
 * Tracking Issue: [#7907](https://github.com/rust-lang/cargo/issues/7907)
 
diff --git a/tests/testsuite/features.rs b/tests/testsuite/features.rs
@@ -1358,80 +1358,6 @@ fn many_cli_features_comma_and_space_delimited() {
         .run();
 }
 
-#[cargo_test]
-fn combining_features_and_package() {
-    Package::new("dep", "1.0.0").publish();
-
-    let p = project()
-        .file(
-            "Cargo.toml",
-            r#"
-            [project]
-            name = "foo"
-            version = "0.0.1"
-            authors = []
-
-            [workspace]
-            members = ["bar"]
-
-            [dependencies]
-            dep = "1"
-        "#,
-        )
-        .file("src/lib.rs", "")
-        .file(
-            "bar/Cargo.toml",
-            r#"
-            [package]
-            name = "bar"
-            version = "0.0.1"
-            authors = []
-            [features]
-            main = []
-        "#,
-        )
-        .file(
-            "bar/src/main.rs",
-            r#"
-            #[cfg(feature = "main")]
-            fn main() {}
-        "#,
-        )
-        .build();
-
-    p.cargo("build -Z package-features --workspace --features main")
-        .masquerade_as_nightly_cargo()
-        .with_status(101)
-        .with_stderr_contains("[ERROR] cannot specify features for more than one package")
-        .run();
-
-    p.cargo("build -Z package-features --package dep --features main")
-        .masquerade_as_nightly_cargo()
-        .with_status(101)
-        .with_stderr_contains("[ERROR] cannot specify features for packages outside of workspace")
-        .run();
-    p.cargo("build -Z package-features --package dep --all-features")
-        .masquerade_as_nightly_cargo()
-        .with_status(101)
-        .with_stderr_contains("[ERROR] cannot specify features for packages outside of workspace")
-        .run();
-    p.cargo("build -Z package-features --package dep --no-default-features")
-        .masquerade_as_nightly_cargo()
-        .with_status(101)
-        .with_stderr_contains("[ERROR] cannot specify features for packages outside of workspace")
-        .run();
-
-    p.cargo("build -Z package-features --workspace --all-features")
-        .masquerade_as_nightly_cargo()
-        .run();
-    p.cargo("run -Z package-features --package bar --features main")
-        .masquerade_as_nightly_cargo()
-        .run();
-    p.cargo("build -Z package-features --package dep")
-        .masquerade_as_nightly_cargo()
-        .run();
-}
-
 #[cargo_test]
 fn namespaced_invalid_feature() {
     let p = project()
diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs
@@ -73,6 +73,7 @@ mod offline;
 mod out_dir;
 mod owner;
 mod package;
+mod package_features;
 mod patch;
 mod path;
 mod paths;
diff --git a/tests/testsuite/package_features.rs b/tests/testsuite/package_features.rs
