Skip to content

Commit 2cc50bc

Browse files
borsbors
committed
Auto merge of #12515 - arlosi:high-pri-last, r=weihanglo
config: merge lists in precedence order 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. When a list in configuration exists in multiple places, Cargo merges the lists together. The ordering of this merging is unexpected and does not follow the precedence rules that non-list configuration uses. The current merging order appears to be: * project-specific `config.toml` * global `config.toml` * command-line (`--config`) * environment variable (`CARGO_*`) This PR changes the order to follow the precedence rules with higher precedence configuration merging later in the lists. * global `config.toml` * project-specific `config.toml` * environment variable (`CARGO_*`) * command-line (`--config`) This aligns with config such as `build.rustflags` where later flags take precedence over earlier ones. Since `--config` is relatively new, it's unlikely to cause too much breakage by making it come after environment variables. Switching global and project-specific ordering is more likely to cause breakage, since it's been around longer (reported as an issue in #8128). Projects relying on global configuration flags (in `$CARGO_HOME\config.toml` or in `.cargo/config.toml` further from the project) being merged first in lists will be broken. For most uses of merged lists (such as `build.rustflags`), if the flags do not conflict with each other, there will be no impact. Fixes #12506 Fixes #8128
2 parents 43585c1 + 66bda83 commit 2cc50bc

File tree

6 files changed

+55
-30
lines changed

6 files changed

+55
-30
lines changed

‎src/cargo/util/config/mod.rs

+10-3
Original file line numberDiff line numberDiff line change
@@ -938,6 +938,7 @@ impl Config {
938938
.map(|s| (s.to_string(), def.clone())),
939939
);
940940
}
941+
output.sort_by(|a, b| a.1.cmp(&b.1));
941942
Ok(())
942943
}
943944

