Auto merge of #7536 - ehuss:profile-override-warning-fix, r=alexcrichton

Fix profile override warning in a workspace.

Profile overrides would erroneously warn about unused packages in a workspace if the package was not being built.  The fix here is to retain the `Resolve` for the entire workspace, and use that to determine the valid set of packages.

I think it would be good for a long-term goal to get rid of the second ("targeted") resolve.  I'm still contemplating how a separate features resolver could achieve that, but it seems feasible long-term.

Closes #7378

Author: bors

src/cargo/core/compiler/standard_lib.rs

@@ -103,7 +103,7 @@ pub fn resolve_std<'cfg>(
         /*uses_default_features*/ true,
     );
     let resolve = ops::resolve_ws_with_opts(&std_ws, opts, &specs)?;
-    Ok(resolve)
+    Ok((resolve.pkg_set, resolve.targeted_resolve))
 }
 
 /// Generate a list of root `Unit`s for the standard library.

src/cargo/core/profiles.rs

@@ -6,7 +6,7 @@ use serde::Deserialize;
 
 use crate::core::compiler::{CompileMode, ProfileKind};
 use crate::core::interning::InternedString;
-use crate::core::{Feature, Features, PackageId, PackageIdSpec, PackageSet, Shell};
+use crate::core::{Feature, Features, PackageId, PackageIdSpec, Resolve, Shell};
 use crate::util::errors::CargoResultExt;
 use crate::util::toml::{ProfilePackageSpec, StringOrBool, TomlProfile, TomlProfiles, U32OrBool};
 use crate::util::{closest_msg, CargoResult, Config};
@@ -429,13 +429,9 @@ impl Profiles {
     }
 
     /// Used to check for overrides for non-existing packages.
