From d73330d5a40910ec259036d52257434ad3ded70e Mon Sep 17 00:00:00 2001 From: Roman Volosatovs Date: Tue, 13 Dec 2022 12:17:07 +0100 Subject: [PATCH] fix: deduplicate dependencies by artifact target Signed-off-by: Roman Volosatovs --- src/cargo/core/compiler/standard_lib.rs | 1 + src/cargo/core/compiler/unit.rs | 17 +++++++- src/cargo/core/compiler/unit_dependencies.rs | 5 +++ src/cargo/ops/cargo_compile/mod.rs | 43 +++++++++++-------- src/cargo/ops/cargo_compile/unit_generator.rs | 1 + 5 files changed, 48 insertions(+), 19 deletions(-) diff --git a/src/cargo/core/compiler/standard_lib.rs b/src/cargo/core/compiler/standard_lib.rs index d1ff7fbd343a..2955dce05c5e 100644 --- a/src/cargo/core/compiler/standard_lib.rs +++ b/src/cargo/core/compiler/standard_lib.rs @@ -214,6 +214,7 @@ pub fn generate_std_roots( /*is_std*/ true, /*dep_hash*/ 0, IsArtifact::No, + None, )); } } diff --git a/src/cargo/core/compiler/unit.rs b/src/cargo/core/compiler/unit.rs index 32a98cb8cd70..c2da4b985c62 100644 --- a/src/cargo/core/compiler/unit.rs +++ b/src/cargo/core/compiler/unit.rs @@ -1,6 +1,8 @@ -use crate::core::compiler::{unit_dependencies::IsArtifact, CompileKind, CompileMode, CrateType}; +use crate::core::compiler::unit_dependencies::IsArtifact; +use crate::core::compiler::{CompileKind, CompileMode, CompileTarget, CrateType}; use crate::core::manifest::{Target, TargetKind}; -use crate::core::{profiles::Profile, Package}; +use crate::core::profiles::Profile; +use crate::core::Package; use crate::util::hex::short_hash; use crate::util::interning::InternedString; use crate::util::Config; @@ -72,6 +74,11 @@ pub struct UnitInner { /// This value initially starts as 0, and then is filled in via a /// second-pass after all the unit dependencies have been computed. pub dep_hash: u64, + + /// This is used for target-dependent feature resolution and is copied from + /// [`FeaturesFor::ArtifactDep`](crate::core::resolver::features::FeaturesFor::ArtifactDep), if the enum + /// matches the variant. + pub artifact_target_for_features: Option, } impl UnitInner { @@ -139,6 +146,10 @@ impl fmt::Debug for Unit { .field("mode", &self.mode) .field("features", &self.features) .field("artifact", &self.artifact.is_true()) + .field( + "artifact_target_for_features", + &self.artifact_target_for_features, + ) .field("is_std", &self.is_std) .field("dep_hash", &self.dep_hash) .finish() @@ -184,6 +195,7 @@ impl UnitInterner { is_std: bool, dep_hash: u64, artifact: IsArtifact, + artifact_target_for_features: Option, ) -> Unit { let target = match (is_std, target.kind()) { // This is a horrible hack to support build-std. `libstd` declares @@ -216,6 +228,7 @@ impl UnitInterner { is_std, dep_hash, artifact, + artifact_target_for_features, }); Unit { inner } } diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index 28a47b0dee87..8d87f4af655a 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -885,6 +885,10 @@ fn new_unit_dep_with_profile( .resolve() .is_public_dep(parent.pkg.package_id(), pkg.package_id()); let features_for = unit_for.map_to_features_for(artifact); + let artifact_target = match features_for { + FeaturesFor::ArtifactDep(target) => Some(target), + _ => None, + }; let features = state.activated_features(pkg.package_id(), features_for); let unit = state.interner.intern( pkg, @@ -896,6 +900,7 @@ fn new_unit_dep_with_profile( state.is_std, /*dep_hash*/ 0, artifact.map_or(IsArtifact::No, |_| IsArtifact::Yes), + artifact_target, ); Ok(UnitDep { unit, diff --git a/src/cargo/ops/cargo_compile/mod.rs b/src/cargo/ops/cargo_compile/mod.rs index b520275bf872..049f0d20a03e 100644 --- a/src/cargo/ops/cargo_compile/mod.rs +++ b/src/cargo/ops/cargo_compile/mod.rs @@ -424,20 +424,24 @@ pub fn create_bcx<'a, 'cfg>( remove_duplicate_doc(build_config, &units, &mut unit_graph); } - if build_config + let host_kind_requested = build_config .requested_kinds .iter() - .any(CompileKind::is_host) + .any(CompileKind::is_host); + if host_kind_requested + || unit_graph + .iter() + .any(|(unit, _)| unit.artifact_target_for_features.is_some()) { // Rebuild the unit graph, replacing the explicit host targets with - // CompileKind::Host, merging any dependencies shared with build - // dependencies. + // CompileKind::Host, removing `artifact_target_for_features` and merging any dependencies + // shared with build and artifact dependencies. let new_graph = rebuild_unit_graph_shared( interner, unit_graph, &units, &scrape_units, - explicit_host_kind, + host_kind_requested.then_some(explicit_host_kind), ); // This would be nicer with destructuring assignment. units = new_graph.0; @@ -549,29 +553,31 @@ pub fn create_bcx<'a, 'cfg>( /// This is used to rebuild the unit graph, sharing host dependencies if possible. /// /// This will translate any unit's `CompileKind::Target(host)` to -/// `CompileKind::Host` if the kind is equal to `to_host`. This also handles -/// generating the unit `dep_hash`, and merging shared units if possible. +/// `CompileKind::Host` if `to_host` is not `None` and the kind is equal to `to_host`. +/// This also handles generating the unit `dep_hash`, and merging shared units if possible. /// /// This is necessary because if normal dependencies used `CompileKind::Host`, /// there would be no way to distinguish those units from build-dependency -/// units. This can cause a problem if a shared normal/build dependency needs +/// units or artifact dependency units. +/// This can cause a problem if a shared normal/build/artifact dependency needs /// to link to another dependency whose features differ based on whether or -/// not it is a normal or build dependency. If both units used +/// not it is a normal, build or artifact dependency. If all units used /// `CompileKind::Host`, then they would end up being identical, causing a /// collision in the `UnitGraph`, and Cargo would end up randomly choosing one /// value or the other. /// -/// The solution is to keep normal and build dependencies separate when +/// The solution is to keep normal, build and artifact dependencies separate when /// building the unit graph, and then run this second pass which will try to /// combine shared dependencies safely. By adding a hash of the dependencies /// to the `Unit`, this allows the `CompileKind` to be changed back to `Host` -/// without fear of an unwanted collision. +/// and `artifact_target_for_features` to be removed without fear of an unwanted +/// collision for build or artifact dependencies. fn rebuild_unit_graph_shared( interner: &UnitInterner, unit_graph: UnitGraph, roots: &[Unit], scrape_units: &[Unit], - to_host: CompileKind, + to_host: Option, ) -> (Vec, Vec, UnitGraph) { let mut result = UnitGraph::new(); // Map of the old unit to the new unit, used to avoid recursing into units @@ -604,7 +610,7 @@ fn traverse_and_share( new_graph: &mut UnitGraph, unit_graph: &UnitGraph, unit: &Unit, - to_host: CompileKind, + to_host: Option, ) -> Unit { if let Some(new_unit) = memo.get(unit) { // Already computed, no need to recompute. @@ -624,10 +630,9 @@ fn traverse_and_share( }) .collect(); let new_dep_hash = dep_hash.finish(); - let new_kind = if unit.kind == to_host { - CompileKind::Host - } else { - unit.kind + let new_kind = match to_host { + Some(to_host) if to_host == unit.kind => CompileKind::Host, + _ => unit.kind, }; let new_unit = interner.intern( &unit.pkg, @@ -639,6 +644,9 @@ fn traverse_and_share( unit.is_std, new_dep_hash, unit.artifact, + // Since `dep_hash` is now filled in, there's no need to specify the artifact target + // for target-dependent feature resolution + None, ); assert!(memo.insert(unit.clone(), new_unit.clone()).is_none()); new_graph.entry(new_unit.clone()).or_insert(new_deps); @@ -797,6 +805,7 @@ fn override_rustc_crate_types( unit.is_std, unit.dep_hash, unit.artifact, + unit.artifact_target_for_features, ) }; units[0] = match unit.target.kind() { diff --git a/src/cargo/ops/cargo_compile/unit_generator.rs b/src/cargo/ops/cargo_compile/unit_generator.rs index 84e5d2757a43..46c1488f5b60 100644 --- a/src/cargo/ops/cargo_compile/unit_generator.rs +++ b/src/cargo/ops/cargo_compile/unit_generator.rs @@ -171,6 +171,7 @@ impl<'a> UnitGenerator<'a, '_> { /*is_std*/ false, /*dep_hash*/ 0, IsArtifact::No, + None, ) }) .collect()