Skip to content

Commit

Permalink
Auto merge of rust-lang#8823 - ehuss:minimal-download, r=alexcrichton
Browse files Browse the repository at this point in the history
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 rust-lang#8776
bors committed Nov 3, 2020
2 parents eaab752 + 3d5a908 commit 862df61
Showing 5 changed files with 273 additions and 11 deletions.
19 changes: 13 additions & 6 deletions src/cargo/core/package.rs
Original file line number Diff line number Diff line change
@@ -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())?;
16 changes: 13 additions & 3 deletions src/cargo/core/resolver/features.rs
Original file line number Diff line number Diff line change
@@ -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<_>>();
1 change: 1 addition & 0 deletions src/cargo/ops/resolve.rs
Original file line number Diff line number Diff line change
@@ -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(
2 changes: 2 additions & 0 deletions src/cargo/ops/tree/mod.rs
Original file line number Diff line number Diff line change
@@ -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())?
246 changes: 244 additions & 2 deletions tests/testsuite/features2.rs
Original file line number Diff line number Diff line change
@@ -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();
}

0 comments on commit 862df61

Please sign in to comment.