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

Don't disable codegen tests in PR CI #126173

Merged
merged 1 commit into from
Jun 9, 2024
Merged

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Jun 8, 2024

Fixes #126170.

r? @ghost

@rustbot rustbot added 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-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jun 8, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Jun 8, 2024

Seems like we should be using a different approach for skipping the codegen tests, something akin to a @needs-llvm annotation or something similar, instead of skipping them altogether when the GCC flag is enabled.

@nikic
Copy link
Contributor Author

nikic commented Jun 8, 2024

@Kobzol All codegen tests require LLVM, so the whole suite should get skipped for other backends. Though this already seems to be the case, given that this PR passes CI. Not sure why that condition was added in the first place, but it doesn't seem to be necessary anymore. (I don't think that other codegen backends run "normal" rustc tests at all -- right?)

@workingjubilee
Copy link
Member

Thanks for fixing this!

@Kobzol
Copy link
Contributor

Kobzol commented Jun 8, 2024

All codegen tests require LLVM, so the whole suite should get skipped for other backends. Though this already seems to be the case, given that this PR passes CI. Not sure why that condition was added in the first place, but it doesn't seem to be necessary anymore. (I don't think that other codegen backends run "normal" rustc tests at all -- right?)

I would expect GCC/Cranelift to run at least a subset of the UI test suite, but I'm not sure.

Anyway, since this seems to work, let's ship it, we ideally shouldn't skip these tests.

r? @Kobzol

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Jun 8, 2024

📌 Commit 9cd6c32 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 Jun 8, 2024
@bors
Copy link
Contributor

bors commented Jun 8, 2024

⌛ Testing commit 9cd6c32 with merge 6c4755d...

@bors
Copy link
Contributor

bors commented Jun 9, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 9, 2024
@bors bors merged commit 6c4755d into rust-lang:master Jun 9, 2024
7 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 9, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6c4755d): 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 -3.9%)

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)
-3.9% [-3.9%, -3.9%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -3.9% [-3.9%, -3.9%] 1

Cycles

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

Binary size

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

Bootstrap: missing data
Artifact size: 319.76 MiB -> 319.68 MiB (-0.02%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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-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.

PR CI (x86_64-gnu-llvm-17) does not run codegen tests
6 participants