-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Disable llvm-bitcode-linker
in the default bootstrap profiles
#126479
Conversation
oh, it's only required for nvptx tests? very good to delete that default then, completely wasted time |
12ba73e
to
64da5e9
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
64da5e9
to
b4df72a
Compare
r? @onur-ozkan |
Wouldn't this cause some (nvptx) tests to fail? |
If yes, then we should configure CI to either enable it for that runner, or skip that test if the linker isn't available. There are a bunch of tests that need special conditions on CI. nvptx tests should be running on a few dedicated jobs, not necessarily everywhere, and not necessarily locally by default. PR CI is green, we'd have to take a look at what happens on an auto build. I'll also try to run the tests locally. |
I'd rather have the tests (that I have never run) fail locally (with an error message telling me what to do) than wait the few seconds every time. |
Yeah, running those tests only if the linker is available would be good solution. |
That kind of already happens, because you explicitly need to compile LLVM with support for nvptx for these tests to not be ignored, IIRC. Unless you have objections, I'd r+ this to check if full CI passes. @bors rollup=never |
I think we do that by default when compiling LLVM Line 99 in 92af831
|
If I run the nvpts UI/assembly tests locally, they are ignored though (even if I enable the LLVM bitcode linker) 🤷♂️ When using LLVM from CI. I'm not sure what causes them to be ignored then. In any case, the linker does not seem to be required to run the "default" test suite, therefore I don't see a reason why it should be enabled and compiled by default. |
Sounds totally broken 😄. Not a blocker for this PR anyway, so let's ship this. @bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (bc3618f): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 671.095s -> 670.967s (-0.02%) |
I don't think that we really need to enable
llvm-bitcode-linker
in the default bootstrap profiles, since it seems that it is only useful for runningnvptx
tests. It should be enabled on CI, which it is, and that should be enough. People can enable it easily locally, if they want.The linker causes occasionally some rebuild issues (#122491, #126464), but more importantly it is just needless work to build it locally.
I kept it enabled for
dist
, because it is distributed as arustup
component (for some reason it's not included inextended
? not sure).Fixes: #126464