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

Test clippy on PR CI on changes #77016

Merged
merged 1 commit into from
Nov 10, 2020
Merged

Test clippy on PR CI on changes #77016

merged 1 commit into from
Nov 10, 2020

Conversation

Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Sep 21, 2020 •
edited
Loading

This runs the tools builder (which builds and tests tools, including clippy) when the clippy submodule changes. This essentially returns us to the prior state when clippy was a submodule; it makes sense for us to test it on CI when it changes. It might make sense for it to be tested regardless of changing but it is somewhat rare for it to fail and we don't want to add to CI time for the majority of PRs which don't affect it.

Fixes #76999.

@rust-highfive
Copy link
Collaborator

r? @pietroalbini

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 21, 2020
@Mark-Simulacrum
Copy link
Member Author

I pulled the 30 second figure from this build (https://github.com/rust-lang-ci/rust/runs/1142917610?check_suite_focus=true) fwiw, but generally ~500 ui tests are going to be pretty fast. (For comparison, regular UI tests for rustc are currently at around 11,000).

@jyn514 jyn514 added A-testsuite Area: The testsuite used to check the correctness of rustc T-dev-tools Relevant to the dev-tools subteam, 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 Sep 21, 2020
@pietroalbini
Copy link
Member

Note that this went from 35 minutes to 40 minutes. I haven't looked into the detailed timings.

@Mark-Simulacrum
Copy link
Member Author

I've seen natural variability of +/- 3-4 minutes on past PRs, but it does look like clippy tests took 2m40s on that PR run -- I guess I looked at the wrong thing or something (maybe excluded clippy build? Not sure).

@Mark-Simulacrum Mark-Simulacrum removed the T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. label Sep 21, 2020
@Mark-Simulacrum
Copy link
Member Author

I don't know that we have a process for approving these -- without capacity figures it's hard to tell. I'd say it's worth it personally.

@pietroalbini
Copy link
Member

Let's nominate infra on this.

@rustbot modify labels: I-nominated

@Mark-Simulacrum
Copy link
Member Author

@rust-lang/clippy @RalfJung -- do we know how common it is for these tests to start failing in PR CI? We were discussing this in the infra team meeting and @pietroalbini brought up the good point that if we're slowing down PR CI by 5 minutes (15%) that's not nothing. If we expect these failures to be really rare, then it probably makes sense to not do this.

(It also looks like there's not a heuristic we can use to determine if running clippy tests makes sense, right? e.g. a file that's usually touched if clippy is to fail, or something in the diff)

@Manishearth
Copy link
Member

do we know how common it is for these tests to start failing in PR CI?

Major compiler changes typically cause failures. We used to have a lot more, stuff has quieted down now but it still happens.

@Manishearth
Copy link
Member

So, clippy's tests should not take that long to run. The slow thing is the dogfood clippy run -- running clippy on clippy. Perhaps that test can be excluded from CI? It's not a huge deal if that fails.

@Mark-Simulacrum
Copy link
Member Author

Hm, so we'd pass whatever the libtest ignore is for dogfood? I could hook that up, though it'd be a bit painful to do it just on the PR builder. We probably want it in mainline CI though right?

@Manishearth
Copy link
Member

Yeah ideally clippy CI doesn't differ between rust-clippy and rust.

@flip1995
Copy link
Member

Currently only the UI-tests and UI-toml tests are run in the rust repo. UI-cargo, dogfood and fmt tests are excluded in the Rust CI. Clippy also takes 2-3min to build, so the additional time probably comes from the build.

A simple heuristic that may improve the situation a bit is to at least run Clippy tests, if files in src/tools/clippy are modified. This doesn't help with internals changes, but when Clippy files are changed, we can be sure that we have to test Clippy. Also if a Clippy breakage is detected by bors and Clippy files are changed after that, we don't need to run bors over and over again, just to test Clippy.

Clippy currently needs about 1-3 syncs per week -> 1-3 PRs break Clippy per week. So I don't think always running Clippy tests is necessary.

@pietroalbini
Copy link
Member

Hmm, with so few PRs being affected by this I don't think it's worth slowing CI down by 5 minutes.

If the x86_64-gnu-tools builder runs Clippy tests (and I'd expect it to) we could enable it when the src/tools/clippy directory is changed. How does that sound?

@flip1995
Copy link
Member

flip1995 commented Sep 24, 2020 •
edited
Loading

If the x86_64-gnu-tools builder runs Clippy tests (and I'd expect it to) we could enable it when the src/tools/clippy directory is changed. How does that sound?

Yes it does, but currently only on bors runs and then for all tools, IIUC. But if it's possible to filter the tested tools in this builder, that SGTM. Or more generally for the future: if a git subtree tool is changed, the x86_64-gnu-tools builder should test that tool in the PR CI.

@pietroalbini
Copy link
Member

Sounds good! Then, I think this PR should update src/ci/scripts/should-skip-this.sh to also consider subtree changes in addition to submodule ones.

@camelid camelid 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 16, 2020
@Dylan-DPC-zz
Copy link

@Mark-Simulacrum any updates?

@Mark-Simulacrum Mark-Simulacrum force-pushed the clippy-tests branch 2 times, most recently from 706ac66 to 38771ae Compare November 8, 2020 02:23
@Mark-Simulacrum Mark-Simulacrum changed the title Test clippy on PR builder Test clippy on PR CI on changes Nov 8, 2020
@Mark-Simulacrum
Copy link
Member Author

r? @pietroalbini -- this should be updated to just test clippy when it's been modified

@Mark-Simulacrum Mark-Simulacrum 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 Nov 8, 2020
@pietroalbini
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 9, 2020

📌 Commit d847299 has been approved by pietroalbini

@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 Nov 9, 2020
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Nov 9, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 10, 2020
@bors
Auto merge of rust-lang#78904 - Dylan-DPC:rollup-8d2w3vu, r=Dylan-DPC
cf9cf7c 
Rollup of 12 pull requests

Successful merges:

 - rust-lang#74754 (Add `#[cfg(panic = '...')]`)
 - rust-lang#76468 (Improve lifetime name annotations for closures & async functions)
 - rust-lang#77016 (Test clippy on PR CI on changes)
 - rust-lang#78480 (BTreeMap: fix pointer provenance rules)
 - rust-lang#78502 (Update Chalk to 0.36.0)
 - rust-lang#78513 (Infer the default host target from the host toolchain if possible)
 - rust-lang#78566 (Enable LLVM Polly via llvm-args.)
 - rust-lang#78580 (inliner: Break inlining cycles)
 - rust-lang#78710 (rustc_ast: Do not panic by default when visiting macro calls)
 - rust-lang#78746 (Demote i686-unknown-freebsd to tier 2 compiler target)
 - rust-lang#78830 (fix `super_visit_with` for `Terminator`)
 - rust-lang#78844 (Monomorphize a type argument of size-of operation during codegen)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 391136e into rust-lang:master Nov 10, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 10, 2020
@jkugelman jkugelman mentioned this pull request Oct 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pietroalbini pietroalbini

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-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.49.0
Development

Successfully merging this pull request may close these issues.

PR CI: also run clippy tests
10 participants
@Mark-Simulacrum @rust-highfive @pietroalbini @Manishearth @flip1995 @Dylan-DPC-zz @bors @jyn514 @camelid @rustbot