Skip to content

Enable Cargo's new build-dir layout#155439

Merged
rust-bors[bot] merged 8 commits into
rust-lang:mainfrom
ranger-ross:cargo-new-build-dir-layout
Jun 21, 2026
Merged

Enable Cargo's new build-dir layout#155439
rust-bors[bot] merged 8 commits into
rust-lang:mainfrom
ranger-ross:cargo-new-build-dir-layout

Conversation

@ranger-ross

@ranger-ross ranger-ross commented Apr 17, 2026

Copy link
Copy Markdown
Member

View all comments

This PR enables the new Cargo build-dir layout in boostrap builds with -Zbuild-dir-new-layout.

See: #t-infra/bootstrap > Has anyone tested `./x` with the new build-dir layout?

Tracked in: rust-lang/cargo#15010

r? @bjorn3

cc: @epage

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 17, 2026
Comment thread src/bootstrap/src/core/build_steps/compile.rs Outdated
// got a list of prefix/extensions and we basically just need to find the
// most recent file in the `deps` folder corresponding to each one.
let contents = target_deps_dir
// most recent file in the `build` folder corresponding to each one.

@weihanglo weihanglo Apr 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not yet read the code by myself. Just wonder why bootstrap are doing this? And if bootstrap needs to do it, it should be a feature in Cargo, at least a (perma-)unstable feature.

View changes since the review

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See the commit block around line 2374. Basically we need the non-uplifted name that contains the unique hash. The artifact notifications provide the uplifted name where possible.

@rust-log-analyzer

This comment has been minimized.

@ranger-ross ranger-ross force-pushed the cargo-new-build-dir-layout branch from bf07fb8 to eae2cbf Compare April 17, 2026 15:27
//
// Cargo's build folder is structured as `build/<pkg>/<hash>/out/<artifacts>` so
// we need to traverse multiple directory layers to get to actual files.
let read_dir = |path: &Path| path.read_dir().ok().into_iter().flatten().filter_map(Result::ok);

@ranger-ross ranger-ross Apr 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

checks failed because if there are any lingering files/dirs in <build-dir>/<target>/build that are not using the new layout it panics.

Update the original code to not panic if we call .read_dir() on a non-directory. Lets see if CI is happy with this

View changes since the review

@ranger-ross

Copy link
Copy Markdown
Member Author

Looks like the miri job timed out.
Out of curiosity, how stable is this job?

