-
Notifications
You must be signed in to change notification settings - Fork 821
Run clang-tidy on pull-request changed lines #4982
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| # Copyright (C) 2019 Intel Corporation. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
|
|
||
| # Runs the repository's existing .clang-tidy configuration, but only on the | ||
| # lines a pull request changes. The .clang-tidy config (bugprone-*, cert-*, | ||
| # clang-analyzer-*, ... with WarningsAsErrors) is already maintained in-tree but | ||
| # is not run by any workflow today, so the null-deref / sign-conversion / | ||
| # use-after-move class of bug it catches is currently caught only by a reviewer | ||
| # reading the diff. Diff-scoping (clang-tidy-diff.py, same idea as the | ||
| # git-clang-format-14 gate in coding_guidelines.yml) keeps it low-noise: a PR is | ||
| # only flagged for issues it introduces on the lines it touches. | ||
| # | ||
| # Like coding_guidelines.yml this needs only `contents: read` and computes the | ||
| # diff from the pull_request base SHA, so it behaves identically for pull | ||
| # requests opened within a repository and from forks. | ||
| name: Clang Tidy | ||
|
|
||
| on: | ||
| # PR-only: the gate diffs against the pull-request base, so there is no | ||
| # meaningful base to diff for a manual (workflow_dispatch) run. | ||
| pull_request: | ||
|
|
||
| # Cancel any in-flight run for the same PR/branch so there's only one active. | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| env: | ||
| # Match the clang tooling version pinned by the existing coding-guidelines | ||
| # gate (ci/coding_guidelines_check.py uses clang-format-14). | ||
| LLVM_VER: "14" | ||
|
|
||
| jobs: | ||
| clang_tidy: | ||
| runs-on: ubuntu-22.04 | ||
| timeout-minutes: 30 | ||
| steps: | ||
| - name: checkout | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| # full history so the pull_request base SHA is available for the diff | ||
| fetch-depth: 0 | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It may be worth adding a quick-exit path for PRs that only change documentation, test data, or other non-source files. In those cases, the workflow could skip the clang-tidy step entirely and save GitHub Actions minutes. |
||
| - name: install clang-tidy-${{ env.LLVM_VER }} and build tools | ||
| run: | | ||
| sudo apt-get -qq update | ||
| sudo apt-get install -y -qq \ | ||
| "clang-tidy-${LLVM_VER}" cmake ninja-build build-essential | ||
|
|
||
| - name: fetch clang-tidy-diff.py | ||
| run: | | ||
| # Pinned to the same LLVM major as clang-tidy-14; this helper ships in | ||
| # the llvm-project source tree, not in the Ubuntu clang-tidy package. | ||
| curl -fsSL -o clang-tidy-diff.py \ | ||
| https://raw.githubusercontent.com/llvm/llvm-project/llvmorg-14.0.6/clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py | ||
| chmod +x clang-tidy-diff.py | ||
|
|
||
| - name: generate compile_commands.json | ||
| run: | | ||
| # Default linux iwasm config (interp + AOT runtime + libc), no LLVM | ||
| # required. Covers the highest-traffic loader / interpreter / common | ||
| # sources; building also materializes any generated headers so | ||
| # clang-tidy can parse the changed translation units. | ||
| cmake -S product-mini/platforms/linux -B build \ | ||
| -G Ninja \ | ||
| -DCMAKE_BUILD_TYPE=Debug \ | ||
| -DCMAKE_EXPORT_COMPILE_COMMANDS=ON | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generating compile_commands.json from the default Linux configuration does cover the highest-traffic sources, but it does not guarantee coverage of every file or every changed line in a PR. As a result, this workflow may miss some modified code paths or analyze files without the correct compilation context |
||
| cmake --build build --target iwasm --parallel | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For compile_commands.json, there is no need to |
||
| test -f build/compile_commands.json | ||
|
|
||
| - name: clang-tidy on changed lines | ||
| env: | ||
| BASE_SHA: ${{ github.event.pull_request.base.sha }} | ||
| run: | | ||
| set -o pipefail | ||
| TIDY="$(command -v "clang-tidy-${LLVM_VER}")" | ||
| echo "using: $("$TIDY" --version | head -1)" | ||
|
|
||
| # -U0: exact changed-line ranges. clang-tidy-diff.py turns those into | ||
| # -line-filter, so only lines this PR touches are analyzed. | ||
| if git diff -U0 --no-color "${BASE_SHA}" HEAD -- \ | ||
| '*.c' '*.cc' '*.cpp' '*.h' \ | ||
| | python3 clang-tidy-diff.py \ | ||
| -p1 -path build \ | ||
| -clang-tidy-binary "$TIDY" \ | ||
| -j "$(nproc)" 2>&1 \ | ||
| | tee clang-tidy.log ; then | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When clang-tidy is asked to analyze a changed file that is not present in compile_commands.json , it may fall back to analyzing the file directly without the proper build context. That can produce false failures due to missing macro definitions, wrong conditional-compilation branches, missing include paths, or even diagnostics on unchanged lines in the modified file. |
||
| DIFF_RC=0 | ||
| else | ||
| DIFF_RC=$? | ||
| fi | ||
|
|
||
| if [ "${DIFF_RC}" -ne 0 ] \ | ||
| || grep -Eq ':[0-9]+:[0-9]+: (warning|error): ' clang-tidy.log ; then | ||
| echo "::error::clang-tidy reported issues on lines changed by this PR" | ||
| exit 1 | ||
| fi | ||
| echo "clang-tidy is clean on the changed lines." | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a quick-exit path for those PRs only affect documents, test cases, CIs to save action minutes?