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.

Sign up for GitHub

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

Jump to bottom

Run tidy in its own job in PR CI #106048

Merged
merged 1 commit into from
Dec 23, 2022
Merged

Run tidy in its own job in PR CI #106048

merged 1 commit into from
Dec 23, 2022

Conversation

fee1-dead
Copy link
Member

@fee1-dead fee1-dead commented Dec 22, 2022 •
edited
Loading

This duplicates mingw-check into two jobs where one job runs tidy only while the other job does not. The tidy job will not cancel other jobs on failure.

@rustbot
Copy link
Collaborator

rustbot commented Dec 22, 2022

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 22, 2022
@fee1-dead fee1-dead force-pushed the tidy-ci-continuation branch 2 times, most recently from ec699a7 to 4f0db7e Compare December 22, 2022 15:57
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc 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. labels Dec 22, 2022
@fee1-dead fee1-dead force-pushed the tidy-ci-continuation branch 4 times, most recently from 01134b0 to bc5cb62 Compare December 22, 2022 16:22
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@fee1-dead
Copy link
Member Author

The test run: https://github.com/rust-lang/rust/actions/runs/3759413755 shows that this is working correctly

@fee1-dead fee1-dead marked this pull request as ready for review December 22, 2022 17:05
@jyn514
Copy link
Member

jyn514 commented Dec 22, 2022

This also adds another job to the matrix called mingw-check-tidy that only runs tidy.

Why? Don't we cancel all the jobs if any single job fails?

@jyn514
Copy link
Member

jyn514 commented Dec 22, 2022

Ah, I see you added continue-on-error. Ok, that seems reasonable to me.

I don't think modifying tidy for this is a good idea, though - you can use x test --exclude tidy to avoid running it in the first place on the other builders.

r? @jyn514

@rustbot rustbot assigned jyn514 and unassigned Mark-Simulacrum Dec 22, 2022
@fee1-dead fee1-dead force-pushed the tidy-ci-continuation branch 2 times, most recently from 27da218 to 91bec64 Compare December 22, 2022 17:15
@fee1-dead fee1-dead removed the A-testsuite Area: The testsuite used to check the correctness of rustc label Dec 22, 2022
@rustbot rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Dec 22, 2022
@fee1-dead
Copy link
Member Author

fee1-dead commented Dec 22, 2022 •
edited
Loading

Hmm. I've changed it to run tidy only if RUN_TIDY is set. But that just means that the tidy job will run everything else as well as tidy.

EDIT: let's see if this will work in docker.

@jyn514
Copy link
Member

jyn514 commented Dec 22, 2022

That seems strange to me? I think reusing the mingw-check dockerfile for a new job is kinda weird, if you look at 2314f3b it adds a new dockerfile instead. If you do that I think you should be able to avoid any new env variables, and as a bonus you won't need to install the node+python dependencies.

jyn514
jyn514 approved these changes Dec 22, 2022
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks! r=me when you remove the test commit :)

@jyn514 jyn514 changed the title Make tidy continue on error in CI Run tidy in its own job in PR CI Dec 22, 2022
@fee1-dead
Run tidy in its own job in PR CI
4566db3 
This duplicates mingw-check into two jobs where one job
runs `tidy` only while the other job does not. The tidy
job will not cancel other jobs on failure.
@fee1-dead
Copy link
Member Author

@bors r=jyn514

@bors
Copy link
Contributor

bors commented Dec 22, 2022

📌 Commit 4566db3 has been approved by jyn514

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 Dec 22, 2022
Noratrieb added a commit to Noratrieb/rust that referenced this pull request Dec 23, 2022
@Noratrieb
Rollup merge of rust-lang#106048 - fee1-dead-contrib:tidy-ci-continua…
caded18 
…tion, r=jyn514

Run `tidy` in its own job in PR CI

This duplicates mingw-check into two jobs where one job runs `tidy` only while the other job does not. The tidy job will not cancel other jobs on failure.
@Noratrieb Noratrieb mentioned this pull request Dec 23, 2022
Closed
@Noratrieb Noratrieb mentioned this pull request Dec 23, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 23, 2022
@bors
Auto merge of rust-lang#106087 - Nilstrieb:rollup-2m3nies, r=Nilstrieb
af3e06f 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#105661 (implement the skeleton of the updated trait solver)
 - rust-lang#105853 (Make the pre-push script work on directories with spaces)
 - rust-lang#106043 (Move tests)
 - rust-lang#106048 (Run `tidy` in its own job in PR CI)
 - rust-lang#106055 (Check arg expressions properly on error in `confirm_builtin_call`)
 - rust-lang#106067 (A few metadata nits)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9192874 into rust-lang:master Dec 23, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 23, 2022
@fee1-dead fee1-dead deleted the tidy-ci-continuation branch December 25, 2022 06:57
@jyn514 jyn514 mentioned this pull request Dec 31, 2022
Merged
Noratrieb added a commit to Noratrieb/rust that referenced this pull request Dec 31, 2022
@Noratrieb
Rollup merge of rust-lang#106315 - jyn514:cleanup-mingw-tidy, r=fee1-…
0f50014 
…dead

Cleanup `mingw-tidy` docker job

Fixes a couple small regressions from rust-lang#106048 and rust-lang#105714.

- Avoid `/checkout/src/ci/run.sh: line 187: [: =: unary operator expected`: https://github.com/rust-lang/rust/actions/runs/3809902408/jobs/6481611301#step:26:1701
- Avoid running `x check` in the tidy test, to get faster feedback. It's already run on the normal `mingw-check` job.

r? `@fee1-dead`
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jan 2, 2023
@compiler-errors
Rollup merge of rust-lang#106315 - jyn514:cleanup-mingw-tidy, r=fee1-…
0670a61 
…dead

Cleanup `mingw-tidy` docker job

Fixes a couple small regressions from rust-lang#106048 and rust-lang#105714.

- Avoid `/checkout/src/ci/run.sh: line 187: [: =: unary operator expected`: https://github.com/rust-lang/rust/actions/runs/3809902408/jobs/6481611301#step:26:1701
- Avoid running `x check` in the tidy test, to get faster feedback. It's already run on the normal `mingw-check` job.

r? `@fee1-dead`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 approved these changes

Assignees

@jyn514 jyn514

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc 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
Milestone
1.68.0
Development

Successfully merging this pull request may close these issues.
6 participants
@fee1-dead @rustbot @rust-log-analyzer @jyn514 @bors @Mark-Simulacrum