Consolidate -Zpackage-features member iteration logic.

Author: ehuss

src/cargo/core/resolver/features.rs

@@ -182,8 +182,6 @@ pub struct FeatureResolver<'a, 'cfg> {
     /// The platform to build for, requested by the user.
     requested_target: CompileKind,
     resolve: &'a Resolve,
-    /// Packages to build, requested on the command-line.
-    specs: &'a [PackageIdSpec],
     /// Options that change how the feature resolver operates.
     opts: FeatureOpts,
     /// Map of features activated for each package.
@@ -222,12 +220,11 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
             target_data,
             requested_target,
             resolve,
-            specs,
             opts,
             activated_features: HashMap::new(),
             processed_deps: HashSet::new(),
         };
-        r.do_resolve(requested_features)?;
+        r.do_resolve(specs, requested_features)?;
         log::debug!("features={:#?}", r.activated_features);
         if r.opts.compare {
             r.compare();
@@ -240,44 +237,14 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
     }
 
     /// Performs the process of resolving all features for the resolve graph.
-    fn do_resolve(&mut self, requested_features: &RequestedFeatures) -> CargoResult<()> {
-        if self.opts.package_features {
-            let mut found = false;
-            for member in self.ws.members() {
-                let member_id = member.package_id();
-                if self.specs.iter().any(|spec| spec.matches(member_id)) {
-                    found = true;
-                    self.activate_member(member_id, requested_features)?;
-                }
-            }
-            if !found {
-                // -p for a non-member. Just resolve all with defaults.
-                let default = RequestedFeatures::new_all(false);
-                for member in self.ws.members() {
-                    self.activate_member(member.package_id(), &default)?;
-                }
-            }
-        } else {
-            for member in self.ws.members() {
-                let member_id = member.package_id();
-                match self.ws.current_opt() {
-                    Some(current) if member_id == current.package_id() => {
-                        // The "current" member gets activated with the flags
-                        // from the command line.
-                        self.activate_member(member_id, requested_features)?;
-                    }
-                    _ => {
-                        // Ignore members that are not enabled on the command-line.
-                        if self.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`.
-                            let not_current_requested =
-                                RequestedFeatures::new_all(requested_features.all_features);
-                            self.activate_member(member_id, &not_current_requested)?;
-                        }
-                    }
-                }
-            }
+    fn do_resolve(
+        &mut self,
+        specs: &[PackageIdSpec],
+        requested_features: &RequestedFeatures,
+    ) -> CargoResult<()> {
+        let member_features = self.ws.members_with_features(specs, requested_features)?;
+        for (member, requested_features) in member_features {
+            self.activate_member(member.package_id(), &requested_features)?;
         }
         Ok(())
     }

src/cargo/core/workspace.rs

@@ -10,6 +10,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::{EitherManifest, Package, SourceId, VirtualManifest};
 use crate::ops;
@@ -833,6 +834,86 @@ impl<'cfg> Workspace<'cfg> {
     pub fn set_target_dir(&mut self, target_dir: Filesystem) {
         self.target_dir = Some(target_dir);
     }
+
+    /// Returns a Vec of `(&Package, RequestedFeatures)` tuples that
+    /// represent the workspace members that were requested on the command-line.
+    ///
+    /// `specs` may be empty, which indicates it should return all workspace
+    /// members. In this case, `requested_features.all_features` must be
+    /// `true`. This is used for generating `Cargo.lock`, which must include
+    /// all members with all features enabled.
+    pub fn members_with_features(
+        &self,
+        specs: &[PackageIdSpec],
+        requested_features: &RequestedFeatures,
+    ) -> CargoResult<Vec<(&Package, RequestedFeatures)>> {
+        assert!(
+            !specs.is_empty() || requested_features.all_features,
+            "no specs requires all_features"
+        );
+        if specs.is_empty() {
+            // When resolving the entire workspace, resolve each member with
+            // all features enabled.
+            return Ok(self
+                .members()
+                .map(|m| (m, RequestedFeatures::new_all(true)))
+                .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");
+            }
+            let members: Vec<(&Package, RequestedFeatures)> = self
+                .members()
+                .filter(|m| specs.iter().any(|spec| spec.matches(m.package_id())))
+                .map(|m| (m, requested_features.clone()))
+                .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");
+                }
+                // 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());
+            }
+            return 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
+                        }
+                    }
+                }
+            });
+            return Ok(ms.collect());
+        }
+    }
 }
 
 impl<'cfg> Packages<'cfg> {

src/cargo/ops/resolve.rs

@@ -12,8 +12,9 @@
 
 use crate::core::compiler::{CompileKind, RustcTargetData};
 use crate::core::registry::PackageRegistry;
-use crate::core::resolver::features::{FeatureResolver, RequestedFeatures, ResolvedFeatures};
+use crate::core::resolver::features::{FeatureResolver, ResolvedFeatures};
 use crate::core::resolver::{self, Resolve, ResolveOpts};
+use crate::core::summary::Summary;
 use crate::core::Feature;
 use crate::core::{PackageId, PackageIdSpec, PackageSet, Source, SourceId, Workspace};
 use crate::ops;
@@ -188,11 +189,6 @@ pub fn resolve_with_previous<'cfg>(
     specs: &[PackageIdSpec],
     register_patches: bool,
 ) -> CargoResult<Resolve> {
-    assert!(
-        !specs.is_empty() || opts.features.all_features,
-        "no specs requires all_features"
-    );
-
     // We only want one Cargo at a time resolving a crate graph since this can
     // involve a lot of frobbing of the global caches.
     let _lock = ws.config().acquire_package_cache_lock()?;
@@ -272,81 +268,20 @@ pub fn resolve_with_previous<'cfg>(
         registry.add_sources(Some(member.package_id().source_id()))?;
     }
 
-    let mut summaries = Vec::new();
-    if ws.config().cli_unstable().package_features {
-        let mut members = Vec::new();
-        if specs.is_empty() {
-            members.extend(ws.members());
-        } else {
-            if specs.len() > 1 && !opts.features.features.is_empty() {
-                anyhow::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 !(opts.features.features.is_empty()
-                    && !opts.features.all_features
-                    && opts.features.uses_default_features)
-                {
-                    anyhow::bail!("cannot specify features for packages outside of workspace");
-                }
-                members.extend(ws.members());
-            }
-        }
-        for member in members {
+    let summaries: Vec<(Summary, ResolveOpts)> = ws
+        .members_with_features(specs, &opts.features)?
+        .into_iter()
+        .map(|(member, features)| {
             let summary = registry.lock(member.summary().clone());
-            summaries.push((summary, opts.clone()))
-        }
-    } else {
-        for member in ws.members() {
-            let summary_resolve_opts = if specs.is_empty() {
-                // When resolving the entire workspace, resolve each member
-                // with all features enabled.
-                opts.clone()
-            } else {
-                // 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 `opts` 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 `opts` specified. Otherwise we use a
-                // base `opts` with no features specified but using default features
-                // for any other packages specified with `-p`.
-                let member_id = member.package_id();
-                match ws.current_opt() {
-                    Some(current) if member_id == current.package_id() => opts.clone(),
-                    _ => {
-                        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`.
-                            ResolveOpts {
-                                dev_deps: opts.dev_deps,
-                                features: RequestedFeatures::new_all(opts.features.all_features),
-                            }
-                        } else {
-                            // This member was not requested on the command-line, skip.
-                            continue;
-                        }
-                    }
-                }
-            };
-
-            let summary = registry.lock(member.summary().clone());
-            summaries.push((summary, summary_resolve_opts));
-        }
-    };
+            (
+                summary,
+                ResolveOpts {
+                    dev_deps: opts.dev_deps,
+                    features,
+                },
+            )
+        })
+        .collect();
 
     let root_replace = ws.root_replace();