Explicitly request copying rustc-dev artifacts for rustfmt/clippy and add rustc libs path for rustc-private tools under download-rustc#156528
Open
jieyouxu wants to merge 3 commits into
Conversation
Collaborator
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
Contributor
|
@jieyouxu I tested these changes locally and they mostly work. Testing is fine now, but running rustfmt is broken for stage 2. I also found it odd that it's not testing stage 1 when explicitly passing # These defaults are meant for contributors to tools which build on the
# compiler, but do not modify it directly.
[rust]
# This greatly increases the speed of rebuilds, especially when there are only minor changes. However, it makes the initial build slightly slower.
incremental = true
# Most commonly, tools contributors do not need to modify the compiler, so
# downloading a CI rustc is a good default for tools profile.
download-rustc = "if-unchanged"
[build]
# cargo and clippy tests don't pass on stage 1
test-stage = 2
# Document with the in-tree rustdoc by default, since `download-rustc` makes it quick to compile.
doc-stage = 2
# Contributors working on tools will probably expect compiler docs to be generated, so they can figure out how to use the API.
compiler-docs = true
# Contributors working on tools are the most likely to change non-rust programs.
tidy-extra-checks = "auto:js,auto:py,auto:cpp,auto:spellcheck"
[llvm]
# Will download LLVM from CI if available on your platform.
# If you intend to modify `src/llvm-project`, use `"if-unchanged"` or `false` instead.
download-ci-llvm = trueTerminal OutputTrying to run run rustfmt with Terminal OutputTrying to run run rustfmt with Terminal Output |
Member
Author
|
Ugh, yeah, more staging problems. Thanks for the testing. I'll take a look after this week |
`compile::Rustc`'s sysroot copying logic intentionally does not implicitly copy `rustc-dev` artifacts unless explicitly requested through `builder.ensure(compile::Rustc)`, due to a previous issue RUST-108767: ```text // NOTE(rust-lang#108767): We intentionally don't copy `rustc-dev` artifacts until they're // requested with `builder.ensure(Rustc)`. This fixes an issue where we'd have multiple // copies of libc in the sysroot with no way to tell which to load. There are a few // quirks of bootstrap that interact to make this reliable: // 1. The order `Step`s are run is hard-coded in `builder.rs` and not configurable. This // avoids e.g. reordering `test::UiFulldeps` before `test::Ui` and causing the latter // to fail because of duplicate metadata. // 2. The sysroot is deleted and recreated between each invocation, so running `x test // ui-fulldeps && x test ui` can't cause failures. ``` So, for rustfmt/clippy, we insert intentionally explicit `builder.ensure(compile::Rustc)` as a short-term band-aid, leaving FIXMEs pointing to RUST-156525 to investigate if the multiple libc copies is still a problem and if that can be fixed properly. This is by no means a proper fix, but it should unblock local tool profile workflows trying to use `download-rustc` with {rustfmt,clippy}.
Centrally in `tool::prepare_tool_cargo`. So that `./x run rustfmt` + `download-rustc` can find the correct rustc libs.
26a44dc to
71487e8
Compare
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. |
This comment was marked as off-topic.
This comment was marked as off-topic.
Member
Author
|
Not yet ready for proper review (I need to do more local testing). Note to self:
@ytmimi could you try out this new set of changes locally to check if both |
Contributor
|
@jieyouxu was able to test this again.
|
Member
Author
This is expected (albeit somewhat confusing yes) from using ci rustc -- since when using download rustc we are trying to avoid building a rustc, the |
rustc-dev artifacts for rustfmt/clippy under download-rustcrustc-dev artifacts for rustfmt/clippy and add rustc libs path for rustc-private tools under download-rustc
rustc-dev artifacts for rustfmt/clippy and add rustc libs path for rustc-private tools under download-rustcrustc-dev artifacts for rustfmt/clippy and add rustc libs path for rustc-private tools under download-rustc
Member
Author
|
Retested:
I did look at a step resolution shapshot test, but it turns out the way we implement download-rustc makes it "not show up" materially different in terms of steps versus non-download-rustc. Oh well. @rustbot review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#t-infra/bootstrap > x run rustfmt works but x test rustfmt fails to compile reported that
./x test rustfmt --stage={1,2}no longer works withdownload-rustc.builder.ensure(Rustc)s around the times of stage 0 rejiggling.compile::Sysroot::runhas a quirky non-trivial behavior regardingrustc-devartifact copying underdownload-rustc, tracing back to sysroot dep resolution troubles (cf.tests/ui/allocator/no_std-alloc-error-handler*fail whendownload-rustcis enabled #108767), we need to explicitlyensure(Rustc)to opt-in torustc-devartifact copying underdownload-rustc.ensures to forcerustc-devartifact copying underdownload-rustcto unblock rustfmt/clippy contributors who want to usedownload-rustc(to avoid having to build the compiler).Additionally, I needed to explicitly add rustc libs path for
RustcPrivatetools, or else it seems like the ci-rustc sysroot is not always correctly set.Testing
Use a dummy config:
bootstrap.rustfmt.tomlRun:
./x test rustfmt --config=bootstrap.rustfmt.toml --stage=1./x test rustfmt --config=bootstrap.rustfmt.toml --stage=2./x test clippy --config=bootstrap.rustfmt.toml --stage=1./x test clippy --config=bootstrap.rustfmt.toml --stage=2When reviewing, also check that if you revert the commit / this PR, that invocations above fail (which is the case against
mainfor me).Tested also without
download-rustcto make sure./x test {rustfmt,clippy} --stage={1,2}still works.Notes
This is by no means a proper fix, but it should unblock local tool profile workflows trying to use
download-rustcwith {rustfmt,clippy}. I don't think it's worth blocking over that.Efforts for a more proper fix should be tracked by #156525, I left FIXMEs pointing to that.