From 13be0cfa8b91c73acce589b953f0adf9da676462 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 24 Apr 2024 10:00:26 +0100 Subject: [PATCH 1/5] Tests: Rename config symlink creation function "symlink A to B" is confusing; it is ambiguous (at leaset to me) whether it means A -> B or B -> A. And I'm about to introduce a function that does the reverse, and also one that makes a relative rather than full path link. So rename this function. --- tests/testsuite/config.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 39fcee2cd73..32102e4d65d 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -184,7 +184,7 @@ 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)); @@ -298,7 +298,7 @@ f1 = 1 ", ); - symlink_config_to_config_toml(); + make_config_symlink_to_config_toml_absolute(); let gctx = new_gctx(); From dcce00745d53d9539521ddda00920c79d47dc007 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 24 Apr 2024 10:05:31 +0100 Subject: [PATCH 2/5] Tests: Add test case for config -> config.toml (relative) --- tests/testsuite/config.rs | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 32102e4d65d..c3b2391ad43 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -190,6 +190,11 @@ fn make_config_symlink_to_config_toml_absolute() { 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)); +} + #[track_caller] pub fn assert_error>(error: E, msgs: &str) { let causes = error @@ -309,6 +314,36 @@ f1 = 1 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(); + + assert_eq!(gctx.get::>("foo.f1").unwrap(), Some(1)); + + // It should NOT have warned for the symlink. + // But, currently it does! + let output = read_output(gctx); + let expected = "\ +[WARNING] both `[..]/.cargo/config` and `[..]/.cargo/config.toml` exist. Using `[..]/.cargo/config` +"; + assert_match(expected, &output); +} + #[cargo_test] fn config_ambiguous_filename() { write_config_extless( From 91f3e457abc8f3b102bb33683c05977af630e33d Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 24 Apr 2024 10:09:42 +0100 Subject: [PATCH 3/5] Tests: Add test case for config.toml -> config --- tests/testsuite/config.rs | 42 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index c3b2391ad43..768c144e9b1 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -195,6 +195,18 @@ fn make_config_symlink_to_config_toml_relative() { 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>(error: E, msgs: &str) { let causes = error @@ -344,6 +356,36 @@ f1 = 1 assert_match(expected, &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::>("foo.f1").unwrap(), Some(1)); + + // It should NOT have warned for this situation. + // But, currently it does! + let output = read_output(gctx); + let expected = "\ +[WARNING] both `[..]/.cargo/config` and `[..]/.cargo/config.toml` exist. Using `[..]/.cargo/config` +"; + assert_match(expected, &output); +} + #[cargo_test] fn config_ambiguous_filename() { write_config_extless( From 23440c0dcda85e285b7f51393b388a1a7fec01f1 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 24 Apr 2024 11:44:46 +0100 Subject: [PATCH 4/5] config reading: use same_file for suppressing "both files" warning This is 100% reliable on Unix, and better on Windows. (In this commit I avoid reindenting things to make review easier; the formatting will be fixed in the next commit.) Fixes #13667 --- Cargo.toml | 1 + src/cargo/util/context/mod.rs | 18 +++++++----------- tests/testsuite/config.rs | 12 ++---------- 3 files changed, 10 insertions(+), 21 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index d0f2afb3972..38d0f5bb4e0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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 diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index 507e6a6ad14..2ddfd13a626 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -1537,28 +1537,25 @@ 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 { 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(), @@ -1566,7 +1563,6 @@ impl GlobalContext { 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`"), )?; - } } } diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 768c144e9b1..2ad81486dc9 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -348,12 +348,8 @@ f1 = 1 assert_eq!(gctx.get::>("foo.f1").unwrap(), Some(1)); // It should NOT have warned for the symlink. - // But, currently it does! let output = read_output(gctx); - let expected = "\ -[WARNING] both `[..]/.cargo/config` and `[..]/.cargo/config.toml` exist. Using `[..]/.cargo/config` -"; - assert_match(expected, &output); + assert_match("", &output); } #[cargo_test] @@ -378,12 +374,8 @@ f1 = 1 assert_eq!(gctx.get::>("foo.f1").unwrap(), Some(1)); // It should NOT have warned for this situation. - // But, currently it does! let output = read_output(gctx); - let expected = "\ -[WARNING] both `[..]/.cargo/config` and `[..]/.cargo/config.toml` exist. Using `[..]/.cargo/config` -"; - assert_match(expected, &output); + assert_match("", &output); } #[cargo_test] From 2f168383858fac36b5829e777879474af6487eb9 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 24 Apr 2024 11:25:20 +0100 Subject: [PATCH 5/5] config reading: use same_file for suppressing "both files" warning (fmt) Apply deferred indentation changes. Whitespace change only. --- src/cargo/util/context/mod.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index 2ddfd13a626..914d57f2138 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -1542,11 +1542,11 @@ impl GlobalContext { 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. + // 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. if possible_handle != possible_with_extension_handle { self.shell().warn(format!( "both `{}` and `{}` exist. Using `{}`", @@ -1556,13 +1556,13 @@ impl GlobalContext { ))?; } } 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`"), - )?; + 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`"), + )?; } }