Skip to content
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

Run the full stage 2 run-make test suite in x86_64-gnu-debug #131917

Merged
merged 6 commits into from
Oct 25, 2024

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Oct 19, 2024

Run the full run-make test suite in the x86_64-gnu-debug CI job. This is currently the only CI job where //@ needs-force-clang-based-test will be satisfied, so some run-make tests will literally never be run otherwise. Before this PR, the CI job only ran run-make tests which contains the substring clang in its test name, which is both (1) a footgun because it's very easy to forget and (2) it masks tests that would otherwise fail (even failing to compile) because the test is skipped if doesn't have a clang in its test name.

With the environment of x86_64-gnu-debug, two run-make tests failed before this PR:

  1. tests/run-make/issue-84395-lto-embed-bitcode/rmake.rs: this was broken for a long time because objcopy in llvm bin tools was renamed to llvm-objcopy. This test was converted into a rmake.rs test, rather straight forward.
  2. tests/run-make/cross-lang-lto-riscv-abi/rmake.rs: this was broken for a long time and never worked. The old version inspected human-readable output of llvm-readobj --file-header looking for substring EF_RISCV_FLOAT_ABI_DOUBLE, but the human-readable output will only contain something like Flags: 0x5, RVC, double-float ABI, hence it will never match. This test was fixed by instead using the object crate to actually decode the ELF headers looking for the specific e_flags based on reading the RISCV ELF psABI docs.

This PR is best reviewed commit-by-commit, two commits setup the support library for functionality and two commits are for each of the failing run-make tests.

I had to bump the x86_64-gnu-debug job to be ran with a runner with larger disk space.

Part of #132034.

try-job: x86_64-gnu-debug

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Oct 19, 2024
@jieyouxu

This comment was marked as outdated.

@bors

This comment was marked as outdated.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 19, 2024
[WIP] CI: try to drop `--test-args=clang` in `x86_64-gnu-debug`

Ok what if I open a separate PR...

r? `@ghost`

try-job: x86_64-gnu-debug
@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as outdated.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2024
@jieyouxu

This comment was marked as outdated.

@bors

This comment was marked as outdated.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 20, 2024
[WIP] CI: try to drop `--test-args=clang` in `x86_64-gnu-debug`

Ok what if I open a separate PR...

r? `@ghost`

try-job: x86_64-gnu-debug
@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as outdated.

@jieyouxu

This comment was marked as outdated.

@bors

This comment was marked as outdated.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 21, 2024
[WIP] CI: try to drop `--test-args=clang` in `x86_64-gnu-debug`

Ok what if I open a separate PR...

r? `@ghost`

try-job: x86_64-gnu-debug
@jieyouxu

This comment was marked as outdated.

@rust-log-analyzer

This comment has been minimized.

@jieyouxu

This comment was marked as outdated.

@bors

This comment was marked as outdated.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 21, 2024
[WIP] CI: try to drop `--test-args=clang` in `x86_64-gnu-debug`

Ok what if I open a separate PR...

r? `@ghost`

try-job: x86_64-gnu-debug
@rust-log-analyzer

This comment has been minimized.

Full stage 2 build + run-make with full debug info seems to overwhelm
the standard 4c runner's storage capacity.
@jieyouxu
Copy link
Member Author

What if we downloaded more ram disk space
@bors try

@bors
Copy link
Contributor

bors commented Oct 22, 2024

⌛ Trying commit 03c7f99 with merge 7b157b7...

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 22, 2024
Run the full stage 2 `run-make` test suite in `x86_64-gnu-debug`

Run the full `run-make` test suite in the `x86_64-gnu-debug` CI job. This is currently the *only* CI job where `//@ needs-force-clang-based-test` will be satisfied, so some `run-make` tests will literally never be run otherwise. Before this PR, the CI job only ran `run-make` tests which contains the substring `clang` in its test name, which is both (1) a footgun because it's very easy to forget and (2) it masks tests that would otherwise fail (even failing to compile) because the test is skipped if doesn't have a `clang` in its test name.

With the environment of `x86_64-gnu-debug`, two `run-make` tests failed before this PR:

1. `tests/run-make/issue-84395-lto-embed-bitcode/rmake.rs`: this was broken for a long time because `objcopy` in llvm bin tools was renamed to `llvm-objcopy`. This test was converted into a rmake.rs test, rather straight forward.
2. `tests/run-make/cross-lang-lto-riscv-abi/rmake.rs`: this was broken for a long time and never worked. The old version inspected human-readable output of `llvm-readobj --file-header` looking for substring `EF_RISCV_FLOAT_ABI_DOUBLE`, but the human-readable output will only contain something like `Flags: 0x5, RVC, double-float ABI`, hence it will never match. This test was fixed by instead using the `object` crate to actually decode the ELF headers looking for the specific `e_flags` based on reading the RISCV ELF psABI docs.

This PR is best reviewed commit-by-commit, two commits setup the support library for functionality and two commits are for each of the failing `run-make` tests.

try-job: x86_64-gnu-debug
@bors
Copy link
Contributor

bors commented Oct 22, 2024

☀️ Try build successful - checks-actions
Build commit: 7b157b7 (7b157b7bca4b26d4f40a49c7147d9e3ba8e472fe)

@jieyouxu jieyouxu removed the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Oct 22, 2024
@jieyouxu
Copy link
Member Author

try-job passed, so: @rustbot ready

Rolling two reviewers:

r? compiler (for tests and basic support lib changes themselves)
r? infra (for approval to modify runner type)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 22, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Oct 22, 2024

+1 on the runner change. If more disk is needed, we don't really have a lot of other options than to just switch to a larger runner..

@petrochenkov petrochenkov removed their assignment Oct 23, 2024
@jieyouxu
Copy link
Member Author

@Kobzol this doesn't specifically need a compiler reviewer, if you think the tests look reasonable then I think it's fine

@Kobzol
Copy link
Contributor

Kobzol commented Oct 25, 2024

Ok! Nice to see one more makefile test removed.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 25, 2024

📌 Commit 03c7f99 has been approved by Kobzol

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 25, 2024
@bors
Copy link
Contributor

bors commented Oct 25, 2024

⌛ Testing commit 03c7f99 with merge b188577...

@bors
Copy link
Contributor

bors commented Oct 25, 2024

☀️ Test successful - checks-actions
Approved by: Kobzol
Pushing b188577 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 25, 2024
@bors bors merged commit b188577 into rust-lang:master Oct 25, 2024
7 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Oct 25, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b188577): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -0.4%)

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.

mean range count
Regressions ❌
(primary)
1.1% [1.1%, 1.1%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.1% [-1.3%, -0.9%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.4% [-1.3%, 1.1%] 3

Cycles

Results (secondary -2.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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.1% [-2.1%, -2.0%] 4
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 788.765s -> 788.853s (0.01%)
Artifact size: 333.61 MiB -> 333.59 MiB (-0.00%)

@jieyouxu jieyouxu deleted the rmake-clang branch October 26, 2024 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants