Skip to content

Commit

Permalink
Make CI Clippy static analysis checks more robust
Browse files Browse the repository at this point in the history
I have identified two potential improvements for how we perform static analysis
on our code in our CI pipeline:

- The `giraffate/clippy-action` we currently use has not been updated to Node
  20, and GitHub has repeatedly indicated that they will phase out actions that
  do not support the latest Node versions. Despite my efforts to help with the
  update by submitting a pull request upstream, it has been ignored for months
  despite its perceived ease of review, raising concerns about the ongoing
  maintenance of the action. This situation suggests we should explore
  alternative methods for integrating Clippy with GitHub's UI.
- As evidenced by PR 632, thoroughly testing Rust crates for every possible
  feature combination is often overlooked due to the tedious nature of the task.
  Our current CI setup only checks two feature combinations, which is far from
  comprehensive.

To address the first improvement, these changes drop `clippy-action` entirely in
favor of utilizing GitHub's native CodeQL SARIF (Static Analysis Results
Interchange Format) file integration. Since Clippy cannot directly output lints
in SARIF, `clippy-sarif` is used to convert Clippy's JSON output to SARIF.
Additionally, `sarif-fmt` is added to turn SARIF into a human-friendly display
format in the workflow run logs.

For the second improvement, let's use `cargo hack` with the `--feature-powerset`
flag to run Clippy for every possible feature combination. This approach strikes
a good balance between CI runtime and thoroughness, as the number of feature
combinations grows superlinearly with the number of features: running `cargo
nextest` for every powerset element would lead to excessively long CI times.
  • Loading branch information
AlexTMjugador committed Jul 10, 2024
1 parent f7b837a commit d9d90ee
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 16 deletions.
39 changes: 27 additions & 12 deletions .github/workflows/oxipng.yml
Original file line number Diff line number Diff line change
Expand Up @@ -102,25 +102,33 @@ jobs:
- name: Install nextest
uses: taiki-e/install-action@nextest

- name: Run rustfmt
- name: Install cargo-hack
if: matrix.target == 'x86_64-unknown-linux-gnu'
run: cargo fmt --check
uses: taiki-e/install-action@cargo-hack

- name: Run Clippy (no default features)
- name: Install clippy-sarif
if: matrix.target == 'x86_64-unknown-linux-gnu'
uses: giraffate/clippy-action@v1
uses: taiki-e/install-action@v2
with:
clippy_flags: --no-deps --all-targets --no-default-features -- -D warnings
reporter: github-check
fail_on_error: true
tool: clippy-sarif

- name: Run Clippy (all features)
- name: Install sarif-fmt
if: matrix.target == 'x86_64-unknown-linux-gnu'
uses: giraffate/clippy-action@v1
uses: taiki-e/install-action@v2
with:
clippy_flags: --no-deps --all-targets --all-features -- -D warnings
reporter: github-check
fail_on_error: true
tool: sarif-fmt

- name: Run rustfmt
if: matrix.target == 'x86_64-unknown-linux-gnu'
run: cargo fmt --check

- name: Run Clippy for all feature combinations
if: matrix.target == 'x86_64-unknown-linux-gnu'
run: >
cargo hack clippy --no-deps --all-targets --feature-powerset --exclude-features sanity-checks --message-format=json -- -D warnings
| clippy-sarif
| tee clippy-results.sarif
| sarif-fmt
- name: Run tests
run: |
Expand All @@ -145,6 +153,13 @@ jobs:
target/${{ env.CARGO_BUILD_TARGET }}/release/oxipng
target/${{ env.CARGO_BUILD_TARGET }}/release/oxipng.exe
- name: Upload analysis results to GitHub
uses: github/codeql-action/upload-sarif@v3
if: always() && matrix.target == 'x86_64-unknown-linux-gnu'
with:
sarif_file: clippy-results.sarif
category: clippy

msrv-check:
name: MSRV check

Expand Down
8 changes: 4 additions & 4 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,10 +329,10 @@ fn parse_opts_into_struct(
if let Some(iterations) = NonZeroU8::new(15) {
opts.deflate = Deflaters::Zopfli { iterations };
}
} else if let Deflaters::Libdeflater { compression } = &mut opts.deflate {
if let Some(x) = matches.get_one::<i64>("compression") {
*compression = *x as u8;
}
} else if let (Deflaters::Libdeflater { compression }, Some(x)) =
(&mut opts.deflate, matches.get_one::<i64>("compression"))
{
*compression = *x as u8;
}

#[cfg(feature = "parallel")]
Expand Down
2 changes: 2 additions & 0 deletions src/rayon.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![allow(dead_code)]

pub mod prelude {
pub use super::*;
}
Expand Down

0 comments on commit d9d90ee

Please sign in to comment.