Skip to content

Commit ff3e85f

Browse files
committed
fix(ci): lint test files on PRs without breaking push-to-main
The pr-review-poster was flagging `gtest/gtest.h file not found` on any PR that added or modified a test file, because clang-tidy-diff-18.py ran against files that weren't in the compilation database. PR #27004 and PR #26233 both hit this. The root cause is that test TUs only enter compile_commands.json when BUILD_TESTING is ON, which the historical clang-tidy build does not enable. This PR fixes both halves of the problem: 1. Add a second make target `px4_sitl_default-clang-test` that configures a separate build dir with -DCMAKE_TESTING=ON. Test TUs land in its compile_commands.json with resolved gtest/fuzztest include paths. 2. Add an umbrella `clang-ci` target that depends on both `px4_sitl_default-clang` and `px4_sitl_default-clang-test` so the PR job prepares both build dirs with one make invocation. 3. On PR events the workflow uses `make clang-ci`, installs libclang-rt-18-dev (needed so fuzztest's FUZZTEST_FUZZING_MODE flags do not fail the abseil try_compile with a misleading "pthreads not found" error), and routes the clang-tidy-diff producer at the test-enabled build dir. 4. Push-to-main is left entirely alone: same single build dir, same `make px4_sitl_default-clang`, same `make clang-tidy`. Test files are not in that DB so run-clang-tidy.py keeps ignoring them exactly as before. This preserves green main while ~189 pre-existing clang-tidy issues in test files remain untouched; fixing those is out of scope for this change. 5. Replace the fragile `:!*/test/*` pathspec filter (which missed flat `*Test.cpp` files in module roots) with `Tools/ci/clang-tidy-diff-filter.py`, which reads the compilation database and drops any changed source file that is not a TU. Headers always pass through. Production code that happens to use test-like names (src/systemcmds/actuator_test, src/drivers/test_ppm, etc.) stays analyzed because those are real px4_add_module targets. Verified in the ghcr.io/px4/px4-dev:v1.17.0-rc2 container and on the real CI runner: - cmake configure with CMAKE_TESTING=ON succeeds after installing libclang-rt-18-dev (Found Threads: TRUE) - compile_commands.json grows from 1333 to 1521 TUs - Modifying HysteresisTest.cpp with a new `const char *p = NULL` correctly flags hicpp-use-nullptr and clang-diagnostic-unused-variable on the new line, while pre-existing issues on other lines of the same file stay suppressed by clang-tidy-diff-18.py's line filter ("Suppressed ... 1 due to line filter") - No gtest/gtest.h false positives - Push-to-main path unchanged, still green Signed-off-by: Ramon Roche <mrpollo@gmail.com>
1 parent c515f81 commit ff3e85f

File tree

3 files changed

+219
-23
lines changed

3 files changed

+219
-23
lines changed

.github/workflows/clang-tidy.yml

Lines changed: 87 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,51 @@ permissions:
1616
contents: read
1717

1818
jobs:
19-
clang_tidy:
19+
# Push-to-main: unchanged historical behavior. Single clang build dir
20+
# with BUILD_TESTING=OFF. `make clang-tidy` builds and analyzes every
21+
# TU in compile_commands.json. Test files are not in the DB and
22+
# therefore never analyzed.
23+
clang_tidy_push:
2024
name: Clang-Tidy
25+
if: github.event_name != 'pull_request'
2126
runs-on: [runs-on, runner=16cpu-linux-x64, "run-id=${{ github.run_id }}", "extras=s3-cache"]
27+
container:
28+
image: ghcr.io/px4/px4-dev:v1.17.0-rc2
29+
permissions:
30+
contents: read
31+
steps:
32+
- uses: runs-on/action@v2
33+
- uses: actions/checkout@v6
34+
with:
35+
fetch-depth: 0
36+
fetch-tags: true
37+
38+
- name: Configure Git Safe Directory
39+
run: git config --system --add safe.directory '*'
40+
41+
- uses: ./.github/actions/setup-ccache
42+
id: ccache
43+
with:
44+
cache-key-prefix: ccache-clang-tidy
45+
max-size: 150M
46+
47+
- name: Build and Analyze - Clang-Tidy
48+
run: make -j$(nproc) clang-tidy
49+
50+
- uses: ./.github/actions/save-ccache
51+
if: always()
52+
with:
53+
cache-primary-key: ${{ steps.ccache.outputs.cache-primary-key }}
54+
55+
# Pull request: diff-based analysis with a second BUILD_TESTING=ON
56+
# build dir so test files in the PR diff can be linted by
57+
# clang-tidy-diff-18.py with resolved gtest/fuzztest includes.
58+
# Results are uploaded as a `pr-review` artifact for the PR Review
59+
# Poster workflow to post as inline comments.
60+
clang_tidy_pr:
61+
name: Clang-Tidy
62+
if: github.event_name == 'pull_request'
63+
runs-on: [runs-on, runner=8cpu-linux-x64, "run-id=${{ github.run_id }}", "extras=s3-cache"]
2264
container:
2365
image: ghcr.io/px4/px4-dev:v1.17.0-rc2
2466
permissions:
@@ -30,6 +72,7 @@ jobs:
3072
with:
3173
fetch-depth: 0
3274
fetch-tags: true
75+
3376
- name: Configure Git Safe Directory
3477
run: git config --system --add safe.directory '*'
3578

