Skip to content

fix(ci): let the clang-tidy header filter match absolute paths so headers are ratcheted - #1504

Merged
lusoris merged 5 commits into
masterfrom
fix/tidy-header-filter
Sep 20, 2026
Merged

lusoris merged 5 commits into
masterfrom
fix/tidy-header-filter

Conversation

@lusoris

@lusoris lusoris commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

.clang-tidy's header filter could never match a header, so the ADR-1142 whole-tree ratchet has never counted one.

HeaderFilterRegex: '^(core/(include|src|tools|test)|python|ai)/.*\.(h|hpp|hxx|cuh)$'

clang-tidy tests that regex against the path as it saw it — from compile_commands.json, absolute: /home/…/vmafx/core/src/picture.h. An expression anchored at ^core/ cannot match that, so every in-repo header has been filtered as "non-user code" since the file was written. One translation unit (core/src/picture.c):

Suppressed 540 warnings (480 in non-user code, 60 NOLINT).
Use -header-filter=.* or leave it as default to display errors from all non-system headers.

Tree-wide, the CPU baseline holds 462 warnings in 72 files and not one is in a header, while the tree has 60 headers carrying 313 findings — core/src/feature/adm_tools.h alone has 142. Ledger L-45, reported by the CAMBI agent and confirmed by measurement.

The regex gains a (^|/) prefix — same file set, matched relative or absolute, nothing outside the repository (no system header contains /core/src/). ADR-1265 records the four alternatives (an .* filter, moving the filter into the script, keeping headers excluded as policy, fixing the regex without re-recording) and why each loses.

How the baselines are re-recorded

CPU lane — from CI, not from my machine. The Tidy Ratchet job measures with clang-tidy-22 on ubuntu-26.04 and uploads tidy-ratchet-cpu.json "to commit as the new baseline when the ratchet asks". A locally recorded file would carry a different clang-tidy and gcc and would drift immediately. So this PR has two commits: the fix (this one, whose Tidy Ratchet run fails by design and uploads the measurement), then that artifact committed verbatim as scripts/ci/tidy-baseline-cpu.json.

GPU lanes — locally, advisory. No CI job measures cuda, hip or sycl; their baselines are re-recorded with make tidy-ratchet-write LANE=<lane> on this machine (clang-tidy 22.1.8, /opt/cuda, /opt/rocm, oneAPI). Only headers next to each lane's translation units are attributed to that lane, per the ratchet's own ownership rule.