-    pub fn validate_packages(
-        &self,
-        shell: &mut Shell,
-        packages: &PackageSet<'_>,
-    ) -> CargoResult<()> {
+    pub fn validate_packages(&self, shell: &mut Shell, resolve: &Resolve) -> CargoResult<()> {
         for profile in self.by_name.values() {
-            profile.validate_packages(shell, packages)?;
+            profile.validate_packages(shell, resolve)?;
         }
         Ok(())
     }
@@ -502,16 +498,16 @@ impl ProfileMaker {
         profile
     }
 
-    fn validate_packages(&self, shell: &mut Shell, packages: &PackageSet<'_>) -> CargoResult<()> {
-        self.validate_packages_toml(shell, packages, &self.toml, true)?;
-        self.validate_packages_toml(shell, packages, &self.config, false)?;
+    fn validate_packages(&self, shell: &mut Shell, resolve: &Resolve) -> CargoResult<()> {
+        self.validate_packages_toml(shell, resolve, &self.toml, true)?;
+        self.validate_packages_toml(shell, resolve, &self.config, false)?;
         Ok(())
     }
 
     fn validate_packages_toml(
         &self,
         shell: &mut Shell,
-        packages: &PackageSet<'_>,
+        resolve: &Resolve,
         toml: &Option<TomlProfile>,
         warn_unmatched: bool,
     ) -> CargoResult<()> {
@@ -525,7 +521,7 @@ impl ProfileMaker {
         };
         // Verify that a package doesn't match multiple spec overrides.
         let mut found = HashSet::new();
-        for pkg_id in packages.package_ids() {
+        for pkg_id in resolve.iter() {
             let matches: Vec<&PackageIdSpec> = overrides
                 .keys()
                 .filter_map(|key| match *key {
@@ -575,8 +571,8 @@ impl ProfileMaker {
         });
         for spec in missing_specs {
             // See if there is an exact name match.
-            let name_matches: Vec<String> = packages
-                .package_ids()
+            let name_matches: Vec<String> = resolve
+                .iter()
                 .filter_map(|pkg_id| {
                     if pkg_id.name() == spec.name() {
                         Some(pkg_id.to_string())
@@ -586,8 +582,7 @@ impl ProfileMaker {
                 })
                 .collect();
             if name_matches.is_empty() {
-                let suggestion =
-                    closest_msg(&spec.name(), packages.package_ids(), |p| p.name().as_str());
+                let suggestion = closest_msg(&spec.name(), resolve.iter(), |p| p.name().as_str());
                 shell.warn(format!(
                     "package profile spec `{}` did not match any packages{}",
                     spec, suggestion

src/cargo/ops/cargo_compile.rs

@@ -38,6 +38,7 @@ use crate::core::resolver::{Resolve, ResolveOpts};
 use crate::core::{LibKind, Package, PackageSet, Target};
 use crate::core::{PackageId, PackageIdSpec, TargetKind, Workspace};
 use crate::ops;
+use crate::ops::resolve::WorkspaceResolve;
 use crate::util::config::Config;
 use crate::util::{closest_msg, profile, CargoResult};
 
@@ -303,7 +304,11 @@ pub fn compile_ws<'a>(
     let dev_deps = ws.require_optional_deps() || filter.need_dev_deps(build_config.mode);
     let opts = ResolveOpts::new(dev_deps, features, all_features, !no_default_features);
     let resolve = ops::resolve_ws_with_opts(ws, opts, &specs)?;
-    let (mut packages, resolve_with_overrides) = resolve;
+    let WorkspaceResolve {
+        mut pkg_set,
+        workspace_resolve,
+        targeted_resolve: resolve,
+    } = resolve;
 
     let std_resolve = if let Some(crates) = &config.cli_unstable().build_std {
         if build_config.build_plan {
@@ -319,7 +324,7 @@ pub fn compile_ws<'a>(
         }
         let (mut std_package_set, std_resolve) = standard_lib::resolve_std(ws, crates)?;
         remove_dylib_crate_type(&mut std_package_set)?;
-        packages.add_set(std_package_set);
+        pkg_set.add_set(std_package_set);
         Some(std_resolve)
     } else {
         None
@@ -330,12 +335,12 @@ pub fn compile_ws<'a>(
     // Vec<PackageIdSpec> to a Vec<&PackageId>.
     let to_build_ids = specs
         .iter()
-        .map(|s| s.query(resolve_with_overrides.iter()))
+        .map(|s| s.query(resolve.iter()))
         .collect::<CargoResult<Vec<_>>>()?;
     // Now get the `Package` for each `PackageId`. This may trigger a download
     // if the user specified `-p` for a dependency that is not downloaded.
     // Dependencies will be downloaded during build_unit_dependencies.
-    let mut to_builds = packages.get_many(to_build_ids)?;
+    let mut to_builds = pkg_set.get_many(to_build_ids)?;
 
     // The ordering here affects some error messages coming out of cargo, so
     // let's be test and CLI friendly by always printing in the same order if
@@ -370,12 +375,15 @@ pub fn compile_ws<'a>(
         );
     }
 
-    profiles.validate_packages(&mut config.shell(), &packages)?;
+    profiles.validate_packages(
+        &mut config.shell(),
+        workspace_resolve.as_ref().unwrap_or(&resolve),
+    )?;
 
     let interner = UnitInterner::new();
     let mut bcx = BuildContext::new(
         ws,
-        &packages,
+        &pkg_set,
         config,
         build_config,
         profiles,
@@ -388,7 +396,7 @@ pub fn compile_ws<'a>(
         &to_builds,
         filter,
         build_config.requested_kind,
-        &resolve_with_overrides,
+        &resolve,
         &bcx,
     )?;
 
@@ -434,13 +442,8 @@ pub fn compile_ws<'a>(
         }
     }
 
-    let unit_dependencies = build_unit_dependencies(
-        &bcx,
-        &resolve_with_overrides,
-        std_resolve.as_ref(),
-        &units,
-        &std_roots,
-    )?;
+    let unit_dependencies =
+        build_unit_dependencies(&bcx, &resolve, std_resolve.as_ref(), &units, &std_roots)?;
 
     let ret = {
         let _p = profile::start("compiling");

src/cargo/ops/cargo_doc.rs

@@ -24,14 +24,13 @@ pub fn doc(ws: &Workspace<'_>, options: &DocOptions<'_>) -> CargoResult<()> {
         options.compile_opts.all_features,
         !options.compile_opts.no_default_features,
     );
-    let resolve = ops::resolve_ws_with_opts(ws, opts, &specs)?;
-    let (packages, resolve_with_overrides) = resolve;
+    let ws_resolve = ops::resolve_ws_with_opts(ws, opts, &specs)?;
 
     let ids = specs
         .iter()
-        .map(|s| s.query(resolve_with_overrides.iter()))
+        .map(|s| s.query(ws_resolve.targeted_resolve.iter()))
         .collect::<CargoResult<Vec<_>>>()?;
-    let pkgs = packages.get_many(ids)?;
+    let pkgs = ws_resolve.pkg_set.get_many(ids)?;
 
     let mut lib_names = HashMap::new();
     let mut bin_names = HashMap::new();

src/cargo/ops/cargo_install.rs

@@ -491,14 +491,14 @@ fn check_yanked_install(ws: &Workspace<'_>) -> CargoResult<()> {
     // It would be best if `source` could be passed in here to avoid a
     // duplicate "Updating", but since `source` is taken by value, then it
     // wouldn't be available for `compile_ws`.
-    let (pkg_set, resolve) = ops::resolve_ws_with_opts(ws, ResolveOpts::everything(), &specs)?;
-    let mut sources = pkg_set.sources_mut();
+    let ws_resolve = ops::resolve_ws_with_opts(ws, ResolveOpts::everything(), &specs)?;
+    let mut sources = ws_resolve.pkg_set.sources_mut();
 
     // Checking the yanked status involves taking a look at the registry and
     // maybe updating files, so be sure to lock it here.
     let _lock = ws.config().acquire_package_cache_lock()?;
 
-    for pkg_id in resolve.iter() {
+    for pkg_id in ws_resolve.targeted_resolve.iter() {
         if let Some(source) = sources.get_mut(pkg_id.source_id()) {
             if source.is_yanked(pkg_id)? {
                 ws.config().shell().warn(format!(

src/cargo/ops/cargo_output_metadata.rs

@@ -56,17 +56,20 @@ fn metadata_full(ws: &Workspace<'_>, opt: &OutputMetadataOptions) -> CargoResult
         opt.all_features,
         !opt.no_default_features,
     );
-    let (package_set, resolve) = ops::resolve_ws_with_opts(ws, opts, &specs)?;
+    let ws_resolve = ops::resolve_ws_with_opts(ws, opts, &specs)?;
     let mut packages = HashMap::new();
-    for pkg in package_set.get_many(package_set.package_ids())? {
+    for pkg in ws_resolve
+        .pkg_set
+        .get_many(ws_resolve.pkg_set.package_ids())?
+    {
         packages.insert(pkg.package_id(), pkg.clone());
     }
 
     Ok(ExportInfo {
         packages: packages.values().map(|p| (*p).clone()).collect(),
         workspace_members: ws.members().map(|pkg| pkg.package_id()).collect(),
         resolve: Some(MetadataResolve {
-            resolve: (packages, resolve),
+            resolve: (packages, ws_resolve.targeted_resolve),
             root: ws.current_opt().map(|pkg| pkg.package_id()),
         }),
         target_directory: ws.target_dir().into_path_unlocked(),

src/cargo/ops/cargo_package.rs

@@ -155,15 +155,19 @@ fn build_lock(ws: &Workspace<'_>) -> CargoResult<String> {
     // Regenerate Cargo.lock using the old one as a guide.
     let specs = vec![PackageIdSpec::from_package_id(new_pkg.package_id())];
     let tmp_ws = Workspace::ephemeral(new_pkg, ws.config(), None, true)?;
-    let (pkg_set, new_resolve) =
-        ops::resolve_ws_with_opts(&tmp_ws, ResolveOpts::everything(), &specs)?;
+    let new_resolve = ops::resolve_ws_with_opts(&tmp_ws, ResolveOpts::everything(), &specs)?;
 
     if let Some(orig_resolve) = orig_resolve {
-        compare_resolve(config, tmp_ws.current()?, &orig_resolve, &new_resolve)?;
+        compare_resolve(
+            config,
+            tmp_ws.current()?,
+            &orig_resolve,
+            &new_resolve.targeted_resolve,
+        )?;
     }
-    check_yanked(config, &pkg_set, &new_resolve)?;
+    check_yanked(config, &new_resolve.pkg_set, &new_resolve.targeted_resolve)?;
 
-    ops::resolve_to_string(&tmp_ws, &new_resolve)
+    ops::resolve_to_string(&tmp_ws, &new_resolve.targeted_resolve)
 }
 
 // Checks that the package has some piece of metadata that a human can

src/cargo/ops/resolve.rs

@@ -24,6 +24,20 @@ use crate::sources::PathSource;
 use crate::util::errors::{CargoResult, CargoResultExt};
 use crate::util::profile;
 
+/// Result for `resolve_ws_with_opts`.
+pub struct WorkspaceResolve<'a> {
+    /// Packages to be downloaded.
+    pub pkg_set: PackageSet<'a>,
+    /// The resolve for the entire workspace.
+    ///
+    /// This may be `None` for things like `cargo install` and `-Zavoid-dev-deps`.
+    /// This does not include `paths` overrides.
+    pub workspace_resolve: Option<Resolve>,
+    /// The narrowed resolve, with the specific features enabled, and only the
+    /// given package specs requested.
+    pub targeted_resolve: Resolve,
+}
+
 const UNUSED_PATCH_WARNING: &str = "\
 Check that the patched package version and available features are compatible
 with the dependency requirements. If the patch has a different version from
@@ -60,7 +74,7 @@ pub fn resolve_ws_with_opts<'a>(
     ws: &Workspace<'a>,
     opts: ResolveOpts,
     specs: &[PackageIdSpec],
-) -> CargoResult<(PackageSet<'a>, Resolve)> {
+) -> CargoResult<WorkspaceResolve<'a>> {
     let mut registry = PackageRegistry::new(ws.config())?;
     let mut add_patches = true;
 
@@ -106,9 +120,13 @@ pub fn resolve_ws_with_opts<'a>(
         add_patches,
     )?;
 
-    let packages = get_resolved_packages(&resolved_with_overrides, registry)?;
+    let pkg_set = get_resolved_packages(&resolved_with_overrides, registry)?;
 
-    Ok((packages, resolved_with_overrides))
+    Ok(WorkspaceResolve {
+        pkg_set,
+        workspace_resolve: resolve,
+        targeted_resolve: resolved_with_overrides,
+    })
 }
 
 fn resolve_with_registry<'cfg>(

tests/testsuite/profile_overrides.rs

@@ -512,3 +512,35 @@ fn override_package_rename() {
 ")
         .run();
 }
+
+#[cargo_test]
+fn no_warning_ws() {
+    // https://github.com/rust-lang/cargo/issues/7378, avoid warnings in a workspace.
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+            cargo-features = ["profile-overrides"]
+            [workspace]
+            members = ["a", "b"]
+
+            [profile.dev.package.a]
+            codegen-units = 3
+            "#,
+        )
+        .file("a/Cargo.toml", &basic_manifest("a", "0.1.0"))
+        .file("a/src/lib.rs", "")
+        .file("b/Cargo.toml", &basic_manifest("b", "0.1.0"))
+        .file("b/src/lib.rs", "")
+        .build();
+
+    p.cargo("build -p b")
+        .masquerade_as_nightly_cargo()
+        .with_stderr(
+            "\
+[COMPILING] b [..]
+[FINISHED] [..]
+",
+        )
+        .run();
+}