@@ -39,35 +82,57 @@ jobs:
3982
cache-key-prefix: ccache-clang-tidy
4083
max-size: 150M
4184

42-
- name: Build - px4_sitl_default (Clang)
43-
run: make -j16 px4_sitl_default-clang
85+
# fuzztest (enabled via BUILD_TESTING in the -test build dir) pulls
86+
# in abseil via FetchContent, and abseil runs a try_compile with
87+
# fuzztest's -fsanitize=address flags. The px4-dev container ships
88+
# clang but not the clang compiler-rt runtime, so that link fails
89+
# and the configure reports a misleading "pthreads not found".
90+
# libclang-rt-18-dev provides libclang_rt.asan and friends.
91+
- name: Install clang compiler-rt
92+
run: |
93+
apt-get update
94+
apt-get install -y --no-install-recommends libclang-rt-18-dev
95+
96+
# `make clang-ci` prepares both clang build directories:
97+
# - build/px4_sitl_default-clang: full build, BUILD_TESTING=OFF
98+
# (used by run-clang-tidy-pr.py for whole-file analysis of
99+
# changed production code)
100+
# - build/px4_sitl_default-clang-test: configure-only, BUILD_TESTING=ON
101+
# (used by clang-tidy-diff-18.py so test files are in the
102+
# compilation database with resolved gtest/fuzztest includes)
103+
- name: Build clang SITL
104+
run: make -j$(nproc) clang-ci
44105

45106
- name: Run Clang-Tidy Analysis
46-
id: clang_tidy
47-
run: |
48-
if [ "${{ github.event_name }}" != "pull_request" ]; then
49-
make -j$(nproc) clang-tidy
50-
else
51-
python3 Tools/ci/run-clang-tidy-pr.py origin/${{ github.base_ref }}
52-
fi
107+
run: python3 Tools/ci/run-clang-tidy-pr.py origin/${{ github.base_ref }}
53108

