Split run-make into two {run-make,run-make-cargo} test suites
#146233
+177
−78
Conversation
This file contains hidden or 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
rustbot
added
A-CI
This comment was marked as resolved.
This comment was marked as resolved.
rust-bors bot
added a commit
that referenced
this pull request
Sep 5, 2025
[EXPERIMENTAL] `run-make` fission try-job: aarch64-msvc-1 try-job: test-various try-job: x86_64-debug try-job: aarch64-gnu-debug try-job: aarch64-apple try-job: dist-various-1
This comment has been minimized.
This comment has been minimized.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
rust-bors bot
added a commit
that referenced
this pull request
Sep 5, 2025
[EXPERIMENTAL] `run-make` fission try-job: aarch64-msvc-1 try-job: test-various try-job: x86_64-gnu-debug try-job: aarch64-gnu-debug try-job: aarch64-apple try-job: dist-various-1
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
bjorn3
reviewed
Sep 5, 2025
…uites So that contributors who don't need to run `run-make` tests that require in-tree `cargo` can run the non-cargo `run-make` tests without having to wait for `cargo` (which would require rebuilding as the build cache would be invalidated by compiler modifications without some kind of `--keep-stage-cargo`).
- `run-make` test suite will now no longer receive a `cargo`.
- NOTE: the user could technically still write
`Command::new("cargo")` which might find *a* cargo from the
environment, but that is not a supported case.
- `run-make-cargo` will receive a built in-tree `cargo`.
jieyouxu
force-pushed
the
run-make-fission
branch
2 times, most recently
from
223d926 to
10771b2
Compare
jieyouxu
force-pushed
the
run-make-fission
branch
from
10771b2 to
f755e64
Compare
|
@bors try |
This comment has been minimized.
This comment has been minimized.
rust-bors bot
added a commit
that referenced
this pull request
Sep 5, 2025
[EXPERIMENTAL] `run-make` fission try-job: aarch64-msvc-1 try-job: test-various try-job: x86_64-gnu-debug try-job: aarch64-gnu-debug try-job: aarch64-apple try-job: dist-various-1
jieyouxu
changed the title
run-make fissionrun-make into two {run-make,run-make-cargo} test suites
jieyouxu
commented
Sep 6, 2025
| @@ -1083,27 +1083,36 @@ where | |||
|
|
|||
| fn test_rustc(env: &Env, args: &TestArg) -> Result<(), String> { | |||
| test_rustc_inner(env, args, |_| Ok(false), false, "run-make")?; | |||
| test_rustc_inner(env, args, |_| Ok(false), false, "run-make-cargo")?; | |||
There was a problem hiding this comment.
cc @antoyo @GuillaumeGomez: could you check if these changes look right?
| let run_make_cargo_result = test_rustc_inner( | ||
| env, | ||
| args, | ||
| retain_files_callback("tests/failing-run-make-tests.txt", "run-make-cargo"), |
There was a problem hiding this comment.
Remark: I thought about splitting out failing-run-make-cargo-tests.txt but that seem like a lot of ceremony, and the ones that were moved into run-make-cargo weren't previously in the list anyway.
| @@ -1773,7 +1779,7 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the | |||
| target, | |||
| }); | |||
| } | |||
| if suite == "run-make" { | |||
| if mode == "run-make" { | |||
There was a problem hiding this comment.
Remark: I almost missed this until I noticed that the snapshot tests for ./x test run-make-cargo didn't have a step for building run-make-support, and I was like "huh, how strange".
| test::RunMake, | ||
| test::RunMakeCargo, |
There was a problem hiding this comment.
Remark: make comment above was outdated.
| ENV WASM_WASIP_SCRIPT python3 /checkout/x.py --stage 2 test --host='' --target $WASM_WASIP_TARGET \ | ||
| tests/run-make \ | ||
| tests/run-make-cargo \ |
There was a problem hiding this comment.
Remark: these CI changes should not cause CI time to regress, because we should be doing approximately the same amount of work as before (i.e. still building in-tree cargo).
|
r? @Kobzol |
rustbot
added
S-waiting-on-review
|
Some changes occurred in compiler/rustc_codegen_gcc Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging. The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes. |
|
Looks great, thank you! @bors r+ rollup=never |
bors
added
S-waiting-on-bors
|
☀️ 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 bea625f (parent) -> 1ed3cd7 (this PR) Test differencesShow 44 test diffsStage 0
Stage 1
Stage 2
Additionally, 2 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 1ed3cd7030718935a5c5e5c8f6581f36d8be179f --output-dir test-dashboardAnd 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 (1ed3cd7): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 2.2%, secondary 3.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -2.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 468.001s -> 465.747s (-0.48%) |
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.
Summary
Split
tests/run-makeinto two test suites, to make it faster and more convenient for contributors to run run-make tests that do not need in-treecargo.tests/run-makecargo. These tests may not usecargo.tests/run-make-cargocargosubmodule and building in-treecargo, and thus will have access to in-treecargo. In practice, these constitute a very small portion of the originalrun-maketests.This PR carries out MCP 847: Split run-make test suite into slower-building test suite with suitably-staged cargo and faster-building test suite without cargo.
Fixes #135573 (for the tests that do not need in-tree
cargo).Fixes #134109.
Remarks
run-makeinto two test suites: fast-path that don't need to build cargo and a slow-path that builds cargo #134109, in practicerustdocis not very slow to build, butcargotakes a good few minutes. So, the partition boundary was determined to be along in-treecargoavailability.run-maketests previously that wanted to usecargocannot just use the bootstrapcargo, otherwise they would run into situations where bootstrapcargocan significantly diverge from in-treecargo(see Pass the current cargo torun-maketests #130642).try-job: aarch64-msvc-1
try-job: test-various
try-job: x86_64-gnu-debug
try-job: aarch64-gnu-debug
try-job: aarch64-apple
try-job: dist-various-1