Auto merge of #11478 - rvolosatovs:fix/bindeps-target, r=weihanglo

fix: deduplicate dependencies by artifact target

### What does this PR try to resolve?

In cases when a compile target is specified for a bindep and the crate depending on it, cargo fails to deduplicate the crate dependencies and attempts to build the dependent crate only once with non-deterministic feature set, which breaks e.g. https://github.com/rvolosatovs/musl-bindep-feature-bug

Fix the issue by including the optional artifact compile target in the `Unit` in order to avoid wrongfully deduplicating the dependent crates

Fixes #11463
Fixes #10837
Fixes #10525

Note, that this issue is already accounted for by `cargo`, but in different context a similar situation can occur while building the build script, which:
1. may be built for different target than the actual package target
2. may contain dependencies with different feature sets than the same dependencies in the dependency graph of the package itself

That's why this PR is simply reusing the existing functionality for deduplication

### How should we test and review this PR?

Build https://github.com/rvolosatovs/musl-bindep-feature-bug

### Additional information

This is based on analysis by `@weihanglo` in #10837 (comment)
I experimented with adding the whole `UnitFor` to the internal unit struct, but that seems unnecessary.

It would probably be nicer to refactor `IsArtifact` and instead turn it into a 3-variant enum with a possible compile target, but I decided against that to minimize the diff. Perhaps it's worth a follow-up?

Author: bors

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,
             ));
         }
     }

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,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 {
@@ -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()
@@ -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
@@ -216,6 +229,7 @@ impl UnitInterner {
             is_std,
             dep_hash,
             artifact,
+            artifact_target_for_features,
         });
         Unit { inner }
     }

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,

src/cargo/ops/cargo_compile/mod.rs

@@ -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;
@@ -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
@@ -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.
@@ -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,
@@ -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,
     );
     assert!(memo.insert(unit.clone(), new_unit.clone()).is_none());
     new_graph.entry(new_unit.clone()).or_insert(new_deps);
@@ -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() {

src/cargo/ops/cargo_compile/unit_generator.rs

@@ -172,6 +172,7 @@ impl<'a> UnitGenerator<'a, '_> {
                     /*is_std*/ false,
                     /*dep_hash*/ 0,
                     IsArtifact::No,
+                    None,
                 )
             })
             .collect()