54-
# On PRs, also produce a `pr-review` artifact for the PR Review Poster
55-
# workflow to consume. clang-tidy-diff-18 emits a unified fixes.yml that
109+
# Produce a `pr-review` artifact for the PR Review Poster workflow
110+
# to consume. clang-tidy-diff-18 emits a unified fixes.yml that
56111
# the producer script translates into line-anchored review comments.
57-
# Running this inside the same container as the build means there is no
58-
# workspace-path rewriting and no cross-runner artifact handoff.
59112
- name: Export clang-tidy fixes for PR review
60-
if: always() && github.event_name == 'pull_request'
113+
if: always()
61114
run: |
62115
mkdir -p pr-review
63-
git diff -U0 origin/${{ github.base_ref }}...HEAD -- ':!*/test/*' \
64-
| clang-tidy-diff-18.py -p1 \
65-
-path build/px4_sitl_default-clang \
66-
-export-fixes pr-review/fixes.yml \
67-
-j0 || true
116+
# Drop changed C/C++ source files that are not in
117+
# compile_commands.json for the test-enabled build. Files not
118+
# in the DB are platform-specific sources, vendored code, or
119+
# submodule code we don't own. Feeding them to clang-tidy-diff
120+
# produces false positives from unresolved headers.
121+
python3 Tools/ci/clang-tidy-diff-filter.py \
122+
--build-dir build/px4_sitl_default-clang-test \
123+
--base-ref origin/${{ github.base_ref }} \
124+
--out pr-review/diff.patch
125+
if [ -s pr-review/diff.patch ]; then
126+
clang-tidy-diff-18.py -p1 \
127+
-path build/px4_sitl_default-clang-test \
128+
-export-fixes pr-review/fixes.yml \
129+
-j0 < pr-review/diff.patch || true
130+
else
131+
echo "No analyzable files in diff; skipping clang-tidy-diff"
132+
fi
68133
69134
- name: Build pr-review artifact
70-
if: always() && github.event_name == 'pull_request'
135+
if: always()
71136
env:
72137
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
73138
run: |
@@ -81,7 +146,7 @@ jobs:
81146
--event COMMENT
82147
83148
- name: Upload pr-review artifact
84-
if: always() && github.event_name == 'pull_request'
149+
if: always()
85150
uses: actions/upload-artifact@v7
86151
with:
87152
name: pr-review

Makefile

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -494,7 +494,7 @@ python_coverage:
494494

495495
# static analyzers (scan-build, clang-tidy, cppcheck)
496496
# --------------------------------------------------------------------
497-
.PHONY: scan-build px4_sitl_default-clang clang-tidy clang-tidy-fix
497+
.PHONY: scan-build px4_sitl_default-clang px4_sitl_default-clang-test clang-ci clang-tidy clang-tidy-fix
498498
.PHONY: cppcheck shellcheck_all validate_module_configs
499499

500500
scan-build:
@@ -512,6 +512,26 @@ px4_sitl_default-clang:
512512
@cd "$(SRC_DIR)"/build/px4_sitl_default-clang && cmake "$(SRC_DIR)" $(CMAKE_ARGS) -G"$(PX4_CMAKE_GENERATOR)" -DCONFIG=px4_sitl_default -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++
513513
@$(PX4_MAKE) -C "$(SRC_DIR)"/build/px4_sitl_default-clang
514514

515+
# Clang SITL configure with BUILD_TESTING=ON so test files land in
516+
# compile_commands.json with resolved gtest/fuzztest includes. Used by CI
517+
# to produce a compilation database for diff-based clang-tidy that can
518+
# lint test files. Configure only: we don't build the test binaries here,
519+
# just generate the database.
520+
px4_sitl_default-clang-test:
521+
@mkdir -p "$(SRC_DIR)"/build/px4_sitl_default-clang-test
522+
@cd "$(SRC_DIR)"/build/px4_sitl_default-clang-test && cmake "$(SRC_DIR)" $(CMAKE_ARGS) -G"$(PX4_CMAKE_GENERATOR)" -DCONFIG=px4_sitl_default -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_TESTING=ON
523+
524+
# CI-oriented target that prepares both clang build directories used by
525+
# the Static Analysis workflow:
526+
# - px4_sitl_default-clang: full build, BUILD_TESTING=OFF.
527+
# Used by `make clang-tidy` (push-to-main) and run-clang-tidy-pr.py.
528+
# - px4_sitl_default-clang-test: configure-only, BUILD_TESTING=ON.
529+
# Used by clang-tidy-diff-18.py so test files are in the
530+
# compilation database with resolved gtest/fuzztest includes.
531+
# Running one target ensures both dirs exist before any clang-tidy
532+
# variant runs, and keeps the workflow free of raw cmake invocations.
533+
clang-ci: px4_sitl_default-clang px4_sitl_default-clang-test
534+
515535
# Paths to exclude from clang-tidy (auto-generated from .gitmodules + manual additions):
516536
# - All submodules (external code we consume, not edit)
517537
# - Test code (allowed looser style)