(Sorry, haven't touch the rl/r repo much so I wondering if the failure is related to the new layout or if its normal for spurious failures)

@mati865

mati865 commented Apr 19, 2026

Copy link
Copy Markdown
Member

x86_64 Linux jobs rarely hang, I've retried it to be sure.

@ranger-ross

Copy link
Copy Markdown
Member Author

Okay it's hanged on both runs so it's probably the changes.

Is there a way for me to run this check on my machine? I was trying to parse through the GitHub actions to see what command is being run but I was struggling to find it.

@weihanglo

Copy link
Copy Markdown
Member

@ranger-ross see https://rustc-dev-guide.rust-lang.org/tests/docker.html#testing-with-docker

cargo run --manifest-path src/ci/citool/Cargo.toml run-local x86_64-gnu-miri

This is the easiest way to run it locally.

@ranger-ross

Copy link
Copy Markdown
Member Author

Okay, so I did some testing and locally it runs fine. (takes around ~20 mins on my machine)

After looking a bit closer at the job logs, it actually never got stuck.
It just take WAY longer than normal....
According to the github stats x86_64-gnu-miri normally takes ~58 mins to run, but we took over 6 hrs and hit the timeout.

Looking at the other jobs, most of them did not have a large perf regression (except x86_64-gnu-tools which was also pretty bad)

That is concerning... 😅

@ranger-ross

This comment was marked as outdated.

@ranger-ross ranger-ross force-pushed the cargo-new-build-dir-layout branch from eae2cbf to ad47c85 Compare April 27, 2026 15:15
@rustbot

rustbot commented Apr 27, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@rustbot rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc T-rustfmt Relevant to the rustfmt team, which will review and decide on the PR/issue. labels Apr 27, 2026
@rustbot

This comment has been minimized.

@ranger-ross ranger-ross changed the title Enable Cargo's new build-dir layout in boostrap Enable Cargo's new build-dir layout Apr 27, 2026
Comment thread src/tools/miri/test-cargo-miri/run-test.py Outdated
Comment thread src/tools/compiletest/src/runtest/run_make.rs Outdated
@ranger-ross

Copy link
Copy Markdown
Member Author

I realized the issue is not with the speed of compilation, but rather that for some reason in CI the compiler (and many other units) are being re-built multiple times which slows everything down and times out.

I haven't figured out why yet. While testing the CI on my fork, I've noticed its inconsistent and sometimes everything only builds once. I'm still investigating why this could be happening. rust-lang/cargo#16345 comes to mind as something that might change this behavior, but I am still unsure whats happening.

I went ahead and pushed some new changes with more fixes for tests and cranelift.

I'll continue investigating this week, but any rl/r experts see any obvious issues with my changes lmk
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 27, 2026
Comment thread compiler/rustc_codegen_cranelift/build_system/build_sysroot.rs
Comment thread src/tools/rustfmt/src/test/mod.rs Outdated
Comment on lines +1080 to +1086
// Chop off `deps`.
// Chop off `out`.
me.pop();
// Chop off `<hash>`.
me.pop();
// Chop off `<pkgname>`.
me.pop();
// Chop off `build`.

@ytmimi ytmimi Apr 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this going to break things in rustfmt's CI when we do a subtree push?

View changes since the review

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ahh yeah it would probably break, maybe better for me to remove that from this PR and raise a separate PR in the rustfmt repo?

I'm still trying to understand what changes we need in this PR verse what changes can be split out into smaller more targeted changes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In that case, I'd prefer we open up a PR to add the new cargo build dir layout support to rustfmt directly and verify that things are working before making any changes to the rustfmt subtree here in r-l/rust

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fair enough. I tested on my fork and I think rust-lang/rust CI fails with out rustfmt changes.

I went ahead and opened rust-lang/rustfmt#6879

@ytmimi ytmimi Apr 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ranger-ross If you're able to duplicate the rustfmt code changes you made in rust-lang/rustfmt#6879 here that would be great.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added those commits to this PR. I also noticed that RUSTFLAGS: -D warning was missing from the github actions workflows in rust-lang/rust so I went ahead and added that while I was at it. I guess it doesn't matter for this repo but oh well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We've recently made some changes to those CI files in rustfmt so it's possible that the RUSTFLAGS: -D warning changes just haven't been synced up yet, but I don't think there's any issue in explicitly including that here.

@rust-bors

This comment has been minimized.

This also syncs `RUSTFLAGS: -D warnings` which is added in
rust-lang/rustfmt
@rustbot

rustbot commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@ranger-ross

Copy link
Copy Markdown
Member Author

@bors try jobs=x86_64-msvc-ext1

@rust-bors

This comment has been minimized.

@rust-bors

rust-bors Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: efd5bf1 (efd5bf1b4495b0894eace2381078876673f8b432)
Base parent: 0c1748c (0c1748ce7c56091b659757e488dbaf782b814137)

@ranger-ross

Copy link
Copy Markdown
Member Author

@bors try jobs=x86_64-msvc-1

@rust-bors

This comment has been minimized.

@rust-bors

rust-bors Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

💔 Test for e07d099 failed: CI. Failed job:

@rust-log-analyzer

This comment has been minimized.

This is an optimization to reduce the amount for paths that get added to
the dylib search path. This is especially important for Windows as it
has issues when PATH gets too long. With the new Cargo build-dir we
increased the number of paths being passed.
@ranger-ross

Copy link
Copy Markdown
Member Author

@bors try jobs=x86_64-msvc-1,x86_64-msvc-ext1

@rust-bors

This comment has been minimized.

@rust-bors

rust-bors Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 5edfdba (5edfdbaef81d9050f7642bdcebbaa8d081df3186)
Base parent: 32ea361 (32ea3615cc027bcb8fd720c7511ffb484f6223a3)

@ranger-ross

ranger-ross commented Jun 20, 2026

Copy link
Copy Markdown
Member Author

Okay, I think now the windows CI is looking good.

Tracking down the issue was a bit difficult but the fix is relatively straight forward.
In the new layout, we pass more -L flags use build units get thier own dir for the compiler output.
On windows the command length is limited to about ~32K including env vars like PATH.
Also windows uses PATH for searching during dylib linking. During rustc appends all of the dependency paths (-L) to PATH.

// Windows dlls do not have rpaths, so they don't know how to find their
// dependencies. It's up to us to tell the system where to find all the
// dependent dlls. Note that this uses cfg!(windows) as opposed to
// targ_cfg because syntax extensions are always loaded for the host
// compiler, not for the target.

With the new layout, the increased PATH length gets too long and hits the windows limit causing a failure.
In our case the #[cargo_test] proc macro was failing to run rustc -V, but it would fail to spawn any process.

The fix in 093a8ac changes the logic in rustc to only add native dependency types to the PATH meaning that all of the dependency paths with just an rlib aren't added greatly reducing the size.
I think this should be safe and both Cargo and rustc's test suites pass on windows with the change, but let me know if I am over looking something here.

Hopefully the third r plus is the charm 🙏

@rustbot ready

@bjorn3

bjorn3 commented Jun 20, 2026

Copy link
Copy Markdown
Member

That commit would break if proc-macros ever get the ability to depend on rust dylibs, right?

@ranger-ross

Copy link
Copy Markdown
Member Author

That commit would break if proc-macros ever get the ability to depend on rust dylibs, right?

Correct, we'd need a smarter way add the paths without exploding the PATH on Windows

@weihanglo

Copy link
Copy Markdown
Member

Should we See the limitation of proc-macro loading rdylib as a blocker, or should we just go?

@ranger-ross

ranger-ross commented Jun 20, 2026

Copy link
Copy Markdown
Member Author

@bjorn3 reached out and mentioned they would not be available to review over the few days and suggested I request a review from someone in T-bootstrap.

r? @Kobzol

edit:

Kobzol is not on the review rotation at the moment.

oops, didn't realize I should have checked the review rotation, feel free to reassign

@rustbot

rustbot commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

Kobzol is not on the review rotation at the moment.
They may take a while to respond.

@Kobzol

Kobzol commented Jun 21, 2026

Copy link
Copy Markdown
Member

You don't really have to check it, the review rotation mostly prevents auto assignment, but you can still r? people individually. It's mostly a warning to let people know that the reviewer might not be available. I am available (just don't want to join the rotation atm), maybe we should allow configuring the warning.

Anyway, I'll take a look.

@Kobzol Kobzol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The argfile stuff is pretty annoying :( Windows strikes back again, I guess. I wonder if we should add some path placeholders to rustc, so that you could say something like --placeholder=FOO=/very/long/path -L[FOO]/d/e -L[FOO]/d/e 😆

Left some comments related to bootstrap.

View changes since this review

/// This is useful as we have some commands that can get very long and at times
/// hit the OS limit (usually Windows)
///
/// This implementation is based off the of `ProcessBuilder` implementation in Cargo

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
/// This implementation is based off the of `ProcessBuilder` implementation in Cargo
/// This implementation is based off the `ProcessBuilder` implementation in Cargo

/// but simplified.
///
/// NOTE: In most scenarios we want to avoid arg files as it makes debugging more complicated
/// so we try to avoid it if the command is not close the the OS limit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
/// so we try to avoid it if the command is not close the the OS limit.
/// so we try to avoid it if the command is not close to the OS limit.

}

impl ArgFileCommand {
#[track_caller]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This attribute is mostly useless, because this call can't really panic, right?

@ranger-ross ranger-ross Jun 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yeah, I can remove. That was a carry over from the Cargo impl that should probably should have removed

Comment thread src/bootstrap/src/utils/shared_helpers.rs Outdated
Comment thread src/bootstrap/src/core/builder/cargo.rs
Comment thread src/bootstrap/Cargo.toml
toml = "0.5"
walkdir = "2.4"
xz2 = "0.1"
tempfile = "3.15.0"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This adds ~10 new dependencies to bootstrap, which is a bit unfortunate, we have been trying to avoid depending on tempfile before. But for the rustc.rs usage we'd have to pre-generate a temporary directory in the build dir and let it store the argfile there, that is annoying, because Cargo could invoke multiple rustc instances in parallel.

Let's keep it, but it makes me a bit grouchy 😆

Comment thread src/tools/compiletest/src/util.rs Outdated
/// NOTE: In most scenarios we want to avoid arg files as it makes debugging more complicated
/// so we try to avoid it if the command is not close the the OS limit.
#[derive(Debug)]
pub(crate) struct ArgFileCommand {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is probably the tenth command abstraction we have in this repo, and now also duplicated in two places 😆 Could you please use the same trick we use in the shared_helpers in bootstrap to remove this copy paste?

  1. Put ArgFileCommand into src/build_helper/arg_file_command.rs. Put a comment on top of it that says it has to be a self-contained file.
  2. Use the ArgFileCommand in build_helper from bootstrap and compiletest normally
  3. In bootstrap's shared_helpers.rs, manually include the arg_file_command.rs file via a #[path] mod (or include!, but I think that the former is better), so that the bootstrap rustc.rs binaries can use it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

sure, that a good idea. It hurt a bit having to duplicate it.

I used the #[path] approach like you mentioned in de3fd29 and d7a58fa

This allows compiletest to support the new Cargo `build-dir` layout
which passes more `-L` flags as the `deps` dir has been split per build
unit. This can be an issue on Windows as the max command size is fairly
small.
This reduces the size of PATH on windows during macro expansion to avoid
hitting windows limits. With the new Cargo build-dir layout this becomes
more important as Cargo now passes more `-L` args which end up bloating
PATH.
@ranger-ross

Copy link
Copy Markdown
Member Author

I wonder if we should add some path placeholders to rustc, so that you could say something like --placeholder=FOO=/very/long/path -L[FOO]/d/e -L[FOO]/d/e

I think this makes a lot of sense. It'd help reduce command length.
The big downside of the new build-dir layout is that we need to pass all of the out dirs to rustc explictly.
I think upsides of the new build-dir layout still make it worth it, but I think we should try to eventually optimimize the length args sent to rustc.

Comment thread src/tools/compiletest/src/util.rs
@Kobzol

Kobzol commented Jun 21, 2026

Copy link
Copy Markdown
Member

Let's try!

@bors r=bjorn3,kobzol

@rust-bors

rust-bors Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 61f3e08 has been approved by bjorn3,kobzol

It is now in the queue for this repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-CI Area: Our Github Actions CI A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-rustfmt Relevant to the rustfmt team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.