Enable Cargo's new build-dir layout#155439
Conversation
| // 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This comment has been minimized.
This comment has been minimized.
bf07fb8 to
eae2cbf
Compare
| // | ||
| // 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); |
There was a problem hiding this comment.
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
|
Looks like the miri job timed out. (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) |
|
x86_64 Linux jobs rarely hang, I've retried it to be sure. |
|
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. |
|
@ranger-ross see https://rustc-dev-guide.rust-lang.org/tests/docker.html#testing-with-docker This is the easiest way to run it locally. |
|
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. Looking at the other jobs, most of them did not have a large perf regression (except That is concerning... 😅 |
This comment was marked as outdated.
This comment was marked as outdated.
eae2cbf to
ad47c85
Compare
This comment has been minimized.
This comment has been minimized.
|
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 |
| // Chop off `deps`. | ||
| // Chop off `out`. | ||
| me.pop(); | ||
| // Chop off `<hash>`. | ||
| me.pop(); | ||
| // Chop off `<pkgname>`. | ||
| me.pop(); | ||
| // Chop off `build`. |
There was a problem hiding this comment.
Is this going to break things in rustfmt's CI when we do a subtree push?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@ranger-ross If you're able to duplicate the rustfmt code changes you made in rust-lang/rustfmt#6879 here that would be great.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This comment has been minimized.
This comment has been minimized.
This also syncs `RUSTFLAGS: -D warnings` which is added in rust-lang/rustfmt
|
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. |
|
@bors try jobs=x86_64-msvc-ext1 |
This comment has been minimized.
This comment has been minimized.
|
@bors try jobs=x86_64-msvc-1 |
This comment has been minimized.
This comment has been minimized.
|
💔 Test for e07d099 failed: CI. Failed job:
|
This comment has been minimized.
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.
|
@bors try jobs=x86_64-msvc-1,x86_64-msvc-ext1 |
This comment has been minimized.
This comment has been minimized.
|
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. rust/compiler/rustc_interface/src/passes.rs Lines 167 to 171 in 8c75e93 With the new layout, the increased The fix in 093a8ac changes the logic in rustc to only add native dependency types to the Hopefully the third r plus is the charm 🙏 @rustbot ready |
|
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 |
|
Should we See the limitation of proc-macro loading rdylib as a blocker, or should we just go? |
|
@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:
oops, didn't realize I should have checked the review rotation, feel free to reassign |
|
|
|
You don't really have to check it, the review rotation mostly prevents auto assignment, but you can still Anyway, I'll take a look. |
There was a problem hiding this comment.
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.
| /// 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 |
There was a problem hiding this comment.
| /// 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. |
There was a problem hiding this comment.
| /// 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] |
There was a problem hiding this comment.
This attribute is mostly useless, because this call can't really panic, right?
There was a problem hiding this comment.
yeah, I can remove. That was a carry over from the Cargo impl that should probably should have removed
| toml = "0.5" | ||
| walkdir = "2.4" | ||
| xz2 = "0.1" | ||
| tempfile = "3.15.0" |
There was a problem hiding this comment.
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 😆
| /// 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 { |
There was a problem hiding this comment.
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?
- Put
ArgFileCommandintosrc/build_helper/arg_file_command.rs. Put a comment on top of it that says it has to be a self-contained file. - Use the
ArgFileCommandinbuild_helperfrombootstrapandcompiletestnormally - In
bootstrap'sshared_helpers.rs, manually include thearg_file_command.rsfile via a#[path]mod (orinclude!, but I think that the former is better), so that the bootstraprustc.rsbinaries can use it.
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.
I think this makes a lot of sense. It'd help reduce command length. |
|
Let's try! @bors r=bjorn3,kobzol |
View all comments
This PR enables the new Cargo
build-dirlayout 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