@@ -2106,16 +2107,22 @@ impl ConfigValue {
21062107

21072108
/// Merge the given value into self.
21082109
///
2109-
/// If `force` is true, primitive (non-container) types will override existing values.
2110-
/// If false, the original will be kept and the new value ignored.
2110+
/// If `force` is true, primitive (non-container) types will override existing values
2111+
/// of equal priority. For arrays, incoming values of equal priority will be placed later.
21112112
///
21122113
/// Container types (tables and arrays) are merged with existing values.
21132114
///
21142115
/// Container and non-container types cannot be mixed.
21152116
fn merge(&mut self, from: ConfigValue, force: bool) -> CargoResult<()> {
21162117
match (self, from) {
21172118
(&mut CV::List(ref mut old, _), CV::List(ref mut new, _)) => {
2118-
old.extend(mem::take(new).into_iter());
2119+
if force {
2120+
old.append(new);
2121+
} else {
2122+
new.append(old);
2123+
mem::swap(new, old);
2124+
}
2125+
old.sort_by(|a, b| a.1.cmp(&b.1));
21192126
}
21202127
(&mut CV::Table(ref mut old, _), CV::Table(ref mut new, _)) => {
21212128
for (key, value) in mem::take(new) {

‎src/cargo/util/config/value.rs

+19
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
1111
use crate::util::config::Config;
1212
use serde::de;
13+
use std::cmp::Ordering;
1314
use std::fmt;
1415
use std::marker;
1516
use std::mem;
@@ -63,6 +64,24 @@ pub enum Definition {
6364
Cli(Option<PathBuf>),
6465
}
6566

67+
impl PartialOrd for Definition {
68+
fn partial_cmp(&self, other: &Definition) -> Option<Ordering> {
69+
Some(self.cmp(other))
70+
}
71+
}
72+
73+
impl Ord for Definition {
74+
fn cmp(&self, other: &Definition) -> Ordering {
75+
if mem::discriminant(self) == mem::discriminant(other) {
76+
Ordering::Equal
77+
} else if self.is_higher_priority(other) {
78+
Ordering::Greater
79+
} else {
80+
Ordering::Less
81+
}
82+
}
83+
}
84+
6685
impl Definition {
6786
/// Root directory where this is defined.
6887
///

‎src/doc/src/reference/config.md

+2-1
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,8 @@ with a configuration file in your home directory.
2828
If a key is specified in multiple config files, the values will get merged
2929
together. Numbers, strings, and booleans will use the value in the deeper
3030
config directory taking precedence over ancestor directories, where the
31-
home directory is the lowest priority. Arrays will be joined together.
31+
home directory is the lowest priority. Arrays will be joined together
32+
with higher precedence items being placed later in the merged array.
3233

3334
At present, when being invoked from a workspace, Cargo does not read config
3435
files from crates within the workspace. i.e. if a workspace has two crates in

‎tests/testsuite/cargo_config/mod.rs

+11-11
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ fn get_toml() {
8484
alias.foo = \"abc --xyz\"
8585
alias.sub-example = [\"sub\", \"example\"]
8686
build.jobs = 99
87-
build.rustflags = [\"--flag-directory\", \"--flag-global\"]
87+
build.rustflags = [\"--flag-global\", \"--flag-directory\"]
8888
extra-table.somekey = \"somevalue\"
8989
profile.dev.opt-level = 3
9090
profile.dev.package.foo.opt-level = 1
@@ -111,7 +111,7 @@ target.\"cfg(target_os = \\\"linux\\\")\".runner = \"runme\"
111111
cargo_process("config get build.rustflags -Zunstable-options")
112112
.cwd(&sub_folder.parent().unwrap())
113113
.masquerade_as_nightly_cargo(&["cargo-config"])
114-
.with_stdout("build.rustflags = [\"--flag-directory\", \"--flag-global\"]")
114+
.with_stdout("build.rustflags = [\"--flag-global\", \"--flag-directory\"]")
115115
.with_stderr("")
116116
.run();
117117

@@ -171,8 +171,8 @@ fn get_json() {
171171
"build": {
172172
"jobs": 99,
173173
"rustflags": [
174-
"--flag-directory",
175-
"--flag-global"
174+
"--flag-global",
175+
"--flag-directory"
176176
]
177177
},
178178
"extra-table": {
@@ -259,8 +259,8 @@ alias.sub-example = [
259259
]
260260
build.jobs = 99 # [ROOT]/home/.cargo/config.toml
261261
build.rustflags = [
262-
\"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
263262
\"--flag-global\", # [ROOT]/home/.cargo/config.toml
263+
\"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
264264
]
265265
extra-table.somekey = \"somevalue\" # [ROOT]/home/.cargo/config.toml
266266
profile.dev.opt-level = 3 # [ROOT]/home/.cargo/config.toml
@@ -280,8 +280,8 @@ target.\"cfg(target_os = \\\"linux\\\")\".runner = \"runme\" # [ROOT]/home/.carg
280280
.with_stdout(
281281
"\
282282
build.rustflags = [
283-
\"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
284283
\"--flag-global\", # [ROOT]/home/.cargo/config.toml
284+
\"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
285285
\"env1\", # environment variable `CARGO_BUILD_RUSTFLAGS`
286286
\"env2\", # environment variable `CARGO_BUILD_RUSTFLAGS`
287287
]
@@ -310,12 +310,12 @@ fn show_origin_toml_cli() {
310310
.with_stdout(
311311
"\
312312
build.rustflags = [
313-
\"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
314313
\"--flag-global\", # [ROOT]/home/.cargo/config.toml
315-
\"cli1\", # --config cli option
316-
\"cli2\", # --config cli option
314+
\"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
317315
\"env1\", # environment variable `CARGO_BUILD_RUSTFLAGS`
318316
\"env2\", # environment variable `CARGO_BUILD_RUSTFLAGS`
317+
\"cli1\", # --config cli option
318+
\"cli2\", # --config cli option
319319
]
320320
",
321321
)
@@ -471,7 +471,7 @@ fn includes() {
471471
cargo_process("config get build.rustflags -Zunstable-options -Zconfig-include")
472472
.cwd(&sub_folder.parent().unwrap())
473473
.masquerade_as_nightly_cargo(&["cargo-config", "config-include"])
474-
.with_stdout(r#"build.rustflags = ["--flag-other", "--flag-directory", "--flag-global"]"#)
474+
.with_stdout(r#"build.rustflags = ["--flag-global", "--flag-other", "--flag-directory"]"#)
475475
.with_stderr("")
476476
.run();
477477

@@ -481,9 +481,9 @@ fn includes() {
481481
.with_stdout(
482482
"\
483483
build.rustflags = [
484+
\"--flag-global\", # [ROOT]/home/.cargo/config.toml
484485
\"--flag-other\", # [ROOT]/foo/.cargo/other.toml
485486
\"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
486-
\"--flag-global\", # [ROOT]/home/.cargo/config.toml
487487
]
488488
",
489489
)

‎tests/testsuite/config.rs

+9-9
Original file line numberDiff line numberDiff line change
@@ -541,18 +541,18 @@ expected boolean, but found array",
541541
config.get::<VSOB>("b").unwrap(),
542542
VSOB::VecString(vec![
543543
"b".to_string(),
544-
"clib".to_string(),
545544
"env1".to_string(),
546-
"env2".to_string()
545+
"env2".to_string(),
546+
"clib".to_string(),
547547
])
548548
);
549549
assert_eq!(
550550
config.get::<VSOB>("c").unwrap(),
551551
VSOB::VecString(vec![
552552
"c".to_string(),
553-
"clic".to_string(),
554553
"e1".to_string(),
555-
"e2".to_string()
554+
"e2".to_string(),
555+
"clic".to_string(),
556556
])
557557
);
558558
}
@@ -1582,12 +1582,12 @@ known-hosts = [
15821582
.as_ref()
15831583
.unwrap();
15841584
assert_eq!(kh.len(), 4);
1585-
assert_eq!(kh[0].val, "example.org ...");
1586-
assert_eq!(kh[0].definition, Definition::Path(foo_path.clone()));
1587-
assert_eq!(kh[1].val, "example.com ...");
1585+
assert_eq!(kh[0].val, "example.com ...");
1586+
assert_eq!(kh[0].definition, Definition::Path(root_path.clone()));
1587+
assert_eq!(kh[1].val, "example.net ...");
15881588
assert_eq!(kh[1].definition, Definition::Path(root_path.clone()));
1589-
assert_eq!(kh[2].val, "example.net ...");
1590-
assert_eq!(kh[2].definition, Definition::Path(root_path.clone()));
1589+
assert_eq!(kh[2].val, "example.org ...");
1590+
assert_eq!(kh[2].definition, Definition::Path(foo_path.clone()));
15911591
assert_eq!(kh[3].val, "env-example");
15921592
assert_eq!(
15931593
kh[3].definition,

‎tests/testsuite/config_cli.rs

+4-6
Original file line numberDiff line numberDiff line change
@@ -143,11 +143,9 @@ fn merges_array() {
143143
.env("CARGO_BUILD_RUSTFLAGS", "--env1 --env2")
144144
.config_arg("build.rustflags = ['--cli']")
145145
.build();
146-
// The order of cli/env is a little questionable here, but would require
147-
// much more complex merging logic.
148146
assert_eq!(
149147
config.get::<Vec<String>>("build.rustflags").unwrap(),
150-
["--file", "--cli", "--env1", "--env2"]
148+
["--file", "--env1", "--env2", "--cli"]
151149
);
152150

153151
// With advanced-env.
@@ -158,7 +156,7 @@ fn merges_array() {
158156
.build();
159157
assert_eq!(
160158
config.get::<Vec<String>>("build.rustflags").unwrap(),
161-
["--file", "--cli", "--env"]
159+
["--file", "--env", "--cli"]
162160
);
163161

164162
// Merges multiple instances.
@@ -202,7 +200,7 @@ fn string_list_array() {
202200
.get::<cargo::util::config::StringList>("build.rustflags")
203201
.unwrap()
204202
.as_slice(),
205-
["--file", "--cli", "--env1", "--env2"]
203+
["--file", "--env1", "--env2", "--cli"]
206204
);
207205

208206
// With advanced-env.
@@ -216,7 +214,7 @@ fn string_list_array() {
216214
.get::<cargo::util::config::StringList>("build.rustflags")
217215
.unwrap()
218216
.as_slice(),
219-
["--file", "--cli", "--env"]
217+
["--file", "--env", "--cli"]
220218
);
221219
}
222220

0 commit comments

Comments
 (0)