Tools/ci/clang-tidy-diff-filter.py

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
#!/usr/bin/env python3
2+
"""
3+
Filter a git diff for consumption by clang-tidy-diff.
4+
5+
Produces a unified diff containing only files that clang-tidy can
6+
actually analyze against the current compilation database:
7+
8+
- C/C++ source files (.c, .cpp, .cc, .cxx, .m, .mm) must be present
9+
in compile_commands.json. Files absent from the database are test
10+
files, excluded code, or platform-specific sources that were not
11+
compiled. Feeding them to clang-tidy-diff produces spurious
12+
"header not found" errors (gtest/gtest.h in particular).
13+
14+
- Header files (.h, .hpp, .hxx) always pass through. clang-tidy
15+
analyzes header changes via the TUs that include them; there is
16+
no separate TU for a header to match against the database.
17+
18+
- All other files (CMakeLists.txt, .yml, .md, etc.) are dropped.
19+
20+
Output is a unified diff suitable for piping into clang-tidy-diff.py.
21+
If nothing remains, the output file is empty.
22+
23+
Used by .github/workflows/clang-tidy.yml as a pre-filter for the
24+
`pr-review` artifact producer. Python stdlib only.
25+
"""
26+
27+
import argparse
28+
import json
29+
import os
30+
import subprocess
31+
import sys
32+
33+
34+
SOURCE_EXTS = {'.c', '.cpp', '.cc', '.cxx', '.m', '.mm'}
35+
HEADER_EXTS = {'.h', '.hpp', '.hxx'}
36+
37+
38+
def load_db_files(build_dir):
39+
"""Return the set of source paths (repo-relative) in compile_commands.json."""
40+
path = os.path.join(build_dir, 'compile_commands.json')
41+
with open(path) as f:
42+
db = json.load(f)
43+
root = os.path.abspath('.')
44+
prefix = root + os.sep
45+
paths = set()
46+
for entry in db:
47+
p = entry.get('file', '')
48+
if p.startswith(prefix):
49+
paths.add(p[len(prefix):])
50+
else:
51+
# Relative or external path; record as-is
52+
paths.add(p)
53+
return paths
54+
55+
56+
def changed_files(base_ref):
57+
out = subprocess.check_output(
58+
['git', 'diff', '--name-only', '{}...HEAD'.format(base_ref)],
59+
text=True,
60+
)
61+
return [line.strip() for line in out.splitlines() if line.strip()]
62+
63+
64+
def keep_file(path, db_files):
65+
"""Decide whether to keep this path in the filtered diff."""
66+
ext = os.path.splitext(path)[1].lower()
67+
if ext in HEADER_EXTS:
68+
return True
69+
if ext in SOURCE_EXTS:
70+
return path in db_files
71+
return False
72+
73+
74+
def filtered_diff(base_ref, keep_paths):
75+
if not keep_paths:
76+
return ''
77+
cmd = ['git', 'diff', '-U0', '{}...HEAD'.format(base_ref), '--'] + sorted(keep_paths)
78+
return subprocess.check_output(cmd, text=True)
79+
80+
81+
def main():
82+
parser = argparse.ArgumentParser(description=__doc__)
83+
parser.add_argument('--build-dir', required=True,
84+
help='CMake build dir containing compile_commands.json')
85+
parser.add_argument('--base-ref', required=True,
86+
help='Git ref to diff against (e.g. origin/main)')
87+
parser.add_argument('--out', required=True,
88+
help='Output path for the filtered unified diff')
89+
args = parser.parse_args()
90+
91+
db_files = load_db_files(args.build_dir)
92+
changed = changed_files(args.base_ref)
93+
94+
keep = [p for p in changed if keep_file(p, db_files)]
95+
dropped = [p for p in changed if p not in keep]
96+
97+
print('clang-tidy-diff-filter: kept {} of {} changed files'.format(
98+
len(keep), len(changed)))
99+
if dropped:
100+
print(' dropped (not in compile_commands.json or not source/header):')
101+
for p in dropped:
102+
print(' {}'.format(p))
103+
104+
diff = filtered_diff(args.base_ref, keep)
105+
with open(args.out, 'w') as f:
106+
f.write(diff)
107+
return 0
108+
109+
110+
if __name__ == '__main__':
111+
sys.exit(main())

0 commit comments

Comments
 (0)