Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
Warn on Windows about reserved target names.
  • Loading branch information
ehuss committed Mar 3, 2020
commit 15ac82b677a7055c3e67fa06fd32e8446b2b377f
18 changes: 14 additions & 4 deletions src/cargo/util/toml/targets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ fn clean_lib(
None => return Ok(None),
};

validate_has_name(lib, "library", "lib")?;
validate_target_name(lib, "library", "lib", warnings)?;

let path = match (lib.path.as_ref(), inferred) {
(Some(path), _) => package_root.join(&path.0),
Expand Down Expand Up @@ -264,7 +264,7 @@ fn clean_bins(
);

for bin in &bins {
validate_has_name(bin, "binary", "bin")?;
validate_target_name(bin, "binary", "bin", warnings)?;

let name = bin.name();

Expand Down Expand Up @@ -529,7 +529,7 @@ fn clean_targets_with_legacy_path(
);

for target in &toml_targets {
validate_has_name(target, target_kind_human, target_kind)?;
validate_target_name(target, target_kind_human, target_kind, warnings)?;
}

validate_unique_names(&toml_targets, target_kind)?;
Expand Down Expand Up @@ -720,16 +720,26 @@ fn inferred_to_toml_targets(inferred: &[(String, PathBuf)]) -> Vec<TomlTarget> {
.collect()
}

fn validate_has_name(
fn validate_target_name(
target: &TomlTarget,
target_kind_human: &str,
target_kind: &str,
warnings: &mut Vec<String>,
) -> CargoResult<()> {
match target.name {
Some(ref name) => {
if name.trim().is_empty() {
anyhow::bail!("{} target names cannot be empty", target_kind_human)
}
if cfg!(windows) {
if restricted_names::is_windows_reserved(name) {
warnings.push(format!(
"{} target `{}` is a reserved Windows filename, \
this target will not work on Windows platforms",
target_kind_human, name
));
Copy link
Member

Choose a reason for hiding this comment

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

Hm can you clarify this a bit for me? How come we'd only issue this warning on windows, and how comit it applies to all targets? It should be ok to have a library called con, right? I don't think that creates any of the bad file names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking for only showing on Windows is: if someone wants to create an executable called con, and they never intend for it to be used on Windows, then that would create a warning that they cannot silence. Maybe if we had the "compatibility" table we discussed at the last meeting, we could warn on all platforms and then allow the warning to be silenced by specifying that it is incompatible with windows?

FWIW, I scanned all of crates.io, and didn't find any targets with a reserved name. Part of this came up with a user on Discord who had a package named prune with a binary named prn, and was confused when their Windows CI failed. I don't think they've published to crates.io, though.

As for libraries, in some situations rustc creates filenames like con.tr1cof326ip8vwg.rcgu.o which fail on windows. It also seems like it would just be a risky thing to do, and I didn't want to put in effort and complexity to handle that edge case, which is pretty far on the edge.

Copy link
Member

Choose a reason for hiding this comment

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

Oh for sure binaries make sense here, I was just confused about the library part. The point about crate names showing up in object files makes sense though!

}
}
}
None => anyhow::bail!(
"{} target {}.name is required",
Expand Down
39 changes: 39 additions & 0 deletions tests/testsuite/cargo_targets.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
//! Tests specifically related to target handling (lib, bins, examples, tests, benches).

use cargo_test_support::project;

#[cargo_test]
fn reserved_windows_target_name() {
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"

[[bin]]
name = "con"
path = "src/main.rs"
"#,
)
.file("src/main.rs", "fn main() {}")
.build();

if cfg!(windows) {
p.cargo("check")
.with_stderr(
"\
[WARNING] binary target `con` is a reserved Windows filename, \
this target will not work on Windows platforms
[CHECKING] foo[..]
[FINISHED][..]
",
)
.run();
} else {
p.cargo("check")
.with_stderr("[CHECKING] foo[..]\n[FINISHED][..]")
.run();
}
}
1 change: 1 addition & 0 deletions tests/testsuite/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ mod cache_messages;
mod cargo_alias_config;
mod cargo_command;
mod cargo_features;
mod cargo_targets;
mod cfg;
mod check;
mod clean;
Expand Down