From c530720b294fe4381b706954fdba6c14667d31b4 Mon Sep 17 00:00:00 2001 From: Zach Lute Date: Sat, 24 Aug 2019 12:43:15 -0700 Subject: [PATCH 1/3] Allow using 'config.toml' instead of just 'config' files. 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. We should also consider a separate change to make config.toml the default and update docs, etc. --- src/cargo/util/config.rs | 82 ++++++++++++++++++++++++++------------- tests/testsuite/config.rs | 57 +++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 26 deletions(-) diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index fa7b2b2bcb0..a7659288bb8 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,61 @@ impl Config { } } + 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) { + let possible = current.join(".cargo").join("config"); + let possible_with_extension = current.join(".cargo").join("config.toml"); + + // If both 'config' and 'config.toml' exist, we should use 'config' + // for backward compatibility, but we should warn the user. + if fs::metadata(&possible).is_ok() { + if fs::metadata(&possible_with_extension).is_ok() { + self.shell().warn(format!( + "Both `{}` and `{}` exist. Using `{}`", + possible.display(), + possible_with_extension.display(), + possible.display() + ))?; + } + + walk(&possible)?; + stash.insert(possible); + } else if fs::metadata(&possible_with_extension).is_ok() { + walk(&possible_with_extension)?; + 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"); + let config_with_extension = home.join("config.toml"); + if !stash.contains(&config) && fs::metadata(&config).is_ok() { + if fs::metadata(&config_with_extension).is_ok() { + self.shell().warn(format!( + "Both `{}` and `{}` exist. Using `{}`", + config.display(), + config_with_extension.display(), + config.display() + ))?; + } + + walk(&config)?; + } else if !stash.contains(&config_with_extension) + && fs::metadata(&config_with_extension).is_ok() + { + walk(&config_with_extension)?; + } + + Ok(()) + } + /// Gets the index for a registry. pub fn get_registry_index(&self, registry: &str) -> CargoResult { validate_package_name(registry, "registry name", "")?; @@ -1673,31 +1728,6 @@ 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<()> { let mut file = { cfg.home_path.create_dir()?; diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 5f3cd76c38b..8f7549c4309 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -48,6 +48,12 @@ 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(); +} + 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 +118,57 @@ 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() { + 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( From 25e3fee3c5d6d6d4a747d5316f91a9c3dadfbf02 Mon Sep 17 00:00:00 2001 From: Zach Lute Date: Wed, 28 Aug 2019 23:29:31 -0700 Subject: [PATCH 2/3] Allow using 'credentials.toml' instead of just 'credentials' files. This matches a similar change to config[.toml]. Note that this change only makes 'credentials.toml' optional to use instead of 'credentials'. If both exist, we will print a warning and prefer 'credentials', since that would be the existing behavior if both existed. --- src/cargo/util/config.rs | 96 +++++++++++++++++++++++----------------- tests/testsuite/login.rs | 46 +++++++++++++++++++ 2 files changed, 101 insertions(+), 41 deletions(-) diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index a7659288bb8..bdc344b83a0 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -689,6 +689,38 @@ 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() { + 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<()>, @@ -696,49 +728,19 @@ impl Config { let mut stash: HashSet = HashSet::new(); for current in paths::ancestors(pwd) { - let possible = current.join(".cargo").join("config"); - let possible_with_extension = current.join(".cargo").join("config.toml"); - - // If both 'config' and 'config.toml' exist, we should use 'config' - // for backward compatibility, but we should warn the user. - if fs::metadata(&possible).is_ok() { - if fs::metadata(&possible_with_extension).is_ok() { - self.shell().warn(format!( - "Both `{}` and `{}` exist. Using `{}`", - possible.display(), - possible_with_extension.display(), - possible.display() - ))?; - } - - walk(&possible)?; - stash.insert(possible); - } else if fs::metadata(&possible_with_extension).is_ok() { - walk(&possible_with_extension)?; - stash.insert(possible); + 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. - let config = home.join("config"); - let config_with_extension = home.join("config.toml"); - if !stash.contains(&config) && fs::metadata(&config).is_ok() { - if fs::metadata(&config_with_extension).is_ok() { - self.shell().warn(format!( - "Both `{}` and `{}` exist. Using `{}`", - config.display(), - config_with_extension.display(), - config.display() - ))?; + if let Some(path) = self.get_file_path(home, "config", true)? { + if !stash.contains(&path) { + walk(&path)?; } - - walk(&config)?; - } else if !stash.contains(&config_with_extension) - && fs::metadata(&config_with_extension).is_ok() - { - walk(&config_with_extension)?; } Ok(()) @@ -781,10 +783,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)?; @@ -1729,10 +1731,22 @@ pub fn homedir(cwd: &Path) -> Option { } 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/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(); From 71196832513b4f2c7f90be16f740dbc6fd0a8d4e Mon Sep 17 00:00:00 2001 From: Zach Lute Date: Thu, 29 Aug 2019 00:39:15 -0700 Subject: [PATCH 3/3] Symlinking 'config' to 'config.toml' should not produce a warning. Also true cor symlinking 'credentials.toml' to 'credentials'. This will allow users to run multiple versions of Cargo without generating a warning if they want to use the .toml versions. Note that Windows symlinks require special permission, so in the case the user doesn't have that permission, we don't run the symlink tests. This matches behavior in Rust libstd. --- src/cargo/util/config.rs | 25 ++++++++++--- tests/testsuite/config.rs | 77 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 6 deletions(-) diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index bdc344b83a0..6848733116b 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -705,12 +705,25 @@ impl Config { if fs::metadata(&possible).is_ok() { if warn && fs::metadata(&possible_with_extension).is_ok() { - self.shell().warn(format!( - "Both `{}` and `{}` exist. Using `{}`", - possible.display(), - possible_with_extension.display(), - possible.display() - ))?; + // 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)) diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 8f7549c4309..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}; @@ -54,6 +57,44 @@ fn write_config_toml(config: &str) { 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()); @@ -132,6 +173,42 @@ f1 = 1 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(