google
[ci.yml] Add clippy_check job
To test how this works when a clippy lint is triggered, I tried creating a dummy PR here that should trigger a clippy warning.
Sure! I didn't realize that clippy warning could differ based on toolchain/target.
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).
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
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!
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 https://github.com/google/zerocopy/pull/51#issuecomment-1279193054 makes the most sense.
Oh, I see! I didn't understand this limitation existed.. Trying now the changes you suggested.
So, it seems like some clippy errors are triggered when the "alloc" features is enabled!
Can reproduce locally with:
$ cargo clippy --features "alloc"
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.