From 1cf7d7552511a918d3c74b89250947003abeccc2 Mon Sep 17 00:00:00 2001 From: Ramon Roche Date: Sat, 11 Apr 2026 08:07:05 -0700 Subject: [PATCH] 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 --- .github/workflows/clang-tidy.yml | 109 ++++++++++++++++++++++------ Makefile | 22 +++++- Tools/ci/clang-tidy-diff-filter.py | 111 +++++++++++++++++++++++++++++ 3 files changed, 219 insertions(+), 23 deletions(-) create mode 100644 Tools/ci/clang-tidy-diff-filter.py diff --git a/.github/workflows/clang-tidy.yml b/.github/workflows/clang-tidy.yml index 2416fb3611..e62718751a 100644 --- a/.github/workflows/clang-tidy.yml +++ b/.github/workflows/clang-tidy.yml @@ -16,9 +16,51 @@ permissions: contents: read jobs: - clang_tidy: + # Push-to-main: unchanged historical behavior. Single clang build dir + # with BUILD_TESTING=OFF. `make clang-tidy` builds and analyzes every + # TU in compile_commands.json. Test files are not in the DB and + # therefore never analyzed. + clang_tidy_push: name: Clang-Tidy + if: github.event_name != 'pull_request' runs-on: [runs-on, runner=16cpu-linux-x64, "run-id=${{ github.run_id }}", "extras=s3-cache"] + container: + image: ghcr.io/px4/px4-dev:v1.17.0-rc2 + permissions: + contents: read + steps: + - uses: runs-on/action@v2 + - uses: actions/checkout@v6 + with: + fetch-depth: 0 + fetch-tags: true + + - name: Configure Git Safe Directory + run: git config --system --add safe.directory '*' + + - uses: ./.github/actions/setup-ccache + id: ccache + with: + cache-key-prefix: ccache-clang-tidy + max-size: 150M + + - name: Build and Analyze - Clang-Tidy + run: make -j$(nproc) clang-tidy + + - uses: ./.github/actions/save-ccache + if: always() + with: + cache-primary-key: ${{ steps.ccache.outputs.cache-primary-key }} + + # Pull request: diff-based analysis with a second BUILD_TESTING=ON + # build dir so test files in the PR diff can be linted by + # clang-tidy-diff-18.py with resolved gtest/fuzztest includes. + # Results are uploaded as a `pr-review` artifact for the PR Review + # Poster workflow to post as inline comments. + clang_tidy_pr: + name: Clang-Tidy + if: github.event_name == 'pull_request' + runs-on: [runs-on, runner=8cpu-linux-x64, "run-id=${{ github.run_id }}", "extras=s3-cache"] container: image: ghcr.io/px4/px4-dev:v1.17.0-rc2 permissions: @@ -30,6 +72,7 @@ jobs: with: fetch-depth: 0 fetch-tags: true + - name: Configure Git Safe Directory run: git config --system --add safe.directory '*' @@ -39,35 +82,57 @@ jobs: cache-key-prefix: ccache-clang-tidy max-size: 150M - - name: Build - px4_sitl_default (Clang) - run: make -j16 px4_sitl_default-clang + # fuzztest (enabled via BUILD_TESTING in the -test build dir) pulls + # in abseil via FetchContent, and abseil runs a try_compile with + # fuzztest's -fsanitize=address flags. The px4-dev container ships + # clang but not the clang compiler-rt runtime, so that link fails + # and the configure reports a misleading "pthreads not found". + # libclang-rt-18-dev provides libclang_rt.asan and friends. + - name: Install clang compiler-rt + run: | + apt-get update + apt-get install -y --no-install-recommends libclang-rt-18-dev + + # `make clang-ci` prepares both clang build directories: + # - build/px4_sitl_default-clang: full build, BUILD_TESTING=OFF + # (used by run-clang-tidy-pr.py for whole-file analysis of + # changed production code) + # - build/px4_sitl_default-clang-test: configure-only, BUILD_TESTING=ON + # (used by clang-tidy-diff-18.py so test files are in the + # compilation database with resolved gtest/fuzztest includes) + - name: Build clang SITL + run: make -j$(nproc) clang-ci - name: Run Clang-Tidy Analysis - id: clang_tidy - run: | - if [ "${{ github.event_name }}" != "pull_request" ]; then - make -j$(nproc) clang-tidy - else - python3 Tools/ci/run-clang-tidy-pr.py origin/${{ github.base_ref }} - fi + run: python3 Tools/ci/run-clang-tidy-pr.py origin/${{ github.base_ref }} - # On PRs, also produce a `pr-review` artifact for the PR Review Poster - # workflow to consume. clang-tidy-diff-18 emits a unified fixes.yml that + # Produce a `pr-review` artifact for the PR Review Poster workflow + # to consume. clang-tidy-diff-18 emits a unified fixes.yml that # the producer script translates into line-anchored review comments. - # Running this inside the same container as the build means there is no - # workspace-path rewriting and no cross-runner artifact handoff. - name: Export clang-tidy fixes for PR review - if: always() && github.event_name == 'pull_request' + if: always() run: | mkdir -p pr-review - git diff -U0 origin/${{ github.base_ref }}...HEAD -- ':!*/test/*' \ - | clang-tidy-diff-18.py -p1 \ - -path build/px4_sitl_default-clang \ - -export-fixes pr-review/fixes.yml \ - -j0 || true + # Drop changed C/C++ source files that are not in + # compile_commands.json for the test-enabled build. Files not + # in the DB are platform-specific sources, vendored code, or + # submodule code we don't own. Feeding them to clang-tidy-diff + # produces false positives from unresolved headers. + python3 Tools/ci/clang-tidy-diff-filter.py \ + --build-dir build/px4_sitl_default-clang-test \ + --base-ref origin/${{ github.base_ref }} \ + --out pr-review/diff.patch + if [ -s pr-review/diff.patch ]; then + clang-tidy-diff-18.py -p1 \ + -path build/px4_sitl_default-clang-test \ + -export-fixes pr-review/fixes.yml \ + -j0 < pr-review/diff.patch || true + else + echo "No analyzable files in diff; skipping clang-tidy-diff" + fi - name: Build pr-review artifact - if: always() && github.event_name == 'pull_request' + if: always() env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | @@ -81,7 +146,7 @@ jobs: --event COMMENT - name: Upload pr-review artifact - if: always() && github.event_name == 'pull_request' + if: always() uses: actions/upload-artifact@v7 with: name: pr-review diff --git a/Makefile b/Makefile index 066a36a0f4..7c57f15727 100644 --- a/Makefile +++ b/Makefile @@ -494,7 +494,7 @@ python_coverage: # static analyzers (scan-build, clang-tidy, cppcheck) # -------------------------------------------------------------------- -.PHONY: scan-build px4_sitl_default-clang clang-tidy clang-tidy-fix +.PHONY: scan-build px4_sitl_default-clang px4_sitl_default-clang-test clang-ci clang-tidy clang-tidy-fix .PHONY: cppcheck shellcheck_all validate_module_configs scan-build: @@ -512,6 +512,26 @@ px4_sitl_default-clang: @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++ @$(PX4_MAKE) -C "$(SRC_DIR)"/build/px4_sitl_default-clang +# Clang SITL configure with BUILD_TESTING=ON so test files land in +# compile_commands.json with resolved gtest/fuzztest includes. Used by CI +# to produce a compilation database for diff-based clang-tidy that can +# lint test files. Configure only: we don't build the test binaries here, +# just generate the database. +px4_sitl_default-clang-test: + @mkdir -p "$(SRC_DIR)"/build/px4_sitl_default-clang-test + @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 + +# CI-oriented target that prepares both clang build directories used by +# the Static Analysis workflow: +# - px4_sitl_default-clang: full build, BUILD_TESTING=OFF. +# Used by `make clang-tidy` (push-to-main) and run-clang-tidy-pr.py. +# - px4_sitl_default-clang-test: configure-only, BUILD_TESTING=ON. +# Used by clang-tidy-diff-18.py so test files are in the +# compilation database with resolved gtest/fuzztest includes. +# Running one target ensures both dirs exist before any clang-tidy +# variant runs, and keeps the workflow free of raw cmake invocations. +clang-ci: px4_sitl_default-clang px4_sitl_default-clang-test + # Paths to exclude from clang-tidy (auto-generated from .gitmodules + manual additions): # - All submodules (external code we consume, not edit) # - Test code (allowed looser style) diff --git a/Tools/ci/clang-tidy-diff-filter.py b/Tools/ci/clang-tidy-diff-filter.py new file mode 100644 index 0000000000..5f516c5d01 --- /dev/null +++ b/Tools/ci/clang-tidy-diff-filter.py @@ -0,0 +1,111 @@ +#!/usr/bin/env python3 +""" +Filter a git diff for consumption by clang-tidy-diff. + +Produces a unified diff containing only files that clang-tidy can +actually analyze against the current compilation database: + + - C/C++ source files (.c, .cpp, .cc, .cxx, .m, .mm) must be present + in compile_commands.json. Files absent from the database are test + files, excluded code, or platform-specific sources that were not + compiled. Feeding them to clang-tidy-diff produces spurious + "header not found" errors (gtest/gtest.h in particular). + + - Header files (.h, .hpp, .hxx) always pass through. clang-tidy + analyzes header changes via the TUs that include them; there is + no separate TU for a header to match against the database. + + - All other files (CMakeLists.txt, .yml, .md, etc.) are dropped. + +Output is a unified diff suitable for piping into clang-tidy-diff.py. +If nothing remains, the output file is empty. + +Used by .github/workflows/clang-tidy.yml as a pre-filter for the +`pr-review` artifact producer. Python stdlib only. +""" + +import argparse +import json +import os +import subprocess +import sys + + +SOURCE_EXTS = {'.c', '.cpp', '.cc', '.cxx', '.m', '.mm'} +HEADER_EXTS = {'.h', '.hpp', '.hxx'} + + +def load_db_files(build_dir): + """Return the set of source paths (repo-relative) in compile_commands.json.""" + path = os.path.join(build_dir, 'compile_commands.json') + with open(path) as f: + db = json.load(f) + root = os.path.abspath('.') + prefix = root + os.sep + paths = set() + for entry in db: + p = entry.get('file', '') + if p.startswith(prefix): + paths.add(p[len(prefix):]) + else: + # Relative or external path; record as-is + paths.add(p) + return paths + + +def changed_files(base_ref): + out = subprocess.check_output( + ['git', 'diff', '--name-only', '{}...HEAD'.format(base_ref)], + text=True, + ) + return [line.strip() for line in out.splitlines() if line.strip()] + + +def keep_file(path, db_files): + """Decide whether to keep this path in the filtered diff.""" + ext = os.path.splitext(path)[1].lower() + if ext in HEADER_EXTS: + return True + if ext in SOURCE_EXTS: + return path in db_files + return False + + +def filtered_diff(base_ref, keep_paths): + if not keep_paths: + return '' + cmd = ['git', 'diff', '-U0', '{}...HEAD'.format(base_ref), '--'] + sorted(keep_paths) + return subprocess.check_output(cmd, text=True) + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument('--build-dir', required=True, + help='CMake build dir containing compile_commands.json') + parser.add_argument('--base-ref', required=True, + help='Git ref to diff against (e.g. origin/main)') + parser.add_argument('--out', required=True, + help='Output path for the filtered unified diff') + args = parser.parse_args() + + db_files = load_db_files(args.build_dir) + changed = changed_files(args.base_ref) + + keep = [p for p in changed if keep_file(p, db_files)] + dropped = [p for p in changed if p not in keep] + + print('clang-tidy-diff-filter: kept {} of {} changed files'.format( + len(keep), len(changed))) + if dropped: + print(' dropped (not in compile_commands.json or not source/header):') + for p in dropped: + print(' {}'.format(p)) + + diff = filtered_diff(args.base_ref, keep) + with open(args.out, 'w') as f: + f.write(diff) + return 0 + + +if __name__ == '__main__': + sys.exit(main())