CI: Add rustfmt and cargo clippy to CI #95
Conversation
Signed-off-by: Ismo Puustinen <[email protected]>
Signed-off-by: jiaxiao zhou <[email protected]>
Signed-off-by: jiaxiao zhou <[email protected]>
Signed-off-by: jiaxiao zhou <[email protected]>
.github/workflows/ci.yml
Outdated
| - uses: actions/checkout@v3 | ||
| - uses: actions-rs/toolchain@v1 | ||
| with: | ||
| toolchain: stable |
There was a problem hiding this comment.
This will be a rolling version. It means PR might pass a check when merging but main branch might fail later and need to be updated.
There was a problem hiding this comment.
Can you explain why will the main branch might fail later if the CI passes?
There was a problem hiding this comment.
if this runs on 1.68 and passes, later 1.69 comes out with new clippy rules. Then locally update to 1.69, pull main, run clippy and it will fail.
This can be annoying for contributors as they might have a PR open and rebased on the main branch but now this starts failing when the new tool chain comes out. Either they need to submit a sperate PR and fix then rebase or fix it in same PR to get it in.
If you do release branches this can be a similar issue, where before the fix goes into the branch the formats need to be applied for the new version.
There was a problem hiding this comment.
Ah I see. What do you recommend to mitigate this issue? I think what we can do is that whenever a new release of clippy comes out (together with a new cargo release), we manually submit a PR to fix the main branch.
There was a problem hiding this comment.
IMO we should be pinning the version of Rust that we build against. This creates a little bit more work from a maintenance perspective but it allows more control, sets up for repeatable builds and avoids the pitfalls I mentioned above.
There was a problem hiding this comment.
Done. I've pinned the rust version to 1.69.0
|
I made a PR (Mossaka#2) for amending to this PR for marking all the existing clippy issues as "allowed". We can fix them later and we will get the CI to catch any new clippy issues. |
Mark unfixed clippy warnings as allowed. The idea is that we'll catch new errors in CI and also we can fix the existing warnings whenever we like. Signed-off-by: Ismo Puustinen <[email protected]>
Mark unfixed clippy warnings as allowed
Thanks for your contribution! Merged it in |
|
I will be holding off on this PR until #78 merged in to main |
Signed-off-by: jiaxiao zhou <[email protected]>
Signed-off-by: Ismo Puustinen <[email protected]>
|
Needs a rebase. |
Last clippy fixes.
Signed-off-by: jiaxiao zhou <[email protected]>
Signed-off-by: jiaxiao zhou <[email protected]>
Signed-off-by: jiaxiao zhou <[email protected]>
Signed-off-by: jiaxiao zhou <[email protected]>
|
Rebased, PTAL @cpuguy83 |
|
LGTM |
Signed-off-by: jiaxiao zhou <[email protected]>
Signed-off-by: jiaxiao zhou <[email protected]>
|
Looks like a fmt error:
|
Signed-off-by: jiaxiao zhou <[email protected]>
|
Alright, should be passing now as cargo clippy passed on my local env. |
This PR follows the discussion from #93