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 4 commits into
base: master
Choose a base branch
from
+1,624 −237
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 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
030ddce
feat(build-dir): Reorganize build-dir layout
ranger-ross Oct 4, 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
45 changes: 30 additions & 15 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 @@ -230,17 +230,25 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> {
self.export_dir.clone()
}

/// Directory name to use for a package in the form `NAME-HASH`.
/// Directory name to use for a package in the form `{NAME}{SEPERATOR}{HASH}`.
///
/// The seperator will usually be `/` making nested hashed directories, but will sometimes be
/// `-` for backwards compatiblity (artifact directory)
///
/// Note that some units may share the same directory, so care should be
/// taken in those cases!
fn pkg_dir(&self, unit: &Unit) -> String {
fn pkg_dir(&self, unit: &Unit, seperator: &str) -> String {
let seperator = match self.ws.gctx().cli_unstable().build_dir_new_layout {
true => seperator,
false => "-",
};

let name = unit.pkg.package_id().name();
let meta = self.metas[unit];
if let Some(c_extra_filename) = meta.c_extra_filename() {
format!("{}-{}", name, c_extra_filename)
format!("{}{}{}", name, seperator, c_extra_filename)
} else {
format!("{}-{}", name, self.target_short_hash(unit))
format!("{}{}{}", name, seperator, self.target_short_hash(unit))
}
}

Expand All @@ -255,20 +263,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)
let dir = self.pkg_dir(unit, "/");
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 @@ -302,16 +317,16 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> {
assert!(unit.target.is_custom_build());
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)
let dir = self.pkg_dir(unit, "/");
self.layout(CompileKind::Host).build_script(&dir)
}

