-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Revert compiletest new-executor, to re-land without download-rustc #140233
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
Conversation
Revert <rust-lang#139998> because the original merge triggered download-rustc, which messes with test metrics and prevents us from properly comparing them before/after the change. The plan is to re-land this PR as-is, combined with a trivial compiler change to avoid download-rustc and get proper test metrics for comparison. This reverts commit be181dd, reversing changes made to 645d0ad.
Some changes occurred in src/tools/compiletest cc @jieyouxu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@bors r+ rollup |
Actually, let's not roll this up either in case it affects test diffs in a rollup (I don't expect that it does, but just in case). Giving this a higher priority to minimize number of merged PRs potentially affected (again, I don't expect differences but just to be extra safe). @bors rollup=never p=5 (revert to previous libtest executor) |
…ouxu Revert compiletest new-executor, to re-land without download-rustc Revert <rust-lang#139998> because the original merge triggered download-rustc, which messes with test metrics and prevents us from properly comparing them before/after the change. The plan is to re-land this PR as-is, combined with a trivial compiler change to avoid download-rustc and get proper test metrics for comparison. This reverts commit be181dd, reversing changes made to 645d0ad. r? ghost
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
@bors retry rust-lld heap corruption |
…ouxu Revert compiletest new-executor, to re-land without download-rustc Revert <rust-lang#139998> because the original merge triggered download-rustc, which messes with test metrics and prevents us from properly comparing them before/after the change. The plan is to re-land this PR as-is, combined with a trivial compiler change to avoid download-rustc and get proper test metrics for comparison. This reverts commit be181dd, reversing changes made to 645d0ad. r? ghost
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
@bors retry (msvc; couldn't create a temp dir) |
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 847e3ee (parent) -> 862156d (this PR) Test differencesShow 33060 test diffsStage 1
Stage 2
(and 16442 additional test diffs) Additionally, 16518 doctest diffs were found. These are ignored, as they are noisy. Job group index Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 862156d6f25ccb0d915d2a0c8ebab520d05e4f72 --output-dir test-dashboard And then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Finished benchmarking commit (862156d): 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)Results (primary -1.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.7%, secondary -0.1%)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: 776.153s -> 777.547s (0.18%) |
compiletest: Re-land using the new non-libtest executor by default This PR re-lands rust-lang#139998, which had the misfortune of triggering download-rustc in its CI jobs, so we didn't get proper test metrics for comparison with the old implementation. So that was PR was reverted in rust-lang#140233, with the intention of re-landing it alongside a dummy compiler change to inhibit download-rustc. --- Original PR description for rust-lang#139998: >The new executor was implemented in rust-lang#139660, but required a manual opt-in. This PR activates the new executor by default, but leaves the old libtest-based executor in place (temporarily) to make reverting easier if something unexpectedly goes horribly wrong. > >Currently the new executor can be explicitly disabled by passing the `-N` flag to compiletest (e.g. `./x test ui -- -N`), but eventually that flag will be removed, alongside the removal of the libtest dependency. The flag is mostly there to make manual comparative testing easier if something does go wrong. > >As before, there *should* be no user-visible difference between the old executor and the new executor. --- r? jieyouxu
Revert #139998 because the original merge triggered download-rustc, which messes with test metrics and prevents us from properly comparing them before/after the change.
The plan is to re-land this PR as-is, combined with a trivial compiler change to avoid download-rustc and get proper test metrics for comparison.
This reverts commit be181dd, reversing changes made to 645d0ad.
r? ghost