From 82ae7fdc47dff5f51852e9ea0d7f5bea898273e7 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 7 Oct 2025 22:17:38 -0400 Subject: [PATCH 1/5] refactor(gctx): inline `GlobalContext::get_list_or_string` It is only used in one place. --- src/cargo/util/context/de.rs | 20 ++++++++++++++++++-- src/cargo/util/context/mod.rs | 21 --------------------- 2 files changed, 18 insertions(+), 23 deletions(-) diff --git a/src/cargo/util/context/de.rs b/src/cargo/util/context/de.rs index fd149e7dbe5..42e030b26a4 100644 --- a/src/cargo/util/context/de.rs +++ b/src/cargo/util/context/de.rs @@ -155,8 +155,24 @@ impl<'de, 'gctx> de::Deserializer<'de> for Deserializer<'gctx> { V: de::Visitor<'de>, { if name == "StringList" { - let vals = self.gctx.get_list_or_string(&self.key)?; - let vals: Vec = vals.into_iter().map(|vd| vd.0).collect(); + let mut res = Vec::new(); + + match self.gctx.get_cv(&self.key)? { + Some(CV::List(val, _def)) => res.extend(val), + Some(CV::String(val, def)) => { + let split_vs = val.split_whitespace().map(|s| (s.to_string(), def.clone())); + res.extend(split_vs); + } + Some(val) => { + self.gctx + .expected("string or array of strings", &self.key, &val)?; + } + None => {} + } + + self.gctx.get_env_list(&self.key, &mut res)?; + + let vals: Vec = res.into_iter().map(|vd| vd.0).collect(); visitor.visit_newtype_struct(vals.into_deserializer()) } else { visitor.visit_newtype_struct(self) diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index 0062018988a..6e8592d0ae7 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -1006,27 +1006,6 @@ impl GlobalContext { } } - /// Helper for `StringList` type to get something that is a string or list. - fn get_list_or_string(&self, key: &ConfigKey) -> CargoResult> { - let mut res = Vec::new(); - - match self.get_cv(key)? { - Some(CV::List(val, _def)) => res.extend(val), - Some(CV::String(val, def)) => { - let split_vs = val.split_whitespace().map(|s| (s.to_string(), def.clone())); - res.extend(split_vs); - } - Some(val) => { - return self.expected("string or array of strings", key, &val); - } - None => {} - } - - self.get_env_list(key, &mut res)?; - - Ok(res) - } - /// Internal method for getting an environment variable as a list. /// If the key is a non-mergeable list and a value is found in the environment, existing values are cleared. fn get_env_list( From e70e63d89a23d99bd39877995fbcf6c50e951530 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 7 Oct 2025 22:30:45 -0400 Subject: [PATCH 2/5] refactor(gctx): inline `GlobalContext::_get_list` --- src/cargo/util/context/de.rs | 11 +++++++++-- src/cargo/util/context/mod.rs | 8 ++------ 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/cargo/util/context/de.rs b/src/cargo/util/context/de.rs index 42e030b26a4..5f6325b4b2a 100644 --- a/src/cargo/util/context/de.rs +++ b/src/cargo/util/context/de.rs @@ -402,8 +402,15 @@ struct ConfigSeqAccess { impl ConfigSeqAccess { fn new(de: Deserializer<'_>) -> Result { let mut res = Vec::new(); - if let Some(v) = de.gctx._get_list(&de.key)? { - res.extend(v.val); + + match de.gctx.get_cv(&de.key)? { + Some(CV::List(val, _definition)) => { + res.extend(val); + } + Some(val) => { + de.gctx.expected("list", &de.key, &val)?; + } + None => {} } de.gctx.get_env_list(&de.key, &mut res)?; diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index 6e8592d0ae7..96ecc69882c 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -995,13 +995,9 @@ impl GlobalContext { /// if you want that. pub fn get_list(&self, key: &str) -> CargoResult>> { let key = ConfigKey::from_str(key); - self._get_list(&key) - } - - fn _get_list(&self, key: &ConfigKey) -> CargoResult>> { - match self.get_cv(key)? { + match self.get_cv(&key)? { Some(CV::List(val, definition)) => Ok(Some(Value { val, definition })), - Some(val) => self.expected("list", key, &val), + Some(val) => self.expected("list", &key, &val), None => Ok(None), } } From 9031c50721fb9f80d06f6828c3318cdbcc8c3262 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 7 Oct 2025 22:54:11 -0400 Subject: [PATCH 3/5] refactor(gctx): make paths overrides getter explicit --- src/cargo/ops/resolve.rs | 2 +- src/cargo/util/context/mod.rs | 13 ++++--------- tests/testsuite/config_cli.rs | 2 +- 3 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index c741ee4be37..6ba00685c39 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -527,7 +527,7 @@ pub fn add_overrides<'a>( ws: &Workspace<'a>, ) -> CargoResult<()> { let gctx = ws.gctx(); - let Some(paths) = gctx.get_list("paths")? else { + let Some(paths) = gctx.paths_overrides()? else { return Ok(()); }; diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index 96ecc69882c..ea9a976df8e 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -986,15 +986,10 @@ impl GlobalContext { } } - /// Get a list of strings. - /// - /// DO NOT USE outside of the config module. `pub` will be removed in the - /// future. - /// - /// NOTE: this does **not** support environment variables. Use `get` instead - /// if you want that. - pub fn get_list(&self, key: &str) -> CargoResult>> { - let key = ConfigKey::from_str(key); + /// Get the `paths` overrides config value. + pub fn paths_overrides(&self) -> CargoResult>> { + let key = ConfigKey::from_str("paths"); + // paths overrides cannot be set via env config, so use get_cv here. match self.get_cv(&key)? { Some(CV::List(val, definition)) => Ok(Some(Value { val, definition })), Some(val) => self.expected("list", &key, &val), diff --git a/tests/testsuite/config_cli.rs b/tests/testsuite/config_cli.rs index e95d8aed518..b126ea9a02e 100644 --- a/tests/testsuite/config_cli.rs +++ b/tests/testsuite/config_cli.rs @@ -281,7 +281,7 @@ fn merge_array_mixed_def_paths() { // env is currently ignored for get_list() .env("CARGO_PATHS", "env") .build(); - let paths = gctx.get_list("paths").unwrap().unwrap(); + let paths = gctx.paths_overrides().unwrap().unwrap(); // The definition for the root value is somewhat arbitrary, but currently starts with the file because that is what is loaded first. assert_eq!(paths.definition, Definition::Path(paths::root())); assert_eq!(paths.val.len(), 2); From cfb22304cc82faa284d040d3f9cede9ffd52c799 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 7 Oct 2025 23:04:35 -0400 Subject: [PATCH 4/5] refactor(gctx): move paths overrides out from ConfigValue method section --- src/cargo/util/context/mod.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index ea9a976df8e..a5b3ad19277 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -986,17 +986,6 @@ impl GlobalContext { } } - /// Get the `paths` overrides config value. - pub fn paths_overrides(&self) -> CargoResult>> { - let key = ConfigKey::from_str("paths"); - // paths overrides cannot be set via env config, so use get_cv here. - match self.get_cv(&key)? { - Some(CV::List(val, definition)) => Ok(Some(Value { val, definition })), - Some(val) => self.expected("list", &key, &val), - None => Ok(None), - } - } - /// Internal method for getting an environment variable as a list. /// If the key is a non-mergeable list and a value is found in the environment, existing values are cleared. fn get_env_list( @@ -1777,6 +1766,17 @@ impl GlobalContext { .unwrap_or_else(|| PathBuf::from(tool_str)) } + /// Get the `paths` overrides config value. + pub fn paths_overrides(&self) -> CargoResult>> { + let key = ConfigKey::from_str("paths"); + // paths overrides cannot be set via env config, so use get_cv here. + match self.get_cv(&key)? { + Some(CV::List(val, definition)) => Ok(Some(Value { val, definition })), + Some(val) => self.expected("list", &key, &val), + None => Ok(None), + } + } + pub fn jobserver_from_env(&self) -> Option<&jobserver::Client> { self.jobserver.as_ref() } From baa3f92d85b1917dc3ce5cd5ad467bd372a78d30 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 7 Oct 2025 23:15:55 -0400 Subject: [PATCH 5/5] test(paths): migrate paths overrides config to e2e test It is weird that we are testing our internal API for the case of path overrides unsupporting env vars --- tests/testsuite/config_cli.rs | 28 ------------------ tests/testsuite/paths.rs | 53 +++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 28 deletions(-) diff --git a/tests/testsuite/config_cli.rs b/tests/testsuite/config_cli.rs index b126ea9a02e..c2978e892f0 100644 --- a/tests/testsuite/config_cli.rs +++ b/tests/testsuite/config_cli.rs @@ -3,7 +3,6 @@ use std::{collections::HashMap, fs}; use crate::prelude::*; -use cargo::util::context::Definition; use cargo_test_support::compare::assert_e2e; use cargo_test_support::paths; use cargo_test_support::str; @@ -264,33 +263,6 @@ fn merges_table() { assert_eq!(gctx.get::("foo.key5").unwrap(), 9); } -#[cargo_test] -fn merge_array_mixed_def_paths() { - // Merging of arrays with different def sites. - write_config_toml( - " - paths = ['file'] - ", - ); - // Create a directory for CWD to differentiate the paths. - let somedir = paths::root().join("somedir"); - fs::create_dir(&somedir).unwrap(); - let gctx = GlobalContextBuilder::new() - .cwd(&somedir) - .config_arg("paths=['cli']") - // env is currently ignored for get_list() - .env("CARGO_PATHS", "env") - .build(); - let paths = gctx.paths_overrides().unwrap().unwrap(); - // The definition for the root value is somewhat arbitrary, but currently starts with the file because that is what is loaded first. - assert_eq!(paths.definition, Definition::Path(paths::root())); - assert_eq!(paths.val.len(), 2); - assert_eq!(paths.val[0].0, "file"); - assert_eq!(paths.val[0].1.root(&gctx), paths::root()); - assert_eq!(paths.val[1].0, "cli"); - assert_eq!(paths.val[1].1.root(&gctx), somedir); -} - #[cargo_test] fn enforces_format() { // These dotted key expressions should all be fine. diff --git a/tests/testsuite/paths.rs b/tests/testsuite/paths.rs index 2fe242fc664..e1107a21e1f 100644 --- a/tests/testsuite/paths.rs +++ b/tests/testsuite/paths.rs @@ -250,3 +250,56 @@ https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html "#]]) .run(); } + +#[cargo_test] +fn env_paths_overrides_not_supported() { + Package::new("file", "0.1.0").publish(); + Package::new("cli", "0.1.0").publish(); + Package::new("env", "0.1.0").publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + edition = "2015" + + [dependencies] + file = "0.1.0" + cli = "0.1.0" + env = "0.1.0" + "#, + ) + .file("src/lib.rs", "") + .file("file/Cargo.toml", &basic_manifest("file", "0.2.0")) + .file("file/src/lib.rs", "") + .file("cli/Cargo.toml", &basic_manifest("cli", "0.2.0")) + .file("cli/src/lib.rs", "") + .file("env/Cargo.toml", &basic_manifest("env", "0.2.0")) + .file("env/src/lib.rs", "") + .file(".cargo/config.toml", r#"paths = ["file"]"#) + .build(); + + p.cargo("check") + .arg("--config") + .arg("paths=['cli']") + // paths overrides ignore env + .env("CARGO_PATHS", "env") + .with_stderr_data( + str![[r#" +[UPDATING] `dummy-registry` index +[LOCKING] 3 packages to latest compatible versions +[DOWNLOADING] crates ... +[DOWNLOADED] env v0.1.0 (registry `dummy-registry`) +[CHECKING] file v0.2.0 ([ROOT]/foo/file) +[CHECKING] cli v0.2.0 ([ROOT]/foo/cli) +[CHECKING] env v0.1.0 +[CHECKING] foo v0.0.0 ([ROOT]/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]] + .unordered(), + ) + .run(); +}