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

Check for duplicate output filenames. #6308

Merged
merged 3 commits into from
Nov 14, 2018
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
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/build_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
15 changes: 14 additions & 1 deletion src/cargo/core/compiler/context/compilation_files.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
Expand Down Expand Up @@ -251,6 +253,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
ret.push(OutputFile {
path,
hardlink: None,
export_path: None,
flavor: FileFlavor::Linkable,
});
} else {
Expand All @@ -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,
});
},
Expand Down
109 changes: 88 additions & 21 deletions src/cargo/core/compiler/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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`.
Expand Down Expand Up @@ -72,18 +71,6 @@ impl<'a> Unit<'a> {
}
}

impl<'a> Ord for Unit<'a> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, was this done for performance reasons or for correctness reasons?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is only for performance. The old code was rehashing every field for every comparison.

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>,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)]
Expand Down
11 changes: 2 additions & 9 deletions src/cargo/core/compiler/fingerprint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down
13 changes: 6 additions & 7 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
}
}

Expand Down
17 changes: 15 additions & 2 deletions src/cargo/core/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
13 changes: 13 additions & 0 deletions src/cargo/core/package.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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> {
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/core/profiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions tests/testsuite/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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();
}

Expand Down
Loading