shssoichiro / oxipng

Multithreaded PNG optimizer written in Rust
MIT License
2.95k stars 125 forks source link

Make CI Clippy static analysis checks more robust #635

Closed AlexTMjugador closed 4 months ago

AlexTMjugador commented 4 months ago

I have identified two potential improvements for how we perform static analysis on our code in our CI pipeline:

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.

While at it, I have fixed the Clippy lints that were catched by the more exhaustive checks.

github-advanced-security[bot] commented 4 months ago

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

AlexTMjugador commented 4 months ago

The bot comment above is expected due to the usage of GitHub's CodeQL SARIF integration. Please heed it accordingly before this PR is merged.

AlexTMjugador commented 4 months ago

[edit] Actually I may be wrong about this, it might only be available for teams and enterprises...

Indeed, that's sadly the case at the moment. However, in the blog post I link to GitHub said that it expects to "begin offering Arm runners for open source and personal accounts by the end of the year", so if all goes according to plan we can get rid of QEMU soon enough :smile:

andrews05 commented 4 months ago

Yeah, sounds good! Let's go ahead and merge this. We've had a few bug fixes, do you think we should do a 9.1.2 release?

AlexTMjugador commented 4 months ago

We've had a few bug fixes, do you think we should do a 9.1.2 release?

Yeah, I deem the current mainline stable enough and those bugfixes may indeed come in handy. Let's do it!