Revert "checkpatch: remove and use clang-format instead"#10692
Revert "checkpatch: remove and use clang-format instead"#10692tmleman wants to merge 1 commit intothesofproject:mainfrom
Conversation
The reverted commit intended to replace the old and no longer maintained checkpatch with clang-format. While I agree with this direction, the implementation was problematic. The script was completely removed without providing an alternative, leaving no verification for introduced code style. Additionally, the clang-format configuration file that was added is not being used anywhere. Furthermore, checkpatch performed two critical functions: it verified both code quality and commit message format (including signoff requirements). This second aspect was completely overlooked. As a result, commits lacking proper signoff have already been merged. Zephyr provides a better example of how this should be handled: checkpatch was removed from GitHub workflows but remains available in the repository for voluntary developer use. In parallel, CI enforces coding style and commit quality through other dedicated tools. This reverts commit aadcea2. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
There was a problem hiding this comment.
Pull request overview
Reverts the prior change that removed checkpatch in favor of clang-format, restoring checkpatch-based style and commit-quality verification (including signoff checks) via CI and optional developer hooks.
Changes:
- Reintroduces
checkpatchin CI (codestyle.yml) using a newcheckpatch_list.shhelper to process all PR commits. - Adds optional pre-commit and post-commit git hook scripts to run
checkpatch.pllocally. - Removes the repository
.clang-formatconfiguration and adjusts related documentation/ignores.
Reviewed changes
Copilot reviewed 8 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/codestyle.yml |
Adds a checkpatch job to enforce style/commit checks in CI. |
.github/workflows/checkpatch_list.sh |
New helper script to run checkpatch.pl across a list of PR commit SHAs. |
.github/workflows/SPDX-README.md |
Updates guidance text to reference checkpatch SPDX warnings. |
scripts/sof-pre-commit-hook.sh |
New optional hook to run checkpatch on staged diffs. |
scripts/sof-post-commit-hook.sh |
New optional hook to run checkpatch on the latest commit email format. |
scripts/const_structs.checkpatch |
Adds SOF-specific const-struct name allowlist for checkpatch. |
tools/rimage/.checkpatch.conf |
Adds checkpatch configuration for the tools/rimage subtree. |
.gitignore |
Ignores checkpatch camelcase cache artifacts. |
.clang-format |
Removes the clang-format configuration file added by the reverted commit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # $0 --codespell --strict < PR_SHAs.txt | ||
| main() | ||
| { | ||
| local failures=0 |
There was a problem hiding this comment.
checkpatch_list.sh is declared with #!/bin/sh but uses local inside main(), which is not POSIX and will fail on Ubuntu where /bin/sh is dash. Either switch the shebang to bash or remove local usage to keep the script portable.
| local failures=0 | |
| failures=0 |
| local failures=0 | ||
| while read -r sha ; do | ||
| printf '\n -------------- \n\n' | ||
| ./scripts/checkpatch.pl $@ -g $sha || failures=$((failures+1)) |
There was a problem hiding this comment.
Arguments are expanded unquoted in the checkpatch invocation ($@ and $sha). This can lead to word-splitting/globbing issues; use quoted expansions (e.g. preserve each arg) and quote the SHA variable.
| ./scripts/checkpatch.pl $@ -g $sha || failures=$((failures+1)) | |
| ./scripts/checkpatch.pl "$@" -g "$sha" || failures=$((failures+1)) |
| fi | ||
|
|
||
| return $failures |
There was a problem hiding this comment.
The script returns the number of failures as the process exit status. Shell exit codes are limited to 0–255, so large PRs could overflow/wrap and potentially misreport success/failure. Consider exiting with 1 when any failure occurred (and 0 otherwise), while still printing the full summary.
| fi | |
| return $failures | |
| return 1 | |
| fi | |
| return 0 |
The reverted commit intended to replace the old and no longer maintained checkpatch with clang-format.
While I agree with this direction, the implementation was problematic. The script was completely removed without providing an alternative, leaving no verification for introduced code style. Additionally, the clang-format configuration file that was added is not being used anywhere. Furthermore, checkpatch performed two critical functions: it verified both code quality and commit message format (including signoff requirements). This second aspect was completely overlooked. As a result, commits lacking proper signoff have already been merged.
Zephyr provides a better example of how this should be handled: checkpatch was removed from GitHub workflows but remains available in the repository for voluntary developer use. In parallel, CI enforces coding style and commit quality through other dedicated tools.
This reverts commit aadcea2.