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.

Sign up for GitHub

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: merge lists in precedence order #12515

Merged
merged 2 commits into from
Aug 22, 2023
Merged

config: merge lists in precedence order #12515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
687ace1
Merge higher precedence config lists later
arlosi Aug 16, 2023
66bda83
Update config.md with array merging information
arlosi Aug 16, 2023
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
13 changes: 10 additions & 3 deletions src/cargo/util/config/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -938,6 +938,7 @@ impl Config {
.map(|s| (s.to_string(), def.clone())),
);
}
output.sort_by(|a, b| a.1.cmp(&b.1));
Ok(())
}

Expand Down Expand Up @@ -2106,16 +2107,22 @@ impl ConfigValue {

/// Merge the given value into self.
///
/// If `force` is true, primitive (non-container) types will override existing values.
/// If false, the original will be kept and the new value ignored.
/// If `force` is true, primitive (non-container) types will override existing values
/// of equal priority. For arrays, incoming values of equal priority will be placed later.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks correct. Just what to make sure we have test to verify this behavior.

///
/// Container types (tables and arrays) are merged with existing values.
///
/// Container and non-container types cannot be mixed.
fn merge(&mut self, from: ConfigValue, force: bool) -> CargoResult<()> {
match (self, from) {
(&mut CV::List(ref mut old, _), CV::List(ref mut new, _)) => {
old.extend(mem::take(new).into_iter());
if force {
old.append(new);
} else {
new.append(old);
mem::swap(new, old);
}
old.sort_by(|a, b| a.1.cmp(&b.1));
}
(&mut CV::Table(ref mut old, _), CV::Table(ref mut new, _)) => {
for (key, value) in mem::take(new) {
Expand Down
19 changes: 19 additions & 0 deletions src/cargo/util/config/value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

use crate::util::config::Config;
use serde::de;
use std::cmp::Ordering;
use std::fmt;
use std::marker;
use std::mem;
Expand Down Expand Up @@ -63,6 +64,24 @@ pub enum Definition {
Cli(Option<PathBuf>),
}

impl PartialOrd for Definition {
fn partial_cmp(&self, other: &Definition) -> Option<Ordering> {
Some(self.cmp(other))
}
}

impl Ord for Definition {
fn cmp(&self, other: &Definition) -> Ordering {
if mem::discriminant(self) == mem::discriminant(other) {
Ordering::Equal
} else if self.is_higher_priority(other) {
Ordering::Greater
} else {
Ordering::Less
}
}
}

impl Definition {
/// Root directory where this is defined.
///
Expand Down
3 changes: 2 additions & 1 deletion src/doc/src/reference/config.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ with a configuration file in your home directory.
If a key is specified in multiple config files, the values will get merged
together. Numbers, strings, and booleans will use the value in the deeper
config directory taking precedence over ancestor directories, where the
home directory is the lowest priority. Arrays will be joined together.
home directory is the lowest priority. Arrays will be joined together
with higher precedence items being placed later in the merged array.

At present, when being invoked from a workspace, Cargo does not read config
files from crates within the workspace. i.e. if a workspace has two crates in
Expand Down
22 changes: 11 additions & 11 deletions tests/testsuite/cargo_config/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ fn get_toml() {
alias.foo = \"abc --xyz\"
alias.sub-example = [\"sub\", \"example\"]
build.jobs = 99
build.rustflags = [\"--flag-directory\", \"--flag-global\"]
build.rustflags = [\"--flag-global\", \"--flag-directory\"]
extra-table.somekey = \"somevalue\"
profile.dev.opt-level = 3
profile.dev.package.foo.opt-level = 1
Expand All @@ -111,7 +111,7 @@ target.\"cfg(target_os = \\\"linux\\\")\".runner = \"runme\"
cargo_process("config get build.rustflags -Zunstable-options")
.cwd(&sub_folder.parent().unwrap())
.masquerade_as_nightly_cargo(&["cargo-config"])
.with_stdout("build.rustflags = [\"--flag-directory\", \"--flag-global\"]")
.with_stdout("build.rustflags = [\"--flag-global\", \"--flag-directory\"]")
.with_stderr("")
.run();

Expand Down Expand Up @@ -171,8 +171,8 @@ fn get_json() {
"build": {
"jobs": 99,
"rustflags": [
"--flag-directory",
"--flag-global"
"--flag-global",
"--flag-directory"
]
},
"extra-table": {
Expand Down Expand Up @@ -259,8 +259,8 @@ alias.sub-example = [
]
build.jobs = 99 # [ROOT]/home/.cargo/config.toml
build.rustflags = [
\"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
\"--flag-global\", # [ROOT]/home/.cargo/config.toml
\"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
]
extra-table.somekey = \"somevalue\" # [ROOT]/home/.cargo/config.toml
profile.dev.opt-level = 3 # [ROOT]/home/.cargo/config.toml
Expand All @@ -280,8 +280,8 @@ target.\"cfg(target_os = \\\"linux\\\")\".runner = \"runme\" # [ROOT]/home/.carg
.with_stdout(
"\
build.rustflags = [
\"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
\"--flag-global\", # [ROOT]/home/.cargo/config.toml
\"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
\"env1\", # environment variable `CARGO_BUILD_RUSTFLAGS`
\"env2\", # environment variable `CARGO_BUILD_RUSTFLAGS`
]
Expand Down Expand Up @@ -310,12 +310,12 @@ fn show_origin_toml_cli() {
.with_stdout(
"\
build.rustflags = [
\"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
\"--flag-global\", # [ROOT]/home/.cargo/config.toml
\"cli1\", # --config cli option
\"cli2\", # --config cli option
\"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
\"env1\", # environment variable `CARGO_BUILD_RUSTFLAGS`
\"env2\", # environment variable `CARGO_BUILD_RUSTFLAGS`
\"cli1\", # --config cli option
\"cli2\", # --config cli option
]
",
)
Expand Down Expand Up @@ -471,7 +471,7 @@ fn includes() {
cargo_process("config get build.rustflags -Zunstable-options -Zconfig-include")
.cwd(&sub_folder.parent().unwrap())
.masquerade_as_nightly_cargo(&["cargo-config", "config-include"])
.with_stdout(r#"build.rustflags = ["--flag-other", "--flag-directory", "--flag-global"]"#)
.with_stdout(r#"build.rustflags = ["--flag-global", "--flag-other", "--flag-directory"]"#)
.with_stderr("")
.run();

Expand All @@ -481,9 +481,9 @@ fn includes() {
.with_stdout(
"\
build.rustflags = [
\"--flag-global\", # [ROOT]/home/.cargo/config.toml
\"--flag-other\", # [ROOT]/foo/.cargo/other.toml
\"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
\"--flag-global\", # [ROOT]/home/.cargo/config.toml
]
",
)
Expand Down
18 changes: 9 additions & 9 deletions tests/testsuite/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,18 +541,18 @@ expected boolean, but found array",
config.get::<VSOB>("b").unwrap(),
VSOB::VecString(vec![
"b".to_string(),
"clib".to_string(),
"env1".to_string(),
"env2".to_string()
"env2".to_string(),
"clib".to_string(),
])
);
assert_eq!(
config.get::<VSOB>("c").unwrap(),
VSOB::VecString(vec![
"c".to_string(),
"clic".to_string(),
"e1".to_string(),
"e2".to_string()
"e2".to_string(),
"clic".to_string(),
])
);
}
Expand Down Expand Up @@ -1582,12 +1582,12 @@ known-hosts = [
.as_ref()
.unwrap();
assert_eq!(kh.len(), 4);
assert_eq!(kh[0].val, "example.org ...");
assert_eq!(kh[0].definition, Definition::Path(foo_path.clone()));
assert_eq!(kh[1].val, "example.com ...");
assert_eq!(kh[0].val, "example.com ...");
assert_eq!(kh[0].definition, Definition::Path(root_path.clone()));
assert_eq!(kh[1].val, "example.net ...");
assert_eq!(kh[1].definition, Definition::Path(root_path.clone()));
assert_eq!(kh[2].val, "example.net ...");
assert_eq!(kh[2].definition, Definition::Path(root_path.clone()));
assert_eq!(kh[2].val, "example.org ...");
assert_eq!(kh[2].definition, Definition::Path(foo_path.clone()));
assert_eq!(kh[3].val, "env-example");
assert_eq!(
kh[3].definition,
Expand Down
10 changes: 4 additions & 6 deletions tests/testsuite/config_cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,11 +143,9 @@ fn merges_array() {
.env("CARGO_BUILD_RUSTFLAGS", "--env1 --env2")
.config_arg("build.rustflags = ['--cli']")
.build();
// The order of cli/env is a little questionable here, but would require
// much more complex merging logic.
assert_eq!(
config.get::<Vec<String>>("build.rustflags").unwrap(),
["--file", "--cli", "--env1", "--env2"]
["--file", "--env1", "--env2", "--cli"]
);

// With advanced-env.
Expand All @@ -158,7 +156,7 @@ fn merges_array() {
.build();
assert_eq!(
config.get::<Vec<String>>("build.rustflags").unwrap(),
["--file", "--cli", "--env"]
["--file", "--env", "--cli"]
);

// Merges multiple instances.
Expand Down Expand Up @@ -202,7 +200,7 @@ fn string_list_array() {
.get::<cargo::util::config::StringList>("build.rustflags")
.unwrap()
.as_slice(),
["--file", "--cli", "--env1", "--env2"]
["--file", "--env1", "--env2", "--cli"]
);

// With advanced-env.
Expand All @@ -216,7 +214,7 @@ fn string_list_array() {
.get::<cargo::util::config::StringList>("build.rustflags")
.unwrap()
.as_slice(),
["--file", "--cli", "--env"]
["--file", "--env", "--cli"]
);
}

Expand Down