/// Returns the directory for compiled artifacts files.
/// `/path/to/target/{debug,release}/deps/artifact/KIND/PKG-HASH`
fn artifact_dir(&self, unit: &Unit) -> PathBuf {
assert!(self.metas.contains_key(unit));
assert!(unit.artifact.is_true());
let dir = self.pkg_dir(unit);
let dir = self.pkg_dir(unit, "-");
let kind = match unit.target.kind() {
TargetKind::Bin => "bin",
TargetKind::Lib(lib_kinds) => match lib_kinds.as_slice() {
Expand All @@ -336,8 +351,8 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> {
pub fn build_script_run_dir(&self, unit: &Unit) -> PathBuf {
assert!(unit.target.is_custom_build());
assert!(unit.mode.is_run_custom_build());
let dir = self.pkg_dir(unit);
self.layout(unit.kind).build().join(dir)
let dir = self.pkg_dir(unit, "/");
self.layout(unit.kind).build_script_execution(&dir)
}

/// Returns the "`OUT_DIR`" directory for running a build script.
Expand Down
28 changes: 22 additions & 6 deletions 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 @@ -6,10 +6,11 @@ use std::sync::{Arc, Mutex};

use crate::core::PackageId;
use crate::core::compiler::compilation::{self, UnitOutput};
use crate::core::compiler::{self, Unit, artifact};
use crate::core::compiler::{self, CompileTarget, Unit, artifact};
use crate::util::cache_lock::CacheLockMode;
use crate::util::errors::CargoResult;
use anyhow::{Context as _, bail};
use cargo_util::paths;
use filetime::FileTime;
use itertools::Itertools;
use jobserver::Client;
Expand Down Expand Up @@ -358,11 +359,18 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
#[tracing::instrument(skip_all)]
pub fn prepare_units(&mut self) -> CargoResult<()> {
let dest = self.bcx.profiles.get_dir_name();
let host_layout = Layout::new(self.bcx.ws, None, &dest)?;
let host = &self.compilation.host;
let host_target = CompileTarget::new(&host)?;
let host_layout = if self.bcx.gctx.cli_unstable().build_dir_new_layout {
Layout::new(self.bcx.ws, Some(host_target), &dest, true)?
} else {
Layout::new(self.bcx.ws, None, &dest, true)?
};
let mut targets = HashMap::new();
for kind in self.bcx.all_kinds.iter() {
if let CompileKind::Target(target) = *kind {
let layout = Layout::new(self.bcx.ws, Some(target), &dest)?;
let is_host = target == host_target;
let layout = Layout::new(self.bcx.ws, Some(target), &dest, is_host)?;
targets.insert(target, layout);
}
}
Expand Down Expand Up @@ -401,9 +409,17 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
self.compilation
.root_output
.insert(kind, layout.dest().to_path_buf());
self.compilation
.deps_output
.insert(kind, layout.deps().to_path_buf());
if self.bcx.gctx.cli_unstable().build_dir_new_layout {
for (unit, _) in self.bcx.unit_graph.iter() {
let dep_dir = self.files().deps_dir(unit);
paths::create_dir_all(&dep_dir)?;
self.compilation.deps_output.insert(kind, dep_dir);
}
} else {
self.compilation
.deps_output
.insert(kind, layout.legacy_deps().to_path_buf());
}
}
Ok(())
}
Expand Down
67 changes: 59 additions & 8 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 @@ -142,6 +142,7 @@ pub struct Layout {
/// Will be `None` when the build-dir and target-dir are the same path as we cannot
/// lock the same path twice.
_build_lock: Option<FileLock>,
is_new_layout: bool,
}

impl Layout {
Expand All @@ -155,12 +156,23 @@ impl Layout {
ws: &Workspace<'_>,
target: Option<CompileTarget>,
dest: &str,
is_host_layout: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

bools are always a precarious thing in parameter lists because their meaning is unclear. When the functions are local, its not too bad.

Making an enum would likely be too much boilerplate.

Options

  • Maybe we hold off on the adjusting the host layout until later, further reducing the scope of this change. Like I said, I wonder if we can get rid of some of our directory nesting which, depending on what we do, could make this short lived anyways.
  • Always have the caller create a variable for the bool and pass that in

) -> CargoResult<Layout> {
let is_new_layout = ws.gctx().cli_unstable().build_dir_new_layout;
let mut root = ws.target_dir();
let mut build_root = ws.build_dir();
if let Some(target) = target {
root.push(target.short_name());
build_root.push(target.short_name());
if is_new_layout {
assert!(target.is_some());
let short_name = target.as_ref().unwrap().short_name();
if !is_host_layout {
root.push(short_name);
}
build_root.push(short_name);
} else {
if let Some(target) = target {
root.push(target.short_name());
build_root.push(target.short_name());
}
}
let build_dest = build_root.join(dest);
let dest = root.join(dest);
Expand Down Expand Up @@ -212,14 +224,17 @@ impl Layout {
dest,
_lock: lock,
_build_lock: build_lock,
is_new_layout,
})
}

/// Makes sure all directories stored in the Layout exist on the filesystem.
pub fn prepare(&mut self) -> CargoResult<()> {
paths::create_dir_all(&self.deps)?;
if !self.is_new_layout {
paths::create_dir_all(&self.deps)?;
paths::create_dir_all(&self.fingerprint)?;
}
paths::create_dir_all(&self.incremental)?;
paths::create_dir_all(&self.fingerprint)?;
paths::create_dir_all(&self.examples)?;
paths::create_dir_all(&self.build_examples)?;
paths::create_dir_all(&self.build)?;
Expand All @@ -232,7 +247,15 @@ impl Layout {
&self.dest
}
/// Fetch the deps path.
pub fn deps(&self) -> &Path {
pub fn deps(&self, pkg_dir: &str) -> PathBuf {
if self.is_new_layout {
self.build_unit(pkg_dir).join("deps")
} else {
self.deps.clone()
}
}
/// Fetch the deps path. (old layout)
pub fn legacy_deps(&self) -> &Path {
&self.deps
}
/// Fetch the examples path.
Expand All @@ -256,17 +279,45 @@ impl Layout {
&self.incremental
}
/// Fetch the fingerprint path.
pub fn fingerprint(&self) -> &Path {
pub fn fingerprint(&self, pkg_dir: &str) -> PathBuf {
if self.is_new_layout {
self.build_unit(pkg_dir).join("fingerprint")
} else {
self.fingerprint.join(pkg_dir)
}
}
/// Fetch the fingerprint path. (old layout)
pub fn legacy_fingerprint(&self) -> &Path {
&self.fingerprint
}
/// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you split the creation of these more specific directories into a refactor commit before this one? Reducing what we have to look at will make it easier to make sure we understand and agree with the new logic.

if self.is_new_layout {
self.build_unit(pkg_dir).join("build-script")
} else {
self.build().join(pkg_dir)
}
}
/// Fetch the build script execution path.
pub fn build_script_execution(&self, pkg_dir: &str) -> PathBuf {
if self.is_new_layout {
self.build_unit(pkg_dir).join("build-script-execution")
} else {
self.build().join(pkg_dir)
}
}
/// Fetch the artifact path.
pub fn artifact(&self) -> &Path {
&self.artifact
}
/// Fetch the build unit path
pub fn build_unit(&self, pkg_dir: &str) -> PathBuf {
self.build().join(pkg_dir)
}
/// Create and return the tmp path.
pub fn prepare_tmp(&self) -> CargoResult<&Path> {
paths::create_dir_all(&self.tmp)?;
Expand Down
55 changes: 42 additions & 13 deletions src/cargo/core/compiler/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ pub mod unit_dependencies;
pub mod unit_graph;

use std::borrow::Cow;
use std::collections::{HashMap, HashSet};
use std::collections::{BTreeMap, HashMap, HashSet};
use std::env;
use std::ffi::{OsStr, OsString};
use std::fmt::Display;
Expand All @@ -69,6 +69,7 @@ use std::sync::{Arc, LazyLock};
use annotate_snippets::{AnnotationKind, Group, Level, Renderer, Snippet};
use anyhow::{Context as _, Error};
use cargo_platform::{Cfg, Platform};
use itertools::Itertools;
use lazycell::LazyCell;
use regex::Regex;
use tracing::{debug, instrument, trace};
Expand Down Expand Up @@ -1353,12 +1354,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 @@ -1668,18 +1665,35 @@ fn build_deps_args(
unit: &Unit,
) -> CargoResult<()> {
let bcx = build_runner.bcx;
cmd.arg("-L").arg(&{
let mut deps = OsString::from("dependency=");
deps.push(build_runner.files().deps_dir(unit));
deps
});
if build_runner.bcx.gctx.cli_unstable().build_dir_new_layout {
let mut map = BTreeMap::new();

// Recusively add all depenendency args to rustc process
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo on Recusively

add_dep_arg(&mut map, build_runner, unit);

let paths = map.into_iter().map(|(_, path)| path).sorted_unstable();

for path in paths {
cmd.arg("-L").arg(&{
let mut deps = OsString::from("dependency=");
deps.push(path);
deps
});
}
} else {
cmd.arg("-L").arg(&{
let mut deps = OsString::from("dependency=");
deps.push(build_runner.files().deps_dir(unit));
deps
});
}

// Be sure that the host path is also listed. This'll ensure that proc macro
// dependencies are correctly found (for reexported macros).
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 Expand Up @@ -1755,6 +1769,21 @@ fn build_deps_args(
Ok(())
}

fn add_dep_arg<'a, 'b: 'a>(
map: &mut BTreeMap<&'a Unit, PathBuf>,
build_runner: &'b BuildRunner<'b, '_>,
unit: &'a Unit,
) {
map.insert(&unit, build_runner.files().deps_dir(&unit));

for dep in build_runner.unit_deps(unit) {
if map.contains_key(&dep.unit) {
continue;
}
add_dep_arg(map, build_runner, &dep.unit);
}
Comment on lines +1777 to +1784
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if you move the du-duplication check to the insert call, you only need to do BTreeMap lookups once per item, rather than twice at the cost of the coverhead of a function call that may have an early return.

Unsure if we'll even notice any of this

}

/// Adds extra rustc flags and environment variables collected from the output
/// of a build-script to the command to execute, include custom environment
/// variables and `cfg`.
Expand Down
Loading