-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
9121077
0c143b1
0b81b44
8603e30
c849440
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)) | ||
} | ||
} | ||
|
||
|
@@ -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. | ||
|
@@ -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, "-"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to be the only case for
|
||
let kind = match unit.target.kind() { | ||
TargetKind::Bin => "bin", | ||
TargetKind::Lib(lib_kinds) => match lib_kinds.as_slice() { | ||
|
@@ -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. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So while this reduces the "max content per directory" (since Should we change from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Could we expand a bit more on the benefit of the proposed change? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) That said I also found an article that claims a flat structure is better on ext4 for linux. Given the long paths issue on windows, perhaps it would be better to shorten names to help mitigate this? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. As for the original suggestion
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that is a good idea 👍 The code for There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -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 { | ||
|
@@ -155,12 +156,23 @@ impl Layout { | |
ws: &Workspace<'_>, | ||
target: Option<CompileTarget>, | ||
dest: &str, | ||
is_host_layout: bool, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Making an enum would likely be too much boilerplate. Options
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this bool is only used for to know if we should omit the platform in path. ( One idea i have is to split There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are your thoughts on not adding the platform to the directory unconditionally, at least at this phase? I feel like that is a source of some extra complication in this change, making it harder to understand and its a change for a directory that is not user visible, limiting the benefits. |
||
) -> 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); | ||
|
@@ -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)?; | ||
|
@@ -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. | ||
|
@@ -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 { | ||
epage marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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)?; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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}; | ||
|
@@ -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 { | ||
|
@@ -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(); | ||
|
||
// Recursively add all depenendency args to rustc process | ||
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 | ||
}); | ||
} | ||
Comment on lines
+1668
to
+1689
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something for us to keep in mind is that this change will likely lead to complaints similar to #13941. |
||
|
||
// 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 | ||
}); | ||
} | ||
|
@@ -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, | ||
) { | ||
if map.contains_key(&unit) { | ||
return; | ||
} | ||
map.insert(&unit, build_runner.files().deps_dir(&unit)); | ||
epage marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
for dep in build_runner.unit_deps(unit) { | ||
add_dep_arg(map, build_runner, &dep.unit); | ||
} | ||
} | ||
|
||
/// 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`. | ||
|
There was a problem hiding this comment.
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 forname/hash
rather thanname-hash
There was a problem hiding this comment.
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
toname/hash