Simplify CI LLVM checks in bootstrap #138704
Conversation
rustbot
added
S-waiting-on-review
|
This PR changes how GCC is built. Consider updating src/bootstrap/download-ci-gcc-stamp. This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp. This PR modifies If appropriate, please update |
This comment has been minimized.
This comment has been minimized.
|
Uhh, crap, this breaks the |
This comment has been minimized.
This comment has been minimized.
|
Hmm, a lot of bootstrap tests parse flags to create a config, and the default value for |
|
"if-unchanged" syncs llvm submodule, that's why |
Kobzol
force-pushed
the
ci-llvm-checks
branch
2 times, most recently
from
c349e6b to
59c3417
Compare
rustbot
added
A-testsuite
rustbot
added
S-blocked
This comment has been minimized.
This comment has been minimized.
Before it was using a different set of paths in different call-sites.
…ci-llvm=true` on CI
To avoid working around some code being unused in tests due to it being stubbed out with `#[cfg(test)]`.
Kobzol
removed
the
S-blocked
|
Changes look good but this is one of those PRs where it's hard to find issues without merging it... So let's try that now. @bors r+ rollup=never |
bors
added
the
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (a4a11ac): comparison URL. Overall result: ❌✅ regressions and improvements - no action needed@rustbot label: -perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (secondary 3.7%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary -0.8%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 775.474s -> 775.351s (-0.02%) |
…ozkan Remove unneeded LLVM CI test assertions The `download_ci_llvm` bootstrap test was checking implementation details of the LLVM CI download check, which isn't very useful. It was essentially testing "if function_that_checks_if_llvm_ci_is_available returns true, we enable CI LLVM", but the usage of the function was an implementation detail. After rust-lang#138704, the inner implementation has changed, so the test now breaks if LLVM is updated. I don't think that it's very useful to test implementation details like this, without taking the outside git state into account. Ideally, we should mock the git state for the test, otherwise the test will randomly break when executed in environments which the test does not control (e.g. on CI when a LLVM change happens). I only kept the part of the test that checks that LLVM CI isn't used when we specify `download-ci-llvm = false`, as that should hold under all conditions, CI/local, and all git states. I also kept the `if-unchanged` assertion, but only on CI, and as a temporary measure. After rust-lang#138591, we should have a proper way of mocking the git state to make the test robust, and make it test what we actually want. Fixes [this](rust-lang#138784 (comment)). r? `@ghost`
Rollup merge of rust-lang#139015 - Kobzol:llvm-ci-test-fixes, r=onur-ozkan Remove unneeded LLVM CI test assertions The `download_ci_llvm` bootstrap test was checking implementation details of the LLVM CI download check, which isn't very useful. It was essentially testing "if function_that_checks_if_llvm_ci_is_available returns true, we enable CI LLVM", but the usage of the function was an implementation detail. After rust-lang#138704, the inner implementation has changed, so the test now breaks if LLVM is updated. I don't think that it's very useful to test implementation details like this, without taking the outside git state into account. Ideally, we should mock the git state for the test, otherwise the test will randomly break when executed in environments which the test does not control (e.g. on CI when a LLVM change happens). I only kept the part of the test that checks that LLVM CI isn't used when we specify `download-ci-llvm = false`, as that should hold under all conditions, CI/local, and all git states. I also kept the `if-unchanged` assertion, but only on CI, and as a temporary measure. After rust-lang#138591, we should have a proper way of mocking the git state to make the test robust, and make it test what we actually want. Fixes [this](rust-lang#138784 (comment)). r? `@ghost`
Extracted out of #138591. Apart from simplifying the checks, it will make it easier to run E2E tests of bootstrap on a mostly empty directory without checking out LLVM on CI :)
The commits should be mostly self-explanatory.
r? @onur-ozkan