Skip to content

Avoid some extra downloads with new feature resolver. #8823

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 3, 2020
Merged

Avoid some extra downloads with new feature resolver. #8823

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions src/cargo/core/package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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>,
Expand All @@ -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(());
Expand All @@ -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
})
Expand All @@ -527,6 +532,7 @@ impl<'cfg> PackageSet<'cfg> {
has_dev_units,
requested_kinds,
target_data,
force_all_targets,
)?;
}
Ok(())
Expand All @@ -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())?;
Expand Down
16 changes: 13 additions & 3 deletions src/cargo/core/resolver/features.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down Expand Up @@ -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,
Expand All @@ -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);
Expand All @@ -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
Expand Down Expand Up @@ -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| {
Expand All @@ -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<_>>();
Expand Down
1 change: 1 addition & 0 deletions src/cargo/ops/resolve.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 2 additions & 0 deletions src/cargo/ops/tree/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())?
Expand Down
246 changes: 244 additions & 2 deletions tests/testsuite/features2.rs
View file
Open in desktop
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};
Expand Down Expand Up @@ -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 [..]
Expand All @@ -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();
}