lintcheck: clippy fixes, test on ci #6829

matthiaskrgr · 2021-03-02T19:52:51Z

fixes clippy warnings in lintcheck and adds a small test (run on 3 small crates) to gha ci

changelog: none

rust-highfive · 2021-03-02T19:52:54Z

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

matthiaskrgr · 2021-03-02T19:53:26Z

r? @camsteffen or @flip1995

flip1995 · 2021-03-02T20:04:25Z

The Clippy Dev workflow hasn't installed rustc-dev-preview.

matthiaskrgr · 2021-03-02T20:16:12Z

Mmh... I removed the step that replaces the rust-toolchain file with the toolchain action because I actually need to build and run clippy during the lintcheck test and it needs a matching (none-"nightly") version.

I don't think I can make the toolchain action use the pinned nightly somehow for installing rustfmt...?

matthiaskrgr · 2021-03-02T20:59:50Z

Ok, I think this works now:

We first remove the rust-toolchain file and grab an independent rustfmt version to check the formatting.
Then we restore the toolchain file via git and build clippy with the pinned nightly version (as part of the test, running lintcheck afterwards)

matthiaskrgr · 2021-03-02T21:09:08Z

@bors try

just to be sure...

lintcheck: clippy fixes, test on ci fixes clippy warnings in lintcheck and adds a small test (run on 3 small crates) to gha ci changelog: none

bors · 2021-03-02T21:09:23Z

⌛ Trying commit d2a9917 with merge 676e913...

bors · 2021-03-02T21:21:02Z

☀️ Try build successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Build commit: 676e913 (676e9136093fc83ca6feb368f859ae7cfc7a26b7)

clippy_dev/src/lintcheck.rs

flip1995 · 2021-03-02T22:10:44Z

WDYT about making a new workflow to check lintcheck and only run it, if lintcheck.rs is changed?

matthiaskrgr · 2021-03-02T22:15:20Z

if we only check lintcheck if lintcheck.rs is touched, we won't be able to find out when lintcheck breaks as it did when we had the [workspace]-related changes, which was my main motivation behind the test!

flip1995 · 2021-03-02T22:57:08Z

Ah right, good point. CI changes LGTM then. Changes to lintcheck also seem to resolve almost all Clippy complaints. (I think the pedantic lints would still trigger on it though)

matthiaskrgr · 2021-03-03T09:12:14Z

Didn't see any pedantic warnings with cargo run --bin cargo-clippy --manifest-path=rust-clippy/Cargo.toml -- -Wclippy::pedantic --features lintcheck

flip1995 · 2021-03-03T09:40:20Z

.github/workflows/clippy_dev.yml

+      # Restore the rust-toolchain-file
+      - name: Restore rust-toolchain-file
+        run: git checkout rust-toolchain


I would move this right before the lintcheck test. That way clippy_dev has to be only rebuild once.

Currently it is built three times:

For rustfmt with +nightly

For the other tests with +

For lintcheck --feature lintcheck

flip1995 · 2021-03-03T09:44:14Z

.github/workflows/clippy_dev.yml

+      - name: Test fmt
+        run: cargo dev fmt --check


And this should stay where it currently is, so it can profit from the Build job.

flip1995 · 2021-03-03T17:16:24Z

I just wonder if the lintcheck test should be moved to the Clippy workflow to not have to build clippy again.

flip1995 · 2021-03-03T17:29:31Z

@bors try

(hijacking this PR to fix up the Clippy CI.)

lintcheck: clippy fixes, test on ci fixes clippy warnings in lintcheck and adds a small test (run on 3 small crates) to gha ci changelog: none

bors · 2021-03-03T17:29:39Z

⌛ Trying commit 24111ec with merge 8b67d89...

flip1995 · 2021-03-03T17:47:00Z

Hmm.. I really don't like checking the lintcheck tool in CI. It will be compile-checked by dogfood, once this PR is done and I can continue to work on the dogfood fix. I don't think we should put 3-5min CI time on any workflow to run lintcheck tests though. Thoughts? @matthiaskrgr

