Skip to content

Commit

Permalink
Merge higher precedence config lists later
Browse files Browse the repository at this point in the history 
When merging configuration lists, the current order does not match
the expected precedence. This makes merged lists follow precedence
order, with higher precedence items merged later in lists.
  • Loading branch information
@arlosi
arlosi committed Aug 16, 2023
1 parent 7c3904d commit 687ace1
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 29 deletions.
13 changes: 10 additions & 3 deletions src/cargo/util/config/mod.rs
View file
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.
///
/// 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
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
22 changes: 11 additions & 11 deletions tests/testsuite/cargo_config/mod.rs
View file
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
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
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

0 comments on commit 687ace1

Please sign in to comment.