-
Notifications
You must be signed in to change notification settings - Fork 13k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix x test src/tools/error_index_generator --stage {0,1}
#95440
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
There were two fixes needed: 1. Use `top_stage` instead of `top_stage - 1`. There was a long and torturous comment about trying to match rustdoc's version, but it works better without the hard-coding than with. 2. Make sure that `ci-llvm/lib` is added to LD_LIBRARY_PATH. Previously the error index would be unable to load LLVM for stage0 builds. At some point we should probably have a discussion about how rustdoc stages should be numbered; confusion between 0/1/2 has come up several times in bootstrap now. Note that this is still broken when using `download-rustc = true` and `--stage 1`, but that's *really* a corner case and should affect almost no one. `--stage {0,2}` work fine with download-rustc.
fwiw, I've been running into the libllvm not found issue consistently recently, specifically the one consistent with nick's description here, and tested out this fix and it resolved the issue. I am eager for this to land. |
@bors r+
Yeah, I agree that there's work to be done here. The comment there was probably authored by me (too long ago for it to be in cache, but I recall vaguely). I think this patch is probably fine -- particularly given your testing -- though it may cause some additional builds in edge cases. I think our bootstrap tests should be largely catching any likely regressions, and my own CI timing scripts will catch anything on the slower builders, so I'm hopeful we're OK there. We can revert or further iterate if this causes trouble to some workloads. My personal take has long been that consuming rustdoc as a library, not a binary, is a mistake and we should avoid it. I think the easiest way to do that is to just special-case any of the logic the error index wants behind a -Z flag and use that (rustdoc -Z ...) in combination with moving any generation logic into rustbuild itself. That should be fairly straightforward and will likely eliminate a good chunk of operational pain here. |
📌 Commit 7470592 has been approved by |
☀️ Test successful - checks-actions |
Finished benchmarking commit (dc1f829): comparison url. Summary:
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Footnotes |
Looks like the perf regressions here we just a blip. The run for the next PR shows the exact same test cases improving by the same amount as they regressed here. Going to remove the perf regression label since this isn't actually a regression at all. @rustbot label: -perf-regression |
There were two fixes needed:
top_stage
instead oftop_stage - 1
. There was a long and torturous comment about trying to match rustdoc's version, but it works better without the hard-coding than with (before it gave errors thatlibtest.so
couldn't be found).ci-llvm/lib
is added to LD_LIBRARY_PATH. Previously the error index would be unable to load LLVM for stage0 builds.At some point we should probably have a discussion about how rustdoc stages should be numbered;
confusion between 0/1/2 has come up several times in bootstrap now. cc #92538
Note that this is still broken when using
download-rustc = true
and--stage 1
,but that's really a corner case and should affect almost no one.
--stage {0,2}
work fine with download-rustc.
Fixes #80096.