flip1995 · 2021-03-03T17:52:24Z

Also please review this commit: e465a8b

IIUC the --fix test only checks that the --fix command is working? In that case it shouldn't matter on which crate we run the test on.

Previously, we removed the rust-toolchain file in ci and that used the toolchain action to install rustfmt and run `cargo dev fmt`. For testing lintcheck however, we need the rust-toolchain file and a pinned nightly version to build clippy. Solution: First remove the rust-toolchain file and check the formatting. Then restore the toolchain file from the git repo and run the other tests

Also -Zunstable-options isn't required anymore

It will be compile-checked by dogfood

flip1995 · 2021-03-04T08:35:40Z

The lintcheck job took 2m52s, which is approximately the same amount of time, that I got yesterday while testing. I'm still not convinced that that is worth it, since it is compile-checked by dogfood anyway.

reformat clippy.ymk and clippy_bors.yml 458cf05

The previous formatting was done according to the examples in https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions. I don't think it makes a difference, so I think reformatting to a yaml style that feels more natural (IMO) should be fine.

matthiaskrgr · 2021-03-04T10:02:58Z

I've split out the clippy lint fixes for lintcheck into #6839, I think there was something waiting on that?

I guess I'll then rework this branch to not test lintcheck on ci, sigh :/

lintcheck: fix clippy warnings split out from #6829 changelog: none

flip1995 · 2021-03-04T11:47:57Z

I think there was something waiting on that?

Yes my dogfood fix, thanks for splitting it out!

I guess I'll then rework this branch to not test lintcheck on ci, sigh :/

Sorry. I don't think 3min CI time is worth for testing lintcheck, since it is a rarely used dev-tool, I don't expect to break often, once it gets a bit more "stable". If I'm proven wrong in this regard, I'm of course happy to include it.

bors · 2021-03-04T11:56:27Z

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

