Skip to content

Revert "checkpatch: remove and use clang-format instead"#10692

Open
tmleman wants to merge 1 commit intothesofproject:mainfrom
tmleman:topic/upstream/pr/revert/checkpatch_remove
Open

Revert "checkpatch: remove and use clang-format instead"#10692
tmleman wants to merge 1 commit intothesofproject:mainfrom
tmleman:topic/upstream/pr/revert/checkpatch_remove

Conversation

@tmleman
Copy link
Copy Markdown
Contributor

@tmleman tmleman commented Apr 9, 2026

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.

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>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 checkpatch in CI (codestyle.yml) using a new checkpatch_list.sh helper to process all PR commits.
  • Adds optional pre-commit and post-commit git hook scripts to run checkpatch.pl locally.
  • Removes the repository .clang-format configuration 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
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
local failures=0
failures=0

Copilot uses AI. Check for mistakes.
local failures=0
while read -r sha ; do
printf '\n -------------- \n\n'
./scripts/checkpatch.pl $@ -g $sha || failures=$((failures+1))
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
./scripts/checkpatch.pl $@ -g $sha || failures=$((failures+1))
./scripts/checkpatch.pl "$@" -g "$sha" || failures=$((failures+1))

Copilot uses AI. Check for mistakes.
Comment on lines +22 to +24
fi

return $failures
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
fi
return $failures
return 1
fi
return 0

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants