Run a diff of lintcheck against the merge base for pull requests #10398

Alexendoo · 2023-02-24T18:04:41Z

changelog: none

This is an MVP of sorts, it consists of #9764 + a GitHub action that feeds the output to the job summary. It doesn't yet do anything fancy like --recursive or adding comments to the PR, so you'd have to click through to the action to see the results

Example output of a change (Alexendoo@0be1ab8): https://github.com/Alexendoo/rust-clippy/actions/runs/4264858870#summary-11583333018

r? @flip1995

bors · 2023-03-04T23:53:15Z

☔ The latest upstream changes (presumably #10445) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2023-03-09T17:19:48Z

☔ The latest upstream changes (presumably #10458) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2023-07-13T13:15:20Z

☔ The latest upstream changes (presumably #11095) made this pull request unmergeable. Please resolve the merge conflicts.

xFrednet · 2024-03-21T15:59:33Z

Hey @flip1995 this is a ping from triage. Would you mind taking a look at this?

If you don't have the time, you can reassign it to me or a random other member by commenting r? clippy.

bors · 2024-04-05T18:25:07Z

☔ The latest upstream changes (presumably #12439) made this pull request unmergeable. Please resolve the merge conflicts.

xFrednet · 2024-06-12T14:39:30Z

r? @xFrednet

xFrednet · 2024-06-14T10:14:07Z

I've started to review this PR. Could you push a dummy commit, that breaks a lint and adds some diffs in the lintcheck step?

Alexendoo · 2024-06-14T14:23:22Z

https://github.com/rust-lang/rust-clippy/actions/runs/9517584950?pr=10398#summary-26236690016

Will have to take a look why those #[must_use] lints showed up as being added & removed

Alexendoo · 2024-06-14T17:51:44Z

It seems to be a GitHub bug? 🤔

The merge commit is correct (8290f72) https://github.com/rust-lang/rust-clippy/actions/runs/9517584950/job/26236510601?pr=10398#step:2:300

But ${{ github.event.pull_request.base.sha }} is resolving to aaade2d (one commit behind master) https://github.com/rust-lang/rust-clippy/actions/runs/9517584950/job/26236510601?pr=10398#step:3:1

Despite the base of 8290f72 (the merge commit) being 3e5a02b (current master)

But on the later runs it has updated to be the current master https://github.com/rust-lang/rust-clippy/actions/runs/9519822264/job/26243848420#step:4:1. Seems like it can be out of sync with the actual CI that's running

I guess we can just use HEAD^ instead so we don't run into this again

Alexendoo · 2024-06-14T18:33:47Z

Fixed summary: https://github.com/rust-lang/rust-clippy/actions/runs/9520445181?pr=10398#summary-26245901042

xFrednet

The changes (besides the dummy error changes) look mostly good to me. I have some questions and comments, but most of those can be pushed into follow-up PRs or addressed here. Thank you for putting this together! :D

.github/workflows/lintcheck.yml

xFrednet · 2024-06-14T09:53:53Z

lintcheck/src/config.rs

+#[derive(Subcommand, Clone, Debug)]
+pub(crate) enum Commands {
+    Diff { old: PathBuf, new: PathBuf },
+}


It feels like lintcheck is becoming more and more a monster that can do everything related to lint checking. I think this makes sense, especially if we might consider extracting lintcheck to be used outside of Clippy as well. We should consider merging lintcheck/src/popular-crates.rs into this as well.

This is a followup though, nothing that should be done in this PR

.github/workflows/lintcheck.yml

lintcheck/src/config.rs

lintcheck/src/json.rs

.github/workflows/lintcheck.yml

xFrednet · 2024-06-16T10:02:04Z

LGTM, let's get this prototype merged and see how it goes :D

Roses are red,
CI is green,
lincheck will run,
for everyones fun!

bors · 2024-06-16T10:02:07Z

📌 Commit feb0671 has been approved by xFrednet

It is now in the queue for this repository.

bors · 2024-06-16T10:03:14Z

⌛ Testing commit feb0671 with merge a2c9782...

bors · 2024-06-16T10:13:20Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing a2c9782 to master...

flip1995

A few comments on the workflow. Thanks for getting this through!

.github/workflows/lintcheck.yml

flip1995 · 2024-06-17T08:01:42Z

.github/workflows/lintcheck.yml

+    - name: Create cache key
+      id: key
+      run: echo "key=lintcheck-base-${{ hashfiles('lintcheck/**') }}-$(git rev-parse HEAD)" >> "$GITHUB_OUTPUT"


Using cache here is a bit weird IMO. I think the upload-artifacts action would fit better here? Or am I missing something? https://github.com/actions/upload-artifact

It does do some caching as well as act as a way to share it between steps e.g. here base had a cache hit

I don't know if you can do that with artifacts, mostly I went with cache because I'm already familiar with it

Ah ok. True, base can have cache hits between PRs based on the same master commit.

head however will never have a cache hit, neither will diff. I think the cleaner way would be to just cache base. To share the json files however, it would be cleaner to use {down,up}load-artifacts IMO.

But this is a NIT on the action

Cache lintcheck binary in ci Always trims ~40s off the `diff` job as it no longer needs to install the rust toolchain or compile lintcheck. Saves a further ~20s for the `base`/`head` jobs when the cache is warm It now uses artifacts for restoring the JSON between jobs as per #10398 (comment), cc `@flip1995` The lintcheck changes are to make `./target/debug/lintcheck` work, running `cargo-clippy`/`clippy-driver` directly doesn't work without `LD_LIBRARY_PATH`/etc being set which is currently being done by `cargo run`. By merging the `--recursive` and normal cases to both go via regular `cargo check` we can have Cargo set up the environment for us r? `@xFrednet` changelog: none

rustbot assigned flip1995 Feb 24, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 24, 2023

Alexendoo mentioned this pull request Feb 24, 2023

lintcheck: Add JSON output, diff subcommand #9764

Closed

Alexendoo force-pushed the auto-lintcheck branch from 47ec899 to 74a316f Compare March 5, 2023 17:10

Alexendoo force-pushed the auto-lintcheck branch 3 times, most recently from 454bfa2 to 80879c6 Compare June 4, 2023 18:00

Alexendoo force-pushed the auto-lintcheck branch from 80879c6 to 0840841 Compare July 13, 2023 13:45

rustbot assigned xFrednet and unassigned flip1995 Jun 12, 2024

rust-cloud-vms bot force-pushed the auto-lintcheck branch 2 times, most recently from 383dc1b to 346ce89 Compare June 12, 2024 16:39

rust-cloud-vms bot force-pushed the auto-lintcheck branch from f0fa3a9 to 9129dcb Compare June 14, 2024 17:10

rust-cloud-vms bot force-pushed the auto-lintcheck branch from 9129dcb to da6d8b2 Compare June 14, 2024 18:05

xFrednet reviewed Jun 14, 2024

View reviewed changes

Alexendoo mentioned this pull request Jun 15, 2024

Test PR for GH actions with a merge conflict #12933

Closed

Alexendoo added 2 commits June 15, 2024 13:58

lintcheck: Add JSON output, diff subcommand

477b0c6

Add lintcheck diff github action

feb0671

rust-cloud-vms bot force-pushed the auto-lintcheck branch 2 times, most recently from c8de273 to feb0671 Compare June 15, 2024 14:11

xFrednet approved these changes Jun 16, 2024

View reviewed changes

bors merged commit a2c9782 into rust-lang:master Jun 16, 2024
8 checks passed

flip1995 reviewed Jun 17, 2024

View reviewed changes

Alexendoo deleted the auto-lintcheck branch June 17, 2024 13:32

Alexendoo mentioned this pull request Jun 23, 2024

Cache lintcheck binary in ci #12986

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run a diff of lintcheck against the merge base for pull requests #10398

Run a diff of lintcheck against the merge base for pull requests #10398

Alexendoo commented Feb 24, 2023 •

edited

Loading

bors commented Mar 4, 2023

bors commented Mar 9, 2023

bors commented Jul 13, 2023

xFrednet commented Mar 21, 2024

bors commented Apr 5, 2024

xFrednet commented Jun 12, 2024

xFrednet commented Jun 14, 2024 •

edited

Loading

Alexendoo commented Jun 14, 2024

Alexendoo commented Jun 14, 2024

Alexendoo commented Jun 14, 2024

xFrednet left a comment

xFrednet Jun 14, 2024

xFrednet commented Jun 16, 2024

bors commented Jun 16, 2024

bors commented Jun 16, 2024

bors commented Jun 16, 2024

flip1995 left a comment

flip1995 Jun 17, 2024 •

edited

Loading

Alexendoo Jun 17, 2024

flip1995 Jun 17, 2024

Run a diff of lintcheck against the merge base for pull requests #10398

Run a diff of lintcheck against the merge base for pull requests #10398

Conversation

Alexendoo commented Feb 24, 2023 • edited Loading

bors commented Mar 4, 2023

bors commented Mar 9, 2023

bors commented Jul 13, 2023

xFrednet commented Mar 21, 2024

bors commented Apr 5, 2024

xFrednet commented Jun 12, 2024

xFrednet commented Jun 14, 2024 • edited Loading

Alexendoo commented Jun 14, 2024

Alexendoo commented Jun 14, 2024

Alexendoo commented Jun 14, 2024

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet Jun 14, 2024

Choose a reason for hiding this comment

xFrednet commented Jun 16, 2024

bors commented Jun 16, 2024

bors commented Jun 16, 2024

bors commented Jun 16, 2024

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Alexendoo Jun 17, 2024

Choose a reason for hiding this comment

flip1995 Jun 17, 2024

Choose a reason for hiding this comment

Alexendoo commented Feb 24, 2023 •

edited

Loading

xFrednet commented Jun 14, 2024 •

edited

Loading

flip1995 Jun 17, 2024 •

edited

Loading