From d9d90ee19a9482e3d3b58a37597c227587bd70f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Gonz=C3=A1lez?= Date: Wed, 10 Jul 2024 23:04:27 +0200 Subject: [PATCH 1/3] Make CI Clippy static analysis checks more robust 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. --- .github/workflows/oxipng.yml | 39 +++++++++++++++++++++++++----------- src/main.rs | 8 ++++---- src/rayon.rs | 2 ++ 3 files changed, 33 insertions(+), 16 deletions(-) diff --git a/.github/workflows/oxipng.yml b/.github/workflows/oxipng.yml index 871a8b35..e606ef14 100644 --- a/.github/workflows/oxipng.yml +++ b/.github/workflows/oxipng.yml @@ -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: | @@ -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 diff --git a/src/main.rs b/src/main.rs index 7fe66a26..33b793a7 100644 --- a/src/main.rs +++ b/src/main.rs @@ -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::("compression") { - *compression = *x as u8; - } + } else if let (Deflaters::Libdeflater { compression }, Some(x)) = + (&mut opts.deflate, matches.get_one::("compression")) + { + *compression = *x as u8; } #[cfg(feature = "parallel")] diff --git a/src/rayon.rs b/src/rayon.rs index d53ae024..8f538909 100644 --- a/src/rayon.rs +++ b/src/rayon.rs @@ -1,3 +1,5 @@ +#![allow(dead_code)] + pub mod prelude { pub use super::*; } From 393de6ebf32728c0da2ef938257fa54deba6c6e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Gonz=C3=A1lez?= Date: Wed, 10 Jul 2024 23:41:31 +0200 Subject: [PATCH 2/3] Remove some now unnecessary Clippy lints allowances --- src/rayon.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/rayon.rs b/src/rayon.rs index 8f538909..d7f75fbc 100644 --- a/src/rayon.rs +++ b/src/rayon.rs @@ -72,12 +72,10 @@ where impl ParallelIterator for I {} -#[allow(dead_code)] pub fn join(a: impl FnOnce() -> A, b: impl FnOnce() -> B) -> (A, B) { (a(), b()) } -#[allow(dead_code)] pub fn spawn(a: impl FnOnce() -> A) -> A { a() } From a662c8c4809d65c76ebdbbb73074674aedf03a95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Gonz=C3=A1lez?= Date: Wed, 10 Jul 2024 23:49:48 +0200 Subject: [PATCH 3/3] Do not fail workflow on static analysis results upload failures When Clippy finds lints, it already aborts the workflow. No double failure is needed. --- .github/workflows/oxipng.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/oxipng.yml b/.github/workflows/oxipng.yml index e606ef14..4f20302d 100644 --- a/.github/workflows/oxipng.yml +++ b/.github/workflows/oxipng.yml @@ -156,6 +156,7 @@ jobs: - name: Upload analysis results to GitHub uses: github/codeql-action/upload-sarif@v3 if: always() && matrix.target == 'x86_64-unknown-linux-gnu' + continue-on-error: true with: sarif_file: clippy-results.sarif category: clippy