From 687ace1ddc1413f12eebe8fbfcf0056d542205ab Mon Sep 17 00:00:00 2001 From: Arlo Siemsen Date: Tue, 15 Aug 2023 23:08:39 -0500 Subject: [PATCH 1/2] Merge higher precedence config lists later 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. --- src/cargo/util/config/mod.rs | 13 ++++++++++--- src/cargo/util/config/value.rs | 19 +++++++++++++++++++ tests/testsuite/cargo_config/mod.rs | 22 +++++++++++----------- tests/testsuite/config.rs | 18 +++++++++--------- tests/testsuite/config_cli.rs | 10 ++++------ 5 files changed, 53 insertions(+), 29 deletions(-) diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index cf977d38d13..b4280049cc7 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -938,6 +938,7 @@ impl Config { .map(|s| (s.to_string(), def.clone())), ); } + output.sort_by(|a, b| a.1.cmp(&b.1)); Ok(()) } @@ -2106,8 +2107,8 @@ 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. /// @@ -2115,7 +2116,13 @@ impl ConfigValue { 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) { diff --git a/src/cargo/util/config/value.rs b/src/cargo/util/config/value.rs index a70d75a07c5..7232bf39ee1 100644 --- a/src/cargo/util/config/value.rs +++ b/src/cargo/util/config/value.rs @@ -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; @@ -63,6 +64,24 @@ pub enum Definition { Cli(Option), } +impl PartialOrd for Definition { + fn partial_cmp(&self, other: &Definition) -> Option { + 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. /// diff --git a/tests/testsuite/cargo_config/mod.rs b/tests/testsuite/cargo_config/mod.rs index dc0a40ed8bf..c1769fb536a 100644 --- a/tests/testsuite/cargo_config/mod.rs +++ b/tests/testsuite/cargo_config/mod.rs @@ -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 @@ -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(); @@ -171,8 +171,8 @@ fn get_json() { "build": { "jobs": 99, "rustflags": [ - "--flag-directory", - "--flag-global" + "--flag-global", + "--flag-directory" ] }, "extra-table": { @@ -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 @@ -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` ] @@ -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 ] ", ) @@ -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(); @@ -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 ] ", ) diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 5d4163818c3..009f3d5a3ff 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -541,18 +541,18 @@ expected boolean, but found array", config.get::("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::("c").unwrap(), VSOB::VecString(vec![ "c".to_string(), - "clic".to_string(), "e1".to_string(), - "e2".to_string() + "e2".to_string(), + "clic".to_string(), ]) ); } @@ -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, diff --git a/tests/testsuite/config_cli.rs b/tests/testsuite/config_cli.rs index 1120e279d9d..67aca0d19c5 100644 --- a/tests/testsuite/config_cli.rs +++ b/tests/testsuite/config_cli.rs @@ -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::>("build.rustflags").unwrap(), - ["--file", "--cli", "--env1", "--env2"] + ["--file", "--env1", "--env2", "--cli"] ); // With advanced-env. @@ -158,7 +156,7 @@ fn merges_array() { .build(); assert_eq!( config.get::>("build.rustflags").unwrap(), - ["--file", "--cli", "--env"] + ["--file", "--env", "--cli"] ); // Merges multiple instances. @@ -202,7 +200,7 @@ fn string_list_array() { .get::("build.rustflags") .unwrap() .as_slice(), - ["--file", "--cli", "--env1", "--env2"] + ["--file", "--env1", "--env2", "--cli"] ); // With advanced-env. @@ -216,7 +214,7 @@ fn string_list_array() { .get::("build.rustflags") .unwrap() .as_slice(), - ["--file", "--cli", "--env"] + ["--file", "--env", "--cli"] ); } From 66bda8382f60e9bfe0d0db6b7a69e276c69080b3 Mon Sep 17 00:00:00 2001 From: Arlo Siemsen Date: Wed, 16 Aug 2023 13:25:17 -0500 Subject: [PATCH 2/2] Update config.md with array merging information --- src/doc/src/reference/config.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/doc/src/reference/config.md b/src/doc/src/reference/config.md index d1f2b04d35e..1c09300bb26 100644 --- a/src/doc/src/reference/config.md +++ b/src/doc/src/reference/config.md @@ -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