Auto merge of #7295 - zachlute:config-toml-extension, r=alexcrichton

Allow using 'config.toml' instead of just 'config' files.

Fixes #7273

Note that this change only makes 'config.toml' optional to use instead of 'config'. If both exist, we will print a warning and prefer 'config', since that would be the existing behavior if both existed today.

We should also consider a separate change to make config.toml the default and update docs, etc.

Author: bors

src/cargo/util/config.rs

@@ -662,7 +662,7 @@ impl Config {
         let mut cfg = CV::Table(HashMap::new(), PathBuf::from("."));
         let home = self.home_path.clone().into_path_unlocked();
 
-        walk_tree(path, &home, |path| {
+        self.walk_tree(path, &home, |path| {
             let mut contents = String::new();
             let mut file = File::open(&path)?;
             file.read_to_string(&mut contents)
@@ -689,6 +689,76 @@ impl Config {
         }
     }
 
+    /// The purpose of this function is to aid in the transition to using
+    /// .toml extensions on Cargo's config files, which were historically not used.
+    /// Both 'config.toml' and 'credentials.toml' should be valid with or without extension.
+    /// When both exist, we want to prefer the one without an extension for
+    /// backwards compatibility, but warn the user appropriately.
+    fn get_file_path(
+        &self,
+        dir: &Path,
+        filename_without_extension: &str,
+        warn: bool,
+    ) -> CargoResult<Option<PathBuf>> {
+        let possible = dir.join(filename_without_extension);
+        let possible_with_extension = dir.join(format!("{}.toml", filename_without_extension));
+
+        if fs::metadata(&possible).is_ok() {
+            if warn && fs::metadata(&possible_with_extension).is_ok() {
+                // 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 {
+                    self.shell().warn(format!(
+                        "Both `{}` and `{}` exist. Using `{}`",
+                        possible.display(),
+                        possible_with_extension.display(),
+                        possible.display()
+                    ))?;
+                }
+            }
+
+            Ok(Some(possible))
+        } else if fs::metadata(&possible_with_extension).is_ok() {
+            Ok(Some(possible_with_extension))
+        } else {
+            Ok(None)
+        }
+    }
+
+    fn walk_tree<F>(&self, pwd: &Path, home: &Path, mut walk: F) -> CargoResult<()>
+    where
+        F: FnMut(&Path) -> CargoResult<()>,
+    {
+        let mut stash: HashSet<PathBuf> = HashSet::new();
+
+        for current in paths::ancestors(pwd) {
+            if let Some(path) = self.get_file_path(&current.join(".cargo"), "config", true)? {
+                walk(&path)?;
+                stash.insert(path);
+            }
+        }
+
+        // Once we're done, also be sure to walk the home directory even if it's not
+        // in our history to be sure we pick up that standard location for
+        // information.
+        if let Some(path) = self.get_file_path(home, "config", true)? {
+            if !stash.contains(&path) {
+                walk(&path)?;
+            }
+        }
+
+        Ok(())
+    }
+
     /// Gets the index for a registry.
     pub fn get_registry_index(&self, registry: &str) -> CargoResult<Url> {
         validate_package_name(registry, "registry name", "")?;
@@ -726,10 +796,10 @@ impl Config {
     /// present.
     fn load_credentials(&self, cfg: &mut ConfigValue) -> CargoResult<()> {
         let home_path = self.home_path.clone().into_path_unlocked();
-        let credentials = home_path.join("credentials");
-        if fs::metadata(&credentials).is_err() {
-            return Ok(());
-        }
+        let credentials = match self.get_file_path(&home_path, "credentials", true)? {
+            Some(credentials) => credentials,
+            None => return Ok(()),
+        };
 
         let mut contents = String::new();
         let mut file = File::open(&credentials)?;
@@ -1673,36 +1743,23 @@ pub fn homedir(cwd: &Path) -> Option<PathBuf> {
     ::home::cargo_home_with_cwd(cwd).ok()
 }
 
-fn walk_tree<F>(pwd: &Path, home: &Path, mut walk: F) -> CargoResult<()>
-where
-    F: FnMut(&Path) -> CargoResult<()>,
-{
-    let mut stash: HashSet<PathBuf> = HashSet::new();
-
-    for current in paths::ancestors(pwd) {
-        let possible = current.join(".cargo").join("config");
-        if fs::metadata(&possible).is_ok() {
-            walk(&possible)?;
-            stash.insert(possible);
-        }
-    }
-
-    // Once we're done, also be sure to walk the home directory even if it's not
-    // in our history to be sure we pick up that standard location for
-    // information.
-    let config = home.join("config");
-    if !stash.contains(&config) && fs::metadata(&config).is_ok() {
-        walk(&config)?;
-    }
-
-    Ok(())
-}
-
 pub fn save_credentials(cfg: &Config, token: String, registry: Option<String>) -> CargoResult<()> {
+    // If 'credentials.toml' exists, we should write to that, otherwise
+    // use the legacy 'credentials'. There's no need to print the warning
+    // here, because it would already be printed at load time.
+    let home_path = cfg.home_path.clone().into_path_unlocked();
+    let filename = match cfg.get_file_path(&home_path, "credentials", false)? {
+        Some(path) => match path.file_name() {
+            Some(filename) => Path::new(filename).to_owned(),
+            None => Path::new("credentials").to_owned(),
+        },
+        None => Path::new("credentials").to_owned(),
+    };
+
     let mut file = {
         cfg.home_path.create_dir()?;
         cfg.home_path
-            .open_rw(Path::new("credentials"), cfg, "credentials' config file")?
+            .open_rw(filename, cfg, "credentials' config file")?
     };
 
     let (key, value) = {

tests/testsuite/config.rs

@@ -1,6 +1,9 @@
 use std::borrow::Borrow;
 use std::collections;
 use std::fs;
+use std::io;
+use std::os;
+use std::path::Path;
 
 use crate::support::{paths, project};
 use cargo::core::{enable_nightly_features, Shell};
@@ -48,6 +51,50 @@ fn write_config(config: &str) {
     fs::write(path, config).unwrap();
 }
 
+fn write_config_toml(config: &str) {
+    let path = paths::root().join(".cargo/config.toml");
+    fs::create_dir_all(path.parent().unwrap()).unwrap();
+    fs::write(path, config).unwrap();
+}
+
+// Several test fail on windows if the user does not have permission to
+// create symlinks (the `SeCreateSymbolicLinkPrivilege`). Instead of
+// disabling these test on Windows, use this function to test whether we
+// have permission, and return otherwise. This way, we still don't run these
+// tests most of the time, but at least we do if the user has the right
+// permissions.
+// This function is derived from libstd fs tests.
+pub fn got_symlink_permission() -> bool {
+    if cfg!(unix) {
+        return true;
+    }
+    let link = paths::root().join("some_hopefully_unique_link_name");
+    let target = paths::root().join("nonexisting_target");
+
+    match symlink_file(&target, &link) {
+        Ok(_) => true,
+        // ERROR_PRIVILEGE_NOT_HELD = 1314
+        Err(ref err) if err.raw_os_error() == Some(1314) => false,
+        Err(_) => true,
+    }
+}
+
+#[cfg(unix)]
+fn symlink_file(target: &Path, link: &Path) -> io::Result<()> {
+    os::unix::fs::symlink(target, link)
+}
+
+#[cfg(windows)]
+fn symlink_file(target: &Path, link: &Path) -> io::Result<()> {
+    os::windows::fs::symlink_file(target, link)
+}
+
+fn symlink_config_to_config_toml() {
+    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 new_config(env: &[(&str, &str)]) -> Config {
     enable_nightly_features(); // -Z advanced-env
     let output = Box::new(fs::File::create(paths::root().join("shell.out")).unwrap());
@@ -112,6 +159,93 @@ f1 = 123
     assert_eq!(s, S { f1: Some(456) });
 }
 
+#[cargo_test]
+fn config_works_with_extension() {
+    write_config_toml(
+        "\
+[foo]
+f1 = 1
+",
+    );
+
+    let config = new_config(&[]);
+
+    assert_eq!(config.get::<Option<i32>>("foo.f1").unwrap(), Some(1));
+}
+
+#[cargo_test]
+fn config_ambiguous_filename_symlink_doesnt_warn() {
+    // Windows requires special permissions to create symlinks.
+    // If we don't have permission, just skip this test.
+    if !got_symlink_permission() {
+        return;
+    };
+
+    write_config_toml(
+        "\
+[foo]
+f1 = 1
+",
+    );
+
+    symlink_config_to_config_toml();
+
+    let config = new_config(&[]);
+
+    assert_eq!(config.get::<Option<i32>>("foo.f1").unwrap(), Some(1));
+
+    // It should NOT have warned for the symlink.
+    drop(config); // Paranoid about flushing the file.
+    let path = paths::root().join("shell.out");
+    let output = fs::read_to_string(path).unwrap();
+    let unexpected = "\
+warning: Both `[..]/.cargo/config` and `[..]/.cargo/config.toml` exist. Using `[..]/.cargo/config`
+";
+    if lines_match(unexpected, &output) {
+        panic!(
+            "Found unexpected:\n{}\nActual error:\n{}\n",
+            unexpected, output
+        );
+    }
+}
+
+#[cargo_test]
+fn config_ambiguous_filename() {
+    write_config(
+        "\
+[foo]
+f1 = 1
+",
+    );
+
+    write_config_toml(
+        "\
+[foo]
+f1 = 2
+",
+    );
+
+    let config = new_config(&[]);
+
+    // It should use the value from the one without the extension for
+    // backwards compatibility.
+    assert_eq!(config.get::<Option<i32>>("foo.f1").unwrap(), Some(1));
+
+    // But it also should have warned.
+    drop(config); // Paranoid about flushing the file.
+    let path = paths::root().join("shell.out");
+    let output = fs::read_to_string(path).unwrap();
+    let expected = "\
+warning: Both `[..]/.cargo/config` and `[..]/.cargo/config.toml` exist. Using `[..]/.cargo/config`
+";
+    if !lines_match(expected, &output) {
+        panic!(
+            "Did not find expected:\n{}\nActual error:\n{}\n",
+            expected, output
+        );
+    }
+}
+
 #[cargo_test]
 fn config_unused_fields() {
     write_config(

tests/testsuite/login.rs

@@ -1,5 +1,6 @@
 use std::fs::{self, File};
 use std::io::prelude::*;
+use std::path::PathBuf;
 
 use crate::support::cargo_process;
 use crate::support::install::cargo_home;
@@ -13,6 +14,15 @@ const ORIGINAL_TOKEN: &str = "api-token";
 
 fn setup_new_credentials() {
     let config = cargo_home().join("credentials");
+    setup_new_credentials_at(config);
+}
+
+fn setup_new_credentials_toml() {
+    let config = cargo_home().join("credentials.toml");
+    setup_new_credentials_at(config);
+}
+
+fn setup_new_credentials_at(config: PathBuf) {
     t!(fs::create_dir_all(config.parent().unwrap()));
     t!(t!(File::create(&config))
         .write_all(format!(r#"token = "{token}""#, token = ORIGINAL_TOKEN).as_bytes()));
@@ -84,6 +94,42 @@ fn login_with_new_credentials() {
     assert!(check_token(TOKEN, None));
 }
 
+#[cargo_test]
+fn credentials_work_with_extension() {
+    registry::init();
+    setup_new_credentials_toml();
+
+    cargo_process("login --host")
+        .arg(registry_url().to_string())
+        .arg(TOKEN)
+        .run();
+
+    // Ensure that we get the new token for the registry
+    assert!(check_token(TOKEN, None));
+}
+
+#[cargo_test]
+fn credentials_ambiguous_filename() {
+    registry::init();
+    setup_new_credentials();
+    setup_new_credentials_toml();
+
+    cargo_process("login --host")
+        .arg(registry_url().to_string())
+        .arg(TOKEN)
+        .with_stderr_contains(
+            "\
+[WARNING] Both `[..]/credentials` and `[..]/credentials.toml` exist. Using `[..]/credentials`
+",
+        )
+        .run();
+
+    // It should use the value from the one without the extension
+    // for backwards compatibility. check_token explicitly checks
+    // 'credentials' without the extension, which is what we want.
+    assert!(check_token(TOKEN, None));
+}
+
 #[cargo_test]
 fn login_with_old_and_new_credentials() {
     setup_new_credentials();