Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Config file loaded via CLI takes priority over env vars #11077

Merged
merged 7 commits into from
Oct 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/cargo/util/config/de.rs
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,13 @@ impl<'de, 'config> de::MapAccess<'de> for ValueDeserializer<'config> {
Definition::Environment(env) => {
seed.deserialize(Tuple2Deserializer(1i32, env.as_str()))
}
Definition::Cli => seed.deserialize(Tuple2Deserializer(2i32, "")),
Definition::Cli(path) => {
let str = path
.as_ref()
.map(|p| p.to_string_lossy())
.unwrap_or_default();
seed.deserialize(Tuple2Deserializer(2i32, str))
}
}
}
}
Expand Down
95 changes: 70 additions & 25 deletions src/cargo/util/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,20 @@ macro_rules! get_value_typed {
};
}

/// Indicates why a config value is being loaded.
#[derive(Clone, Copy, Debug)]
enum WhyLoad {
/// Loaded due to a request from the global cli arg `--config`
///
/// Indirect configs loaded via [`config-include`] are also seen as from cli args,
/// if the initial config is being loaded from cli.
///
/// [`config-include`]: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#config-include
Cli,
/// Loaded due to config file discovery.
FileDiscovery,
}

/// Configuration information for cargo. This is not specific to a build, it is information
/// relating to cargo itself.
#[derive(Debug)]
Expand Down Expand Up @@ -1005,12 +1019,15 @@ impl Config {
self.load_values_from(&self.cwd)
}

/// Like [`load_values`](Config::load_values) but without merging config values.
///
/// This is primarily crafted for `cargo config` command.
pub(crate) fn load_values_unmerged(&self) -> CargoResult<Vec<ConfigValue>> {
let mut result = Vec::new();
let mut seen = HashSet::new();
let home = self.home_path.clone().into_path_unlocked();
self.walk_tree(&self.cwd, &home, |path| {
let mut cv = self._load_file(path, &mut seen, false)?;
let mut cv = self._load_file(path, &mut seen, false, WhyLoad::FileDiscovery)?;
if self.cli_unstable().config_include {
self.load_unmerged_include(&mut cv, &mut seen, &mut result)?;
}
Expand All @@ -1021,6 +1038,9 @@ impl Config {
Ok(result)
}

/// Like [`load_includes`](Config::load_includes) but without merging config values.
///
/// This is primarily crafted for `cargo config` command.
fn load_unmerged_include(
&self,
cv: &mut CV,
Expand All @@ -1029,23 +1049,26 @@ impl Config {
) -> CargoResult<()> {
let includes = self.include_paths(cv, false)?;
for (path, abs_path, def) in includes {
let mut cv = self._load_file(&abs_path, seen, false).with_context(|| {
format!("failed to load config include `{}` from `{}`", path, def)
})?;
let mut cv = self
._load_file(&abs_path, seen, false, WhyLoad::FileDiscovery)
.with_context(|| {
format!("failed to load config include `{}` from `{}`", path, def)
})?;
self.load_unmerged_include(&mut cv, seen, output)?;
output.push(cv);
}
Ok(())
}

/// Start a config file discovery from a path and merges all config values found.
fn load_values_from(&self, path: &Path) -> CargoResult<HashMap<String, ConfigValue>> {
// This definition path is ignored, this is just a temporary container
// representing the entire file.
let mut cfg = CV::Table(HashMap::new(), Definition::Path(PathBuf::from(".")));
let home = self.home_path.clone().into_path_unlocked();

self.walk_tree(path, &home, |path| {
let value = self.load_file(path, true)?;
let value = self.load_file(path)?;
cfg.merge(value, false).with_context(|| {
format!("failed to merge configuration at `{}`", path.display())
})?;
Expand All @@ -1059,15 +1082,28 @@ impl Config {
}
}

fn load_file(&self, path: &Path, includes: bool) -> CargoResult<ConfigValue> {
self._load_file(path, &mut HashSet::new(), includes)
/// Loads a config value from a path.
///
/// This is used during config file discovery.
fn load_file(&self, path: &Path) -> CargoResult<ConfigValue> {
self._load_file(path, &mut HashSet::new(), true, WhyLoad::FileDiscovery)
}

/// Loads a config value from a path with options.
///
/// This is actual implementation of loading a config value from a path.
///
/// * `includes` determines whether to load configs from [`config-include`].
/// * `seen` is used to check for cyclic includes.
/// * `why_load` tells why a config is being loaded.
///
/// [`config-include`]: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#config-include
fn _load_file(
&self,
path: &Path,
seen: &mut HashSet<PathBuf>,
includes: bool,
why_load: WhyLoad,
) -> CargoResult<ConfigValue> {
if !seen.insert(path.to_path_buf()) {
bail!(
Expand All @@ -1080,15 +1116,18 @@ impl Config {
let toml = cargo_toml::parse(&contents, path, self).with_context(|| {
format!("could not parse TOML configuration in `{}`", path.display())
})?;
let value =
CV::from_toml(Definition::Path(path.to_path_buf()), toml).with_context(|| {
format!(
"failed to load TOML configuration from `{}`",
path.display()
)
})?;
let def = match why_load {
WhyLoad::Cli => Definition::Cli(Some(path.into())),
WhyLoad::FileDiscovery => Definition::Path(path.into()),
};
let value = CV::from_toml(def, toml).with_context(|| {
format!(
"failed to load TOML configuration from `{}`",
path.display()
)
})?;
if includes {
self.load_includes(value, seen)
self.load_includes(value, seen, why_load)
} else {
Ok(value)
}
Expand All @@ -1098,8 +1137,14 @@ impl Config {
///
/// 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<PathBuf>) -> CargoResult<CV> {
/// * `seen` is used to check for cyclic includes.
/// * `why_load` tells why a config is being loaded.
fn load_includes(
&self,
mut value: CV,
seen: &mut HashSet<PathBuf>,
why_load: WhyLoad,
) -> CargoResult<CV> {
// Get the list of files to load.
let includes = self.include_paths(&mut value, true)?;
// Check unstable.
Expand All @@ -1109,7 +1154,7 @@ impl Config {
// Accumulate all values here.
let mut root = CV::Table(HashMap::new(), value.definition().clone());
for (path, abs_path, def) in includes {
self._load_file(&abs_path, seen, true)
self._load_file(&abs_path, seen, true, why_load)
.and_then(|include| root.merge(include, true))
.with_context(|| {
format!("failed to load config include `{}` from `{}`", path, def)
Expand All @@ -1127,8 +1172,8 @@ impl Config {
) -> CargoResult<Vec<(String, PathBuf, Definition)>> {
let abs = |path: &str, def: &Definition| -> (String, PathBuf, Definition) {
let abs_path = match def {
Definition::Path(p) => p.parent().unwrap().join(&path),
Definition::Environment(_) | Definition::Cli => self.cwd().join(&path),
Definition::Path(p) | Definition::Cli(Some(p)) => p.parent().unwrap().join(&path),
Definition::Environment(_) | Definition::Cli(None) => self.cwd().join(&path),
};
(path.to_string(), abs_path, def.clone())
};
Expand Down Expand Up @@ -1162,7 +1207,7 @@ impl Config {

/// Parses the CLI config args and returns them as a table.
pub(crate) fn cli_args_as_table(&self) -> CargoResult<ConfigValue> {
let mut loaded_args = CV::Table(HashMap::new(), Definition::Cli);
let mut loaded_args = CV::Table(HashMap::new(), Definition::Cli(None));
let cli_args = match &self.cli_config {
Some(cli_args) => cli_args,
None => return Ok(loaded_args),
Expand All @@ -1178,7 +1223,7 @@ impl Config {
anyhow::format_err!("config path {:?} is not utf-8", arg_as_path)
})?
.to_string();
self._load_file(&self.cwd().join(&str_path), &mut seen, true)
self._load_file(&self.cwd().join(&str_path), &mut seen, true, WhyLoad::Cli)
.with_context(|| format!("failed to load config from `{}`", str_path))?
} else {
// We only want to allow "dotted key" (see https://toml.io/en/v1.0.0#keys)
Expand Down Expand Up @@ -1273,11 +1318,11 @@ impl Config {
);
}

CV::from_toml(Definition::Cli, toml_v)
CV::from_toml(Definition::Cli(None), toml_v)
.with_context(|| format!("failed to convert --config argument `{arg}`"))?
};
let tmp_table = self
.load_includes(tmp_table, &mut HashSet::new())
.load_includes(tmp_table, &mut HashSet::new(), WhyLoad::Cli)
.with_context(|| "failed to load --config include".to_string())?;
loaded_args
.merge(tmp_table, true)
Expand Down Expand Up @@ -1431,7 +1476,7 @@ impl Config {
None => return Ok(()),
};

let mut value = self.load_file(&credentials, true)?;
let mut value = self.load_file(&credentials)?;
// Backwards compatibility for old `.cargo/credentials` layout.
{
let (value_map, def) = match value {
Expand Down
22 changes: 13 additions & 9 deletions src/cargo/util/config/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ pub enum Definition {
/// Defined in an environment variable, includes the environment key.
Environment(String),
/// Passed in on the command line.
Cli,
/// A path is attached when the config value is a path to a config file.
Cli(Option<PathBuf>),
}

impl Definition {
Expand All @@ -69,8 +70,8 @@ impl Definition {
/// CLI and env are the current working directory.
pub fn root<'a>(&'a self, config: &'a Config) -> &'a Path {
match self {
Definition::Path(p) => p.parent().unwrap().parent().unwrap(),
Definition::Environment(_) | Definition::Cli => config.cwd(),
Definition::Path(p) | Definition::Cli(Some(p)) => p.parent().unwrap().parent().unwrap(),
Definition::Environment(_) | Definition::Cli(None) => config.cwd(),
}
}

Expand All @@ -80,8 +81,8 @@ impl Definition {
pub fn is_higher_priority(&self, other: &Definition) -> bool {
matches!(
(self, other),
(Definition::Cli, Definition::Environment(_))
| (Definition::Cli, Definition::Path(_))
(Definition::Cli(_), Definition::Environment(_))
| (Definition::Cli(_), Definition::Path(_))
| (Definition::Environment(_), Definition::Path(_))
)
}
Expand All @@ -100,9 +101,9 @@ impl PartialEq for Definition {
impl fmt::Display for Definition {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Definition::Path(p) => p.display().fmt(f),
Definition::Path(p) | Definition::Cli(Some(p)) => p.display().fmt(f),
Definition::Environment(key) => write!(f, "environment variable `{}`", key),
Definition::Cli => write!(f, "--config cli option"),
Definition::Cli(None) => write!(f, "--config cli option"),
}
}
}
Expand Down Expand Up @@ -218,8 +219,11 @@ impl<'de> de::Deserialize<'de> for Definition {
match discr {
0 => Ok(Definition::Path(value.into())),
1 => Ok(Definition::Environment(value)),
2 => Ok(Definition::Cli),
_ => panic!("unexpected discriminant {} value {}", discr, value),
2 => {
let path = (value.len() > 0).then_some(value.into());
Ok(Definition::Cli(path))
}
_ => panic!("unexpected discriminant {discr} value {value}"),
}
}
}
70 changes: 69 additions & 1 deletion tests/testsuite/config_cli.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
//! Tests for the --config CLI option.

use super::config::{assert_error, assert_match, read_output, write_config, ConfigBuilder};
use super::config::{
assert_error, assert_match, read_output, write_config, write_config_at, ConfigBuilder,
};
use cargo::util::config::Definition;
use cargo_test_support::paths;
use std::{collections::HashMap, fs};
Expand Down Expand Up @@ -53,6 +55,72 @@ fn cli_priority() {
assert_eq!(config.get::<bool>("term.quiet").unwrap(), true);
}

#[cargo_test]
fn merge_primitives_for_multiple_cli_occurences() {
let config_path0 = ".cargo/file0.toml";
write_config_at(config_path0, "k = 'file0'");
let config_path1 = ".cargo/file1.toml";
write_config_at(config_path1, "k = 'file1'");

// k=env0
let config = ConfigBuilder::new().env("CARGO_K", "env0").build();
assert_eq!(config.get::<String>("k").unwrap(), "env0");

// k=env0
// --config k='cli0'
// --config k='cli1'
let config = ConfigBuilder::new()
.env("CARGO_K", "env0")
.config_arg("k='cli0'")
.config_arg("k='cli1'")
.build();
assert_eq!(config.get::<String>("k").unwrap(), "cli1");

// Env has a lower priority when comparing with file from CLI arg.
//
// k=env0
// --config k='cli0'
// --config k='cli1'
// --config .cargo/file0.toml
let config = ConfigBuilder::new()
.env("CARGO_K", "env0")
.config_arg("k='cli0'")
.config_arg("k='cli1'")
.config_arg(config_path0)
.build();
assert_eq!(config.get::<String>("k").unwrap(), "file0");

// k=env0
// --config k='cli0'
// --config k='cli1'
// --config .cargo/file0.toml
// --config k='cli2'
let config = ConfigBuilder::new()
.env("CARGO_K", "env0")
.config_arg("k='cli0'")
.config_arg("k='cli1'")
.config_arg(config_path0)
.config_arg("k='cli2'")
.build();
assert_eq!(config.get::<String>("k").unwrap(), "cli2");

// k=env0
// --config k='cli0'
// --config k='cli1'
// --config .cargo/file0.toml
// --config k='cli2'
// --config .cargo/file1.toml
let config = ConfigBuilder::new()
.env("CARGO_K", "env0")
.config_arg("k='cli0'")
.config_arg("k='cli1'")
.config_arg(config_path0)
.config_arg("k='cli2'")
.config_arg(config_path1)
.build();
assert_eq!(config.get::<String>("k").unwrap(), "file1");
}

#[cargo_test]
fn merges_array() {
// Array entries are appended.
Expand Down
Loading