-
Notifications
You must be signed in to change notification settings - Fork 498
feat(ci): integrate clang-tidy for changed C/C++ files #116
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
3d03eaa
71011b8
a1a857f
f5aedce
30343b3
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,11 @@ | ||
| Checks: > | ||
| -*, | ||
| clang-analyzer-*, | ||
| bugprone-*, | ||
| performance-*, | ||
| modernize-use-nullptr, | ||
| modernize-use-override | ||
| WarningsAsErrors: "*" | ||
| HeaderFilterRegex: "^(src|tests|tools)/" | ||
| FormatStyle: none | ||
| SystemHeaders: false | ||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,146 @@ | ||||||||
| name: Clang-Tidy | ||||||||
|
|
||||||||
| on: | ||||||||
| push: | ||||||||
| branches: ["main"] | ||||||||
| paths: | ||||||||
| - "**/*.c" | ||||||||
| - "**/*.cc" | ||||||||
| - "**/*.cpp" | ||||||||
| - "**/*.cxx" | ||||||||
| - "**/*.h" | ||||||||
| - "**/*.hpp" | ||||||||
| - "CMakeLists.txt" | ||||||||
| - "**/CMakeLists.txt" | ||||||||
|
Comment on lines
+13
to
+14
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. Redundant The pattern
Suggested change
|
||||||||
| - "cmake/**" | ||||||||
| - ".clang-tidy" | ||||||||
| pull_request: | ||||||||
| branches: ["main"] | ||||||||
| paths: | ||||||||
| - "**/*.c" | ||||||||
| - "**/*.cc" | ||||||||
| - "**/*.cpp" | ||||||||
| - "**/*.cxx" | ||||||||
| - "**/*.h" | ||||||||
| - "**/*.hpp" | ||||||||
| - "CMakeLists.txt" | ||||||||
| - "**/CMakeLists.txt" | ||||||||
| - "cmake/**" | ||||||||
| - ".clang-tidy" | ||||||||
| workflow_dispatch: | ||||||||
|
|
||||||||
| concurrency: | ||||||||
| group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} | ||||||||
| cancel-in-progress: true | ||||||||
|
|
||||||||
| permissions: | ||||||||
| contents: read | ||||||||
|
|
||||||||
| jobs: | ||||||||
| clang_tidy: | ||||||||
| name: Clang-Tidy Checks | ||||||||
| runs-on: ubuntu-latest | ||||||||
| steps: | ||||||||
| - name: Checkout code | ||||||||
| uses: actions/checkout@v4 | ||||||||
| with: | ||||||||
| submodules: recursive | ||||||||
|
|
||||||||
| - name: Install dependencies | ||||||||
| run: | | ||||||||
| sudo apt-get update | ||||||||
| sudo apt-get install -y clang-tidy cmake ninja-build | ||||||||
|
|
||||||||
| - name: Configure CMake and export compile commands | ||||||||
| run: | | ||||||||
| cmake -S . -B build -G Ninja \ | ||||||||
| -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \ | ||||||||
| -DENABLE_HASWELL=ON \ | ||||||||
| -DBUILD_TOOLS=OFF | ||||||||
|
|
||||||||
| - name: Collect changed C/C++ files | ||||||||
| id: changed_files | ||||||||
| uses: tj-actions/changed-files@v46 | ||||||||
|
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. Third-party action not pinned to a commit SHA
Example:
Suggested change
(Replace the SHA with the one that corresponds to the |
||||||||
| with: | ||||||||
| files: | | ||||||||
| **/*.c | ||||||||
| **/*.cc | ||||||||
| **/*.cpp | ||||||||
| **/*.cxx | ||||||||
| !thirdparty/** | ||||||||
| !build/** | ||||||||
| separator: "\n" | ||||||||
|
|
||||||||
| - name: Filter changed files to compile database entries | ||||||||
| id: tidy_files | ||||||||
| if: steps.changed_files.outputs.any_changed == 'true' | ||||||||
| run: | | ||||||||
| python3 - <<'PY' | ||||||||
| import json | ||||||||
| import os | ||||||||
| from pathlib import Path | ||||||||
|
|
||||||||
| changed = [line.strip() for line in """${{ steps.changed_files.outputs.all_changed_files }}""".splitlines() if line.strip()] | ||||||||
|
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. Template injection risk in inline Python The GitHub Actions expression The safe pattern is to pass the value via an environment variable and read it in Python, which avoids any possibility of code injection regardless of the content: By reading the file list from an environment variable inside Python, the content is never evaluated as code. |
||||||||
| compile_db_path = Path("build/compile_commands.json") | ||||||||
| with compile_db_path.open("r", encoding="utf-8") as f: | ||||||||
| compile_db = json.load(f) | ||||||||
|
|
||||||||
| compile_entries = set() | ||||||||
| for entry in compile_db: | ||||||||
| file_path = entry.get("file") | ||||||||
| if not file_path: | ||||||||
| continue | ||||||||
| normalized = os.path.normpath(file_path).replace("\\", "/") | ||||||||
| compile_entries.add(normalized) | ||||||||
|
|
||||||||
| selected = [] | ||||||||
| skipped = [] | ||||||||
| cwd = Path.cwd() | ||||||||
| for file_path in changed: | ||||||||
| if not Path(file_path).is_file(): | ||||||||
| skipped.append(f"{file_path} (missing)") | ||||||||
| continue | ||||||||
|
|
||||||||
| abs_normalized = os.path.normpath(str((cwd / file_path).resolve())).replace("\\", "/") | ||||||||
| rel_normalized = os.path.normpath(file_path).replace("\\", "/") | ||||||||
|
|
||||||||
| if abs_normalized in compile_entries or rel_normalized in compile_entries: | ||||||||
| selected.append(file_path) | ||||||||
| else: | ||||||||
| skipped.append(f"{file_path} (no compile_commands entry)") | ||||||||
|
|
||||||||
| output_path = os.environ["GITHUB_OUTPUT"] | ||||||||
| with open(output_path, "a", encoding="utf-8") as out: | ||||||||
| out.write(f"any_tidy_files={'true' if selected else 'false'}\n") | ||||||||
| out.write("all_tidy_files<<EOF\n") | ||||||||
| out.write("\n".join(selected)) | ||||||||
| out.write("\nEOF\n") | ||||||||
| out.write("skipped_files<<EOF\n") | ||||||||
| out.write("\n".join(skipped)) | ||||||||
| out.write("\nEOF\n") | ||||||||
| PY | ||||||||
|
|
||||||||
| - name: Show skipped files | ||||||||
| if: steps.changed_files.outputs.any_changed == 'true' && steps.tidy_files.outputs.skipped_files != '' | ||||||||
| run: | | ||||||||
| echo "Skipping files:" | ||||||||
| printf '%s\n' "${{ steps.tidy_files.outputs.skipped_files }}" | ||||||||
|
|
||||||||
| - name: Run clang-tidy | ||||||||
| if: steps.tidy_files.outputs.any_tidy_files == 'true' | ||||||||
| run: | | ||||||||
| mapfile -t changed_files <<'EOF' | ||||||||
| ${{ steps.tidy_files.outputs.all_tidy_files }} | ||||||||
| EOF | ||||||||
|
|
||||||||
| for file in "${changed_files[@]}"; do | ||||||||
| if [ -f "$file" ]; then | ||||||||
| echo "Running clang-tidy on $file" | ||||||||
| clang-tidy -p build --warnings-as-errors='*' "$file" | ||||||||
| fi | ||||||||
| done | ||||||||
|
|
||||||||
| - name: No clang-tidy files to analyze | ||||||||
| if: steps.changed_files.outputs.any_changed != 'true' || steps.tidy_files.outputs.any_tidy_files != 'true' | ||||||||
| run: | | ||||||||
| echo "No changed source files with compile_commands entries matched clang-tidy patterns." | ||||||||
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.
HeaderFilterRegexwon't match absolute pathsclang-tidy evaluates
HeaderFilterRegexagainst the full absolute path of each header file. On a typical GitHub-hosted runner the workspace path is something like/home/runner/work/zvec/zvec/, so project headers will have paths like/home/runner/work/zvec/zvec/src/foo.h. The regex^(src|tests|tools)/is anchored to the beginning of the string and will therefore never match an absolute path, silently suppressing all diagnostics originating in project headers.Change the pattern to match the directory name anywhere in the path:
This will correctly flag headers under
src/,tests/, ortools/regardless of the absolute prefix.