From e7eda2f91f379da2f86929164d4ca9cd2ea9cf84 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sun, 1 Dec 2019 10:19:02 -0800 Subject: [PATCH] Implement config-include. --- src/cargo/core/features.rs | 2 + src/cargo/util/config/mod.rs | 162 ++++++++++++++++--------- src/doc/src/reference/unstable.md | 24 ++++ tests/testsuite/config.rs | 133 ++++++++++++++------ tests/testsuite/config_cli.rs | 77 ++++++++++-- tests/testsuite/config_include.rs | 195 ++++++++++++++++++++++++++++++ tests/testsuite/main.rs | 1 + 7 files changed, 490 insertions(+), 104 deletions(-) create mode 100644 tests/testsuite/config_include.rs diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index f54aa152d..5bfab2eee 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -332,6 +332,7 @@ pub struct CliUnstable { pub package_features: bool, pub advanced_env: bool, pub config_profile: bool, + pub config_include: bool, pub dual_proc_macros: bool, pub mtime_on_use: bool, pub named_profiles: bool, @@ -396,6 +397,7 @@ impl CliUnstable { "package-features" => self.package_features = parse_empty(k, v)?, "advanced-env" => self.advanced_env = parse_empty(k, v)?, "config-profile" => self.config_profile = parse_empty(k, v)?, + "config-include" => self.config_include = parse_empty(k, v)?, "dual-proc-macros" => self.dual_proc_macros = parse_empty(k, v)?, // can also be set in .cargo/config or with and ENV "mtime-on-use" => self.mtime_on_use = parse_empty(k, v)?, diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 2a48a843e..f1dd2e0bb 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -659,20 +659,7 @@ impl Config { let home = self.home_path.clone().into_path_unlocked(); self.walk_tree(path, &home, |path| { - let mut contents = String::new(); - let mut file = File::open(&path)?; - file.read_to_string(&mut contents) - .chain_err(|| format!("failed to read configuration file `{}`", path.display()))?; - let toml = cargo_toml::parse(&contents, path, self).chain_err(|| { - format!("could not parse TOML configuration in `{}`", path.display()) - })?; - let value = - CV::from_toml(Definition::Path(path.to_path_buf()), toml).chain_err(|| { - format!( - "failed to load TOML configuration from `{}`", - path.display() - ) - })?; + let value = self.load_file(path)?; cfg.merge(value, false) .chain_err(|| format!("failed to merge configuration at `{}`", path.display()))?; Ok(()) @@ -686,44 +673,124 @@ impl Config { } } + fn load_file(&self, path: &Path) -> CargoResult { + let mut seen = HashSet::new(); + self._load_file(path, &mut seen) + } + + fn _load_file(&self, path: &Path, seen: &mut HashSet) -> CargoResult { + if !seen.insert(path.to_path_buf()) { + bail!( + "config `include` cycle detected with path `{}`", + path.display() + ); + } + let contents = fs::read_to_string(path) + .chain_err(|| format!("failed to read configuration file `{}`", path.display()))?; + let toml = cargo_toml::parse(&contents, path, self) + .chain_err(|| format!("could not parse TOML configuration in `{}`", path.display()))?; + let value = CV::from_toml(Definition::Path(path.to_path_buf()), toml).chain_err(|| { + format!( + "failed to load TOML configuration from `{}`", + path.display() + ) + })?; + let value = self.load_includes(value, seen)?; + Ok(value) + } + + /// Load any `include` files listed in the given `value`. + /// + /// Returns `value` with the given include files merged into it. + /// + /// `seen` is used to check for cyclic includes. + fn load_includes(&self, mut value: CV, seen: &mut HashSet) -> CargoResult { + // Get the list of files to load. + let (includes, def) = match &mut value { + CV::Table(table, _def) => match table.remove("include") { + Some(CV::String(s, def)) => (vec![(s, def.clone())], def), + Some(CV::List(list, def)) => (list, def), + Some(other) => bail!( + "`include` expected a string or list, but found {} in `{}`", + other.desc(), + other.definition() + ), + None => { + return Ok(value); + } + }, + _ => unreachable!(), + }; + // Check unstable. + if !self.cli_unstable().config_include { + self.shell().warn(format!("config `include` in `{}` ignored, the -Zconfig-include command-line flag is required", + def))?; + return Ok(value); + } + // Accumulate all values here. + let mut root = CV::Table(HashMap::new(), value.definition().clone()); + for (path, def) in includes { + let abs_path = match &def { + Definition::Path(p) => p.parent().unwrap().join(&path), + Definition::Environment(_) | Definition::Cli => self.cwd().join(&path), + }; + self._load_file(&abs_path, seen) + .and_then(|include| root.merge(include, true)) + .chain_err(|| format!("failed to load config include `{}` from `{}`", path, def))?; + } + root.merge(value, true)?; + Ok(root) + } + /// Add config arguments passed on the command line. fn merge_cli_args(&mut self) -> CargoResult<()> { - // The clone here is a bit unfortunate, but needed due to mutable - // borrow, and desire to show the entire arg in the error message. - let cli_args = match self.cli_config.clone() { + let cli_args = match &self.cli_config { Some(cli_args) => cli_args, None => return Ok(()), }; - // Force values to be loaded. - let _ = self.values()?; - let values = self.values_mut()?; + let mut loaded_args = CV::Table(HashMap::new(), Definition::Cli); for arg in cli_args { // TODO: This should probably use a more narrow parser, reject // comments, blank lines, [headers], etc. let toml_v: toml::Value = toml::de::from_str(&arg) .chain_err(|| format!("failed to parse --config argument `{}`", arg))?; - let table = match toml_v { - toml::Value::Table(table) => table, - _ => unreachable!(), - }; - if table.len() != 1 { + let toml_table = toml_v.as_table().unwrap(); + if toml_table.len() != 1 { bail!( - "--config argument `{}` expected exactly one key=value pair, got: {:?}", + "--config argument `{}` expected exactly one key=value pair, got {} keys", arg, - table + toml_table.len() ); } - let (key, value) = table.into_iter().next().unwrap(); - let value = CV::from_toml(Definition::Cli, value) + let tmp_table = CV::from_toml(Definition::Cli, toml_v) .chain_err(|| format!("failed to convert --config argument `{}`", arg))?; + let mut seen = HashSet::new(); + let tmp_table = self + .load_includes(tmp_table, &mut seen) + .chain_err(|| format!("failed to load --config include"))?; + loaded_args + .merge(tmp_table, true) + .chain_err(|| format!("failed to merge --config argument `{}`", arg))?; + } + // Force values to be loaded. + let _ = self.values()?; + let values = self.values_mut()?; + let loaded_map = match loaded_args { + CV::Table(table, _def) => table, + _ => unreachable!(), + }; + for (key, value) in loaded_map.into_iter() { match values.entry(key) { Vacant(entry) => { entry.insert(value); } - Occupied(mut entry) => entry - .get_mut() - .merge(value, true) - .chain_err(|| format!("failed to merge --config argument `{}`", arg))?, + Occupied(mut entry) => entry.get_mut().merge(value, true).chain_err(|| { + format!( + "failed to merge --config key `{}` into `{}`", + entry.key(), + entry.get().definition(), + ) + })?, }; } Ok(()) @@ -841,30 +908,7 @@ impl Config { None => return Ok(()), }; - let mut contents = String::new(); - let mut file = File::open(&credentials)?; - file.read_to_string(&mut contents).chain_err(|| { - format!( - "failed to read configuration file `{}`", - credentials.display() - ) - })?; - - let toml = cargo_toml::parse(&contents, &credentials, self).chain_err(|| { - format!( - "could not parse TOML configuration in `{}`", - credentials.display() - ) - })?; - - let mut value = - CV::from_toml(Definition::Path(credentials.clone()), toml).chain_err(|| { - format!( - "failed to load TOML configuration from `{}`", - credentials.display() - ) - })?; - + let mut value = self.load_file(&credentials)?; // Backwards compatibility for old `.cargo/credentials` layout. { let (value_map, def) = match value { @@ -1265,7 +1309,9 @@ impl ConfigValue { | (expected, found @ CV::List(_, _)) | (expected, found @ CV::Table(_, _)) => { return Err(internal(format!( - "expected {}, but found {}", + "failed to merge config value from `{}` into `{}`: expected {}, but found {}", + found.definition(), + expected.definition(), expected.desc(), found.desc() ))); diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index 3de3aa282..583999607 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -461,3 +461,27 @@ cargo --config "target.'cfg(all(target_arch = \"arm\", target_os = \"none\"))'.r # Example of overriding a profile setting. cargo --config profile.dev.package.image.opt-level=3 … ``` + +### config-include +* Original Issue: [#6699](https://github.com/rust-lang/cargo/issues/6699) + +The `include` key in a config file can be used to load another config file. It +takes a string for a path to another file relative to the config file, or a +list of strings. It requires the `-Zconfig-include` command-line option. + +```toml +# .cargo/config +include = '../../some-common-config.toml' +``` + +The config values are first loaded from the include path, and then the config +file's own values are merged on top of it. + +This can be paired with [config-cli](#config-cli) to specify a file to load +from the command-line: + +```console +cargo +nightly -Zunstable-options -Zconfig-include --config 'include="somefile.toml"' build +``` + +CLI paths are relative to the current working directory. diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 8ed50a56f..c618c31f5 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -8,7 +8,7 @@ use std::os; use std::path::{Path, PathBuf}; use cargo::core::{enable_nightly_features, Shell}; -use cargo::util::config::{self, Config, SslVersionConfig}; +use cargo::util::config::{self, Config, SslVersionConfig, StringList}; use cargo::util::toml::{self, VecStringOrBool as VSOB}; use cargo::CargoResult; use cargo_test_support::{normalized_lines_match, paths, project, t}; @@ -55,8 +55,8 @@ impl ConfigBuilder { } /// Sets the current working directory where config files will be loaded. - pub fn cwd(&mut self, path: impl Into) -> &mut Self { - self.cwd = Some(path.into()); + pub fn cwd(&mut self, path: impl AsRef) -> &mut Self { + self.cwd = Some(paths::root().join(path.as_ref())); self } @@ -97,6 +97,13 @@ fn new_config() -> Config { ConfigBuilder::new().build() } +/// Read the output from Config. +pub fn read_output(config: Config) -> String { + drop(config); // Paranoid about flushing the file. + let path = paths::root().join("shell.out"); + fs::read_to_string(path).unwrap() +} + #[cargo_test] fn read_env_vars_for_config() { let p = project() @@ -126,15 +133,17 @@ fn read_env_vars_for_config() { } pub fn write_config(config: &str) { - let path = paths::root().join(".cargo/config"); + write_config_at(paths::root().join(".cargo/config"), config); +} + +pub fn write_config_at(path: impl AsRef, contents: &str) { + let path = paths::root().join(path.as_ref()); fs::create_dir_all(path.parent().unwrap()).unwrap(); - fs::write(path, config).unwrap(); + fs::write(path, contents).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(); + write_config_at(paths::root().join(".cargo/config.toml"), config); } // Several test fail on windows if the user does not have permission to @@ -175,17 +184,21 @@ fn symlink_config_to_config_toml() { t!(symlink_file(&toml_path, &symlink_path)); } -fn assert_error>(error: E, msgs: &str) { +pub fn assert_error>(error: E, msgs: &str) { let causes = error .borrow() .iter_chain() .map(|e| e.to_string()) .collect::>() .join("\n"); - if !normalized_lines_match(msgs, &causes, None) { + assert_match(msgs, &causes); +} + +pub fn assert_match(expected: &str, actual: &str) { + if !normalized_lines_match(expected, actual, None) { panic!( - "Did not find expected:\n{}\nActual error:\n{}\n", - msgs, causes + "Did not find expected:\n{}\nActual:\n{}\n", + expected, actual ); } } @@ -248,9 +261,7 @@ f1 = 1 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 output = read_output(config); let unexpected = "\ warning: Both `[..]/.cargo/config` and `[..]/.cargo/config.toml` exist. Using `[..]/.cargo/config` "; @@ -285,18 +296,11 @@ f1 = 2 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 output = read_output(config); let expected = "\ warning: Both `[..]/.cargo/config` and `[..]/.cargo/config.toml` exist. Using `[..]/.cargo/config` "; - if !normalized_lines_match(expected, &output, None) { - panic!( - "Did not find expected:\n{}\nActual error:\n{}\n", - expected, output - ); - } + assert_match(expected, &output); } #[cargo_test] @@ -326,18 +330,11 @@ unused = 456 assert_eq!(s, S { f1: None }); // Verify the warnings. - drop(config); // Paranoid about flushing the file. - let path = paths::root().join("shell.out"); - let output = fs::read_to_string(path).unwrap(); + let output = read_output(config); let expected = "\ warning: unused config key `S.unused` in `[..]/.cargo/config` "; - if !normalized_lines_match(expected, &output, None) { - panic!( - "Did not find expected:\n{}\nActual error:\n{}\n", - expected, output - ); - } + assert_match(expected, &output); } #[cargo_test] @@ -491,8 +488,10 @@ c = ['c'] let config = ConfigBuilder::new().config_arg("a = ['a']").build_err(); assert_error( config.unwrap_err(), - "failed to merge --config argument `a = ['a']`\n\ - expected boolean, but found array", + "\ +failed to merge --config key `a` into `[..]/.cargo/config` +failed to merge config value from `--config cli option` into `[..]/.cargo/config`: \ +expected boolean, but found array", ); // config-cli and advanced-env @@ -1040,3 +1039,67 @@ Caused by: .unwrap() .is_none()); } + +#[cargo_test] +fn table_merge_failure() { + // Config::merge fails to merge entries in two tables. + write_config_at( + "foo/.cargo/config", + " + [table] + key = ['foo'] + ", + ); + write_config_at( + ".cargo/config", + " + [table] + key = 'bar' + ", + ); + + #[derive(Debug, Deserialize)] + struct Table { + key: StringList, + } + let config = ConfigBuilder::new().cwd("foo").build(); + assert_error( + config.get::("table").unwrap_err(), + "\ +could not load Cargo configuration + +Caused by: + failed to merge configuration at `[..]/.cargo/config` + +Caused by: + failed to merge key `table` between [..]/foo/.cargo/config and [..]/.cargo/config + +Caused by: + failed to merge key `key` between [..]/foo/.cargo/config and [..]/.cargo/config + +Caused by: + failed to merge config value from `[..]/.cargo/config` into `[..]/foo/.cargo/config`: \ + expected array, but found string", + ); +} + +#[cargo_test] +fn non_string_in_array() { + // Currently only strings are supported. + write_config("foo = [1, 2, 3]"); + let config = new_config(); + assert_error( + config.get::>("foo").unwrap_err(), + "\ +could not load Cargo configuration + +Caused by: + failed to load TOML configuration from `[..]/.cargo/config` + +Caused by: + failed to parse key `foo` + +Caused by: + expected string but found integer in list", + ); +} diff --git a/tests/testsuite/config_cli.rs b/tests/testsuite/config_cli.rs index fe41c260b..dffbb8878 100644 --- a/tests/testsuite/config_cli.rs +++ b/tests/testsuite/config_cli.rs @@ -1,8 +1,8 @@ //! Tests for the --config CLI option. -use super::config::{write_config, ConfigBuilder}; +use super::config::{assert_error, assert_match, read_output, write_config, ConfigBuilder}; use cargo::util::config::Definition; -use cargo_test_support::{normalized_lines_match, paths, project}; +use cargo_test_support::{paths, project}; use std::fs; #[cargo_test] @@ -231,18 +231,11 @@ fn unused_key() { .build(); config.build_config().unwrap(); - drop(config); // Paranoid about flushing the file. - let path = paths::root().join("shell.out"); - let output = fs::read_to_string(path).unwrap(); + let output = read_output(config); let expected = "\ warning: unused config key `build.unused` in `--config cli option` "; - if !normalized_lines_match(expected, &output, None) { - panic!( - "Did not find expected:\n{}\nActual error:\n{}\n", - expected, output - ); - } + assert_match(expected, &output); } #[cargo_test] @@ -273,3 +266,65 @@ fn rerooted_remains() { assert_eq!(config.get::("b").unwrap(), "cli1"); assert_eq!(config.get::("c").unwrap(), "cli2"); } + +#[cargo_test] +fn bad_parse() { + // Fail to TOML parse. + let config = ConfigBuilder::new().config_arg("abc").build_err(); + assert_error( + config.unwrap_err(), + "\ +failed to parse --config argument `abc` +expected an equals, found eof at line 1 column 4", + ); +} + +#[cargo_test] +fn too_many_values() { + // Currently restricted to only 1 value. + let config = ConfigBuilder::new().config_arg("a=1\nb=2").build_err(); + assert_error( + config.unwrap_err(), + "\ +--config argument `a=1 +b=2` expected exactly one key=value pair, got 2 keys", + ); + + let config = ConfigBuilder::new().config_arg("").build_err(); + assert_error( + config.unwrap_err(), + "\ + --config argument `` expected exactly one key=value pair, got 0 keys", + ); +} + +#[cargo_test] +fn bad_cv_convert() { + // ConfigValue does not support all TOML types. + let config = ConfigBuilder::new().config_arg("a=2019-12-01").build_err(); + assert_error( + config.unwrap_err(), + "\ +failed to convert --config argument `a=2019-12-01` +failed to parse key `a` +found TOML configuration value of unknown type `datetime`", + ); +} + +#[cargo_test] +fn fail_to_merge_multiple_args() { + // Error message when multiple args fail to merge. + let config = ConfigBuilder::new() + .config_arg("foo='a'") + .config_arg("foo=['a']") + .build_err(); + // This is a little repetitive, but hopefully the user can figure it out. + assert_error( + config.unwrap_err(), + "\ +failed to merge --config argument `foo=['a']` +failed to merge key `foo` between --config cli option and --config cli option +failed to merge config value from `--config cli option` into `--config cli option`: \ +expected string, but found array", + ); +} diff --git a/tests/testsuite/config_include.rs b/tests/testsuite/config_include.rs new file mode 100644 index 000000000..7f3aea692 --- /dev/null +++ b/tests/testsuite/config_include.rs @@ -0,0 +1,195 @@ +//! Tests for `include` config field. + +use super::config::{ + assert_error, assert_match, read_output, write_config, write_config_at, ConfigBuilder, +}; + +#[cargo_test] +fn gated() { + // Requires -Z flag. + write_config("include='other'"); + let config = ConfigBuilder::new().build(); + let output = read_output(config); + let expected = "\ +warning: config `include` in `[..]/.cargo/config` ignored, \ +the -Zconfig-include command-line flag is required +"; + assert_match(expected, &output); +} + +#[cargo_test] +fn simple() { + // Simple test. + write_config_at( + ".cargo/config", + " + include = 'other' + key1 = 1 + key2 = 2 + ", + ); + write_config_at( + ".cargo/other", + " + key2 = 3 + key3 = 4 + ", + ); + let config = ConfigBuilder::new().unstable_flag("config-include").build(); + assert_eq!(config.get::("key1").unwrap(), 1); + assert_eq!(config.get::("key2").unwrap(), 2); + assert_eq!(config.get::("key3").unwrap(), 4); +} + +#[cargo_test] +fn left_to_right() { + // How it merges multiple includes. + write_config_at( + ".cargo/config", + " + include = ['one', 'two'] + primary = 1 + ", + ); + write_config_at( + ".cargo/one", + " + one = 1 + primary = 2 + ", + ); + write_config_at( + ".cargo/two", + " + two = 2 + primary = 3 + ", + ); + let config = ConfigBuilder::new().unstable_flag("config-include").build(); + assert_eq!(config.get::("primary").unwrap(), 1); + assert_eq!(config.get::("one").unwrap(), 1); + assert_eq!(config.get::("two").unwrap(), 2); +} + +#[cargo_test] +fn missing_file() { + // Error when there's a missing file. + write_config("include='missing'"); + let config = ConfigBuilder::new().unstable_flag("config-include").build(); + assert_error( + config.get::("whatever").unwrap_err(), + "\ +could not load Cargo configuration + +Caused by: + failed to load config include `missing` from `[..]/.cargo/config` + +Caused by: + failed to read configuration file `[..]/.cargo/missing` + +Caused by: + No such file or directory (os error 2)", + ); +} + +#[cargo_test] +fn cycle() { + // Detects a cycle. + write_config_at(".cargo/config", "include='one'"); + write_config_at(".cargo/one", "include='two'"); + write_config_at(".cargo/two", "include='config'"); + let config = ConfigBuilder::new().unstable_flag("config-include").build(); + assert_error( + config.get::("whatever").unwrap_err(), + "\ +could not load Cargo configuration + +Caused by: + failed to load config include `one` from `[..]/.cargo/config` + +Caused by: + failed to load config include `two` from `[..]/.cargo/one` + +Caused by: + failed to load config include `config` from `[..]/.cargo/two` + +Caused by: + config `include` cycle detected with path `[..]/.cargo/config`", + ); +} + +#[cargo_test] +fn cli_include() { + // Using --config with include. + // CLI takes priority over files. + write_config_at( + ".cargo/config", + " + foo = 1 + bar = 2 + ", + ); + write_config_at(".cargo/config-foo", "foo = 2"); + let config = ConfigBuilder::new() + .unstable_flag("config-include") + .config_arg("include='.cargo/config-foo'") + .build(); + assert_eq!(config.get::("foo").unwrap(), 2); + assert_eq!(config.get::("bar").unwrap(), 2); +} + +#[cargo_test] +fn bad_format() { + // Not a valid format. + write_config("include = 1"); + let config = ConfigBuilder::new().unstable_flag("config-include").build(); + assert_error( + config.get::("whatever").unwrap_err(), + "\ +could not load Cargo configuration + +Caused by: + `include` expected a string or list, but found integer in `[..]/.cargo/config`", + ); +} + +#[cargo_test] +fn cli_include_failed() { + // Error message when CLI include fails to load. + let config = ConfigBuilder::new() + .unstable_flag("config-include") + .config_arg("include='foobar'") + .build_err(); + assert_error( + config.unwrap_err(), + "\ +failed to load --config include +failed to load config include `foobar` from `--config cli option` +failed to read configuration file `[..]/foobar` +No such file or directory (os error 2)", + ); +} + +#[cargo_test] +fn cli_merge_failed() { + // Error message when CLI include merge fails. + write_config("foo = ['a']"); + write_config_at( + ".cargo/other", + " + foo = 'b' + ", + ); + let config = ConfigBuilder::new() + .unstable_flag("config-include") + .config_arg("include='.cargo/other'") + .build_err(); + // Maybe this error message should mention it was from an include file? + assert_error( + config.unwrap_err(), + "\ +failed to merge --config key `foo` into `[..]/.cargo/config` +failed to merge config value from `[..]/.cargo/other` into `[..]/.cargo/config`: \ +expected array, but found string", + ); +} diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index 7386aad3b..c96bf9068 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -32,6 +32,7 @@ mod collisions; mod concurrent; mod config; mod config_cli; +mod config_include; mod corrupt_git; mod cross_compile; mod cross_publish; -- 2.39.5