Auto merge of #6308 - ehuss:output-collision, r=alexcrichton

Check for duplicate output filenames.

This generates an error if it detects that different units would output the same file name. This can happen in a few situations:
- Multiple targets in a workspace use the same name.
- `--out-dir` in some cases (like `examples` because it does not include the directory).
- Bugs in cargo (like #5524, #5444)

This includes a few cleanups/consolidations:
- `export_path` added to `OutputFile` so there is one place where the `--out-dir` filename logic is located.
- `TargetKind::description()` added for a simple way to get a human-readable name of a target kind.
- The `PartialOrd` changes are a slight performance improvement (overall things are now slightly faster even though it is doing more work).

Closes #5524, closes #6293.

Author: bors

src/cargo/core/compiler/build_config.rs

@@ -111,7 +111,7 @@ pub enum MessageFormat {
 /// `compile_ws` to tell it the general execution strategy.  This influences
 /// the default targets selected.  The other use is in the `Unit` struct
 /// to indicate what is being done with a specific target.
-#[derive(Clone, Copy, PartialEq, Debug, Eq, Hash)]
+#[derive(Clone, Copy, PartialEq, Debug, Eq, Hash, PartialOrd, Ord)]
 pub enum CompileMode {
     /// A target being built for a test.
     Test,

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

@@ -38,11 +38,13 @@ pub struct CompilationFiles<'a, 'cfg: 'a> {
 
 #[derive(Debug)]
 pub struct OutputFile {
-    /// File name that will be produced by the build process (in `deps`).
+    /// Absolute path to the file that will be produced by the build process.
     pub path: PathBuf,
     /// If it should be linked into `target`, and what it should be called
     /// (e.g. without metadata).
     pub hardlink: Option<PathBuf>,
+    /// If `--out-dir` is specified, the absolute path to the exported file.
+    pub export_path: Option<PathBuf>,
     /// Type of the file (library / debug symbol / else).
     pub flavor: FileFlavor,
 }
@@ -251,6 +253,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
                 ret.push(OutputFile {
                     path,
                     hardlink: None,
+                    export_path: None,
                     flavor: FileFlavor::Linkable,
                 });
             } else {
@@ -273,9 +276,19 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
                             let hardlink = link_stem
                                 .as_ref()
                                 .map(|&(ref ld, ref ls)| ld.join(file_type.filename(ls)));
+                            let export_path = if unit.target.is_custom_build() {
+                                None
+                            } else {
+                                self.export_dir.as_ref().and_then(|export_dir| {
+                                    hardlink.as_ref().and_then(|hardlink| {
+                                        Some(export_dir.join(hardlink.file_name().unwrap()))
+                                    })
+                                })
+                            };
                             ret.push(OutputFile {
                                 path,
                                 hardlink,
+                                export_path,
                                 flavor: file_type.flavor,
                             });
                         },

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

@@ -4,7 +4,6 @@ use std::ffi::OsStr;
 use std::fmt::Write;
 use std::path::PathBuf;
 use std::sync::Arc;
-use std::cmp::Ordering;
 
 use jobserver::Client;
 
@@ -42,7 +41,7 @@ use self::compilation_files::CompilationFiles;
 /// example, it needs to know the target architecture (OS, chip arch etc.) and it needs to know
 /// whether you want a debug or release build. There is enough information in this struct to figure
 /// all that out.
-#[derive(Clone, Copy, Eq, PartialEq, Hash, Debug)]
+#[derive(Clone, Copy, Eq, PartialEq, Hash, Debug, PartialOrd, Ord)]
 pub struct Unit<'a> {
     /// Information about available targets, which files to include/exclude, etc. Basically stuff in
     /// `Cargo.toml`.
@@ -72,18 +71,6 @@ impl<'a> Unit<'a> {
 	}
 }
 
-impl<'a> Ord for Unit<'a> {
-    fn cmp(&self, other: &Unit) -> Ordering {
-        self.buildkey().cmp(&other.buildkey())
-    }
-}
-
-impl<'a> PartialOrd for Unit<'a> {
-    fn partial_cmp(&self, other: &Unit) -> Option<Ordering> {
-        Some(self.cmp(other))
-    }
-}
-
 pub struct Context<'a, 'cfg: 'a> {
     pub bcx: &'a BuildContext<'a, 'cfg>,
     pub compilation: Compilation<'cfg>,
@@ -150,6 +137,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
         self.prepare_units(export_dir, units)?;
         self.prepare()?;
         custom_build::build_map(&mut self, units)?;
+        self.check_collistions()?;
 
         for unit in units.iter() {
             // Build up a list of pending jobs, each of which represent
@@ -353,13 +341,8 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
         self.files.as_mut().unwrap()
     }
 
-    /// Return the filenames that the given target for the given profile will
-    /// generate as a list of 3-tuples (filename, link_dst, linkable)
-    ///
-    ///  - filename: filename rustc compiles to. (Often has metadata suffix).
-    ///  - link_dst: Optional file to link/copy the result to (without metadata suffix)
-    ///  - linkable: Whether possible to link against file (eg it's a library)
-    pub fn outputs(&mut self, unit: &Unit<'a>) -> CargoResult<Arc<Vec<OutputFile>>> {
+    /// Return the filenames that the given unit will generate.
+    pub fn outputs(&self, unit: &Unit<'a>) -> CargoResult<Arc<Vec<OutputFile>>> {
         self.files.as_ref().unwrap().outputs(unit, self.bcx)
     }
 
@@ -468,6 +451,90 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
         inputs.sort();
         Ok(inputs)
     }
+
+    fn check_collistions(&self) -> CargoResult<()> {
+        let mut output_collisions = HashMap::new();
+        let describe_collision = |unit: &Unit, other_unit: &Unit, path: &PathBuf| -> String {
+            format!(
+                "The {} target `{}` in package `{}` has the same output \
+                filename as the {} target `{}` in package `{}`.\n\
+                Colliding filename is: {}\n",
+                unit.target.kind().description(),
+                unit.target.name(),
+                unit.pkg.package_id(),
+                other_unit.target.kind().description(),
+                other_unit.target.name(),
+                other_unit.pkg.package_id(),
+                path.display()
+            )
+        };
+        let suggestion = "Consider changing their names to be unique or compiling them separately.\n\
+            This may become a hard error in the future, see https://github.com/rust-lang/cargo/issues/6313";
+        let report_collision = |unit: &Unit, other_unit: &Unit, path: &PathBuf| -> CargoResult<()> {
+            if unit.target.name() == other_unit.target.name() {
+                self.bcx.config.shell().warn(format!(
+                    "output filename collision.\n\
+                    {}\
+                    The targets should have unique names.\n\
+                    {}",
+                    describe_collision(unit, other_unit, path),
+                    suggestion
+                ))
+            } else {
+                self.bcx.config.shell().warn(format!(
+                    "output filename collision.\n\
+                    {}\
+                    The output filenames should be unique.\n\
+                    {}\n\
+                    If this looks unexpected, it may be a bug in Cargo. Please file a bug report at\n\
+                    https://github.com/rust-lang/cargo/issues/ with as much information as you\n\
+                    can provide.\n\
+                    {} running on `{}` target `{}`\n\
+                    First unit: {:?}\n\
+                    Second unit: {:?}",
+                    describe_collision(unit, other_unit, path),
+                    suggestion,
+                    ::version(), self.bcx.host_triple(), self.bcx.target_triple(),
+                    unit, other_unit))
+            }
+        };
+        let mut keys = self
+            .unit_dependencies
+            .keys()
+            .filter(|unit| !unit.mode.is_run_custom_build())
+            .collect::<Vec<_>>();
+        // Sort for consistent error messages.
+        keys.sort_unstable();
+        for unit in keys {
+            for output in self.outputs(unit)?.iter() {
+                if let Some(other_unit) =
+                    output_collisions.insert(output.path.clone(), unit)
+                {
+                    report_collision(unit, &other_unit, &output.path)?;
+                }
+                if let Some(hardlink) = output.hardlink.as_ref() {
+                    if let Some(other_unit) = output_collisions.insert(hardlink.clone(), unit)
+                    {
+                        report_collision(unit, &other_unit, hardlink)?;
+                    }
+                }
+                if let Some(ref export_path) = output.export_path {
+                    if let Some(other_unit) =
+                        output_collisions.insert(export_path.clone(), unit)
+                    {
+                        self.bcx.config.shell().warn(format!("`--out-dir` filename collision.\n\
+                            {}\
+                            The exported filenames should be unique.\n\
+                            {}",
+                            describe_collision(unit, &other_unit, &export_path),
+                            suggestion
+                            ))?;
+                    }
+                }
+            }
+        }
+        Ok(())
+    }
 }
 
 #[derive(Default)]

