Auto-resolve collisions if using a meta-hash would help

Collisions in binaries or dylibs are quite easy to get into when
compiling artifacts deps in conjunction with build overrides.

The latter will cause multiple builds and prevent deduplication,
while certain platform rules prevent unique filename to be generated
while dropping files into the same folder.

Now we recomputate outputs after detecting certain avoidable collisions
to be more usable in this specific situation.

Author: Byron

src/cargo/core/compiler/context/compilation_files.rs

@@ -89,6 +89,10 @@ struct MetaInfo {
     ///
     /// If this is `true`, the `meta_hash` is used for the filename.
     use_extra_filename: bool,
+
+    /// This flag is set by collision detection and forces the use of a more unique hash
+    /// instead of the stable one.
+    output_conflicts_without_meta_hash: bool,
 }
 
 /// Collection of information about the files emitted by the compiler, and the
@@ -220,7 +224,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
     fn pkg_dir(&self, unit: &Unit) -> String {
         let name = unit.pkg.package_id().name();
         let meta = &self.metas[unit];
-        if meta.use_extra_filename {
+        if meta.use_extra_filename || meta.output_conflicts_without_meta_hash {
             format!("{}-{}", name, meta.meta_hash)
         } else {
             format!("{}-{}", name, self.target_short_hash(unit))
@@ -370,6 +374,12 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
             .map(Arc::clone)
     }
 
+    pub(super) fn set_non_colliding_outputs(&mut self, unit: &Unit, outputs: Arc<Vec<OutputFile>>) {
+        let cell = LazyCell::new();
+        cell.fill(outputs).unwrap();
+        self.outputs.insert(unit.clone(), cell);
+    }
+
     /// Returns the path where the output for the given unit and FileType
     /// should be uplifted to.
     ///
@@ -421,7 +431,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
         Some(uplift_path)
     }
 
-    fn calc_outputs(
+    pub(super) fn calc_outputs(
         &self,
         unit: &Unit,
         bcx: &BuildContext<'a, 'cfg>,
@@ -625,6 +635,7 @@ fn compute_metadata(
     MetaInfo {
         meta_hash: Metadata(hasher.finish()),
         use_extra_filename: should_use_metadata(bcx, unit),
+        output_conflicts_without_meta_hash: false,
     }
 }
 

src/cargo/core/compiler/context/mod.rs

@@ -128,7 +128,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
         self.prepare_units()?;
         self.prepare()?;
         custom_build::build_map(&mut self)?;
-        self.check_collisions()?;
+        self.check_collisions_and_avoid_some()?;
         self.compute_metadata_for_doc_units();
 
         // We need to make sure that if there were any previous docs
@@ -421,7 +421,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
         }
     }
 
-    fn check_collisions(&self) -> CargoResult<()> {
+    fn check_collisions_and_avoid_some(&mut self) -> CargoResult<()> {
         let mut output_collisions = HashMap::new();
         let describe_collision = |unit: &Unit, other_unit: &Unit, path: &PathBuf| -> String {
             format!(
@@ -528,17 +528,36 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
                     doc_collision_error(unit, prev)?;
                 }
             }
+            let mut alt_outputs = None;
+            let mut collisions_prevented = 0;
             for output in self.outputs(unit)?.iter() {
                 if let Some(other_unit) = output_collisions.insert(output.path.clone(), unit) {
                     if unit.mode.is_doc() {
                         // See https://github.com/rust-lang/rust/issues/56169
                         // and https://github.com/rust-lang/rust/issues/61378
                         report_collision(unit, other_unit, &output.path, rustdoc_suggestion)?;
                     } else {
-                        report_collision(unit, other_unit, &output.path, suggestion)?;
+                        let files = self.files.as_mut().unwrap();
+                        if !files.use_extra_filename(unit)
+                            && !alt_outputs
+                                .get_or_insert_with(|| {
+                                    files
+                                        .calc_outputs(unit, &self.bcx)
+                                        .expect("infallible by now")
+                                })
+                                .iter()
+                                .any(|out| out.path == output.path)
+                        {
+                            collisions_prevented += 1;
+                            continue;
+                        } else {
+                            report_collision(unit, other_unit, &output.path, suggestion)?;
+                        }
                     }
                 }
                 if let Some(hardlink) = output.hardlink.as_ref() {
+                    // We don't check for collision prevention on unix as typically these only happen on MSVC platforms
+                    // which doesn't use hardlinks. If this changes, one would do another collision here.
                     if let Some(other_unit) = output_collisions.insert(hardlink.clone(), unit) {
                         report_collision(unit, other_unit, hardlink, suggestion)?;
                     }
@@ -556,6 +575,15 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
                     }
                 }
             }
+
+            if collisions_prevented != 0 {
+                if let Some(alt_outputs) = alt_outputs {
+                    self.files
+                        .as_mut()
+                        .unwrap()
+                        .set_non_colliding_outputs(unit, alt_outputs);
+                }
+            }
         }
         Ok(())
     }

tests/testsuite/artifact_dep.rs

@@ -1384,10 +1384,6 @@ fn profile_override_basic() {
         .run();
 }
 
-// TODO: replace profile_override_basic() with this test if it fixes msvc issues.
-//       Answer: it doesn't as it's a race of the same dep being built twice, in parallel.
-//       This needs a fix on scheduler level or maybe fixes itself once we uplift binaries.
-//       But probably not as the scheduling problem and similar file paths still remains an issue.
 #[cargo_test]
 fn profile_override_basic_multidep() {
     let p = project()
@@ -1403,8 +1399,7 @@ fn profile_override_basic_multidep() {
                 bar = { path = "bar", artifact = "bin" }
                 
                 [dependencies]
-                # renamed just to avoid collisions on MSVC
-                bardep = { package = "bar", path = "bar", artifact = "bin" }
+                bar = { path = "bar", artifact = "bin" }
                 
                 [profile.dev.build-override]
                 opt-level = 1