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

Fix warning suppression for config.toml vs config compat symlinks #13793

Merged
merged 5 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from 4 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
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ rand.workspace = true
regex.workspace = true
rusqlite.workspace = true
rustfix.workspace = true
same-file.workspace = true
semver.workspace = true
serde = { workspace = true, features = ["derive"] }
serde-untagged.workspace = true
Expand Down
18 changes: 7 additions & 11 deletions src/cargo/util/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1537,36 +1537,32 @@ impl GlobalContext {
let possible = dir.join(filename_without_extension);
let possible_with_extension = dir.join(format!("{}.toml", filename_without_extension));

if possible.exists() {
if let Ok(possible_handle) = same_file::Handle::from_path(&possible) {
if warn {
if let Ok(possible_with_extension_handle) =
same_file::Handle::from_path(&possible_with_extension)
{
// We don't want to print a warning if the version
// without the extension is just a symlink to the version
// WITH an extension, which people may want to do to
// support multiple Cargo versions at once and not
// get a warning.
let skip_warning = if let Ok(target_path) = fs::read_link(&possible) {
target_path == possible_with_extension
} else {
false
};

if !skip_warning {
if possible_with_extension.exists() {
if possible_handle != possible_with_extension_handle {
epage marked this conversation as resolved.
Show resolved Hide resolved
self.shell().warn(format!(
"both `{}` and `{}` exist. Using `{}`",
possible.display(),
possible_with_extension.display(),
possible.display()
))?;
} else {
}
} else {
self.shell().warn(format!(
"`{}` is deprecated in favor of `{filename_without_extension}.toml`",
possible.display(),
))?;
self.shell().note(
format!("if you need to support cargo 1.38 or earlier, you can symlink `{filename_without_extension}` to `{filename_without_extension}.toml`"),
)?;
}
}
}

Expand Down
73 changes: 71 additions & 2 deletions tests/testsuite/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,12 +184,29 @@ fn symlink_file(target: &Path, link: &Path) -> io::Result<()> {
os::windows::fs::symlink_file(target, link)
}

fn symlink_config_to_config_toml() {
fn make_config_symlink_to_config_toml_absolute() {
let toml_path = paths::root().join(".cargo/config.toml");
let symlink_path = paths::root().join(".cargo/config");
t!(symlink_file(&toml_path, &symlink_path));
}

fn make_config_symlink_to_config_toml_relative() {
let symlink_path = paths::root().join(".cargo/config");
t!(symlink_file(Path::new("config.toml"), &symlink_path));
}

fn rename_config_toml_to_config_replacing_with_symlink() {
let root = paths::root();
t!(fs::rename(
root.join(".cargo/config.toml"),
root.join(".cargo/config")
));
t!(symlink_file(
Path::new("config"),
&root.join(".cargo/config.toml")
));
}

#[track_caller]
pub fn assert_error<E: Borrow<anyhow::Error>>(error: E, msgs: &str) {
let causes = error
Expand Down Expand Up @@ -298,7 +315,33 @@ f1 = 1
",
);

symlink_config_to_config_toml();
make_config_symlink_to_config_toml_absolute();

let gctx = new_gctx();

assert_eq!(gctx.get::<Option<i32>>("foo.f1").unwrap(), Some(1));

// It should NOT have warned for the symlink.
let output = read_output(gctx);
assert_match("", &output);
}

#[cargo_test]
fn config_ambiguous_filename_symlink_doesnt_warn_relative() {
// Windows requires special permissions to create symlinks.
// If we don't have permission, just skip this test.
if !symlink_supported() {
return;
};

write_config_toml(
"\
[foo]
f1 = 1
",
);

make_config_symlink_to_config_toml_relative();

let gctx = new_gctx();

Expand All @@ -309,6 +352,32 @@ f1 = 1
assert_match("", &output);
}

#[cargo_test]
fn config_ambiguous_filename_symlink_doesnt_warn_backward() {
// Windows requires special permissions to create symlinks.
// If we don't have permission, just skip this test.
if !symlink_supported() {
return;
};

write_config_toml(
"\
[foo]
f1 = 1
",
);

rename_config_toml_to_config_replacing_with_symlink();

let gctx = new_gctx();

assert_eq!(gctx.get::<Option<i32>>("foo.f1").unwrap(), Some(1));

// It should NOT have warned for this situation.
let output = read_output(gctx);
assert_match("", &output);
}

#[cargo_test]
fn config_ambiguous_filename() {
write_config_extless(
Expand Down