Enable Cargo's new build-dir layout#155439
Enable Cargo's new build-dir layout#155439ranger-ross wants to merge 3 commits intorust-lang:mainfrom
Conversation
| let target_root_dir = stamp.path().parent().unwrap(); | ||
| // `target_deps_dir` looks like $dir/$target/release/deps | ||
| let target_deps_dir = target_root_dir.join("deps"); | ||
| // `target_deps_dir` looks like $dir/$target/release/build |
There was a problem hiding this comment.
| // `target_deps_dir` looks like $dir/$target/release/build | |
| // `target_build_dir` looks like $dir/$target/release/build |
| // 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 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. |
| fail(f"`{target_dir}` contains unexpected files") | ||
| # Ensure something exists inside that target dir. | ||
| os.access(os.path.join(target_dir, "miri", "debug", "deps"), os.F_OK) | ||
| os.access(os.path.join(target_dir, "miri", "debug", "build"), os.F_OK) |
There was a problem hiding this comment.
Any idea why this change was not required when we enabled the new build dir layout in Miri's CI?
There was a problem hiding this comment.
I am not sure, perhaps it existed from a previous run or was created from some non-cargo script?
Cargo should not create deps when -Zbuild-dir-new-layout is enabled.
There was a problem hiding this comment.
I don't think we cache these folders so I can't explain how they could be left over from a previous run. And nothing else should be touching those folders.
There was a problem hiding this comment.
| os.access(os.path.join(target_dir, "miri", "debug", "build"), os.F_OK) | |
| os.access(os.path.join(target_dir, "miri", "debug"), os.F_OK) |
That should work with old and new layout so we avoid surprising failures when people run this script locally in the Miri subtree.
| } | ||
| } | ||
|
|
||
| /// Gets the to all `out` dirs in a given Cargo `build-dir/<profile>/build` dir. |
There was a problem hiding this comment.
I cannot make sense of this comment, it seems to have some extra words that shouldn't be there.^^
There was a problem hiding this comment.
Sorry, English is hard. Will fix.
Should be
Gets all of the
outdirs in a given Cargobuild-dir/<profile>/builddir
There was a problem hiding this comment.
Note that the same comment occurs twice in this PR. :)
|
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 |
There was a problem hiding this comment.
I have already changed cg_clif to use -Zbuild-dir-new-layout in https://github.com/rust-lang/rustc_codegen_cranelift/ It just hasn't been synced back yet.
There was a problem hiding this comment.
ahh okay, I'll drop that commit from this PR. I think it shouldn't impact the bootstrap changes. (correct me if I am wrong)
| // 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
|
☔ The latest upstream changes (presumably #155941) made this pull request unmergeable. Please resolve the merge conflicts. |
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