Skip to content

feat(build-dir): Reorganize build-dir layout #15947

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+1,613 −227
Open

feat(build-dir): Reorganize build-dir layout #15947

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9121077
chore: Added new HASH redaction for hashes in paths
ranger-ross Oct 4, 2025
0c143b1
test(build-dir): Added cargo clean -p test
ranger-ross Oct 5, 2025
0b81b44
chore(build-dir): Forked build_dir.rs tests
ranger-ross Sep 30, 2025
8603e30
refactor(layout): Created dedicated layout helper fns for build-dir i…
ranger-ross Oct 7, 2025
c849440
feat(build-dir): Reorganize build-dir layout
ranger-ross Oct 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions crates/cargo-test-support/src/compare.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,9 @@ fn add_regex_redactions(subs: &mut snapbox::Redactions) {
// Match file name hashes like `foo-06b451d0d6f88b1d`
subs.insert("[HASH]", regex!(r"[a-z0-9]+-(?<redacted>[a-f0-9]{16})"))
.unwrap();
// Match path hashes like `../06b451d0d6f88b1d/..` used in directory paths
subs.insert("[HASH]", regex!(r"\/(?<redacted>[0-9a-f]{16})\/"))
.unwrap();
subs.insert(
"[AVG_ELAPSED]",
regex!(r"(?<redacted>[0-9]+(\.[0-9]+)?) ns/iter"),
Expand Down
19 changes: 13 additions & 6 deletions src/cargo/core/compiler/build_runner/compilation_files.rs
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need a test for cargo clean -p foo. Haven't looked at how thats implemented but might at least be a reason for name/hash rather than name-hash

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test for this in 0c143b1.
And good thing to because I silently broke that when I changed from name-hash to name/hash

Original file line number Diff line number Diff line change
Expand Up @@ -255,20 +255,27 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> {
}

/// Returns the host `deps` directory path.
pub fn host_deps(&self) -> &Path {
self.host.deps()
pub fn host_deps(&self, unit: &Unit) -> PathBuf {
let dir = self.pkg_dir(unit);
self.host.deps(&dir)
}

/// Returns the directories where Rust crate dependencies are found for the
/// specified unit.
pub fn deps_dir(&self, unit: &Unit) -> &Path {
self.layout(unit.kind).deps()
pub fn deps_dir(&self, unit: &Unit) -> PathBuf {
let dir = self.pkg_dir(unit);
self.layout(unit.kind).deps(&dir)
}

/// Directory where the fingerprint for the given unit should go.
pub fn fingerprint_dir(&self, unit: &Unit) -> PathBuf {
let dir = self.pkg_dir(unit);
self.layout(unit.kind).fingerprint().join(dir)
self.layout(unit.kind).fingerprint(&dir)
}

/// Directory where incremental output for the given unit should go.
pub fn incremental_dir(&self, unit: &Unit) -> &Path {
self.layout(unit.kind).incremental()
}

/// Returns the path for a file in the fingerprint directory.
Expand Down Expand Up @@ -303,7 +310,7 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> {
assert!(!unit.mode.is_run_custom_build());
assert!(self.metas.contains_key(unit));
let dir = self.pkg_dir(unit);
self.layout(CompileKind::Host).build().join(dir)
self.layout(CompileKind::Host).build_script(&dir)
}

/// Returns the directory for compiled artifacts files.
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/build_runner/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
.insert(kind, layout.dest().to_path_buf());
self.compilation
.deps_output
.insert(kind, layout.deps().to_path_buf());
.insert(kind, layout.legacy_deps().to_path_buf());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this legacy_deps?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a follow up change, this is in an else for if build_dir_new_lay, so it seems like deps() would also work

}
Ok(())
}
Expand Down
22 changes: 19 additions & 3 deletions src/cargo/core/compiler/layout.rs
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

target/x86_64-unknown-linux-gnu/debug/build/proc-macro2-ee66340aaf816e44

So while this reduces the "max content per directory" (since proc-macro2-ee66340aaf816e44 will be a dir, rather than multiple files), we also have more flexibility for handling this.

Should we change from proc-macro2-ee66340aaf816e44 to proc/-macro2/ee66340aaf816e44?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change from proc-macro2-ee66340aaf816e44 to proc/-macro2/ee66340aaf816e44?

Could we expand a bit more on the benefit of the proposed change?

Copy link
Contributor

@epage epage Sep 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There can be performance issues on Windows when a directory has a lot of content. We do this prefix-directory stuff for the index and for the build-dir workspace hash. This would be extending it to the build units within the package dir.

As Ross brought up, we don't have guidance on how big is big, what the growth will look like, etc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we share this layout with the shared cache, then it will likely be more important.

Copy link
Member Author

@ranger-ross ranger-ross Oct 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent some time getting up to speed on Windows path issues. (goodness, its a deep rabbit hole)

As far as I can tell the main issue is Windows has much more restrictive path length than linux. (see #9770)
As for "too many files in a directory", I could not find much information on this.
Only a stack overflow post from 15 yrs ago saying "its well known that windows has poor performance on directories with many files".

That said I also found an article that claims a flat structure is better on ext4 for linux.
Granted the number of files in that article is huge and we would probably never have a build cache that gets that big.

Given the long paths issue on windows, perhaps it would be better to shorten names to help mitigate this? build-script-execution while descriptive is pretty long.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, these are separate conversations, so I moved path lengths over to https://github.com/rust-lang/cargo/pull/15947/files#r2402207606

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

As for the original suggestion

Should we change from proc-macro2-ee66340aaf816e44 to proc/-macro2/ee66340aaf816e44?

I lean towards not splitting these up. It keeps things simple and its questionable if there is even a performance issue in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is unspecified, we at least ahve the freedom to change it over time.

I think we should at minimum do <pkg-name>/<hash> as that would greatly simplify cargo clean -p <pkg-name>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is a good idea 👍

The code for cargo clean -p is a bit hard to follow and I think with the new layout we have the opportunity to make the code simpler.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made this change in the latest push.

Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,11 @@ impl Layout {
&self.dest
}
/// Fetch the deps path.
pub fn deps(&self) -> &Path {
pub fn deps(&self, _pkg_dir: &str) -> PathBuf {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I was interested in things like incremental being created, taking in additional parameters that are unused I feel make this change harder to understand and likely would make the follow up harder to understand

self.deps.clone()
}
/// Fetch the deps path. (old layout)
pub fn legacy_deps(&self) -> &Path {
&self.deps
}
/// Fetch the examples path.
Expand All @@ -256,13 +260,25 @@ impl Layout {
&self.incremental
}
/// Fetch the fingerprint path.
pub fn fingerprint(&self) -> &Path {
pub fn fingerprint(&self, pkg_dir: &str) -> PathBuf {
self.fingerprint.join(pkg_dir)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct for us to be joining pkg_dir to self.fingerprint?

}
/// Fetch the fingerprint path. (old layout)
pub fn legacy_fingerprint(&self) -> &Path {
&self.fingerprint
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep the legacy splits, I feel like this commit should have the existing functions delegate to the legacy ones. Then the follow up commit will call the legacy functions in the else branch. This would make the relationship more explicit and avoid questions like 8603e30#r2411571494

/// Fetch the build script path.
/// Fetch the build path.
pub fn build(&self) -> &Path {
&self.build
}
/// Fetch the build script path.
pub fn build_script(&self, pkg_dir: &str) -> PathBuf {
self.build().join(pkg_dir)
}
/// Fetch the build script execution path.
pub fn build_script_execution(&self, pkg_dir: &str) -> PathBuf {
self.build().join(pkg_dir)
}
/// Fetch the artifact path.
pub fn artifact(&self) -> &Path {
&self.artifact
Expand Down
10 changes: 3 additions & 7 deletions src/cargo/core/compiler/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1353,12 +1353,8 @@ fn build_base_args(
.map(|s| s.as_ref()),
);
if incremental {
let dir = build_runner
.files()
.layout(unit.kind)
.incremental()
.as_os_str();
opt(cmd, "-C", "incremental=", Some(dir));
let dir = build_runner.files().incremental_dir(&unit);
opt(cmd, "-C", "incremental=", Some(dir.as_os_str()));
}

let pkg_hint_mostly_unused = match hints.mostly_unused {
Expand Down Expand Up @@ -1679,7 +1675,7 @@ fn build_deps_args(
if !unit.kind.is_host() {
cmd.arg("-L").arg(&{
let mut deps = OsString::from("dependency=");
deps.push(build_runner.files().host_deps());
deps.push(build_runner.files().host_deps(&unit));
deps
});
}
Expand Down
6 changes: 3 additions & 3 deletions src/cargo/ops/cargo_clean.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ fn clean_specs(

// Clean fingerprints.
for (_, layout) in &layouts_with_host {
let dir = escape_glob_path(layout.fingerprint())?;
let dir = escape_glob_path(layout.legacy_fingerprint())?;
clean_ctx
.rm_rf_package_glob_containing_hash(&pkg.name(), &Path::new(&dir).join(&pkg_dir))?;
}
Expand Down Expand Up @@ -236,8 +236,8 @@ fn clean_specs(
(layout.build_examples(), Some(layout.examples()))
}
// Tests/benchmarks are never uplifted.
TargetKind::Test | TargetKind::Bench => (layout.deps(), None),
_ => (layout.deps(), Some(layout.dest())),
TargetKind::Test | TargetKind::Bench => (layout.legacy_deps(), None),
_ => (layout.legacy_deps(), Some(layout.dest())),
};
let mut dir_glob_str = escape_glob_path(dir)?;
let dir_glob = Path::new(&dir_glob_str);
Expand Down
72 changes: 71 additions & 1 deletion tests/testsuite/build_dir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
use std::path::PathBuf;

use crate::prelude::*;
use cargo_test_support::registry::RegistryBuilder;
use cargo_test_support::registry::{Package, RegistryBuilder};
use cargo_test_support::{paths, prelude::*, project, str};
use std::env::consts::{DLL_PREFIX, DLL_SUFFIX, EXE_SUFFIX};

Expand Down Expand Up @@ -580,6 +580,76 @@ fn cargo_clean_should_clean_the_target_dir_and_build_dir() {
assert_not_exists(&p.root().join("target-dir"));
}

#[cargo_test]
fn cargo_clean_should_remove_correct_files() {
Package::new("bar", "0.1.0").publish();

let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
edition = "2015"
authors = []
exclude = ["*.txt"]
license = "MIT"
description = "foo"

[dependencies]
bar = "0.1"
"#,
)
.file("src/main.rs", r#"fn main() { println!("Hello, World!"); }"#)
.file(
".cargo/config.toml",
r#"
[build]
target-dir = "target-dir"
build-dir = "build-dir"
"#,
)
.build();

p.cargo("build").enable_mac_dsym().run();

p.root().join("build-dir").assert_build_dir_layout(str![[r#"
[ROOT]/foo/build-dir/.rustc_info.json
[ROOT]/foo/build-dir/CACHEDIR.TAG
[ROOT]/foo/build-dir/debug/.cargo-lock
[ROOT]/foo/build-dir/debug/.fingerprint/foo-[HASH]/bin-foo
[ROOT]/foo/build-dir/debug/.fingerprint/foo-[HASH]/bin-foo.json
[ROOT]/foo/build-dir/debug/.fingerprint/foo-[HASH]/dep-bin-foo
[ROOT]/foo/build-dir/debug/.fingerprint/foo-[HASH]/invoked.timestamp
[ROOT]/foo/build-dir/debug/deps/foo[..][EXE]
[ROOT]/foo/build-dir/debug/deps/foo[..].d
[ROOT]/foo/build-dir/debug/.fingerprint/bar-[HASH]/lib-bar
[ROOT]/foo/build-dir/debug/.fingerprint/bar-[HASH]/lib-bar.json
[ROOT]/foo/build-dir/debug/.fingerprint/bar-[HASH]/dep-lib-bar
[ROOT]/foo/build-dir/debug/.fingerprint/bar-[HASH]/invoked.timestamp
[ROOT]/foo/build-dir/debug/deps/bar[..].d
[ROOT]/foo/build-dir/debug/deps/libbar[..].rlib
[ROOT]/foo/build-dir/debug/deps/libbar[..].rmeta

"#]]);

p.cargo("clean -p bar").enable_mac_dsym().run();

p.root().join("build-dir").assert_build_dir_layout(str![[r#"
[ROOT]/foo/build-dir/.rustc_info.json
[ROOT]/foo/build-dir/CACHEDIR.TAG
[ROOT]/foo/build-dir/debug/.cargo-lock
[ROOT]/foo/build-dir/debug/.fingerprint/foo-[HASH]/bin-foo
[ROOT]/foo/build-dir/debug/.fingerprint/foo-[HASH]/bin-foo.json
[ROOT]/foo/build-dir/debug/.fingerprint/foo-[HASH]/dep-bin-foo
[ROOT]/foo/build-dir/debug/.fingerprint/foo-[HASH]/invoked.timestamp
[ROOT]/foo/build-dir/debug/deps/foo[..][EXE]
[ROOT]/foo/build-dir/debug/deps/foo[..].d

"#]]);
}

#[cargo_test]
fn timings_report_should_output_to_target_dir() {
let p = project()
Expand Down
Loading