The measured CPU delta, for the record (local clang-tidy 22.1.8; CI's numbers will differ slightly and are the ones that land):

Files Warnings
Committed baseline 72 462
Measured with headers visible 115 744
of which headers 60 313

No source file changes in this PR. Nothing is cleaned; this is the first recording of a class the gate had never seen, which is why the "never raise the baseline" rule does not apply — there is no prior count to raise.

Type

  • fix — bug fix (in the lint configuration)

Checklist

  • Commits follow Conventional Commits.
  • Pre-push hooks pass.
  • I did not modify any assertAlmostEqual(...) score in the Netflix golden Python tests.
  • docs/state.md: no state delta: a lint-configuration defect tracked as ledger L-45 / L-41; no bug in the fork's tracked set opens or closes.

Deep-dive deliverables (ADR-0108)

  • Research digest — no digest needed: trivial. The whole diagnosis is the one Suppressed … in non-user code line and a two-line regex check.
  • Decision matrix — ADR-1265 ## Alternatives considered, four options.
  • AGENTS.md invariant note — scripts/ci/AGENTS.md, plus docs/rebase-notes.md.
  • Reproducer / smoke-test command — below.
  • CHANGELOG fragment — changelog.d/fixed/clang-tidy-header-filter.md.
  • Rebase note — docs/rebase-notes.md.

Reproducer

meson setup build core -Denable_cuda=false -Denable_sycl=false -Db_lto=false && ninja -C build
clang-tidy -p build core/src/picture.c 2>&1 | grep 'non-user code'
# master:      Suppressed 540 warnings (480 in non-user code, 60 NOLINT).
# this branch: the header diagnostics are printed instead of suppressed.
python3 -c "import re; rx=r'^(core/(include|src|tools|test)|python|ai)/.*\.(h|hpp|hxx|cuh)$'; print(bool(re.search(rx, '/abs/path/vmafx/core/src/picture.h')))"   # False

Rebase hazard

If a sync or a tidy-config refresh restores the ^-only anchor, Tidy Ratchet will report every header as N -> 0 and ask to tighten. That is the filter breaking again, not a cleanup; scripts/ci/AGENTS.md and docs/rebase-notes.md say so.

@github-actions github-actions Bot added the type:bug Something isn't working label Sep 19, 2026
@lusoris
lusoris force-pushed the fix/tidy-header-filter branch 2 times, most recently from d6213c9 to 14220ca Compare September 20, 2026 07:46
…ders are ratcheted

.clang-tidy's HeaderFilterRegex was anchored at ^core/, but clang-tidy
matches the regex against the absolute path it read from
compile_commands.json, which never starts with core/. Every in-repo
header was therefore dropped as non-user code: one translation unit
reports 'Suppressed 540 warnings (480 in non-user code)', and the CPU
baseline holds 462 warnings with none in a header while the tree has 60
headers carrying 313, adm_tools.h alone 142. The ADR-1142 whole-tree
ratchet had never counted a header.

The regex gains a (^|/) prefix, which matches the same files whether the
path is relative or absolute and nothing outside the repository. The CPU
baseline is committed from CI's own tidy-ratchet-cpu artifact in a
follow-up commit on this branch, since that is the clang-tidy the gate
runs; GPU-lane baselines are re-recorded locally where a toolchain
exists. ADR-1265.
…h headers visible

Re-recording the GPU lanes for ADR-1265 showed they could not be
measured at all (ledger L-41), for three separate reasons.

The Makefile passes each lane flag as --extra-arg=<flag>. The ratchet's
own --extra-arg option strips that wrapper and forwarded the values to
clang-tidy bare, so clang-tidy saw '--cuda-host-only', '-x', 'hip' as
its own unknown options and every one of ~350 translation units was
reported 'measurement unusable'. run_one now re-wraps them.

The sycl lane's --clang-tidy scripts/ci/clang-tidy-sycl.sh is
repository-relative, but run_one switches cwd to the build directory
first: [Errno 2]. The path is resolved before the switch.

An icx build records -fp-model=precise on every strict-FP unit; stock
clang spells it -ffp-model=precise and treats the icx form as a hard
'unknown argument' error, which failed 104 sycl-lane units. The wrapper
translates the flag in a copy of the compilation database.

Baselines: cpu is CI's own tidy-ratchet-cpu artifact committed verbatim
(775 warnings / 132 files, 313 of them in 60 headers, clang-tidy 22.1.8,
gcc-15). cuda 1449/173, hip 1250/147 and sycl 815/125 are local
whole-database measurements with 0 unusable units each.

scripts/ci/tests/test_tidy_ratchet_extra_args.py pins both forwarding
fixes; it fails on master's script with the bare flag visible in argv.
The test added with the forwarding fix loaded the hyphenated ratchet
script without guarding spec or spec.loader against None and without a
return annotation, which mypy reports as four findings, and it used
os.getcwd() where ruff wants Path.cwd() (PTH109). Same shape as the
test_gen_gpu_compile_commands.py fix on the ADM train.
@lusoris
lusoris force-pushed the fix/tidy-header-filter branch from 14220ca to 8c9da62 Compare September 20, 2026 08:33
@lusoris
lusoris merged commit 8845ac2 into master Sep 20, 2026
76 checks passed
@lusoris
lusoris deleted the fix/tidy-header-filter branch September 20, 2026 09:26
lusoris added a commit that referenced this pull request Sep 20, 2026
The rebase left a conflict marker in scripts/ci/tidy-ratchet.py: the
branch carried the pre-#1504 file and master now has both forwarding
fixes. master's version is the one that stays.
lusoris added a commit that referenced this pull request Sep 20, 2026
…eep one copy of its tests

The stack's own test_tidy_ratchet.py asserts that run_one() passes an
absolute path to clang-tidy's -p, and it failed: run_one resolves the
binary before switching cwd into the translation unit's directory but
left the build dir relative. For a meson build that directory IS the
build dir, so a relative -p resolves to <build>/<build> and clang-tidy
finds no compilation database. Same class as the wrapper-path fix beside
it; build_dir is resolved once, up front.

#1504 landed an equivalent set of assertions as a standalone
scripts/ci/tests/test_tidy_ratchet_extra_args.py while this branch was
adding them to the in-tree test_tidy_ratchet.py. The in-tree file is the
right home -- it already holds the rest of the ratchet's tests and
covers strictly more -- so the standalone copy goes and its one unique
assertion, that an already-wrapped --extra-arg is not wrapped twice,
moves across. 16 tests pass.
lusoris added a commit that referenced this pull request Sep 20, 2026
The rebase left a conflict marker in scripts/ci/tidy-ratchet.py: the
branch carried the pre-#1504 file and master now has both forwarding
fixes. master's version is the one that stays.
lusoris added a commit that referenced this pull request Sep 20, 2026
…eep one copy of its tests

The stack's own test_tidy_ratchet.py asserts that run_one() passes an
absolute path to clang-tidy's -p, and it failed: run_one resolves the
binary before switching cwd into the translation unit's directory but
left the build dir relative. For a meson build that directory IS the
build dir, so a relative -p resolves to <build>/<build> and clang-tidy
finds no compilation database. Same class as the wrapper-path fix beside
it; build_dir is resolved once, up front.

#1504 landed an equivalent set of assertions as a standalone
scripts/ci/tests/test_tidy_ratchet_extra_args.py while this branch was
adding them to the in-tree test_tidy_ratchet.py. The in-tree file is the
right home -- it already holds the rest of the ratchet's tests and
covers strictly more -- so the standalone copy goes and its one unique
assertion, that an already-wrapped --extra-arg is not wrapped twice,
moves across. 16 tests pass.
lusoris added a commit that referenced this pull request Sep 20, 2026
The rebase left a conflict marker in scripts/ci/tidy-ratchet.py: the
branch carried the pre-#1504 file and master now has both forwarding
fixes. master's version is the one that stays.
lusoris added a commit that referenced this pull request Sep 20, 2026
…eep one copy of its tests

The stack's own test_tidy_ratchet.py asserts that run_one() passes an
absolute path to clang-tidy's -p, and it failed: run_one resolves the
binary before switching cwd into the translation unit's directory but
left the build dir relative. For a meson build that directory IS the
build dir, so a relative -p resolves to <build>/<build> and clang-tidy
finds no compilation database. Same class as the wrapper-path fix beside
it; build_dir is resolved once, up front.

#1504 landed an equivalent set of assertions as a standalone
scripts/ci/tests/test_tidy_ratchet_extra_args.py while this branch was
adding them to the in-tree test_tidy_ratchet.py. The in-tree file is the
right home -- it already holds the rest of the ratchet's tests and
covers strictly more -- so the standalone copy goes and its one unique
assertion, that an already-wrapped --extra-arg is not wrapped twice,
moves across. 16 tests pass.
lusoris added a commit that referenced this pull request Sep 22, 2026
ADR-0880 is Accepted and its changelog fragment shipped, yet
testdata/check_borders.py, testdata/compare_a380.py and
testdata/scores_sycl_b580_576_mq.json were still in the tree.
`git log --all --diff-filter=D` over the three paths returns nothing on
any branch, so the deletion was never made anywhere while the rendered
CHANGELOG.md has claimed it since. Every claim the ADR makes about them
was re-checked before acting: outside that one changelog fragment nothing
in the tree references any of the three, and the _mq snapshot does carry
12 metrics per frame against the canonical 34 in scores_sycl_b580_576.json.
No new changelog fragment — the existing removed/ one becomes true.

docs/state.md records what a bug-ledger verification sweep established.
Four of five rows were stale rather than open: the Go duplicate-
implementation families (1f09437, in the train, still open on
origin/master), the resurrected i686 lane (ef1c160, #1497) and the
clang-tidy header filter (8845ac2, #1504, ADR-1265, all four baselines
re-recorded with headers) are already fixed on this branch.

T-SYCL-A380-SNAPSHOTS-MOTION-ZERO-2026-09-21 is opened with what the
snapshots actually contain rather than what was filed: integer_motion and
integer_motion2 are 0.0 on every one of the 48 frames of all five
scores_sycl_a380_*.json, and at 576 and 640 all 48 frames carry one
identical vmaf value. Every other backend snapshot in testdata/ has 45
non-zero motion frames. They record a broken run, not a stale-but-valid
one. Nothing is regenerated here: icpx and the A380 are both available on
this host, but the 720/1080/4k fixtures are gitignored and absent, and
re-deriving them under today's ffmpeg would force the CPU snapshots at
those resolutions to be re-recorded in the same pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant