Skip to content

Commit 68c26f6

Browse files
authored
Added build.build_dir templating support (#15236)
### What does this PR try to resolve? This PR is a follow up on #15104 and and adds support for the path templating in `build.build-dir` as defined in #14125. Supported templates: * `{workspace-root}` * `{cargo-cache}` (pointing to `CARGO_HOME` for now) * `{workspace-manifest-path-hash}` #### Unresolved questions What should we name `{workspace-manifest-path-hash}` and what should it include? Should we shorten to `{workspace-hash}` or even just `{hash}`? Should we include the Cargo version so we get unique whole-target directories for easier cleanup (#13136) How should this handle unknown variables (error) or unclosed `{` / `}` (ignored), see #15236 (comment) When using `{workspace-manifest-path-hash}` this hash will change based on the project path. In the event of a cargo being executed in a symlinked project, the hash will change. For example, given the following directory ``` /Users/ └─ user1/ └─ projects/ ├─ actual-crate/ │ └─ Cargo.toml └─ symlink-to-crate/ -> actual-crate/ ``` the hash will be unique when running cargo from each of the following directories. * `/Users/user1/actual-crate` * `/Users/user1/symlink-to-crate` Figuring out whether to handle this is deferred out, see - #15236 (comment) - https://github.com/poliorcetics/rfcs/blob/cargo-target-directories/text/3371-cargo-target-dir-templates.md#symbolic-links - #12207 (comment) ### How should we test and review this PR? This PR is fairly small. I included tests for each template variable. You can also clone my branch and test it locally with ```console CARGO_BUILD_BUILD_DIR='{workspace-root}/foo' cargo -Z build-dir build ``` ### Additional information While searching Cargo for any prior art for path templating, I found [`sources/registry/download.rs`](https://github.com/rust-lang/cargo/blob/master/src/cargo/sources/registry/download.rs#L84) doing a simple string replace. Thus I followed the same behavior. r? @epage
2 parents 40a4d1e + 229489e commit 68c26f6

File tree

5 files changed

+250
-4
lines changed

5 files changed

+250
-4
lines changed

src/cargo/core/workspace.rs

+6-1
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,6 @@ impl<'gctx> Workspace<'gctx> {
213213
pub fn new(manifest_path: &Path, gctx: &'gctx GlobalContext) -> CargoResult<Workspace<'gctx>> {
214214
let mut ws = Workspace::new_default(manifest_path.to_path_buf(), gctx);
215215
ws.target_dir = gctx.target_dir()?;
216-
ws.build_dir = gctx.build_dir()?;
217216

218217
if manifest_path.is_relative() {
219218
bail!(
@@ -224,6 +223,12 @@ impl<'gctx> Workspace<'gctx> {
224223
ws.root_manifest = ws.find_root(manifest_path)?;
225224
}
226225

226+
ws.build_dir = gctx.build_dir(
227+
ws.root_manifest
228+
.as_ref()
229+
.unwrap_or(&manifest_path.to_path_buf()),
230+
)?;
231+
227232
ws.custom_metadata = ws
228233
.load_workspace_config()?
229234
.and_then(|cfg| cfg.custom_metadata);

src/cargo/util/context/mod.rs

+35-2
Original file line numberDiff line numberDiff line change
@@ -652,12 +652,45 @@ impl GlobalContext {
652652
/// Falls back to the target directory if not specified.
653653
///
654654
/// Callers should prefer [`Workspace::build_dir`] instead.
655-
pub fn build_dir(&self) -> CargoResult<Option<Filesystem>> {
655+
pub fn build_dir(&self, workspace_manifest_path: &PathBuf) -> CargoResult<Option<Filesystem>> {
656656
if !self.cli_unstable().build_dir {
657657
return self.target_dir();
658658
}
659659
if let Some(val) = &self.build_config()?.build_dir {
660-
let path = val.resolve_path(self);
660+
let replacements = vec![
661+
(
662+
"{workspace-root}",
663+
workspace_manifest_path
664+
.parent()
665+
.unwrap()
666+
.to_str()
667+
.context("workspace root was not valid utf-8")?
668+
.to_string(),
669+
),
670+
(
671+
"{cargo-cache-home}",
672+
self.home()
673+
.as_path_unlocked()
674+
.to_str()
675+
.context("cargo home was not valid utf-8")?
676+
.to_string(),
677+
),
678+
("{workspace-manifest-path-hash}", {
679+
let hash = crate::util::hex::short_hash(&workspace_manifest_path);
680+
format!("{}{}{}", &hash[0..2], std::path::MAIN_SEPARATOR, &hash[2..])
681+
}),
682+
];
683+
684+
let path = val
685+
.resolve_templated_path(self, replacements)
686+
.map_err(|e| match e {
687+
path::ResolveTemplateError::UnexpectedVariable {
688+
variable,
689+
raw_template,
690+
} => anyhow!(
691+
"unexpected variable `{variable}` in build.build-dir path `{raw_template}`"
692+
),
693+
})?;
661694

662695
// Check if the target directory is set to an empty string in the config.toml file.
663696
if val.raw_value().is_empty() {

src/cargo/util/context/path.rs

+37
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
use super::{GlobalContext, StringList, Value};
2+
use regex::Regex;
23
use serde::{de::Error, Deserialize};
34
use std::path::PathBuf;
45

@@ -32,6 +33,34 @@ impl ConfigRelativePath {
3233
self.0.definition.root(gctx).join(&self.0.val)
3334
}
3435

36+
/// Same as [`Self::resolve_path`] but will make string replacements
37+
/// before resolving the path.
38+
///
39+
/// `replacements` should be an an [`IntoIterator`] of tuples with the "from" and "to" for the
40+
/// string replacement
41+
pub fn resolve_templated_path(
42+
&self,
43+
gctx: &GlobalContext,
44+
replacements: impl IntoIterator<Item = (impl AsRef<str>, impl AsRef<str>)>,
45+
) -> Result<PathBuf, ResolveTemplateError> {
46+
let mut value = self.0.val.clone();
47+
48+
for (from, to) in replacements {
49+
value = value.replace(from.as_ref(), to.as_ref());
50+
}
51+
52+
// Check for expected variables
53+
let re = Regex::new(r"\{(.*)\}").unwrap();
54+
if let Some(caps) = re.captures(&value) {
55+
return Err(ResolveTemplateError::UnexpectedVariable {
56+
variable: caps[1].to_string(),
57+
raw_template: self.0.val.clone(),
58+
});
59+
};
60+
61+
Ok(self.0.definition.root(gctx).join(&value))
62+
}
63+
3564
/// Resolves this configuration-relative path to either an absolute path or
3665
/// something appropriate to execute from `PATH`.
3766
///
@@ -103,3 +132,11 @@ impl PathAndArgs {
103132
}
104133
}
105134
}
135+
136+
#[derive(Debug)]
137+
pub enum ResolveTemplateError {
138+
UnexpectedVariable {
139+
variable: String,
140+
raw_template: String,
141+
},
142+
}

src/doc/src/reference/unstable.md

+7
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,13 @@ build-dir = "out"
260260

261261
The path to where internal files used as part of the build are placed.
262262

263+
This option supports path templating.
264+
265+
Avaiable template variables:
266+
* `{workspace-root}` resolves to root of the current workspace.
267+
* `{cargo-cache-home}` resolves to `CARGO_HOME`
268+
* `{workspace-manifest-path-hash}` resolves to a hash of the manifest path
269+
263270

264271
## root-dir
265272
* Original Issue: [#9887](https://github.com/rust-lang/cargo/issues/9887)

tests/testsuite/build_dir.rs

+165-1
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
use std::path::PathBuf;
1313

1414
use cargo_test_support::prelude::*;
15-
use cargo_test_support::project;
15+
use cargo_test_support::{paths, project, str};
1616
use std::env::consts::{DLL_PREFIX, DLL_SUFFIX, EXE_SUFFIX};
1717

1818
#[cargo_test]
@@ -491,6 +491,170 @@ fn future_incompat_should_output_to_build_dir() {
491491
assert_exists(&p.root().join("build-dir/.future-incompat-report.json"));
492492
}
493493

494+
#[cargo_test]
495+
fn template_should_error_for_invalid_variables() {
496+
let p = project()
497+
.file("src/main.rs", r#"fn main() { println!("Hello, World!") }"#)
498+
.file(
499+
".cargo/config.toml",
500+
r#"
501+
[build]
502+
build-dir = "{fake}/build-dir"
503+
target-dir = "target-dir"
504+
"#,
505+
)
506+
.build();
507+
508+
p.cargo("build -Z build-dir")
509+
.masquerade_as_nightly_cargo(&["build-dir"])
510+
.enable_mac_dsym()
511+
.with_status(101)
512+
.with_stderr_data(str![[r#"
513+
[ERROR] unexpected variable `fake` in build.build-dir path `{fake}/build-dir`
514+
515+
"#]])
516+
.run();
517+
}
518+
519+
#[cargo_test]
520+
fn template_workspace_root() {
521+
let p = project()
522+
.file("src/main.rs", r#"fn main() { println!("Hello, World!") }"#)
523+
.file(
524+
".cargo/config.toml",
525+
r#"
526+
[build]
527+
build-dir = "{workspace-root}/build-dir"
528+
target-dir = "target-dir"
529+
"#,
530+
)
531+
.build();
532+
533+
p.cargo("build -Z build-dir")
534+
.masquerade_as_nightly_cargo(&["build-dir"])
535+
.enable_mac_dsym()
536+
.run();
537+
538+
assert_build_dir_layout(p.root().join("build-dir"), "debug");
539+
assert_artifact_dir_layout(p.root().join("target-dir"), "debug");
540+
541+
// Verify the binary was uplifted to the target-dir
542+
assert_exists(&p.root().join(&format!("target-dir/debug/foo{EXE_SUFFIX}")));
543+
}
544+
545+
#[cargo_test]
546+
fn template_cargo_cache_home() {
547+
let p = project()
548+
.file("src/main.rs", r#"fn main() { println!("Hello, World!") }"#)
549+
.file(
550+
".cargo/config.toml",
551+
r#"
552+
[build]
553+
build-dir = "{cargo-cache-home}/build-dir"
554+
target-dir = "target-dir"
555+
"#,
556+
)
557+
.build();
558+
559+
p.cargo("build -Z build-dir")
560+
.masquerade_as_nightly_cargo(&["build-dir"])
561+
.enable_mac_dsym()
562+
.run();
563+
564+
assert_build_dir_layout(paths::home().join(".cargo/build-dir"), "debug");
565+
assert_artifact_dir_layout(p.root().join("target-dir"), "debug");
566+
567+
// Verify the binary was uplifted to the target-dir
568+
assert_exists(&p.root().join(&format!("target-dir/debug/foo{EXE_SUFFIX}")));
569+
}
570+
571+
#[cargo_test]
572+
fn template_workspace_manfiest_path_hash() {
573+
let p = project()
574+
.file("src/main.rs", r#"fn main() { println!("Hello, World!") }"#)
575+
.file(
576+
"Cargo.toml",
577+
r#"
578+
[package]
579+
name = "foo"
580+
version = "1.0.0"
581+
authors = []
582+
edition = "2015"
583+
"#,
584+
)
585+
.file(
586+
".cargo/config.toml",
587+
r#"
588+
[build]
589+
build-dir = "foo/{workspace-manifest-path-hash}/build-dir"
590+
target-dir = "target-dir"
591+
"#,
592+
)
593+
.build();
594+
595+
p.cargo("build -Z build-dir")
596+
.masquerade_as_nightly_cargo(&["build-dir"])
597+
.enable_mac_dsym()
598+
.run();
599+
600+
let foo_dir = p.root().join("foo");
601+
assert_exists(&foo_dir);
602+
let hash_dir = parse_workspace_manifest_path_hash(&foo_dir);
603+
604+
let build_dir = hash_dir.as_path().join("build-dir");
605+
assert_build_dir_layout(build_dir, "debug");
606+
assert_artifact_dir_layout(p.root().join("target-dir"), "debug");
607+
608+
// Verify the binary was uplifted to the target-dir
609+
assert_exists(&p.root().join(&format!("target-dir/debug/foo{EXE_SUFFIX}")));
610+
}
611+
612+
fn parse_workspace_manifest_path_hash(hash_dir: &PathBuf) -> PathBuf {
613+
// Since the hash will change between test runs simply find the first directories and assume
614+
// that is the hash dir. The format is a 2 char directory followed by the remaining hash in the
615+
// inner directory (ie. `34/f9d02eb8411c05`)
616+
let mut dirs = std::fs::read_dir(hash_dir).unwrap().into_iter();
617+
let outer_hash_dir = dirs.next().unwrap().unwrap();
618+
// Validate there are no other directories in `hash_dir`
619+
assert!(
620+
dirs.next().is_none(),
621+
"Found multiple dir entries in {hash_dir:?}"
622+
);
623+
// Validate the outer hash dir hash is a directory and has the correct hash length
624+
assert!(
625+
outer_hash_dir.path().is_dir(),
626+
"{outer_hash_dir:?} was not a directory"
627+
);
628+
assert_eq!(
629+
outer_hash_dir.path().file_name().unwrap().len(),
630+
2,
631+
"Path {:?} should have been 2 chars",
632+
outer_hash_dir.path().file_name()
633+
);
634+
635+
let mut dirs = std::fs::read_dir(outer_hash_dir.path())
636+
.unwrap()
637+
.into_iter();
638+
let inner_hash_dir = dirs.next().unwrap().unwrap();
639+
// Validate there are no other directories in first hash dir
640+
assert!(
641+
dirs.next().is_none(),
642+
"Found multiple dir entries in {outer_hash_dir:?}"
643+
);
644+
// Validate the outer hash dir hash is a directory and has the correct hash length
645+
assert!(
646+
inner_hash_dir.path().is_dir(),
647+
"{inner_hash_dir:?} was not a directory"
648+
);
649+
assert_eq!(
650+
inner_hash_dir.path().file_name().unwrap().len(),
651+
14,
652+
"Path {:?} should have been 2 chars",
653+
inner_hash_dir.path().file_name()
654+
);
655+
return inner_hash_dir.path();
656+
}
657+
494658
#[track_caller]
495659
fn assert_build_dir_layout(path: PathBuf, profile: &str) {
496660
assert_dir_layout(path, profile, true);

0 commit comments

Comments
 (0)