Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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: 2 additions & 1 deletion src/cargo/core/compiler/build_runner/compilation_files.rs
Copy link
Member

Choose a reason for hiding this comment

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

This PR adds a CARGO_BUILD_DIR_LAYOUT_V2 flag read directly from std::env::var that allows users to opt into the new layout, even on stable rust.
And eventually have an option to opt out (temporarily) once the new layout is stabilized to ease the transition for users.

Could we describe the transition plan in the PR description? Or at least briefly summarize it and provide links to any other references.

It is not clear to me what the target audience CARGO_BUILD_DIR_LAYOUT_V2 is for , and if it breaks people what we should do.

Unlike other envs like __CARGO_GITOXIDE_DISABLE_LIST_FILES whose code path block everything completely if bugs happened, CARGO_BUILD_DIR_LAYOUT_V2 seems like an opt-in only environment variable. People can enable unstable Cargo flags on stable actually today with some trick already. Without knowing the plan it seems a bit weird to add a new one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I plan to FCP (#16336 (comment)) and that will include the transition plan

Copy link
Contributor

Choose a reason for hiding this comment

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

I wrote up my proposal for a plan at #15010 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, for the write up. I added a small note mentioning the tracking issue for the transition plan.

See the tracking issue for the transition plan and updates.

Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use tracing::debug;

use super::{BuildContext, BuildRunner, CompileKind, FileFlavor, Layout};
use crate::core::compiler::{CompileMode, CompileTarget, CrateType, FileType, Unit};
use crate::core::features::is_new_build_dir_layout_enabled;
use crate::core::{Target, TargetKind, Workspace};
use crate::util::{self, CargoResult, OnceExt, StableHasher};

Expand Down Expand Up @@ -245,7 +246,7 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> {
/// Note that some units may share the same directory, so care should be
/// taken in those cases!
fn pkg_dir(&self, unit: &Unit) -> String {
let separator = match self.ws.gctx().cli_unstable().build_dir_new_layout {
let separator = match is_new_build_dir_layout_enabled(self.ws.gctx()) {
true => "/",
false => "-",
};
Expand Down
3 changes: 2 additions & 1 deletion src/cargo/core/compiler/build_runner/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use std::sync::{Arc, Mutex};
use crate::core::PackageId;
use crate::core::compiler::compilation::{self, UnitOutput};
use crate::core::compiler::{self, Unit, UserIntent, artifact};
use crate::core::features::is_new_build_dir_layout_enabled;
use crate::util::cache_lock::CacheLockMode;
use crate::util::errors::CargoResult;
use annotate_snippets::{Level, Message};
Expand Down Expand Up @@ -467,7 +468,7 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
.root_output
.insert(kind, artifact_dir.dest().to_path_buf());
}
if self.bcx.gctx.cli_unstable().build_dir_new_layout {
if is_new_build_dir_layout_enabled(self.bcx.gctx) {
for (unit, _) in self.bcx.unit_graph.iter() {
let dep_dir = self.files().deps_dir(unit);
paths::create_dir_all(&dep_dir)?;
Expand Down
3 changes: 2 additions & 1 deletion src/cargo/core/compiler/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@

use crate::core::Workspace;
use crate::core::compiler::CompileTarget;
use crate::core::features::is_new_build_dir_layout_enabled;
use crate::util::flock::is_on_nfs_mount;
use crate::util::{CargoResult, FileLock};
use cargo_util::paths;
Expand All @@ -129,7 +130,7 @@ impl Layout {
dest: &str,
must_take_artifact_dir_lock: bool,
) -> CargoResult<Layout> {
let is_new_layout = ws.gctx().cli_unstable().build_dir_new_layout;
let is_new_layout = is_new_build_dir_layout_enabled(ws.gctx());
let mut root = ws.target_dir();
let mut build_root = ws.build_dir();
if let Some(target) = target {
Expand Down
3 changes: 2 additions & 1 deletion src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ use self::unit_graph::UnitDep;
use crate::core::compiler::future_incompat::FutureIncompatReport;
use crate::core::compiler::timings::SectionTiming;
pub use crate::core::compiler::unit::{Unit, UnitInterner};
use crate::core::features::is_new_build_dir_layout_enabled;
use crate::core::manifest::TargetSourcePath;
use crate::core::profiles::{PanicStrategy, Profile, StripInner};
use crate::core::{Feature, PackageId, Target, Verbosity};
Expand Down Expand Up @@ -1825,7 +1826,7 @@ pub fn lib_search_paths(
unit: &Unit,
) -> CargoResult<Vec<OsString>> {
let mut lib_search_paths = Vec::new();
if build_runner.bcx.gctx.cli_unstable().build_dir_new_layout {
if is_new_build_dir_layout_enabled(build_runner.bcx.gctx) {
let mut map = BTreeMap::new();

// Recursively add all dependency args to rustc process
Expand Down
10 changes: 10 additions & 0 deletions src/cargo/core/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1556,3 +1556,13 @@ pub fn cargo_docs_link(path: &str) -> String {
};
format!("https://doc.rust-lang.org/{url_channel}cargo/{path}")
}

/// Returns true of the new build dir layout is enabled.
#[allow(clippy::disallowed_methods)]
pub fn is_new_build_dir_layout_enabled(gctx: &GlobalContext) -> bool {
match std::env::var("CARGO_BUILD_DIR_LAYOUT_V2").as_deref() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Cases like #16348 have me wondering what bugs are that we don't see due to our isolated testing.

Maybe once the following are merged, do a test run of Cargo's tests with CARGO_BUILD_DIR_LAYOUT_V2=true to see what they uncover

Copy link
Contributor

Choose a reason for hiding this comment

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

#16351 was the follow up. All merged and ready to go on this

Copy link
Contributor

Choose a reason for hiding this comment

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

Once https://github.com/rust-lang/cargo/pull/16336/files#r2593979150 is done, we'll want to do a crater run just in case

Copy link
Member Author

Choose a reason for hiding this comment

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

opened rust-lang/rust#149852 for the crater run

Ok("true") => true,
Ok("false") => false,
Ok(_) | Err(_) => gctx.cli_unstable().build_dir_new_layout,
}
}
3 changes: 2 additions & 1 deletion src/cargo/ops/cargo_clean.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::core::compiler::{CompileKind, CompileMode, Layout, RustcTargetData};
use crate::core::features::is_new_build_dir_layout_enabled;
use crate::core::profiles::Profiles;
use crate::core::{PackageIdSpec, PackageIdSpecQuery, TargetKind, Workspace};
use crate::ops;
Expand Down Expand Up @@ -196,7 +197,7 @@ fn clean_specs(
clean_ctx.progress = Box::new(CleaningPackagesBar::new(clean_ctx.gctx, packages.len()));
let mut dirs_to_clean = DirectoriesToClean::default();

if clean_ctx.gctx.cli_unstable().build_dir_new_layout {
if is_new_build_dir_layout_enabled(clean_ctx.gctx) {
for pkg in packages {
clean_ctx.progress.on_cleaning_package(&pkg.name())?;

Expand Down
6 changes: 6 additions & 0 deletions src/doc/src/reference/unstable.md
Original file line number Diff line number Diff line change
Expand Up @@ -2040,6 +2040,12 @@ enabled = true
Enables the new build-dir filesystem layout.
This layout change unblocks work towards caching and locking improvements.

In addition to `-Zbuild-dir-new-layout`, `CARGO_BUILD_DIR_LAYOUT_V2` also exists as
a way to opt in and out of the new layout during the transition period.
`CARGO_BUILD_DIR_LAYOUT_V2=true` allows users to opt in to the new layout, even on stable.
`CARGO_BUILD_DIR_LAYOUT_V2=false` allows users to opt out to the new layout.
This includes post stabilization but it's important to note that this is a temporary flag and will
eventually be removed. See the tracking issue for the transition plan and updates.

## compile-time-deps

Expand Down
57 changes: 57 additions & 0 deletions tests/testsuite/build_dir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1094,6 +1094,63 @@ fn template_should_handle_reject_unmatched_brackets() {
.run();
}

#[cargo_test]
fn fallback_env_var_should_allow_overriding() {
let p = project()
.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();

// Stable + CARGO_BUILD_DIR_LAYOUT_V2=true should opt in
p.cargo("build")
.env("CARGO_BUILD_DIR_LAYOUT_V2", "true")
.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/build/foo/[HASH]/fingerprint/bin-foo
[ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo.json
[ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/dep-bin-foo
[ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/invoked.timestamp
[ROOT]/foo/build-dir/debug/build/foo/[HASH]/deps/foo[..][EXE]
[ROOT]/foo/build-dir/debug/build/foo/[HASH]/deps/foo[..].d

"#]]);

p.root().join("build-dir").rm_rf();

// Nightly + -Zbuild-dir-new-layout + CARGO_BUILD_DIR_LAYOUT_V2=false should allow user to fall
// back to old layout
p.cargo("-Zbuild-dir-new-layout build")
.masquerade_as_nightly_cargo(&["new build-dir layout"])
.env("CARGO_BUILD_DIR_LAYOUT_V2", "false")
.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

"#]]);
}

fn parse_workspace_manifest_path_hash(hash_dir: &PathBuf) -> PathBuf {
// Since the hash will change between test runs simply find the first directories and assume
// that is the hash dir. The format is a 2 char directory followed by the remaining hash in the
Expand Down