From 3d5a9083930ac1236d5496b5a0fb53018153a5d6 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 31 Oct 2020 16:49:22 -0700 Subject: [PATCH] Avoid some extra downloads with new feature resolver. --- src/cargo/core/package.rs | 19 ++- src/cargo/core/resolver/features.rs | 16 +- src/cargo/ops/resolve.rs | 1 + src/cargo/ops/tree/mod.rs | 2 + tests/testsuite/features2.rs | 246 +++++++++++++++++++++++++++- 5 files changed, 273 insertions(+), 11 deletions(-) diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index ad240b94102..d0a4f847b31 100644 --- a/src/cargo/core/package.rs +++ b/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, @@ -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())?; diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index 78f0b360849..89765e73feb 100644 --- a/src/cargo/core/resolver/features.rs +++ b/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::>(); diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index fe695fd5794..c54a2bc42df 100644 --- a/src/cargo/ops/resolve.rs +++ b/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( diff --git a/src/cargo/ops/tree/mod.rs b/src/cargo/ops/tree/mod.rs index 7c007778c17..8002655f3b7 100644 --- a/src/cargo/ops/tree/mod.rs +++ b/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 = ws_resolve .pkg_set .get_many(ws_resolve.pkg_set.package_ids())? diff --git a/tests/testsuite/features2.rs b/tests/testsuite/features2.rs index 081ac1c7a3f..3be9474436e 100644 --- a/tests/testsuite/features2.rs +++ b/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(); +}