Skip to content

[ci.yml] Add clippy_check job#51

Merged
joshlf merged 1 commit intogoogle:mainfrom
frazar:add-clippy-ci
Oct 17, 2022
Merged

[ci.yml] Add clippy_check job#51
joshlf merged 1 commit intogoogle:mainfrom
frazar:add-clippy-ci

Conversation

@frazar
Copy link
Contributor

@frazar frazar commented Oct 11, 2022

Add a CI job to run the clippy linting software.

Fixes #49

@frazar
Copy link
Contributor Author

frazar commented Oct 11, 2022

To test how this works when a clippy lint is triggered, I tried creating a dummy PR here that should trigger a clippy warning.

Copy link
Member

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

Thanks for this! Can you move the check to be one of the steps under the existing "Build & Test" matrix? Specifically, I'd suggest adding it just after the existing "Check" step - it should be basically a carbon copy of that step. It'd be good to make sure we get coverage on all toolchain/target combinations.

@frazar
Copy link
Contributor Author

frazar commented Oct 11, 2022

Sure! I didn't realize that clippy warning could differ based on toolchain/target.

@joshlf
Copy link
Member

joshlf commented Oct 11, 2022

Yeah, any code that is conditionally compiled, and possibly even code that isn't but whose behavior differs by platform (e.g., some clippy lints (like this one) make use of the word size on the platform).

@frazar frazar force-pushed the add-clippy-ci branch 2 times, most recently from e567f2c to 2fdcaab Compare October 12, 2022 21:58
@joshlf
Copy link
Member

joshlf commented Oct 14, 2022

I think you can fix the current CI failures by invoking Clippy directly from the command line. It comes default with Rust installations now, so you don't need a separate action for it. All I did here was copy-paste the "Check" action and edit it to invoke clippy instead:

    - name: Check
      run: cargo +${{ matrix.channel }} clippy --target ${{ matrix.target }} --features "${{ matrix.features }}" --verbose

@frazar
Copy link
Contributor Author

frazar commented Oct 15, 2022

Unfortunately, I didn't have much time to experiment with this. Surely your solution would be easier. I thought the GitHub Action could be more convenient since it can also show inline error messages like this:

Would that be desirable for you?

I think I'll have more time to work on this next week. If instead it's a priority for you, feel free to close this PR!

@joshlf joshlf mentioned this pull request Oct 16, 2022
@joshlf
Copy link
Member

joshlf commented Oct 16, 2022

Ah that looks really nice! Unfortunately, it looks like it won't work for people submitting PRs from forked repositories (which is basically everyone other than me). Also, since we're enforcing this (rather than just using Clippy lints as suggestions), hopefully most PRs won't have many if any lints, and so a nice UI isn't that important. I opened #69 to track making it easier to run all of our tests (including Clippy) locally.

Given those considerations, I think #51 (comment) makes the most sense.

@frazar
Copy link
Contributor Author

frazar commented Oct 17, 2022

Oh, I see! I didn't understand this limitation existed.. Trying now the changes you suggested.

@frazar frazar force-pushed the add-clippy-ci branch 3 times, most recently from e70eeb4 to 99c24a6 Compare October 17, 2022 07:13
@frazar
Copy link
Contributor Author

frazar commented Oct 17, 2022

So, it seems like some clippy errors are triggered when the "alloc" features is enabled!

Can reproduce locally with:

$ cargo clippy --features "alloc"

@joshlf
Copy link
Member

joshlf commented Oct 17, 2022

Ooops! I think I fixed the rest in #71 .

@joshlf
Copy link
Member

joshlf commented Oct 17, 2022

Looks like you'll need to push again (and maybe even rebase on main?) to get the tests to run against #71. I am able to trigger the tests to re-run from the UI, but it looks like it's not including #71.

Add a CI job to run the clippy linting software.

Fixes google#49
@frazar frazar requested a review from joshlf October 17, 2022 19:22
@frazar
Copy link
Contributor Author

frazar commented Oct 17, 2022

Rebased and pushed. Looks good now!

Copy link
Member

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

Woo, thanks @frazar !

@joshlf joshlf merged commit e5e9b1f into google:main Oct 17, 2022
@frazar frazar deleted the add-clippy-ci branch October 17, 2022 19:31
joshlf pushed a commit that referenced this pull request Aug 3, 2023
Add a CI test to run the Clippy linter.

Fixes #49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test Clippy in CI

2 participants