checkpatch: remove and use clang-format instead#10619
checkpatch: remove and use clang-format instead#10619lgirdwood merged 1 commit intothesofproject:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the existing checkpatch-based style tooling (including CI and git hooks) and introduces clang-format configuration intended to become the new code-style mechanism for SOF/rimage.
Changes:
- Deleted the rimage-local checkpatch implementation and associated configuration/data files.
- Removed checkpatch-based git hooks and the GitHub Actions checkpatch job.
- Added clang-format configuration files at repo root and under
tools/rimage/.
Reviewed changes
Copilot reviewed 9 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/rimage/scripts/checkpatch.pl | Removes the rimage checkpatch script entirely. |
| tools/rimage/.clang-format | Adds clang-format rules for the rimage tree. |
| tools/rimage/.checkpatch.conf | Removes rimage checkpatch configuration. |
| scripts/sof-pre-commit-hook.sh | Removes pre-commit hook invoking checkpatch. |
| scripts/sof-post-commit-hook.sh | Removes post-commit hook invoking checkpatch. |
| scripts/const_structs.checkpatch | Removes checkpatch data file used by constant-struct checks. |
| .gitignore | Drops ignore pattern for checkpatch-generated camelcase cache. |
| .github/workflows/codestyle.yml | Removes the checkpatch CI job (leaves yamllint). |
| .github/workflows/checkpatch_list.sh | Removes helper script used by the checkpatch CI job. |
| .github/workflows/SPDX-README.md | Rewords guidance text to be less checkpatch-centric. |
| .clang-format | Adds repo-root clang-format rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| # yamllint disable-line rule:truthy | ||
| on: [pull_request, workflow_call, workflow_dispatch] | ||
|
|
||
| jobs: | ||
| checkpatch: | ||
| runs-on: ubuntu-22.04 | ||
|
|
||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| strictness: [null, --subjective] | ||
|
|
||
| env: | ||
| PR_NUM: ${{github.event.number}} | ||
| # TODO: reduce duplication with scripts/sof-*-commit-hook.sh | ||
| # thanks to either some .conf file or some wrapper script | ||
| CHK_CMD_OPTS: --ignore UNKNOWN_COMMIT_ID --codespell | ||
|
|
||
| steps: | ||
| # depth 2 so: | ||
| # ^1. we can show the Subject of the current target branch tip | ||
| # ^2. we reconnect/graft to the later fetch pull/1234/head, | ||
| - uses: actions/checkout@v4 | ||
| with: {fetch-depth: 2} | ||
|
|
||
| - name: install codespell | ||
| run: sudo apt-get -y install codespell && dpkg -L codespell | grep dict | ||
|
|
||
| # See shallowness issue https://github.com/thesofproject/linux/issues/2556 | ||
| - name: fetch PR commits | ||
| run: | | ||
| .github/workflows/shallowfetchPRcommits.sh \ | ||
| ${GITHUB_REPOSITORY} "$PR_NUM" | ||
| # show what we got | ||
| git --no-pager log --oneline --graph --decorate --max-count=50 | ||
|
|
||
| - name: checkpatch | ||
| env: | ||
| STRICTNESS: ${{ matrix.strictness }} | ||
| run: .github/workflows/checkpatch_list.sh ${CHK_CMD_OPTS} | ||
| ${STRICTNESS} < PR_SHAs.txt | ||
|
|
||
| jobs: | ||
| yamllint: | ||
| runs-on: ubuntu-latest | ||
| steps: |
There was a problem hiding this comment.
The PR description says checkpatch is removed and clang-format is used instead, but the workflow change removes the checkpatch job without adding any clang-format enforcement. This means CI no longer checks C/C++ formatting at all (only yamllint remains). Add a new GitHub Actions job that runs clang-format in verify mode (e.g., formatting check / diff check) using the committed .clang-format so the intended replacement is actually applied in CI.
There was a problem hiding this comment.
these will be next steps, today agents and developers can align to this and it will be incremental as it can cause a lot of churn.
| # SPDX-License-Identifier: GPL-2.0 | ||
| # | ||
| # clang-format configuration file. Intended for clang-format >= 11. | ||
| # |
There was a problem hiding this comment.
This clang-format configuration appears to be duplicated verbatim in two places (.clang-format and tools/rimage/.clang-format). Keeping identical configs in multiple locations is likely to diverge over time and create inconsistent formatting depending on where clang-format is invoked from. Consider consolidating to a single source of truth (preferably the repo root), or add an explicit comment/guardrail explaining why tools/rimage/ needs its own copy and how it should be kept in sync.
| # | |
| # | |
| # NOTE: This file is intentionally duplicated in tools/rimage/.clang-format. | |
| # Both copies must remain identical. If you change formatting options here, | |
| # make the same change in tools/rimage/.clang-format to keep them in sync. | |
| # |
There was a problem hiding this comment.
yeah, this has now been removed as its from a time when rimage was a git submodule.
| # Documentation/dev-tools/clang-format.rst | ||
| # https://clang.llvm.org/docs/ClangFormat.html | ||
| # https://clang.llvm.org/docs/ClangFormatStyleOptions.html | ||
| # |
There was a problem hiding this comment.
@lgirdwood Should we have a link to the Zephyr version as this is including many definitions from it (and we should probably take in updates from Zephyr if they change converntions)?
tmleman
left a comment
There was a problem hiding this comment.
@lgirdwood maybe it would be worth keeping checkpatch for a transitional period?
Its just garbage output atm that adds noise to CI. |
checkpatch flags too many false positives in the SOF codebase to be considered useful. Notwithstanding it does not cope with assembler, matlab and topology very well. Remove and use clang-format instead which is understood by modern editors and agents for both SOF and rimage. clang-format is based on Linux clang-format with adaptations for SOF and Zephyr. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
|
Checkpatch Freedom for everyone ! |
tmleman
left a comment
There was a problem hiding this comment.
@lgirdwood Code style checking is not garbage! By having this as a required check, we don't have to have discussions in reviews about whether there should be a bracket after if or not. And since this PR is supposed to provide something to replace the removed functionality, it would be nice if it actually worked! Are you able to show me how I can check in a PR that the code complies with coding style? I can't find this anywhere. Not in this one. Not in any other. Additionally, I use checkpatch as a git hook to avoid committing garbage and not waste reviewers' time. How can this replace that? Any instructions? Maybe a hint, anything?
@tmleman I didn't say code style checking was garbage, I did say our checkpatch was garbage. There is negative value in a test that would consistently fail most of the time. No problem if you want to take the Zephyr config checkpatch and upstream that as long as hooks are optional and it can work with topology and hifi. |
checkpatch flags too many false positives in the SOF codebase to be considered useful. Notwithstanding it does not cope with assembler, matlab and topology very well.
Remove and use clang-format instead which is understood by modern editors and agents for both SOF and rimage.
clang-format is based on Linux clang-format with adaptations for SOF and Zephyr.