Auto merge of #8823 - ehuss:minimal-download, r=alexcrichton

Avoid some extra downloads with new feature resolver.

There are some edge cases with the new feature resolver where it can erroneously trigger a download of a package that is not needed. This is due to the call `is_proc_macro` which has to downloaded the manifest to check if it is a proc-macro. The main change here is to defer calling `is_proc_macro` until after dependencies have been filtered. It also avoids calling `is_proc_macro` if the new feature resolver is enabled, but `decouple_host_deps` and `ignore_inactive_targets` are disabled (such as with `-Z weak-dep-features`), in which case it doesn't matter if it is a proc-macro or not.

Fixes #8776

src/cargo/core/package.rs

@@ -19,6 +19,7 @@ use serde::Serialize;
 
 use crate::core::compiler::{CompileKind, RustcTargetData};
 use crate::core::dependency::DepKind;
+use crate::core::resolver::features::ForceAllTargets;
 use crate::core::resolver::{HasDevUnits, Resolve};
 use crate::core::source::MaybePackage;
 use crate::core::{Dependency, Manifest, PackageId, SourceId, Target};
@@ -488,6 +489,7 @@ impl<'cfg> PackageSet<'cfg> {
         has_dev_units: HasDevUnits,
         requested_kinds: &[CompileKind],
         target_data: &RustcTargetData,
+        force_all_targets: ForceAllTargets,
     ) -> CargoResult<()> {
         fn collect_used_deps(
             used: &mut BTreeSet<PackageId>,
@@ -496,6 +498,7 @@ impl<'cfg> PackageSet<'cfg> {
             has_dev_units: HasDevUnits,
             requested_kinds: &[CompileKind],
             target_data: &RustcTargetData,
+            force_all_targets: ForceAllTargets,
         ) -> CargoResult<()> {
             if !used.insert(pkg_id) {
                 return Ok(());
@@ -509,12 +512,14 @@ impl<'cfg> PackageSet<'cfg> {
                     // dependencies are used both for target and host. To tighten this
                     // up, this function would need to track "for_host" similar to how
                     // unit dependencies handles it.
-                    let activated = requested_kinds
-                        .iter()
-                        .chain(Some(&CompileKind::Host))
-                        .any(|kind| target_data.dep_platform_activated(dep, *kind));
-                    if !activated {
-                        return false;
+                    if force_all_targets == ForceAllTargets::No {
+                        let activated = requested_kinds
+                            .iter()
+                            .chain(Some(&CompileKind::Host))
+                            .any(|kind| target_data.dep_platform_activated(dep, *kind));
+                        if !activated {
+                            return false;
+                        }
                     }
                     true
                 })
@@ -527,6 +532,7 @@ impl<'cfg> PackageSet<'cfg> {
                     has_dev_units,
                     requested_kinds,
                     target_data,
+                    force_all_targets,
                 )?;
             }
             Ok(())
@@ -545,6 +551,7 @@ impl<'cfg> PackageSet<'cfg> {
                 has_dev_units,
                 requested_kinds,
                 target_data,
+                force_all_targets,
             )?;
         }
         self.get_many(to_download.into_iter())?;

src/cargo/core/resolver/features.rs

@@ -303,6 +303,14 @@ pub struct FeatureResolver<'a, 'cfg> {
     /// Keeps track of which packages have had its dependencies processed.
     /// Used to avoid cycles, and to speed up processing.
     processed_deps: HashSet<(PackageId, bool)>,
+    /// If this is `true`, then `for_host` needs to be tracked while
+    /// traversing the graph.
+    ///
+    /// This is only here to avoid calling `is_proc_macro` when all feature
+    /// options are disabled (because `is_proc_macro` can trigger downloads).
+    /// This has to be separate from `FeatureOpts.decouple_host_deps` because
+    /// `for_host` tracking is also needed for `itarget` to work properly.
+    track_for_host: bool,
 }
 
 impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
@@ -333,6 +341,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
                 opts,
             });
         }
+        let track_for_host = opts.decouple_host_deps || opts.ignore_inactive_targets;
         let mut r = FeatureResolver {
             ws,
             target_data,
@@ -343,6 +352,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
             activated_features: HashMap::new(),
             activated_dependencies: HashMap::new(),
             processed_deps: HashSet::new(),
+            track_for_host,
         };
         r.do_resolve(specs, requested_features)?;
         log::debug!("features={:#?}", r.activated_features);
@@ -367,7 +377,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
         let member_features = self.ws.members_with_features(specs, requested_features)?;
         for (member, requested_features) in &member_features {
             let fvs = self.fvs_from_requested(member.package_id(), requested_features);
-            let for_host = self.is_proc_macro(member.package_id());
+            let for_host = self.track_for_host && self.is_proc_macro(member.package_id());
             self.activate_pkg(member.package_id(), &fvs, for_host)?;
             if for_host {
                 // Also activate without for_host. This is needed if the
@@ -597,7 +607,6 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
         self.resolve
             .deps(pkg_id)
             .map(|(dep_id, deps)| {
-                let is_proc_macro = self.is_proc_macro(dep_id);
                 let deps = deps
                     .iter()
                     .filter(|dep| {
@@ -613,7 +622,8 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
                         true
                     })
                     .map(|dep| {
-                        let dep_for_host = for_host || dep.is_build() || is_proc_macro;
+                        let dep_for_host = self.track_for_host
+                            && (for_host || dep.is_build() || self.is_proc_macro(dep_id));
                         (dep, dep_for_host)
                     })
                     .collect::<Vec<_>>();

src/cargo/ops/resolve.rs

@@ -138,6 +138,7 @@ pub fn resolve_ws_with_opts<'cfg>(
         has_dev_units,
         requested_targets,
         target_data,
+        force_all_targets,
     )?;
 
     let resolved_features = FeatureResolver::resolve(

src/cargo/ops/tree/mod.rs

@@ -165,6 +165,8 @@ pub fn build_and_print(ws: &Workspace<'_>, opts: &TreeOptions) -> CargoResult<()
         force_all,
     )?;
     // Download all Packages. Some display formats need to display package metadata.
+    // This may trigger some unnecessary downloads, but trying to figure out a
+    // minimal set would be difficult.
     let package_map: HashMap<PackageId, &Package> = ws_resolve
         .pkg_set
         .get_many(ws_resolve.pkg_set.package_ids())?

tests/testsuite/features2.rs

@@ -1,6 +1,7 @@
 //! Tests for the new feature resolver.
 
 use cargo_test_support::cross_compile::{self, alternate};
+use cargo_test_support::install::cargo_home;
 use cargo_test_support::paths::CargoPathExt;
 use cargo_test_support::publish::validate_crate_contents;
 use cargo_test_support::registry::{Dependency, Package};
@@ -1987,8 +1988,6 @@ fn doc_optional() {
 [DOWNLOADING] crates ...
 [DOWNLOADED] spin v1.0.0 [..]
 [DOWNLOADED] bar v1.0.0 [..]
-[DOWNLOADING] crates ...
-[DOWNLOADED] enabler v1.0.0 [..]
 [DOCUMENTING] bar v1.0.0
 [CHECKING] bar v1.0.0
 [DOCUMENTING] foo v0.1.0 [..]
@@ -1997,3 +1996,246 @@ fn doc_optional() {
         )
         .run();
 }
+
+#[cargo_test]
+fn minimal_download() {
+    // Various checks that it only downloads the minimum set of dependencies
+    // needed in various situations.
+    //
+    // This checks several permutations of the different
+    // host_dep/dev_dep/itarget settings. These 3 are planned to be stabilized
+    // together, so there isn't much need to be concerned about how the behave
+    // independently. However, there are some cases where they do behave
+    // independently. Specifically:
+    //
+    // * `cargo test` forces dev_dep decoupling to be disabled.
+    // * `cargo tree --target=all` forces ignore_inactive_targets off and decouple_dev_deps off.
+    // * `cargo tree --target=all -e normal` forces ignore_inactive_targets off.
+    //
+    // However, `cargo tree` is a little weird because it downloads everything
+    // anyways.
+    //
+    // So to summarize the different permutations:
+    //
+    // dev_dep | host_dep | itarget | Notes
+    // --------|----------|---------|----------------------------
+    //         |          |         | -Zfeatures=compare (new resolver should behave same as old)
+    //         |          |    ✓    | This scenario should not happen.
+    //         |     ✓    |         | `cargo tree --target=all -Zfeatures=all`†
+    //         |     ✓    |    ✓    | `cargo test`
+    //    ✓    |          |         | This scenario should not happen.
+    //    ✓    |          |    ✓    | This scenario should not happen.
+    //    ✓    |     ✓    |         | `cargo tree --target=all -e normal -Z features=all`†
+    //    ✓    |     ✓    |    ✓    | A normal build.
+    //
+    // † — However, `cargo tree` downloads everything.
+    Package::new("normal", "1.0.0").publish();
+    Package::new("normal_pm", "1.0.0").publish();
+    Package::new("normal_opt", "1.0.0").publish();
+    Package::new("dev_dep", "1.0.0").publish();
+    Package::new("dev_dep_pm", "1.0.0").publish();
+    Package::new("build_dep", "1.0.0").publish();
+    Package::new("build_dep_pm", "1.0.0").publish();
+    Package::new("build_dep_opt", "1.0.0").publish();
+
+    Package::new("itarget_normal", "1.0.0").publish();
+    Package::new("itarget_normal_pm", "1.0.0").publish();
+    Package::new("itarget_dev_dep", "1.0.0").publish();
+    Package::new("itarget_dev_dep_pm", "1.0.0").publish();
+    Package::new("itarget_build_dep", "1.0.0").publish();
+    Package::new("itarget_build_dep_pm", "1.0.0").publish();
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+
+                [dependencies]
+                normal = "1.0"
+                normal_pm = "1.0"
+                normal_opt = { version = "1.0", optional = true }
+
+                [dev-dependencies]
+                dev_dep = "1.0"
+                dev_dep_pm = "1.0"
+
+                [build-dependencies]
+                build_dep = "1.0"
+                build_dep_pm = "1.0"
+                build_dep_opt = { version = "1.0", optional = true }
+
+                [target.'cfg(whatever)'.dependencies]
+                itarget_normal = "1.0"
+                itarget_normal_pm = "1.0"
+
+                [target.'cfg(whatever)'.dev-dependencies]
+                itarget_dev_dep = "1.0"
+                itarget_dev_dep_pm = "1.0"
+
+                [target.'cfg(whatever)'.build-dependencies]
+                itarget_build_dep = "1.0"
+                itarget_build_dep_pm = "1.0"
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .file("build.rs", "fn main() {}")
+        .build();
+
+    let clear = || {
+        cargo_home().join("registry/cache").rm_rf();
+        cargo_home().join("registry/src").rm_rf();
+        p.build_dir().rm_rf();
+    };
+
+    // none
+    // Should be the same as `-Zfeatures=all`
+    p.cargo("check -Zfeatures=compare")
+        .masquerade_as_nightly_cargo()
+        .with_stderr_unordered(
+            "\
+[UPDATING] [..]
+[DOWNLOADING] crates ...
+[DOWNLOADED] normal_pm v1.0.0 [..]
+[DOWNLOADED] normal v1.0.0 [..]
+[DOWNLOADED] build_dep_pm v1.0.0 [..]
+[DOWNLOADED] build_dep v1.0.0 [..]
+[COMPILING] build_dep v1.0.0
+[COMPILING] build_dep_pm v1.0.0
+[CHECKING] normal_pm v1.0.0
+[CHECKING] normal v1.0.0
+[COMPILING] foo v0.1.0 [..]
+[FINISHED] [..]
+",
+        )
+        .run();
+    clear();
+
+    // all
+    p.cargo("check -Zfeatures=all")
+        .masquerade_as_nightly_cargo()
+        .with_stderr_unordered(
+            "\
+[DOWNLOADING] crates ...
+[DOWNLOADED] normal_pm v1.0.0 [..]
+[DOWNLOADED] normal v1.0.0 [..]
+[DOWNLOADED] build_dep_pm v1.0.0 [..]
+[DOWNLOADED] build_dep v1.0.0 [..]
+[COMPILING] build_dep v1.0.0
+[COMPILING] build_dep_pm v1.0.0
+[CHECKING] normal v1.0.0
+[CHECKING] normal_pm v1.0.0
+[COMPILING] foo v0.1.0 [..]
+[FINISHED] [..]
+",
+        )
+        .run();
+    clear();
+
+    // This disables decouple_dev_deps.
+    p.cargo("test --no-run -Zfeatures=all")
+        .masquerade_as_nightly_cargo()
+        .with_stderr_unordered(
+            "\
+[DOWNLOADING] crates ...
+[DOWNLOADED] normal_pm v1.0.0 [..]
+[DOWNLOADED] normal v1.0.0 [..]
+[DOWNLOADED] dev_dep_pm v1.0.0 [..]
+[DOWNLOADED] dev_dep v1.0.0 [..]
+[DOWNLOADED] build_dep_pm v1.0.0 [..]
+[DOWNLOADED] build_dep v1.0.0 [..]
+[COMPILING] build_dep v1.0.0
+[COMPILING] build_dep_pm v1.0.0
+[COMPILING] normal_pm v1.0.0
+[COMPILING] normal v1.0.0
+[COMPILING] dev_dep_pm v1.0.0
+[COMPILING] dev_dep v1.0.0
+[COMPILING] foo v0.1.0 [..]
+[FINISHED] [..]
+",
+        )
+        .run();
+    clear();
+
+    // This disables itarget, but leaves decouple_dev_deps enabled. However,
+    // `cargo tree` downloads everything anyways. Ideally `cargo tree` should
+    // be a little smarter, but that would make it a bit more complicated. The
+    // two "Downloading" lines are because `download_accessible` doesn't
+    // download enough (`cargo tree` really wants everything). Again, that
+    // could be cleaner, but is difficult.
+    p.cargo("tree -e normal --target=all -Zfeatures=all")
+        .masquerade_as_nightly_cargo()
+        .with_stderr_unordered(
+            "\
+[DOWNLOADING] crates ...
+[DOWNLOADING] crates ...
+[DOWNLOADED] normal v1.0.0 [..]
+[DOWNLOADED] dev_dep v1.0.0 [..]
+[DOWNLOADED] normal_pm v1.0.0 [..]
+[DOWNLOADED] build_dep v1.0.0 [..]
+[DOWNLOADED] dev_dep_pm v1.0.0 [..]
+[DOWNLOADED] build_dep_pm v1.0.0 [..]
+[DOWNLOADED] itarget_normal v1.0.0 [..]
+[DOWNLOADED] itarget_dev_dep v1.0.0 [..]
+[DOWNLOADED] itarget_normal_pm v1.0.0 [..]
+[DOWNLOADED] itarget_build_dep v1.0.0 [..]
+[DOWNLOADED] itarget_dev_dep_pm v1.0.0 [..]
+[DOWNLOADED] itarget_build_dep_pm v1.0.0 [..]
+",
+        )
+        .with_stdout(
+            "\
+foo v0.1.0 ([ROOT]/foo)
+├── itarget_normal v1.0.0
+├── itarget_normal_pm v1.0.0
+├── normal v1.0.0
+└── normal_pm v1.0.0
+",
+        )
+        .run();
+    clear();
+
+    // This disables itarget and decouple_dev_deps.
+    p.cargo("tree --target=all -Zfeatures=all")
+        .masquerade_as_nightly_cargo()
+        .with_stderr_unordered(
+            "\
+[DOWNLOADING] crates ...
+[DOWNLOADED] normal_pm v1.0.0 [..]
+[DOWNLOADED] normal v1.0.0 [..]
+[DOWNLOADED] itarget_normal_pm v1.0.0 [..]
+[DOWNLOADED] itarget_normal v1.0.0 [..]
+[DOWNLOADED] itarget_dev_dep_pm v1.0.0 [..]
+[DOWNLOADED] itarget_dev_dep v1.0.0 [..]
+[DOWNLOADED] itarget_build_dep_pm v1.0.0 [..]
+[DOWNLOADED] itarget_build_dep v1.0.0 [..]
+[DOWNLOADED] dev_dep_pm v1.0.0 [..]
+[DOWNLOADED] dev_dep v1.0.0 [..]
+[DOWNLOADED] build_dep_pm v1.0.0 [..]
+[DOWNLOADED] build_dep v1.0.0 [..]
+",
+        )
+        .with_stdout(
+            "\
+foo v0.1.0 ([ROOT]/foo)
+├── itarget_normal v1.0.0
+├── itarget_normal_pm v1.0.0
+├── normal v1.0.0
+└── normal_pm v1.0.0
+[build-dependencies]
+├── build_dep v1.0.0
+├── build_dep_pm v1.0.0
+├── itarget_build_dep v1.0.0
+└── itarget_build_dep_pm v1.0.0
+[dev-dependencies]
+├── dev_dep v1.0.0
+├── dev_dep_pm v1.0.0
+├── itarget_dev_dep v1.0.0
+└── itarget_dev_dep_pm v1.0.0
+",
+        )
+        .run();
+    clear();
+}