diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index fa7b2b2bcb0..6848733116b 100644 --- a/src/cargo/util/config.rs +++ b/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> { + 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(&self, pwd: &Path, home: &Path, mut walk: F) -> CargoResult<()> + where + F: FnMut(&Path) -> CargoResult<()>, + { + let mut stash: HashSet = HashSet::new(); + + for current in paths::ancestors(pwd) { + if let Some(path) = self.get_file_path(¤t.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 { 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 { ::home::cargo_home_with_cwd(cwd).ok() } -fn walk_tree(pwd: &Path, home: &Path, mut walk: F) -> CargoResult<()> -where - F: FnMut(&Path) -> CargoResult<()>, -{ - let mut stash: HashSet = 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) -> 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) = { diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 5f3cd76c38b..bd56e996e0a 100644 --- a/tests/testsuite/config.rs +++ b/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::>("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::>("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::>("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( diff --git a/tests/testsuite/login.rs b/tests/testsuite/login.rs index a9476159e44..d203d818fdb 100644 --- a/tests/testsuite/login.rs +++ b/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();