lintcheck: add test (but don't run on ci) This is the rest of #6829 but without adding anything to ci *Please write a short comment explaining your change (or "none" for internal only changes)* changelog: none

Dogfood and CI fixes The CI fix is practically #6829 rebased and squashed into one commit Dogfood fix is a follow up of #6802 r? `@matthiaskrgr` for lintcheck changes (best reviewed with whitespace changes hidden) changelog: none

rust-highfive assigned Manishearth Mar 2, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 2, 2021

rust-highfive assigned camsteffen and unassigned Manishearth Mar 2, 2021

matthiaskrgr force-pushed the lintcheck_unclippy branch 3 times, most recently from 70176c4 to d2a9917 Compare March 2, 2021 20:58

bors added a commit that referenced this pull request Mar 2, 2021

Auto merge of #6829 - matthiaskrgr:lintcheck_unclippy, r=<try>

676e913

lintcheck: clippy fixes, test on ci fixes clippy warnings in lintcheck and adds a small test (run on 3 small crates) to gha ci changelog: none

camsteffen reviewed Mar 2, 2021

View reviewed changes

clippy_dev/src/lintcheck.rs Outdated Show resolved Hide resolved

flip1995 reviewed Mar 3, 2021

View reviewed changes

bors added a commit that referenced this pull request Mar 3, 2021

Auto merge of #6829 - matthiaskrgr:lintcheck_unclippy, r=<try>

8b67d89

lintcheck: clippy fixes, test on ci fixes clippy warnings in lintcheck and adds a small test (run on 3 small crates) to gha ci changelog: none

flip1995 force-pushed the lintcheck_unclippy branch from aa7f1b2 to ac13b98 Compare March 3, 2021 17:51

matthiaskrgr and others added 14 commits March 4, 2021 08:52

lintcheck: fix clippy warnings

70fe1ee

lintcheck: add small test that runs lintcheck on 3 crates

0259d69

ci: clippy_dev.yml: formatting

47be9e2

ci: build and test lintcheck

d76de0b

lintcheck: clean up unwrap-handling a bit

088befd

lintcheck: update logs

5b0707b

CI: Move fmt test to its previous place

9f6f05e

fixup! CI: Move fmt test to its previous place

87ee4e0

ci: Don't run the --fix test on all of Clippy

76f8a7c

Also -Zunstable-options isn't required anymore

Don't test lintcheck

51df170

It will be compile-checked by dogfood

ci: --fix still requires -Zunstable-options

fc40d1b

reformat clippy.ymk and clippy_bors.yml

458cf05

ci: add lintcheck test execution to clippy_bors.yml and clippy.yml

e0dbb9f

matthiaskrgr force-pushed the lintcheck_unclippy branch from c1b7712 to e0dbb9f Compare March 4, 2021 07:53

matthiaskrgr mentioned this pull request Mar 4, 2021

lintcheck: fix clippy warnings #6839

Merged

bors added a commit that referenced this pull request Mar 4, 2021

Auto merge of #6839 - matthiaskrgr:lintcheck_clippy_warnings, r=flip1995

d783e3d

lintcheck: fix clippy warnings split out from #6829 changelog: none

matthiaskrgr added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 4, 2021

matthiaskrgr mentioned this pull request Mar 4, 2021

lintcheck: add test (but don't run on ci) #6846

Merged

matthiaskrgr closed this Mar 5, 2021

flip1995 mentioned this pull request Mar 5, 2021

Dogfood and CI fixes #6849

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lintcheck: clippy fixes, test on ci #6829

lintcheck: clippy fixes, test on ci #6829

matthiaskrgr commented Mar 2, 2021

rust-highfive commented Mar 2, 2021

matthiaskrgr commented Mar 2, 2021

flip1995 commented Mar 2, 2021

matthiaskrgr commented Mar 2, 2021 •

edited

Loading

matthiaskrgr commented Mar 2, 2021 •

edited

Loading

matthiaskrgr commented Mar 2, 2021

bors commented Mar 2, 2021

bors commented Mar 2, 2021

flip1995 commented Mar 2, 2021

matthiaskrgr commented Mar 2, 2021

flip1995 commented Mar 2, 2021

matthiaskrgr commented Mar 3, 2021

flip1995 Mar 3, 2021

flip1995 Mar 3, 2021

flip1995 commented Mar 3, 2021

flip1995 commented Mar 3, 2021

bors commented Mar 3, 2021

flip1995 commented Mar 3, 2021

flip1995 commented Mar 3, 2021

flip1995 commented Mar 4, 2021

matthiaskrgr commented Mar 4, 2021

flip1995 commented Mar 4, 2021

bors commented Mar 4, 2021

lintcheck: clippy fixes, test on ci #6829

lintcheck: clippy fixes, test on ci #6829

Conversation

matthiaskrgr commented Mar 2, 2021

rust-highfive commented Mar 2, 2021

matthiaskrgr commented Mar 2, 2021

flip1995 commented Mar 2, 2021

matthiaskrgr commented Mar 2, 2021 • edited Loading

matthiaskrgr commented Mar 2, 2021 • edited Loading

matthiaskrgr commented Mar 2, 2021

bors commented Mar 2, 2021

bors commented Mar 2, 2021

flip1995 commented Mar 2, 2021

matthiaskrgr commented Mar 2, 2021

flip1995 commented Mar 2, 2021

matthiaskrgr commented Mar 3, 2021

flip1995 Mar 3, 2021

Choose a reason for hiding this comment

flip1995 Mar 3, 2021

Choose a reason for hiding this comment

flip1995 commented Mar 3, 2021

flip1995 commented Mar 3, 2021

bors commented Mar 3, 2021

flip1995 commented Mar 3, 2021

flip1995 commented Mar 3, 2021

flip1995 commented Mar 4, 2021

matthiaskrgr commented Mar 4, 2021

flip1995 commented Mar 4, 2021

bors commented Mar 4, 2021

matthiaskrgr commented Mar 2, 2021 •

edited

Loading

matthiaskrgr commented Mar 2, 2021 •

edited

Loading