ci: add GitHub Actions for fmt, clippy, and test checks#44
Conversation
Three separate workflows so each appears as its own status check on PRs: - Format: cargo fmt --all -- --check - Clippy: cargo clippy --all-targets -- -D warnings - Test: cargo test --workspace Uses rust-cache for faster CI runs on clippy and test.
| with: | ||
| components: clippy | ||
| - uses: Swatinem/rust-cache@v2 | ||
| - run: cargo clippy --all-targets -- -D warnings |
There was a problem hiding this comment.
What is the effect of -D warnings? (Unsure of the semantics of deny vs forbid.)
There was a problem hiding this comment.
-D promotes warnings to errors. Still allows overrides.
-F would mean we cant use annotations to override clippy errors such as: #[allow(clippy::too_many_arguments)], as it would treat them as failures also.
jcshepherd
left a comment
jcshepherd
left a comment
There was a problem hiding this comment.
Looks good but would like to understand -D warnings on clippy: specifically, is it CI breaking or not?
| name: Clippy | ||
| on: | ||
| pull_request: | ||
| push: |
There was a problem hiding this comment.
Do these actions happen immediately before the push, or after?
There was a problem hiding this comment.
After. Runs when a PR get created/updated. And runs when we merge to main, but does not rollback if main has failed.
There was a problem hiding this comment.
So before and after? :-) (Before a PR is merged to main, and after.) Cool.
jcshepherd
left a comment
jcshepherd
left a comment
There was a problem hiding this comment.
Okay with me, but I'll let you do the merge once you and Paul are on the same page about CI.
| fmt: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
So this is interesting. I think our current main HEAD (2f65e1b) generates warnings (that block due to -D) in GitGub with this toolchain but does not generate warnings locally on my Linux box which has Rust 1.95.0 installed. Thoughts? Perhaps we want clippy and fmt to use Rust latest but test to use 1.85.0 (the oldest version we advertise support for)?
There was a problem hiding this comment.
huh. Never mind. The latest attempt to run the checks passed. So perhaps something was out of sync somewhere. Merging this before things get out of sync again.
There was a problem hiding this comment.
Clippy was failing because it was missing something locally for me, that the LATEST version caught here. I thought it was better to fix those, and use LATEST as the action, rather than pinning a version that will shoot us in the foot later.
There was a problem hiding this comment.
I'm assuming that this is pinning us to rust 1.85.0, yes?
3 participants
Three separate workflows so each appears as its own status check on PRs:
Uses rust-cache for faster CI runs on clippy and test.