Skip to content
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.

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

fix: deduplicate dependencies by artifact target #11478

Merged
merged 6 commits into from
Dec 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions src/cargo/core/compiler/standard_lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ pub fn generate_std_roots(
/*is_std*/ true,
/*dep_hash*/ 0,
IsArtifact::No,
None,
));
}
}
Expand Down
18 changes: 16 additions & 2 deletions src/cargo/core/compiler/unit.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -72,6 +74,12 @@ 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`], if the enum matches the variant.
///
/// [`FeaturesFor::ArtifactDep`]: crate::core::resolver::features::FeaturesFor::ArtifactDep
pub artifact_target_for_features: Option<CompileTarget>,
}

impl UnitInner {
Expand Down Expand Up @@ -139,6 +147,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()
Expand Down Expand Up @@ -184,6 +196,7 @@ impl UnitInterner {
is_std: bool,
dep_hash: u64,
artifact: IsArtifact,
artifact_target_for_features: Option<CompileTarget>,
) -> Unit {
let target = match (is_std, target.kind()) {
// This is a horrible hack to support build-std. `libstd` declares
Expand Down Expand Up @@ -216,6 +229,7 @@ impl UnitInterner {
is_std,
dep_hash,
artifact,
artifact_target_for_features,
});
Unit { inner }
}
Expand Down
5 changes: 5 additions & 0 deletions src/cargo/core/compiler/unit_dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
46 changes: 28 additions & 18 deletions src/cargo/ops/cargo_compile/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -415,20 +415,25 @@ 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);
let should_share_deps = host_kind_requested
|| config.cli_unstable().bindeps
&& unit_graph
.iter()
.any(|(unit, _)| unit.artifact_target_for_features.is_some());
if should_share_deps {
// 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;
Expand Down Expand Up @@ -540,29 +545,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<CompileKind>,
) -> (Vec<Unit>, Vec<Unit>, UnitGraph) {
let mut result = UnitGraph::new();
// Map of the old unit to the new unit, used to avoid recursing into units
Expand Down Expand Up @@ -595,7 +602,7 @@ fn traverse_and_share(
new_graph: &mut UnitGraph,
unit_graph: &UnitGraph,
unit: &Unit,
to_host: CompileKind,
to_host: Option<CompileKind>,
) -> Unit {
if let Some(new_unit) = memo.get(unit) {
// Already computed, no need to recompute.
Expand All @@ -615,10 +622,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,
Expand All @@ -630,6 +636,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,
rvolosatovs marked this conversation as resolved.
Show resolved Hide resolved
);
assert!(memo.insert(unit.clone(), new_unit.clone()).is_none());
new_graph.entry(new_unit.clone()).or_insert(new_deps);
Expand Down Expand Up @@ -788,6 +797,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() {
Expand Down
1 change: 1 addition & 0 deletions src/cargo/ops/cargo_compile/unit_generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ impl<'a> UnitGenerator<'a, '_> {
/*is_std*/ false,
/*dep_hash*/ 0,
IsArtifact::No,
None,
)
})
.collect()
Expand Down
Loading