Use download-rustc = "if-unchanged" in x86_64-gnu
#112143
Closed
Conversation
This file contains 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
|
r? @ozkanonur (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
A-testsuite
jyn514
added
S-experimental
jyn514
force-pushed
the
test-download-rustc
branch
from
513cfdc to
b7d5951
Compare
|
The last commit changes looks good, do you want me to review first two commits too(as far as I see they aren't reviewed on the based PR)? |
This comment has been minimized.
This comment has been minimized.
|
@ozkanonur sorry for the ping - I'm still getting this working and there will likely be a lot more changes in the meantime. No need to review just yet. |
this happens because the llvm in |
jyn514
force-pushed
the
test-download-rustc
branch
from
c8168fe to
1df123f
Compare
This comment has been minimized.
This comment has been minimized.
|
ugh, this is going to be messy. the latest failure is i think what's going on is that it's also using the llvm from the |
jyn514
added
the
S-waiting-on-author
Ok, I spent some time looking into this today, using If the LLVM in ci-rustc-sysroot comes from I think the problem is not actually the file in I'm looking into where the search path is modified now. |
|
Ok, I don't think this is actually possible to support. Even if the libLLVM in I think I need to add code to bootstrap to disable combining
this is likely because we're setting the llvm version to the version of the host, not the rustc was compiled with - I'll see if I can fix it in compiletest. |
This comment has been minimized.
This comment has been minimized.
|
The x86_64-gnu-llvm-14 image is fundamentally incompatible with download-rustc. If I'm reading this right, you're trying to fix this by ... not actually making it llvm-14 if run in PR CI in particular? That's super confusing. You should probably switch PR CI to use a different image if you want to use the option. I believe historically we used this one because it saves the LLVM build, but given that we have download-llvm now, that choice probably no longer makes sense, and we can use a more standard configuration. |
oh, I hadn't considered that, that's a good idea :) i'm going to fix the rest of the CI failures before switching the image but i'll revert 63ebc7a after that |
This comment has been minimized.
This comment has been minimized.
jyn514
force-pushed
the
test-download-rustc
branch
from
2bc8095 to
35f83b7
Compare
This comment has been minimized.
This comment has been minimized.
jyn514
force-pushed
the
test-download-rustc
branch
from
fc1df6b to
6ec96be
Compare
This comment has been minimized.
This comment has been minimized.
This is a pre-requisite for rust-lang#112143, which wants to start using download-rustc in PRs. download-rustc doesn't allow providing an external LLVM.
This has two main benefits: 1. It tests that download-rustc doesn't regress. This doesn't reduce our test coverage, since we still never use `download-rustc` in a full bors merge, but it should make it a lot less likely that this breaks by accident. 2. It greatly speeds up CI when compiler/ and library/ haven't been modified. Once the changes in rust-lang/compiler-team#619 land, this will also be faster for changes to library/, and only changes to compiler/ will have to rebuild.
…ning in CI Avoids the following error: ``` curl: (22) The requested URL returned error: 404 error: failed to download pre-built rustc from CI note: old builds get deleted after a certain time help: if trying to compile an old commit of rustc, disable `download-rustc` in config.toml: [rust] download-rustc = false ``` Note that this strategy is more complicated than the one for download-ci-llvm, which doesn't have to deal with rollup commits. Eventually we should probably adopt this for LLVM too, to avoid bugs if someone forgets to mark an LLVM bump as rollup=never. I tested this with `git -c user.email=bors@rust-lang.org -c user.name=bors commit --allow-empty -m 'test bors commit' && GITHUB_ACTIONS=true x check`.
these should compile from source; i have reasonable confidence in download-rustc, but not so much that i want to allow shipping it in dist artifacts.
…-error.rs Not only are they a pain to update, but they differ depending on whether `parallel-rustc` is enabled or not
jyn514
force-pushed
the
test-download-rustc
branch
from
6ec96be to
2c3ad2d
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this pull request
Jul 12, 2023
…ertlarsan68 various download-rustc fixes separated out from rust-lang#112143 because it keeps getting stuck in limbo. best reviewed commit-by-commit
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this pull request
Jul 12, 2023
…ertlarsan68 various download-rustc fixes separated out from rust-lang#112143 because it keeps getting stuck in limbo. best reviewed commit-by-commit
|
☔ The latest upstream changes (presumably #113608) made this pull request unmergeable. Please resolve the merge conflicts. |
|
update on the current status: supporting cc @SparrowLii can you confirm whether linking a rustc_driver built with parallel-compiler to a rustdoc built without is expected to work? in any case, if this ends up being significantly more work than expected i'll look into using the -alt builds for LLVM only and not rustc_driver. that will be kinda painful (we'll need 4 states instead of 2) but it will avoid needing to fix all parallel-compiler test failures before landing this. |
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jul 12, 2023
…=ozkanonur Remap paths for ~/.cargo in UI tests Users won't have the original cargo registry from the CI builder available, even if they have `rust-src` installed. Don't show their sources in UI tests even if they're available. As a happy side-effect, this fixes a few UI tests when download-rustc is enabled. Helps with rust-lang#112143.
Sorry, I don't think they will link and work properly :( since the data structures implementation in |
|
hmm, it seems to work ok locally - i think rustdoc loads rustc_data_structures from the sysroot instead of recompiling, so there's only one version :) looking at rustdoc i think the only part of that changes with parallel_compiler (other than the auto traits thing) is Lines 213 to 214 in cef812b |
|
going to set this to waiting-on-review until i hear back about #112143 (comment) |
jyn514
added
S-waiting-on-review
Mark-Simulacrum
added
S-waiting-on-author
Is this still part of this PR? I don't see it in the diff I think. I agree that test coverage doesn't reduce too much (we do lose all the testing on mingw-check and gnu-tools for exercising the alt build paths in bootstrap and in terms of building compiler + tools). But I guess this seems OK for now. I would prefer that we hard gate this on PR CI though if possible, so we don't accidentally stop testing assertions globally in the future. |
jyn514
added a commit
to jyn514/rust
that referenced
this pull request
Nov 13, 2023
This is a pre-requisite for rust-lang#112143, which wants to start using download-rustc in PRs. download-rustc doesn't allow providing an external LLVM.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This has two main benefits:
It tests that download-rustc doesn't regress. This does introduce a new possible failure mode: because we use
if-available, it's possible to land a change that breaks download-rustc only, causing all later PRs to fail unless they modify the compiler. However, I think the risk is pretty small in practice, since for us not to notice you'd have to change both compiler/ and bootstrap/ in such a way that download-rustc breaks.It greatly speeds up CI when compiler/ and library/ haven't been modified. Once the changes in
Redesign bootstrap stages compiler-team#619 land, this will also be faster for changes to
library/, and only changes to compiler/ will have to rebuild.
This includes a couple fixes for download-rustc itself which I can separate out easily if desired:
llvm.assertions,llvm-config,rust.channel. see https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/ban.20rustc.20configs.20when.20download-rustc.20is.20enabled.3F for discussion on whether to extend this to options that end up being ignored instead of actively breaking.x test --stage 0 core; before I'd only tested--stage 2It also fixes a pre-existing bug where sanitizer tests would always fail when cross-compiled; I guess we just didn't have any CI exercising that before? I was able to replicate it with download-rustc disabled.
Finally, it makes some changes to CI itself:
download-rustc = "if-unchanged"for all PR CI jobs. This will rebuild rustc from source ifcompiler/orlibrary/are modified, but download it otherwise:rust/src/bootstrap/config.rs
Lines 1859 to 1861 in bff5ecd
llvm.assertionsif download-rustc is enabled. I think this should not reduce PR test coverage too much; before we were usingllvm-config = /usr/lib/llvm-14, which already didn't have assertions enabled, andmingw-checkdidn't build LLVM at all.x86_64-gnu-llvm-14job tox86_64-gnuin CI. This is because download-rustc doesn't support an externally provided LLVM (see Usedownload-rustc = "if-unchanged"inx86_64-gnu#112143 (comment)).Note that this makes no changes at all to the full bors merge, which has exactly the same amount of coverage as before.
Some timing info:
Before any change x86_64-gnu-llvm-14 takes 37 minutes: https://github.com/rust-lang/rust/actions/runs/5193836686/jobs/9364870143?pr=112366
With just switching to x86_64-gnu, it takes 42 minutes: https://github.com/rust-lang/rust/actions/runs/5194131394/jobs/9365485340?pr=112296
After this change, x86_64-gnu takes 26 minutes: https://github.com/rust-lang/rust/actions/runs/5173293772/jobs/9318431702?pr=112143