From 1dacd119689273184855d96ed9e33bc8e10c5ad0 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 7 Oct 2025 15:42:05 -0500 Subject: [PATCH 1/8] refactor(gctx): Prefer let-else --- src/cargo/util/context/mod.rs | 87 +++++++++++++++++------------------ 1 file changed, 43 insertions(+), 44 deletions(-) diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index 694c27260b9..83cbcbe9d20 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -657,38 +657,42 @@ impl GlobalContext { /// /// Callers should prefer [`Workspace::build_dir`] instead. pub fn build_dir(&self, workspace_manifest_path: &PathBuf) -> CargoResult> { - if let Some(val) = &self.build_config()?.build_dir { - let replacements = vec![ - ( - "{workspace-root}", - workspace_manifest_path - .parent() - .unwrap() - .to_str() - .context("workspace root was not valid utf-8")? - .to_string(), - ), - ( - "{cargo-cache-home}", - self.home() - .as_path_unlocked() - .to_str() - .context("cargo home was not valid utf-8")? - .to_string(), - ), - ("{workspace-path-hash}", { - let real_path = std::fs::canonicalize(workspace_manifest_path)?; - let hash = crate::util::hex::short_hash(&real_path); - format!("{}{}{}", &hash[0..2], std::path::MAIN_SEPARATOR, &hash[2..]) - }), - ]; - - let template_variables = replacements - .iter() - .map(|(key, _)| key[1..key.len() - 1].to_string()) - .collect_vec(); + let Some(val) = &self.build_config()?.build_dir else { + // For now, fallback to the previous implementation. + // This will change in the future. + return self.target_dir(); + }; + let replacements = vec![ + ( + "{workspace-root}", + workspace_manifest_path + .parent() + .unwrap() + .to_str() + .context("workspace root was not valid utf-8")? + .to_string(), + ), + ( + "{cargo-cache-home}", + self.home() + .as_path_unlocked() + .to_str() + .context("cargo home was not valid utf-8")? + .to_string(), + ), + ("{workspace-path-hash}", { + let real_path = std::fs::canonicalize(workspace_manifest_path)?; + let hash = crate::util::hex::short_hash(&real_path); + format!("{}{}{}", &hash[0..2], std::path::MAIN_SEPARATOR, &hash[2..]) + }), + ]; - let path = val + let template_variables = replacements + .iter() + .map(|(key, _)| key[1..key.len() - 1].to_string()) + .collect_vec(); + + let path = val .resolve_templated_path(self, replacements) .map_err(|e| match e { path::ResolveTemplateError::UnexpectedVariable { @@ -716,20 +720,15 @@ impl GlobalContext { } })?; - // Check if the target directory is set to an empty string in the config.toml file. - if val.raw_value().is_empty() { - bail!( - "the build directory is set to an empty string in {}", - val.value().definition - ) - } - - Ok(Some(Filesystem::new(path))) - } else { - // For now, fallback to the previous implementation. - // This will change in the future. - return self.target_dir(); + // Check if the target directory is set to an empty string in the config.toml file. + if val.raw_value().is_empty() { + bail!( + "the build directory is set to an empty string in {}", + val.value().definition + ) } + + Ok(Some(Filesystem::new(path))) } /// Get a configuration value by key. From 63aed0cf337e4b4e7ae76eed246f7b2d2b57fc72 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 7 Oct 2025 15:45:52 -0500 Subject: [PATCH 2/8] refactor(gctx): Don't duplicate defaulting with Workspace --- src/cargo/util/context/mod.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index 83cbcbe9d20..dd050d0b53a 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -653,14 +653,10 @@ impl GlobalContext { /// The directory to use for intermediate build artifacts. /// - /// Falls back to the target directory if not specified. - /// /// Callers should prefer [`Workspace::build_dir`] instead. pub fn build_dir(&self, workspace_manifest_path: &PathBuf) -> CargoResult> { let Some(val) = &self.build_config()?.build_dir else { - // For now, fallback to the previous implementation. - // This will change in the future. - return self.target_dir(); + return Ok(None); }; let replacements = vec![ ( From 30c01dac55c3d6626222c2fc630ebc450dd0af2e Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 7 Oct 2025 15:47:46 -0500 Subject: [PATCH 3/8] refactor(gctx): Pull out build-dir templating logic --- src/cargo/util/context/mod.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index dd050d0b53a..100318cf7bc 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -658,6 +658,18 @@ impl GlobalContext { let Some(val) = &self.build_config()?.build_dir else { return Ok(None); }; + self.custom_build_dir(val, workspace_manifest_path) + .map(Some) + } + + /// The directory to use for intermediate build artifacts. + /// + /// Callers should prefer [`Workspace::build_dir`] instead. + pub fn custom_build_dir( + &self, + val: &ConfigRelativePath, + workspace_manifest_path: &PathBuf, + ) -> CargoResult { let replacements = vec![ ( "{workspace-root}", @@ -724,7 +736,7 @@ impl GlobalContext { ) } - Ok(Some(Filesystem::new(path))) + Ok(Filesystem::new(path)) } /// Get a configuration value by key. From ce58ead133ede5464269c5e2507eef4d56ac0451 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 7 Oct 2025 16:10:40 -0500 Subject: [PATCH 4/8] refactor(gctx): Prefer Path to PathBuf --- src/cargo/util/context/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index 100318cf7bc..7388e807321 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -654,7 +654,7 @@ impl GlobalContext { /// The directory to use for intermediate build artifacts. /// /// Callers should prefer [`Workspace::build_dir`] instead. - pub fn build_dir(&self, workspace_manifest_path: &PathBuf) -> CargoResult> { + pub fn build_dir(&self, workspace_manifest_path: &Path) -> CargoResult> { let Some(val) = &self.build_config()?.build_dir else { return Ok(None); }; @@ -668,7 +668,7 @@ impl GlobalContext { pub fn custom_build_dir( &self, val: &ConfigRelativePath, - workspace_manifest_path: &PathBuf, + workspace_manifest_path: &Path, ) -> CargoResult { let replacements = vec![ ( From e9dced1d4ce9305acfe7a85624d0f6815c99424c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 7 Oct 2025 15:57:27 -0500 Subject: [PATCH 5/8] refactor(ws): Group target-dir/build-dir init --- src/cargo/core/workspace.rs | 2 +- tests/testsuite/bad_config.rs | 5 ++++- tests/testsuite/config.rs | 14 +++++++++++++- 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 802f2566cc0..6b89494ad78 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -215,7 +215,6 @@ impl<'gctx> Workspace<'gctx> { /// before returning it, so `Ok` is only returned for valid workspaces. pub fn new(manifest_path: &Path, gctx: &'gctx GlobalContext) -> CargoResult> { let mut ws = Workspace::new_default(manifest_path.to_path_buf(), gctx); - ws.target_dir = gctx.target_dir()?; if manifest_path.is_relative() { bail!( @@ -226,6 +225,7 @@ impl<'gctx> Workspace<'gctx> { ws.root_manifest = ws.find_root(manifest_path)?; } + ws.target_dir = gctx.target_dir()?; ws.build_dir = gctx.build_dir( ws.root_manifest .as_ref() diff --git a/tests/testsuite/bad_config.rs b/tests/testsuite/bad_config.rs index d8eec977ad0..f453c2f5359 100644 --- a/tests/testsuite/bad_config.rs +++ b/tests/testsuite/bad_config.rs @@ -159,7 +159,10 @@ fn invalid_global_config() { p.cargo("check -v") .with_status(101) .with_stderr_data(str![[r#" -[ERROR] could not load Cargo configuration +[ERROR] failed to parse manifest at `[ROOT]/foo/Cargo.toml` + +Caused by: + could not load Cargo configuration Caused by: could not parse TOML configuration in `[ROOT]/foo/.cargo/config.toml` diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index b54953cffc3..308dce26882 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -1649,7 +1649,19 @@ target-dir = '' #[cargo_test] fn cargo_target_empty_env() { - let project = project().build(); + let project = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + authors = [] + version = "0.0.0" + build = "build.rs" + "#, + ) + .file("src/lib.rs", "") + .build(); project.cargo("check") .env("CARGO_TARGET_DIR", "") From 5ee83a248a29e26ac87463350f75a0816b925d95 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 7 Oct 2025 16:11:57 -0500 Subject: [PATCH 6/8] refactor(ws): Consistently grab root manifest like default_target_dir --- src/cargo/core/workspace.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 6b89494ad78..ca8ac5631b8 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -226,11 +226,7 @@ impl<'gctx> Workspace<'gctx> { } ws.target_dir = gctx.target_dir()?; - ws.build_dir = gctx.build_dir( - ws.root_manifest - .as_ref() - .unwrap_or(&manifest_path.to_path_buf()), - )?; + ws.build_dir = gctx.build_dir(ws.root_manifest())?; ws.custom_metadata = ws .load_workspace_config()? From a12c482a6731cc6245d28d52cfe8028f1276e186 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 7 Oct 2025 16:40:23 -0500 Subject: [PATCH 7/8] refactor(ws): Reuse build-dir logic for script target-dir --- src/cargo/core/workspace.rs | 15 ++++++++------- src/cargo/util/context/de.rs | 7 ++++--- src/cargo/util/context/mod.rs | 4 +++- src/cargo/util/context/value.rs | 23 +++++++++++++++++++---- 4 files changed, 34 insertions(+), 15 deletions(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index ca8ac5631b8..79314ff784a 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -444,13 +444,14 @@ impl<'gctx> Workspace<'gctx> { fn default_target_dir(&self) -> Filesystem { if self.root_maybe().is_embedded() { - let hash = crate::util::hex::short_hash(&self.root_manifest().to_string_lossy()); - let mut rel_path = PathBuf::new(); - rel_path.push("target"); - rel_path.push(&hash[0..2]); - rel_path.push(&hash[2..]); - - self.gctx().home().join(rel_path) + let default = ConfigRelativePath::new( + "{cargo-cache-home}/target/{workspace-path-hash}" + .to_owned() + .into(), + ); + self.gctx() + .custom_build_dir(&default, self.root_manifest()) + .expect("template is correct") } else { Filesystem::new(self.root().join("target")) } diff --git a/src/cargo/util/context/de.rs b/src/cargo/util/context/de.rs index 2413e9f1289..11a2d463407 100644 --- a/src/cargo/util/context/de.rs +++ b/src/cargo/util/context/de.rs @@ -564,18 +564,19 @@ impl<'de, 'gctx> de::MapAccess<'de> for ValueDeserializer<'gctx> { // ... otherwise we're deserializing the `definition` field, so we need // to figure out where the field we just deserialized was defined at. match self.definition() { + Definition::BuiltIn => seed.deserialize(0.into_deserializer()), Definition::Path(path) => { - seed.deserialize(Tuple2Deserializer(0i32, path.to_string_lossy())) + seed.deserialize(Tuple2Deserializer(1i32, path.to_string_lossy())) } Definition::Environment(env) => { - seed.deserialize(Tuple2Deserializer(1i32, env.as_str())) + seed.deserialize(Tuple2Deserializer(2i32, env.as_str())) } Definition::Cli(path) => { let s = path .as_ref() .map(|p| p.to_string_lossy()) .unwrap_or_default(); - seed.deserialize(Tuple2Deserializer(2i32, s)) + seed.deserialize(Tuple2Deserializer(3i32, s)) } } } diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index 7388e807321..9cf869bc607 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -1371,7 +1371,9 @@ impl GlobalContext { let abs = |path: &str, def: &Definition| -> (String, PathBuf, Definition) { let abs_path = match def { Definition::Path(p) | Definition::Cli(Some(p)) => p.parent().unwrap().join(&path), - Definition::Environment(_) | Definition::Cli(None) => self.cwd().join(&path), + Definition::Environment(_) | Definition::Cli(None) | Definition::BuiltIn => { + self.cwd().join(&path) + } }; (path.to_string(), abs_path, def.clone()) }; diff --git a/src/cargo/util/context/value.rs b/src/cargo/util/context/value.rs index dd56276e2d0..0f9f8fe7f36 100644 --- a/src/cargo/util/context/value.rs +++ b/src/cargo/util/context/value.rs @@ -55,6 +55,7 @@ pub(crate) static FIELDS: [&str; 2] = [VALUE_FIELD, DEFINITION_FIELD]; /// Location where a config value is defined. #[derive(Clone, Debug, Eq)] pub enum Definition { + BuiltIn, /// Defined in a `.cargo/config`, includes the path to the file. Path(PathBuf), /// Defined in an environment variable, includes the environment key. @@ -90,7 +91,7 @@ impl Definition { pub fn root<'a>(&'a self, gctx: &'a GlobalContext) -> &'a Path { match self { Definition::Path(p) | Definition::Cli(Some(p)) => p.parent().unwrap().parent().unwrap(), - Definition::Environment(_) | Definition::Cli(None) => gctx.cwd(), + Definition::Environment(_) | Definition::Cli(None) | Definition::BuiltIn => gctx.cwd(), } } @@ -102,7 +103,10 @@ impl Definition { (self, other), (Definition::Cli(_), Definition::Environment(_)) | (Definition::Cli(_), Definition::Path(_)) + | (Definition::Cli(_), Definition::BuiltIn) | (Definition::Environment(_), Definition::Path(_)) + | (Definition::Environment(_), Definition::BuiltIn) + | (Definition::Path(_), Definition::BuiltIn) ) } } @@ -123,6 +127,16 @@ impl fmt::Display for Definition { Definition::Path(p) | Definition::Cli(Some(p)) => p.display().fmt(f), Definition::Environment(key) => write!(f, "environment variable `{}`", key), Definition::Cli(None) => write!(f, "--config cli option"), + Definition::BuiltIn => write!(f, "default"), + } + } +} + +impl From for Value { + fn from(val: T) -> Self { + Self { + val, + definition: Definition::BuiltIn, } } } @@ -236,9 +250,10 @@ impl<'de> de::Deserialize<'de> for Definition { { let (discr, value) = <(u32, String)>::deserialize(deserializer)?; match discr { - 0 => Ok(Definition::Path(value.into())), - 1 => Ok(Definition::Environment(value)), - 2 => { + 0 => Ok(Definition::BuiltIn), + 1 => Ok(Definition::Path(value.into())), + 2 => Ok(Definition::Environment(value)), + 3 => { let path = (value.len() > 0).then_some(value.into()); Ok(Definition::Cli(path)) } From 5b5455c3d1be62077f7577929f46a6722a17d658 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 10 Oct 2025 11:19:36 -0500 Subject: [PATCH 8/8] refactor(gctx): Drop an allocation when doing templating --- src/cargo/util/context/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index 9cf869bc607..7a4831535ed 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -670,7 +670,7 @@ impl GlobalContext { val: &ConfigRelativePath, workspace_manifest_path: &Path, ) -> CargoResult { - let replacements = vec![ + let replacements = [ ( "{workspace-root}", workspace_manifest_path