src/cargo/core/compiler/fingerprint.rs

@@ -9,7 +9,7 @@ use serde::de::{self, Deserialize};
 use serde::ser;
 use serde_json;
 
-use core::{Edition, Package, TargetKind};
+use core::{Edition, Package};
 use util;
 use util::errors::{CargoResult, CargoResultExt};
 use util::paths;
@@ -780,14 +780,7 @@ fn filename<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> String {
     // fingerprint for every metadata hash version. This works because
     // even if the package is fresh, we'll still link the fresh target
     let file_stem = cx.files().file_stem(unit);
-    let kind = match *unit.target.kind() {
-        TargetKind::Lib(..) => "lib",
-        TargetKind::Bin => "bin",
-        TargetKind::Test => "integration-test",
-        TargetKind::ExampleBin | TargetKind::ExampleLib(..) => "example",
-        TargetKind::Bench => "bench",
-        TargetKind::CustomBuild => "build-script",
-    };
+    let kind = unit.target.kind().description();
     let flavor = if unit.mode.is_any_test() {
         "test-"
     } else if unit.mode.is_doc() {

src/cargo/core/compiler/mod.rs

@@ -436,14 +436,13 @@ fn link_targets<'a, 'cfg>(
             };
             destinations.push(dst.display().to_string());
             hardlink_or_copy(src, dst)?;
-            if let Some(ref path) = export_dir {
-                if !target.is_custom_build() {
-                    if !path.exists() {
-                        fs::create_dir_all(path)?;
-                    }
-
-                    hardlink_or_copy(src, &path.join(dst.file_name().unwrap()))?;
+            if let Some(ref path) = output.export_path {
+                let export_dir = export_dir.as_ref().unwrap();
+                if !export_dir.exists() {
+                    fs::create_dir_all(export_dir)?;
                 }
+
+                hardlink_or_copy(src, path)?;
             }
         }
 

src/cargo/core/manifest.rs

@@ -181,9 +181,22 @@ impl fmt::Debug for TargetKind {
     }
 }
 
+impl TargetKind {
+    pub fn description(&self) -> &'static str {
+        match self {
+            TargetKind::Lib(..) => "lib",
+            TargetKind::Bin => "bin",
+            TargetKind::Test => "integration-test",
+            TargetKind::ExampleBin | TargetKind::ExampleLib(..) => "example",
+            TargetKind::Bench => "bench",
+            TargetKind::CustomBuild => "build-script",
+        }
+    }
+}
+
 /// Information about a binary, a library, an example, etc. that is part of the
 /// package.
-#[derive(Clone, Hash, PartialEq, Eq)]
+#[derive(Clone, Hash, PartialEq, Eq, PartialOrd, Ord)]
 pub struct Target {
     kind: TargetKind,
     name: String,
@@ -202,7 +215,7 @@ pub struct Target {
     edition: Edition,
 }
 
-#[derive(Clone, PartialEq, Eq)]
+#[derive(Clone, PartialEq, Eq, PartialOrd, Ord)]
 pub enum TargetSourcePath {
     Path(PathBuf),
     Metabuild,

src/cargo/core/package.rs

@@ -1,4 +1,5 @@
 use std::cell::{Ref, RefCell, Cell};
+use std::cmp::Ordering;
 use std::collections::{HashMap, HashSet};
 use std::fmt;
 use std::hash;
@@ -38,6 +39,18 @@ pub struct Package {
     manifest_path: PathBuf,
 }
 
+impl Ord for Package {
+    fn cmp(&self, other: &Package) -> Ordering {
+        self.package_id().cmp(other.package_id())
+    }
+}
+
+impl PartialOrd for Package {
+    fn partial_cmp(&self, other: &Package) -> Option<Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
 /// A Package in a form where `Serialize` can be derived.
 #[derive(Serialize)]
 struct SerializedPackage<'a> {

src/cargo/core/profiles.rs

@@ -371,7 +371,7 @@ fn merge_profile(profile: &mut Profile, toml: &TomlProfile) {
 
 /// Profile settings used to determine which compiler flags to use for a
 /// target.
-#[derive(Clone, Copy, Eq)]
+#[derive(Clone, Copy, Eq, PartialOrd, Ord)]
 pub struct Profile {
     pub name: &'static str,
     pub opt_level: InternedString,
@@ -523,7 +523,7 @@ impl Profile {
 }
 
 /// The link-time-optimization setting.
-#[derive(Clone, Copy, PartialEq, Eq, Debug, Hash)]
+#[derive(Clone, Copy, PartialEq, Eq, Debug, Hash, PartialOrd, Ord)]
 pub enum Lto {
     /// False = no LTO
     /// True = "Fat" LTO

tests/testsuite/build.rs

@@ -4327,8 +4327,8 @@ fn target_filters_workspace() {
         .file("a/src/lib.rs", "")
         .file("a/examples/ex1.rs", "fn main() {}")
         .file("b/Cargo.toml", &basic_bin_manifest("b"))
+        .file("b/src/lib.rs", "")
         .file("b/src/main.rs", "fn main() {}")
-        .file("b/examples/ex1.rs", "fn main() {}")
         .build();
 
     ws.cargo("build -v --example ex")
@@ -4343,12 +4343,12 @@ Did you mean `ex1`?",
     ws.cargo("build -v --lib")
         .with_status(0)
         .with_stderr_contains("[RUNNING] `rustc [..]a/src/lib.rs[..]")
+        .with_stderr_contains("[RUNNING] `rustc [..]b/src/lib.rs[..]")
         .run();
 
     ws.cargo("build -v --example ex1")
         .with_status(0)
         .with_stderr_contains("[RUNNING] `rustc [..]a/examples/ex1.rs[..]")
-        .with_stderr_contains("[RUNNING] `rustc [..]b/examples/ex1.rs[..]")
         .run();
 }