From 615464518639de308153a2e60b4a6d4801f87034 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sun, 2 Feb 2020 11:54:29 -0800 Subject: [PATCH] Rewrite new feature resolver to simplify DepKind handling. Now that dev-dependencies are a global setting, only build-dependencies matter. This is maybe a little simpler to understand? --- .../compiler/context/compilation_files.rs | 4 +- src/cargo/core/compiler/standard_lib.rs | 7 +- src/cargo/core/compiler/unit_dependencies.rs | 112 ++++------- src/cargo/core/dependency.rs | 17 -- src/cargo/core/profiles.rs | 104 ++++++---- src/cargo/core/resolver/features.rs | 186 +++++++----------- src/cargo/ops/cargo_clean.rs | 13 +- src/cargo/ops/cargo_compile.rs | 21 +- src/cargo/ops/cargo_output_metadata.rs | 2 +- src/cargo/ops/resolve.rs | 2 +- tests/testsuite/profile_config.rs | 2 +- 11 files changed, 187 insertions(+), 283 deletions(-) diff --git a/src/cargo/core/compiler/context/compilation_files.rs b/src/cargo/core/compiler/context/compilation_files.rs index b9a08bc70..313935d8e 100644 --- a/src/cargo/core/compiler/context/compilation_files.rs +++ b/src/cargo/core/compiler/context/compilation_files.rs @@ -147,7 +147,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> { /// Returns `None` if the unit should not use a metadata data hash (like /// rustdoc, or some dylibs). pub fn metadata(&self, unit: &Unit<'a>) -> Option { - self.metas[unit].clone() + self.metas[unit] } /// Gets the short hash based only on the `PackageId`. @@ -515,7 +515,7 @@ fn metadata_of<'a, 'cfg>( metadata_of(&dep.unit, cx, metas); } } - metas[unit].clone() + metas[unit] } fn compute_metadata<'a, 'cfg>( diff --git a/src/cargo/core/compiler/standard_lib.rs b/src/cargo/core/compiler/standard_lib.rs index f4bd0345c..80048f978 100644 --- a/src/cargo/core/compiler/standard_lib.rs +++ b/src/cargo/core/compiler/standard_lib.rs @@ -1,7 +1,6 @@ //! Code for building the standard library. use crate::core::compiler::{BuildContext, CompileKind, CompileMode, RustcTargetData, Unit}; -use crate::core::dependency::DepKind; use crate::core::profiles::UnitFor; use crate::core::resolver::features::ResolvedFeatures; use crate::core::resolver::ResolveOpts; @@ -149,11 +148,7 @@ pub fn generate_std_roots<'a>( unit_for, mode, ); - let features = std_features.activated_features( - pkg.package_id(), - DepKind::Normal, - bcx.build_config.requested_kind, - ); + let features = std_features.activated_features(pkg.package_id(), false); Ok(bcx.units.intern( pkg, lib, profile, kind, mode, features, /*is_std*/ true, )) diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index 9ea46bc1f..b84ff7e70 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -178,7 +178,7 @@ fn deps_of_roots<'a, 'cfg>(roots: &[Unit<'a>], mut state: &mut State<'a, 'cfg>) } else if unit.target.is_custom_build() { // This normally doesn't happen, except `clean` aggressively // generates all units. - UnitFor::new_build() + UnitFor::new_build(false) } else if unit.target.for_host() { // Proc macro / plugin should never have panic set. UnitFor::new_compiler() @@ -230,7 +230,7 @@ fn compute_deps<'a, 'cfg>( unit_for: UnitFor, ) -> CargoResult>> { if unit.mode.is_run_custom_build() { - return compute_deps_custom_build(unit, unit_for.dep_kind(), state); + return compute_deps_custom_build(unit, unit_for, state); } else if unit.mode.is_doc() { // Note: this does not include doc test. return compute_deps_doc(unit, state); @@ -238,45 +238,40 @@ fn compute_deps<'a, 'cfg>( let bcx = state.bcx; let id = unit.pkg.package_id(); - let filtered_deps = state - .resolve() - .deps(id) - .map(|(id, deps)| { - assert!(!deps.is_empty()); - let filtered_deps = deps.iter().filter(|dep| { - // If this target is a build command, then we only want build - // dependencies, otherwise we want everything *other than* build - // dependencies. - if unit.target.is_custom_build() != dep.is_build() { - return false; - } + let filtered_deps = state.resolve().deps(id).filter(|&(_id, deps)| { + assert!(!deps.is_empty()); + deps.iter().any(|dep| { + // If this target is a build command, then we only want build + // dependencies, otherwise we want everything *other than* build + // dependencies. + if unit.target.is_custom_build() != dep.is_build() { + return false; + } - // If this dependency is **not** a transitive dependency, then it - // only applies to test/example targets. - if !dep.is_transitive() - && !unit.target.is_test() - && !unit.target.is_example() - && !unit.mode.is_any_test() - { - return false; - } + // If this dependency is **not** a transitive dependency, then it + // only applies to test/example targets. + if !dep.is_transitive() + && !unit.target.is_test() + && !unit.target.is_example() + && !unit.mode.is_any_test() + { + return false; + } - // If this dependency is only available for certain platforms, - // make sure we're only enabling it for that platform. - if !bcx.target_data.dep_platform_activated(dep, unit.kind) { - return false; - } + // If this dependency is only available for certain platforms, + // make sure we're only enabling it for that platform. + if !bcx.target_data.dep_platform_activated(dep, unit.kind) { + return false; + } - // If we've gotten past all that, then this dependency is - // actually used! - true - }); - (id, filtered_deps.collect::>()) + // If we've gotten past all that, then this dependency is + // actually used! + true }) - .filter(|(_id, deps)| !deps.is_empty()); + }); let mut ret = Vec::new(); - for (id, deps) in filtered_deps { + for (id, _) in filtered_deps { let pkg = match state.get(id)? { Some(pkg) => pkg, None => continue, @@ -286,23 +281,10 @@ fn compute_deps<'a, 'cfg>( None => continue, }; let mode = check_or_build_mode(unit.mode, lib); - let dep_kind = if unit.target.is_custom_build() { - // Custom build scripts can *only* have build-dependencies. - DepKind::Build - } else if deps.iter().any(|dep| !dep.is_transitive()) { - // A dependency can be listed in both [dependencies] and - // [dev-dependencies], so this checks if any of the deps are - // listed in dev-dependencies. Note that `filtered_deps` has - // already removed dev-dependencies if it is not a - // test/bench/example, so it is not necessary to check `unit` - // here. - DepKind::Development - } else { - DepKind::Normal - }; let dep_unit_for = unit_for .with_for_host(lib.for_host()) - .with_dep_kind(dep_kind); + // If it is a custom build script, then it *only* has build dependencies. + .with_build_dep(unit.target.is_custom_build()); if bcx.config.cli_unstable().dual_proc_macros && lib.proc_macro() && !unit.kind.is_host() { let unit_dep = new_unit_dep(state, unit, pkg, lib, dep_unit_for, unit.kind, mode)?; @@ -330,7 +312,7 @@ fn compute_deps<'a, 'cfg>( if unit.target.is_custom_build() { return Ok(ret); } - ret.extend(dep_build_script(unit, unit_for.dep_kind(), state)?); + ret.extend(dep_build_script(unit, unit_for, state)?); // If this target is a binary, test, example, etc, then it depends on // the library of the same package. The call to `resolve.deps` above @@ -382,14 +364,9 @@ fn compute_deps<'a, 'cfg>( /// /// The `unit` provided must represent an execution of a build script, and /// the returned set of units must all be run before `unit` is run. -/// -/// `dep_kind` is the dependency kind of the package this build script is -/// being built for. This ensures that the build script is built with the same -/// features the package is built with (if the build script has cfg!() -/// checks). fn compute_deps_custom_build<'a, 'cfg>( unit: &Unit<'a>, - dep_kind: DepKind, + unit_for: UnitFor, state: &mut State<'a, 'cfg>, ) -> CargoResult>> { if let Some(links) = unit.pkg.manifest().links() { @@ -400,7 +377,7 @@ fn compute_deps_custom_build<'a, 'cfg>( } // All dependencies of this unit should use profiles for custom // builds. - let unit_for = UnitFor::new_build().with_dep_kind(dep_kind); + let script_unit_for = UnitFor::new_build(unit_for.is_for_build_dep()); // When not overridden, then the dependencies to run a build script are: // // 1. Compiling the build script itself. @@ -415,7 +392,7 @@ fn compute_deps_custom_build<'a, 'cfg>( unit, unit.pkg, unit.target, - unit_for, + script_unit_for, // Build scripts always compiled for the host. CompileKind::Host, CompileMode::Build, @@ -482,7 +459,7 @@ fn compute_deps_doc<'a, 'cfg>( } // Be sure to build/run the build script for documented libraries. - ret.extend(dep_build_script(unit, DepKind::Normal, state)?); + ret.extend(dep_build_script(unit, UnitFor::new_normal(), state)?); // If we document a binary/example, we need the library available. if unit.target.is_bin() || unit.target.is_example() { @@ -524,7 +501,7 @@ fn maybe_lib<'a>( /// build script. fn dep_build_script<'a>( unit: &Unit<'a>, - dep_kind: DepKind, + unit_for: UnitFor, state: &State<'a, '_>, ) -> CargoResult>> { unit.pkg @@ -538,7 +515,7 @@ fn dep_build_script<'a>( .bcx .profiles .get_profile_run_custom_build(&unit.profile); - let script_unit_for = UnitFor::new_build().with_dep_kind(dep_kind); + let script_unit_for = UnitFor::new_build(unit_for.is_for_build_dep()); new_unit_dep_with_profile( state, unit, @@ -609,7 +586,7 @@ fn new_unit_dep_with_profile<'a>( let public = state .resolve() .is_public_dep(parent.pkg.package_id(), pkg.package_id()); - let features = state.activated_features(pkg.package_id(), unit_for.dep_kind(), kind); + let features = state.activated_features(pkg.package_id(), unit_for.is_for_build_dep()); let unit = state .bcx .units @@ -714,18 +691,13 @@ impl<'a, 'cfg> State<'a, 'cfg> { } } - fn activated_features( - &self, - pkg_id: PackageId, - dep_kind: DepKind, - compile_kind: CompileKind, - ) -> Vec { + fn activated_features(&self, pkg_id: PackageId, for_build_dep: bool) -> Vec { let features = if self.is_std { self.std_features.unwrap() } else { self.usr_features }; - features.activated_features(pkg_id, dep_kind, compile_kind) + features.activated_features(pkg_id, for_build_dep) } fn get(&mut self, id: PackageId) -> CargoResult> { diff --git a/src/cargo/core/dependency.rs b/src/cargo/core/dependency.rs index 5eff3cd50..82da06129 100644 --- a/src/cargo/core/dependency.rs +++ b/src/cargo/core/dependency.rs @@ -92,23 +92,6 @@ pub enum DepKind { Build, } -impl DepKind { - /// Convert a DepKind from a package to one of its dependencies. - /// - /// The rules here determine how feature decoupling works. This works in - /// conjunction with the new feature resolver. - pub fn sticky_kind(&self, to: DepKind) -> DepKind { - use DepKind::*; - match (self, to) { - (Normal, _) => to, - (Build, _) => Build, - (Development, Normal) => Development, - (Development, Build) => Build, - (Development, Development) => Development, - } - } -} - fn parse_req_with_deprecated( name: InternedString, req: &str, diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index 9bdfafeb8..9c079a7cd 100644 --- a/src/cargo/core/profiles.rs +++ b/src/cargo/core/profiles.rs @@ -1,5 +1,4 @@ use crate::core::compiler::CompileMode; -use crate::core::dependency::DepKind; use crate::core::interning::InternedString; use crate::core::{Feature, Features, PackageId, PackageIdSpec, Resolve, Shell}; use crate::util::errors::CargoResultExt; @@ -484,7 +483,7 @@ fn merge_toml_overrides( profile: &mut Profile, toml: &TomlProfile, ) { - if unit_for.is_build() { + if unit_for.is_for_host() { if let Some(ref build_override) = toml.build_override { merge_profile(profile, build_override); } @@ -767,9 +766,17 @@ pub struct UnitFor { /// A target for `build.rs` or any of its dependencies, or a proc-macro or /// any of its dependencies. This enables `build-override` profiles for /// these targets. - build: bool, - /// The dependency kind (normal, dev, build). - dep_kind: DepKind, + host: bool, + /// A target for a build dependency (or any of its dependencies). This is + /// used for computing features of build dependencies independently of + /// other dependency kinds. + /// + /// The subtle difference between this and `host` is that the build script + /// for a non-host package sets this to `false` because it wants the + /// features of the non-host package (whereas `host` is true because the + /// build script is being built for the host). `build_dep` becomes `true` + /// for build-dependencies, or any of their dependencies. + build_dep: bool, /// How Cargo processes the `panic` setting or profiles. This is done to /// handle test/benches inheriting from dev/release, as well as forcing /// `for_host` units to always unwind. @@ -796,17 +803,22 @@ impl UnitFor { /// proc macro/plugin, or test/bench). pub fn new_normal() -> UnitFor { UnitFor { - build: false, - dep_kind: DepKind::Normal, + host: false, + build_dep: false, panic_setting: PanicSetting::ReadProfile, } } /// A unit for a custom build script or its dependencies. - pub fn new_build() -> UnitFor { + /// + /// The `build_dep` parameter is whether or not this is for a build + /// dependency. Build scripts for non-host units should use `false` + /// because they want to use the features of the package they are running + /// for. + pub fn new_build(build_dep: bool) -> UnitFor { UnitFor { - build: true, - dep_kind: DepKind::Normal, + host: true, + build_dep, // Force build scripts to always use `panic=unwind` for now to // maximally share dependencies with procedural macros. panic_setting: PanicSetting::AlwaysUnwind, @@ -816,8 +828,8 @@ impl UnitFor { /// A unit for a proc macro or compiler plugin or their dependencies. pub fn new_compiler() -> UnitFor { UnitFor { - build: false, - dep_kind: DepKind::Normal, + host: false, + build_dep: false, // Force plugins to use `panic=abort` so panics in the compiler do // not abort the process but instead end with a reasonable error // message that involves catching the panic in the compiler. @@ -825,7 +837,7 @@ impl UnitFor { } } - /// A unit for a test/bench target. + /// A unit for a test/bench target or their dependencies. /// /// Note that `config` is taken here for unstable CLI features to detect /// whether `panic=abort` is supported for tests. Historical versions of @@ -833,8 +845,8 @@ impl UnitFor { /// compiler flag. pub fn new_test(config: &Config) -> UnitFor { UnitFor { - build: false, - dep_kind: DepKind::Development, + host: false, + build_dep: false, // We're testing out an unstable feature (`-Zpanic-abort-tests`) // which inherits the panic setting from the dev/release profile // (basically avoid recompiles) but historical defaults required @@ -847,7 +859,7 @@ impl UnitFor { } } - /// Creates a variant based on `for_host` setting. + /// Returns a new copy based on `for_host` setting. /// /// When `for_host` is true, this clears `panic_abort_ok` in a sticky /// fashion so that all its dependencies also have `panic_abort_ok=false`. @@ -856,8 +868,8 @@ impl UnitFor { /// graph where everything is `panic=unwind`. pub fn with_for_host(self, for_host: bool) -> UnitFor { UnitFor { - build: self.build || for_host, - dep_kind: self.dep_kind, + host: self.host || for_host, + build_dep: self.build_dep, panic_setting: if for_host { PanicSetting::AlwaysUnwind } else { @@ -866,55 +878,65 @@ impl UnitFor { } } - /// Creates a variant for the given dependency kind. + /// Returns a new copy updating it for a build dependency. /// /// This is part of the machinery responsible for handling feature - /// decoupling in the new feature resolver. - pub fn with_dep_kind(mut self, kind: DepKind) -> UnitFor { - self.dep_kind = self.dep_kind.sticky_kind(kind); + /// decoupling for build dependencies in the new feature resolver. + pub fn with_build_dep(mut self, build_dep: bool) -> UnitFor { + self.build_dep = self.build_dep || build_dep; self } - /// Returns `true` if this unit is for a custom build script or one of its - /// dependencies. - pub fn is_build(self) -> bool { - self.build + /// Returns `true` if this unit is for a build script or any of its + /// dependencies, or a proc macro or any of its dependencies. + pub fn is_for_host(&self) -> bool { + self.host } - /// Returns how `panic` settings should be handled for this profile - fn panic_setting(self) -> PanicSetting { - self.panic_setting + pub fn is_for_build_dep(&self) -> bool { + self.build_dep } - /// Returns the dependency kind this unit is intended for. - pub fn dep_kind(&self) -> DepKind { - self.dep_kind + /// Returns how `panic` settings should be handled for this profile + fn panic_setting(&self) -> PanicSetting { + self.panic_setting } /// All possible values, used by `clean`. pub fn all_values() -> &'static [UnitFor] { - // dep_kind isn't needed for profiles, so its value doesn't matter. static ALL: &[UnitFor] = &[ UnitFor { - build: false, - dep_kind: DepKind::Normal, + host: false, + build_dep: false, panic_setting: PanicSetting::ReadProfile, }, UnitFor { - build: true, - dep_kind: DepKind::Normal, + host: true, + build_dep: false, panic_setting: PanicSetting::AlwaysUnwind, }, UnitFor { - build: false, - dep_kind: DepKind::Normal, + host: false, + build_dep: false, panic_setting: PanicSetting::AlwaysUnwind, }, UnitFor { - build: false, - dep_kind: DepKind::Normal, + host: false, + build_dep: false, panic_setting: PanicSetting::Inherit, }, + // build_dep=true must always have host=true + // `Inherit` is not used in build dependencies. + UnitFor { + host: true, + build_dep: true, + panic_setting: PanicSetting::ReadProfile, + }, + UnitFor { + host: true, + build_dep: true, + panic_setting: PanicSetting::AlwaysUnwind, + }, ]; ALL } diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index deeb77590..490ff58a6 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -25,15 +25,11 @@ use crate::util::{CargoResult, Config}; use std::collections::{BTreeSet, HashMap, HashSet}; use std::rc::Rc; -/// Map of activated features for a PackageId/DepKind/CompileKind. +/// Map of activated features. /// -/// `DepKind` is needed, as the same package can be built multiple times with -/// different features. For example, with `decouple_build_deps`, a dependency -/// can be built once as a build dependency (for example with a 'std' -/// feature), and once as a normal dependency (without that 'std' feature). -/// -/// `CompileKind` is used currently not needed. -type ActivateMap = HashMap<(PackageId, DepKind, CompileKind), BTreeSet>; +/// The key is `(PackageId, bool)` where the bool is `true` if these +/// are features for a build dependency. +type ActivateMap = HashMap<(PackageId, bool), BTreeSet>; /// Set of all activated features for all packages in the resolve graph. pub struct ResolvedFeatures { @@ -154,28 +150,16 @@ impl RequestedFeatures { impl ResolvedFeatures { /// Returns the list of features that are enabled for the given package. - pub fn activated_features( - &self, - pkg_id: PackageId, - dep_kind: DepKind, - compile_kind: CompileKind, - ) -> Vec { + pub fn activated_features(&self, pkg_id: PackageId, is_build: bool) -> Vec { if let Some(legacy) = &self.legacy { legacy.get(&pkg_id).map_or_else(Vec::new, |v| v.clone()) } else { - let dep_kind = if (!self.opts.decouple_build_deps && dep_kind == DepKind::Build) - || (!self.opts.decouple_dev_deps && dep_kind == DepKind::Development) - { - // Decoupling disabled, everything is unified under "Normal". - DepKind::Normal - } else { - dep_kind - }; + let is_build = self.opts.decouple_build_deps && is_build; // TODO: Remove panic, return empty set. let fs = self .activated_features - .get(&(pkg_id, dep_kind, compile_kind)) - .unwrap_or_else(|| panic!("features did not find {:?} {:?}", pkg_id, dep_kind)); + .get(&(pkg_id, is_build)) + .unwrap_or_else(|| panic!("features did not find {:?} {:?}", pkg_id, is_build)); fs.iter().cloned().collect() } } @@ -193,7 +177,7 @@ pub struct FeatureResolver<'a, 'cfg> { activated_features: ActivateMap, /// Keeps track of which packages have had its dependencies processed. /// Used to avoid cycles, and to speed up processing. - processed_deps: HashSet<(PackageId, DepKind, CompileKind)>, + processed_deps: HashSet<(PackageId, bool)>, } impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { @@ -248,106 +232,72 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { requested_features: &RequestedFeatures, ) -> CargoResult<()> { let member_features = self.ws.members_with_features(specs, requested_features)?; - for (member, requested_features) in member_features { - self.activate_member(member.package_id(), &requested_features)?; - } - Ok(()) - } - - /// Enable the given features on the given workspace member. - fn activate_member( - &mut self, - pkg_id: PackageId, - requested_features: &RequestedFeatures, - ) -> CargoResult<()> { - let fvs = self.fvs_from_requested(pkg_id, CompileKind::Host, requested_features); - self.activate_member_fvs(pkg_id, CompileKind::Host, &fvs)?; - if let CompileKind::Target(_) = self.requested_target { - let fvs = self.fvs_from_requested(pkg_id, self.requested_target, requested_features); - self.activate_member_fvs(pkg_id, self.requested_target, &fvs)?; - } - Ok(()) - } - - fn activate_member_fvs( - &mut self, - pkg_id: PackageId, - compile_kind: CompileKind, - fvs: &[FeatureValue], - ) -> CargoResult<()> { - self.activate_with_platform(pkg_id, DepKind::Normal, compile_kind, &fvs)?; - if self.opts.decouple_dev_deps { - // Activate the member as a dev dep, assuming it has at least one - // test, bench, or example. This ensures the member's normal deps get - // unified with its dev deps. - self.activate_with_platform(pkg_id, DepKind::Development, compile_kind, &fvs)?; + for (member, requested_features) in &member_features { + let fvs = self.fvs_from_requested(member.package_id(), requested_features); + self.activate_pkg(member.package_id(), &fvs, false)?; } Ok(()) } - fn activate_with_platform( + fn activate_pkg( &mut self, pkg_id: PackageId, - dep_kind: DepKind, - compile_kind: CompileKind, fvs: &[FeatureValue], + for_build: bool, ) -> CargoResult<()> { // Add an empty entry to ensure everything is covered. This is intended for // finding bugs where the resolver missed something it should have visited. // Remove this in the future if `activated_features` uses an empty default. self.activated_features - .entry((pkg_id, dep_kind, compile_kind)) + .entry((pkg_id, for_build)) .or_insert_with(BTreeSet::new); for fv in fvs { - self.activate_fv(pkg_id, dep_kind, compile_kind, fv)?; + self.activate_fv(pkg_id, fv, for_build)?; } - if !self.processed_deps.insert((pkg_id, dep_kind, compile_kind)) { + if !self.processed_deps.insert((pkg_id, for_build)) { // Already processed dependencies. return Ok(()); } - // Activate any of its dependencies. - for (dep_pkg_id, deps) in self.deps(pkg_id, compile_kind) { + for (dep_pkg_id, deps) in self.deps(pkg_id, for_build) { for dep in deps { if dep.is_optional() { continue; } // Recurse into the dependency. let fvs = self.fvs_from_dependency(dep_pkg_id, dep); - self.activate_with_platform( + self.activate_pkg( dep_pkg_id, - self.sticky_dep_kind(dep_kind, dep.kind()), - compile_kind, &fvs, + for_build || (self.opts.decouple_build_deps && dep.is_build()), )?; } } - return Ok(()); + Ok(()) } + /// Activate a single FeatureValue for a package. fn activate_fv( &mut self, pkg_id: PackageId, - dep_kind: DepKind, - compile_kind: CompileKind, fv: &FeatureValue, + for_build: bool, ) -> CargoResult<()> { match fv { FeatureValue::Feature(f) => { - self.activate_rec(pkg_id, dep_kind, compile_kind, *f)?; + self.activate_rec(pkg_id, *f, for_build)?; } FeatureValue::Crate(dep_name) => { // Activate the feature name on self. - self.activate_rec(pkg_id, dep_kind, compile_kind, *dep_name)?; + self.activate_rec(pkg_id, *dep_name, for_build)?; // Activate the optional dep. - for (dep_pkg_id, deps) in self.deps(pkg_id, compile_kind) { + for (dep_pkg_id, deps) in self.deps(pkg_id, for_build) { for dep in deps { if dep.name_in_toml() == *dep_name { let fvs = self.fvs_from_dependency(dep_pkg_id, dep); - self.activate_with_platform( + self.activate_pkg( dep_pkg_id, - self.sticky_dep_kind(dep_kind, dep.kind()), - compile_kind, &fvs, + for_build || (self.opts.decouple_build_deps && dep.is_build()), )?; } } @@ -355,22 +305,21 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { } FeatureValue::CrateFeature(dep_name, dep_feature) => { // Activate a feature within a dependency. - for (dep_pkg_id, deps) in self.deps(pkg_id, compile_kind) { + for (dep_pkg_id, deps) in self.deps(pkg_id, for_build) { for dep in deps { if dep.name_in_toml() == *dep_name { if dep.is_optional() { // Activate the crate on self. let fv = FeatureValue::Crate(*dep_name); - self.activate_fv(pkg_id, dep_kind, compile_kind, &fv)?; + self.activate_fv(pkg_id, &fv, for_build)?; } // Activate the feature on the dependency. let summary = self.resolve.summary(dep_pkg_id); let fv = FeatureValue::new(*dep_feature, summary); self.activate_fv( dep_pkg_id, - self.sticky_dep_kind(dep_kind, dep.kind()), - compile_kind, &fv, + for_build || (self.opts.decouple_build_deps && dep.is_build()), )?; } } @@ -385,13 +334,12 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { fn activate_rec( &mut self, pkg_id: PackageId, - dep_kind: DepKind, - compile_kind: CompileKind, feature_to_enable: InternedString, + for_build: bool, ) -> CargoResult<()> { let enabled = self .activated_features - .entry((pkg_id, dep_kind, compile_kind)) + .entry((pkg_id, for_build)) .or_insert_with(BTreeSet::new); if !enabled.insert(feature_to_enable) { // Already enabled. @@ -414,7 +362,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { } }; for fv in fvs { - self.activate_fv(pkg_id, dep_kind, compile_kind, fv)?; + self.activate_fv(pkg_id, fv, for_build)?; } Ok(()) } @@ -439,7 +387,6 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { fn fvs_from_requested( &self, pkg_id: PackageId, - compile_kind: CompileKind, requested_features: &RequestedFeatures, ) -> Vec { let summary = self.resolve.summary(pkg_id); @@ -450,7 +397,9 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { .map(|k| FeatureValue::Feature(*k)) .collect(); // Add optional deps. - for (_dep_pkg_id, deps) in self.deps(pkg_id, compile_kind) { + // Top-level requested features can never apply to + // build-dependencies, so for_build is `false` here. + for (_dep_pkg_id, deps) in self.deps(pkg_id, false) { for dep in deps { if dep.is_optional() { // This may result in duplicates, but that should be ok. @@ -475,56 +424,55 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { } /// Returns the dependencies for a package, filtering out inactive targets. - fn deps( - &self, - pkg_id: PackageId, - compile_kind: CompileKind, - ) -> Vec<(PackageId, Vec<&'a Dependency>)> { + fn deps(&self, pkg_id: PackageId, for_build: bool) -> Vec<(PackageId, Vec<&'a Dependency>)> { + // Helper for determining if a platform is activated. + let platform_activated = |dep: &Dependency| -> bool { + // We always care about build-dependencies, and they are always + // Host. If we are computing dependencies "for a build script", + // even normal dependencies are host-only. + if for_build || dep.is_build() { + return self + .target_data + .dep_platform_activated(dep, CompileKind::Host); + } + // Not a build dependency, and not for a build script, so must be Target. + return self + .target_data + .dep_platform_activated(dep, self.requested_target); + }; self.resolve .deps(pkg_id) .map(|(dep_id, deps)| { let deps = deps .iter() .filter(|dep| { - !dep.platform().is_some() - || !self.opts.ignore_inactive_targets - || self.target_data.dep_platform_activated(dep, compile_kind) + if dep.platform().is_some() + && self.opts.ignore_inactive_targets + && !platform_activated(dep) + { + return false; + } + if self.opts.decouple_dev_deps && dep.kind() == DepKind::Development { + return false; + } + true }) .collect::>(); (dep_id, deps) }) + .filter(|(_id, deps)| !deps.is_empty()) .collect() } - /// Convert a DepKind from a package to one of its dependencies. - /// - /// The rules here determine how decoupling works. - fn sticky_dep_kind(&self, from: DepKind, to: DepKind) -> DepKind { - if self.opts.decouple_build_deps { - if from == DepKind::Build || to == DepKind::Build { - return DepKind::Build; - } - } - if self.opts.decouple_dev_deps { - if to == DepKind::Development { - return DepKind::Development; - } - if from == DepKind::Development && to != DepKind::Build { - return DepKind::Development; - } - } - return DepKind::Normal; - } - /// Compare the activated features to the resolver. Used for testing. fn compare(&self) { let mut found = false; - for ((pkg_id, dep_kind, compile_kind), features) in &self.activated_features { + for ((pkg_id, dep_kind), features) in &self.activated_features { let r_features = self.resolve.features(*pkg_id); if !r_features.iter().eq(features.iter()) { eprintln!( - "{}/{:?}/{:?} features mismatch\nresolve: {:?}\nnew: {:?}\n", - pkg_id, dep_kind, compile_kind, r_features, features + "{}/{:?} features mismatch\nresolve: {:?}\nnew: {:?}\n", + pkg_id, dep_kind, r_features, features ); found = true; } diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index 223a6c573..ea60bd31f 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -6,7 +6,6 @@ use std::path::Path; use crate::core::compiler::unit_dependencies; use crate::core::compiler::{BuildConfig, BuildContext, CompileKind, CompileMode, Context}; use crate::core::compiler::{RustcTargetData, UnitInterner}; -use crate::core::dependency::DepKind; use crate::core::profiles::{Profiles, UnitFor}; use crate::core::resolver::features::{FeatureResolver, RequestedFeatures}; use crate::core::{PackageIdSpec, Workspace}; @@ -117,13 +116,11 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> { *mode, ) }; - for dep_kind in &[DepKind::Normal, DepKind::Development, DepKind::Build] { - let features = - features.activated_features(pkg.package_id(), *dep_kind, *kind); - units.push(bcx.units.intern( - pkg, target, profile, *kind, *mode, features, /*is_std*/ false, - )); - } + let features = features + .activated_features(pkg.package_id(), unit_for.is_for_build_dep()); + units.push(bcx.units.intern( + pkg, target, profile, *kind, *mode, features, /*is_std*/ false, + )); } } } diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 874b60963..b6300055e 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -33,7 +33,6 @@ use crate::core::compiler::unit_dependencies::build_unit_dependencies; use crate::core::compiler::{BuildConfig, BuildContext, Compilation, Context}; use crate::core::compiler::{CompileKind, CompileMode, RustcTargetData, Unit}; use crate::core::compiler::{DefaultExecutor, Executor, UnitInterner}; -use crate::core::dependency::DepKind; use crate::core::profiles::{Profiles, UnitFor}; use crate::core::resolver::features; use crate::core::resolver::{Resolve, ResolveOpts}; @@ -746,13 +745,9 @@ fn generate_targets<'a>( bcx.profiles .get_profile(pkg.package_id(), ws.is_member(pkg), unit_for, target_mode); - // Root units are not a dependency of anything, so they are always - // DepKind::Normal. Their features are driven by the command-line - // arguments. let features = Vec::from(resolved_features.activated_features( pkg.package_id(), - DepKind::Normal, - kind, + false, // Root units are never build dependencies. )); bcx.units.intern( pkg, @@ -903,12 +898,7 @@ fn generate_targets<'a>( let unavailable_features = match target.required_features() { Some(rf) => { let features = features_map.entry(pkg).or_insert_with(|| { - resolve_all_features( - resolve, - resolved_features, - pkg.package_id(), - default_arch_kind, - ) + resolve_all_features(resolve, resolved_features, pkg.package_id()) }); rf.iter().filter(|f| !features.contains(*f)).collect() } @@ -946,10 +936,9 @@ fn resolve_all_features( resolve_with_overrides: &Resolve, resolved_features: &features::ResolvedFeatures, package_id: PackageId, - default_arch_kind: CompileKind, ) -> HashSet { let mut features: HashSet = resolved_features - .activated_features(package_id, DepKind::Normal, default_arch_kind) + .activated_features(package_id, false) .iter() .map(|s| s.to_string()) .collect(); @@ -957,9 +946,7 @@ fn resolve_all_features( // Include features enabled for use by dependencies so targets can also use them with the // required-features field when deciding whether to be built or skipped. for (dep_id, deps) in resolve_with_overrides.deps(package_id) { - for feature in - resolved_features.activated_features(dep_id, DepKind::Normal, default_arch_kind) - { + for feature in resolved_features.activated_features(dep_id, false) { for dep in deps { features.insert(dep.name_in_toml().to_string() + "/" + &feature); } diff --git a/src/cargo/ops/cargo_output_metadata.rs b/src/cargo/ops/cargo_output_metadata.rs index d1f05f65d..3bdddd047 100644 --- a/src/cargo/ops/cargo_output_metadata.rs +++ b/src/cargo/ops/cargo_output_metadata.rs @@ -173,7 +173,7 @@ fn build_resolve_graph_r( if node_map.contains_key(&pkg_id) { return; } - let features = resolve.features(pkg_id).into_iter().cloned().collect(); + let features = resolve.features(pkg_id).iter().cloned().collect(); let deps: Vec = resolve .deps(pkg_id) diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index 905efee84..da9c70591 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -117,7 +117,7 @@ pub fn resolve_ws_with_opts<'a>( let resolved_with_overrides = resolve_with_previous( &mut registry, ws, - &opts, + opts, resolve.as_ref(), None, specs, diff --git a/tests/testsuite/profile_config.rs b/tests/testsuite/profile_config.rs index 5f62c9061..e9928c5ae 100644 --- a/tests/testsuite/profile_config.rs +++ b/tests/testsuite/profile_config.rs @@ -413,7 +413,7 @@ fn named_config_profile() { assert_eq!(p.overflow_checks, true); // "dev" built-in (ignore package override) // build-override - let bo = profiles.get_profile(a_pkg, true, UnitFor::new_build(), CompileMode::Build); + let bo = profiles.get_profile(a_pkg, true, UnitFor::new_build(false), CompileMode::Build); assert_eq!(bo.name, "foo"); assert_eq!(bo.codegen_units, Some(6)); // "foo" build override from config assert_eq!(bo.opt_level, "1"); // SAME as normal -- 2.39.5