From 61b1fa3e93fdb8645f041c31c5282de560fec767 Mon Sep 17 00:00:00 2001 From: Yael Date: Thu, 20 Aug 2026 21:01:55 +0000 Subject: [PATCH 1/2] ci: make the clang-format job informational, and let it actually report Two problems, one job. The reporting bug (#89). The step runs under `set -euo pipefail` and captures the diff with `out=$(git-clang-format-18 ... --diff "$base")`. But `git-clang-format --diff` exits 1 BY CONTRACT whenever it would reformat something -- that is how it says "here is a diff", not an error. Under `set -e` a failing command substitution in an assignment aborts the shell, so the entire `case` that prints `$out` was unreachable in exactly the situation it exists for. Every finding rendered as a bare exit code: no diff, no annotation, no "Run: git clang-format ..." hint. The clean path reached the report normally, which is why this survived so long. The status is now captured instead of being fatal, and an exit above 1 (a real tool failure) is told apart from a finding rather than being reported as an empty "clean". The policy. The job was `continue-on-error: true`, so it did not block -- but it still put a red mark on essentially every PR, which trains people to skim past the whole checks list. Findings are now reported and the step exits 0, always. When there is a diff it prints it in full in the log, emits a `::notice::` annotation, and writes a step-summary block with the file list and the diff (capped at 400 lines, since GITHUB_STEP_SUMMARY is dropped wholesale past 1 MiB). Being informational is only useful if the information is reachable. `continue-on-error` is dropped, because it was doing two jobs and only one was wanted. It stopped findings from blocking a merge -- now handled properly, by not failing -- and it also swallowed infrastructure failure, so a job whose apt install or checkout died looked the same as one that merely found unformatted lines. Splitting them is the point: findings are always green, while a lint that cannot run at all is a real defect and is allowed to go red, so it cannot rot unnoticed the way the reporting path just did. Verified against a deliberately mis-formatted file, which is precisely what nobody did before: the findings path prints the diff, annotates and exits 0; the clean path and the tool-failure path both exit 0 too. Also corrects .clang-format's pointer to `.github/workflows/lint.yaml`, which has not existed since the consolidation. Closes #89 --- .clang-format | 8 ++- .github/workflows/ci.yml | 122 ++++++++++++++++++++++++++++++++++----- CLAUDE.md | 6 ++ 3 files changed, 117 insertions(+), 19 deletions(-) diff --git a/.clang-format b/.clang-format index 3e7b6c4..ddbfb80 100644 --- a/.clang-format +++ b/.clang-format @@ -10,9 +10,11 @@ # # NOTE: the tree predates this file, so a whole-tree `clang-format --dry-run` # is *not* clean today -- much of the code is hand-column-aligned in ways -# clang-format cannot reproduce. CI therefore only checks the lines a PR -# actually touches (see .github/workflows/lint.yaml); a one-shot reformat is -# tracked separately so it never rides along with an unrelated change. +# clang-format cannot reproduce. CI therefore only reports on the lines a PR +# actually touches (the `lint (clang-format, changed lines)` job in +# .github/workflows/ci.yml), and reports them for information only -- findings +# never fail the build. A one-shot reformat is tracked separately so it never +# rides along with an unrelated change. BasedOnStyle: LLVM Language: Cpp diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c1196fd..29c6c13 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -192,19 +192,32 @@ jobs: - name: Every `<<<` is inside the launch helper run: python3 tools/check-cuda-launches.py --check + # Purely informational, and that is a decision rather than a compromise: + # .clang-format was derived from existing code, but the tree predates it and + # is hand-column-aligned in many places, so a whole-tree check would flag + # essentially every file. Checking only the lines a PR touches makes new code + # conform from now on and leaves the one-shot reformat as its own reviewable + # change. + # + # So findings NEVER fail this job -- they are reported and the step exits 0. + # A gate that is red on essentially every PR is not a gate, it is training + # people to skim past the whole checks list, which costs more than unlinted + # whitespace does. + # + # Note there is no `continue-on-error: true` here any more. It was doing two + # things at once and only one of them was wanted: it stopped findings from + # blocking a merge (now handled properly, by not failing), and it also + # swallowed *infrastructure* failure -- a job whose apt install or checkout + # died would report the same not-quite-green as one that merely found + # unformatted lines. Splitting them is the point: findings are information + # and are always green, while a lint that cannot run at all is a real defect + # and is allowed to go red, so it cannot rot unnoticed. clang-format: name: lint (clang-format, changed lines) # Needs a base ref to diff against; there is none on a push to main. if: github.event_name == 'pull_request' runs-on: ubuntu-latest timeout-minutes: 10 - # Reports findings for visibility without blocking merges: .clang-format - # was derived from existing code, but the tree predates it and is - # hand-column-aligned in many places, so a whole-tree check would flag - # essentially every file. Checking only the lines a PR touches makes new - # code conform from now on and leaves the one-shot reformat as its own - # reviewable change. - continue-on-error: true steps: - uses: actions/checkout@v5 with: @@ -216,27 +229,104 @@ jobs: # only -- clang-format-18 and git-clang-format-18 -- and NOT the # unversioned `git-clang-format`, so both are invoked by -18 name. run: sudo apt-get update && sudo apt-get install -y clang-format-18 - - name: Check the formatting of the lines this PR changes + - name: Report the formatting of the lines this PR changes env: BASE_REF: ${{ github.event.pull_request.base.ref }} run: | set -euo pipefail base=$(git merge-base "origin/${BASE_REF}" HEAD) echo "diffing against merge base ${base}" + + # `git-clang-format --diff` exits 1 BY CONTRACT whenever it would + # reformat something: that is how it says "here is a diff", not an + # error. Under `set -e` a failing command substitution in an + # assignment aborts the shell, so everything below used to be + # unreachable in exactly the case it exists for -- every finding + # rendered as a bare exit code with no diff, no annotation and no + # hint (fastfields-lib#89). The clean path reached the report + # normally, which is why it went unnoticed for so long. + # + # So capture the status rather than dying on it. 0 = nothing to say, + # 1 = there is a diff, anything else = the tool itself failed (bad + # arguments, missing binary), which is worth telling apart from a + # finding instead of reporting an empty diff as "clean". + set +e out=$(git-clang-format-18 --binary clang-format-18 --diff \ --extensions h,hpp,inl,cpp,cu,cuh "${base}") + status=$? + set -e + + if [ "$status" -gt 1 ]; then + echo "$out" + echo "::warning::git-clang-format-18 exited ${status}; no formatting report was produced." + { + echo "### clang-format (informational)" + echo + echo "\`git-clang-format-18\` exited \`${status}\`, so no report could be produced." + } >> "$GITHUB_STEP_SUMMARY" + exit 0 + fi + case "$out" in - "no modified files to format"|"clang-format did not modify any files") - echo "clang-format: changed lines are clean" - ;; - *) - echo "$out" - echo "::error::clang-format would reformat the lines above." - echo "Run: git clang-format $base" - exit 1 + ""|"no modified files to format"|"clang-format did not modify any files") + echo "clang-format: the lines this PR changes are clean" + echo "clang-format: the lines this PR changes are clean." \ + >> "$GITHUB_STEP_SUMMARY" + exit 0 ;; esac + # There is a diff. Print it in full in the log, then surface it + # where a reviewer sees it without opening the job at all: an + # annotation on the run, and the step summary on the PR's checks + # tab. Being informational is only useful if the information is + # actually reachable. + echo "$out" + + files=$(printf '%s\n' "$out" \ + | sed -n 's|^diff --git a/\(.*\) b/.*|\1|p' | sort -u) + nfiles=$(printf '%s\n' "$files" | sed '/^$/d' | wc -l | tr -d ' ') + + echo "::notice::clang-format would reformat lines this PR touches in ${nfiles} file(s). Informational only -- this job does not fail. To apply: git clang-format ${base}" + + # GITHUB_STEP_SUMMARY is capped at 1 MiB and is dropped wholesale if + # exceeded, so cap the embedded diff; the job log always has it in + # full. + max=400 + total=$(printf '%s\n' "$out" | wc -l | tr -d ' ') + shown=$(printf '%s\n' "$out" | head -n "$max") + + { + echo "### clang-format (informational)" + echo + echo "clang-format would reformat lines this PR touches in **${nfiles} file(s)**." + echo "This does **not** fail CI: the tree predates \`.clang-format\` and is" + echo "hand-column-aligned in many places, so only the lines a PR changes are" + echo "reported, and only as information." + echo + echo "Apply just these hunks with:" + echo + echo '```sh' + echo "git clang-format ${base}" + echo '```' + echo + printf '%s\n' "$files" | sed '/^$/d' | sed 's/^/- `/; s/$/`/' + echo + echo "
Proposed diff" + echo + echo '```diff' + printf '%s\n' "$shown" + if [ "$total" -gt "$max" ]; then + echo "... truncated at ${max} of ${total} lines; the full diff is in the job log." + fi + echo '```' + echo + echo "
" + } >> "$GITHUB_STEP_SUMMARY" + + # Informational by design: findings never fail this job. + exit 0 + # ========================================================================== # THE gate: the CPU correctness suite. # ========================================================================== diff --git a/CLAUDE.md b/CLAUDE.md index 0da2591..a2629d7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -116,6 +116,12 @@ sites)` always; `test-cpu` (a 3-leg `BOUNDFLAGS`/`SPLINEFLAGS` matrix + an kernels/cpu/hub changes; `test-hub` on hub changes; `build-cuda` (two legs, one per `FF_INDEX32` position) and `compile-probe-cuda` on kernels/cuda changes. +**`lint (clang-format, changed lines)` is informational and cannot fail on +findings.** It reports what clang-format would change in the lines a PR +touches, as a `::notice::` plus a step-summary diff, and exits 0 either way — +the tree predates `.clang-format`, so a blocking check would be red on +essentially every PR. + **The `tsan` leg is the only one that runs anything in parallel.** With the shipping `GRAIN_SIZE` (32768) every workload in `tests/lib-cpu/` is below the threshold at which `parallel_for` hands work to the thread pool, so the whole From 8399f137bae3ce58a5adb3bf695a305700e294b6 Mon Sep 17 00:00:00 2001 From: Yael Date: Thu, 20 Aug 2026 21:02:41 +0000 Subject: [PATCH 2/2] ci: actually run the three convention checkers CLAUDE.md documents each of `tools/normalise-include-delimiters.py`, `tools/normalise-header-guards.py` and `tools/rename-macros.py` as enforcing its convention via `--check`. No workflow invoked any of them, so that guarantee was fictional. All three pass on `main` today, which makes this latent rather than broken -- and latent is the harder kind to notice, because nothing goes red until long after the convention has already rotted. Each of these conventions was applied across 100+ files in a single sweep (#145, #146, #91). Re-doing such a sweep because it drifted back costs far more than running three scripts on every push, and the drift they guard against -- an unprefixed macro in an installed header taking a name from every downstream translation unit, a stray `#ifndef` guard, a mis-delimited include -- is invisible to every compile and test gate in this repo. A new `lint (source conventions)` job rather than extra steps on `lint (cuda launch sites)`. They are the same shape (fast, stdlib-Python, whole-tree textual invariants), but #154 built that job's name and its comment around one specific rule, and a red tick reading "cuda launch sites" for a header-guard violation would misdirect. Keeping them separate also avoids renaming a check that landed two commits ago. Each checker is its own step, so the failing tick names the convention, and the later steps carry `if: !cancelled()` so a change tripping two of them hears about both in one run instead of one per push. These DO fail the build, unlike the clang-format job. That one reports a style opinion about a tree predating the style file; these encode decisions already taken and already applied everywhere, so a violation is a defect. The job installs nothing -- no apt, not even setup-python -- and uses the runner image's interpreter. All three are pure stdlib and read files. Four jobs were killed at their timeout-minutes inside `apt-get` during the 2026-08-19 mirror stall while `test-hub` rode it out precisely because it installs nothing; a gate that runs in seconds should not have a package mirror on its critical path. Verified each checker still fires by injecting a violation of each into `include/fastfields/api/distance.h` in turn -- a quoted public include, a whole-file `#ifndef` guard, and an unprefixed `#define` -- and confirming exit 1 with a message naming the file and line, the same way #154 proved its lint fires with a rogue launch. --- .github/workflows/ci.yml | 49 ++++++++++++++++++++++++++++++++++++++++ CLAUDE.md | 24 +++++++++++++------- 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 29c6c13..93f16b4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -192,6 +192,55 @@ jobs: - name: Every `<<<` is inside the launch helper run: python3 tools/check-cuda-launches.py --check + # The three source conventions that had a checker but no gate. + # + # CLAUDE.md documents each of these as "enforced by tools/.py --check", + # and until this job existed that was simply not true: nothing invoked any of + # them. All three passed on main, so the guarantee was fictional rather than + # broken -- which is the harder kind to notice, and the kind that quietly + # stops being true the first time somebody adds a header. Each convention was + # applied across 100+ files in one sweep (#145, #146, #91); re-doing that + # sweep because it rotted is much more expensive than running three scripts + # on every push. + # + # Unlike the clang-format job below, these DO fail the build, and the + # difference is deliberate. clang-format reports a style opinion about a tree + # that predates the style file. These encode decisions already taken and + # already applied everywhere, so a violation is a defect: an unprefixed macro + # in an installed header is a name taken from every downstream translation + # unit, and a stray `#ifndef` guard or a mis-delimited include is drift back + # towards the four-conventions-at-once state the consolidation removed. + # + # Kept off `apt` on purpose. All three are pure-stdlib Python that read files + # and exit, needing no toolchain at all, so this job installs nothing and + # uses the runner image's own interpreter -- the same reasoning as `test-hub` + # below, which survived the 2026-08-19 mirror stall precisely because it + # installs nothing while four jobs that do were killed at their + # timeout-minutes inside `apt-get`. A gate that runs in seconds should not + # have a package mirror on its critical path. + # + # Unconditional and not path-filtered, like the launch-site lint: these are + # properties of the whole tree, and the cost is three Python startups. + conventions: + name: lint (source conventions) + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v5 + # Each check is its own step so the red tick in the UI names the + # convention that broke, and each later one runs even after an earlier + # failure (`!cancelled()`) so a PR that trips two of them is told about + # both in one run rather than one per push. The job still fails if any + # step did. + - name: Public includes use , private ones quotes + run: python3 tools/normalise-include-delimiters.py --check + - name: Every header opens with `#pragma once` + if: ${{ !cancelled() }} + run: python3 tools/normalise-header-guards.py --check + - name: Every macro on the installed surface is FF_-prefixed + if: ${{ !cancelled() }} + run: python3 tools/rename-macros.py --check + # Purely informational, and that is a decision rather than a compromise: # .clang-format was derived from existing code, but the tree predates it and # is hand-column-aligned in many places, so a whole-tree check would flag diff --git a/CLAUDE.md b/CLAUDE.md index a2629d7..ac84bd0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -110,17 +110,21 @@ the CPU path is the tested source of truth and CUDA is **compile+link only**. ## CI -`.github/workflows/ci.yml`, path-filtered. `codespell` and `lint (cuda launch -sites)` always; `test-cpu` (a 3-leg `BOUNDFLAGS`/`SPLINEFLAGS` matrix + an -`INDEXFLAGS` leg + a g++ leg), `sanitize` (ASan+UBSan) and `tsan` on -kernels/cpu/hub changes; `test-hub` on hub changes; `build-cuda` (two legs, one -per `FF_INDEX32` position) and `compile-probe-cuda` on kernels/cuda changes. +`.github/workflows/ci.yml`, path-filtered. `codespell`, `lint (cuda launch +sites)` and `lint (source conventions)` always; `test-cpu` (a 3-leg +`BOUNDFLAGS`/`SPLINEFLAGS` matrix + an `INDEXFLAGS` leg + a g++ leg), +`sanitize` (ASan+UBSan) and `tsan` on kernels/cpu/hub changes; `test-hub` on +hub changes; `build-cuda` (two legs, one per `FF_INDEX32` position) and +`compile-probe-cuda` on kernels/cuda changes. **`lint (clang-format, changed lines)` is informational and cannot fail on findings.** It reports what clang-format would change in the lines a PR touches, as a `::notice::` plus a step-summary diff, and exits 0 either way — the tree predates `.clang-format`, so a blocking check would be red on -essentially every PR. +essentially every PR. The three checks in `lint (source conventions)` are the +opposite and *do* fail: they enforce conventions already applied tree-wide +(`normalise-include-delimiters`, `normalise-header-guards`, `rename-macros`), +so a violation there is a defect rather than a style opinion. **The `tsan` leg is the only one that runs anything in parallel.** With the shipping `GRAIN_SIZE` (32768) every workload in `tests/lib-cpu/` is below the @@ -232,6 +236,8 @@ pushpull's fully-static order×bound compile is nightly library, so those *are* `` there) and the non-nvcc `__device__` / `__host__` fallbacks. Prefer an `inline` function to a macro where one will do — a function in `ff::` is collision-safe without any prefix. + `tools/rename-macros.py --check` enforces this, and runs in CI on every push, + in the `lint (source conventions)` job. - **`` for the public interface, `"…"` for private headers.** `include/fastfields/` is what gets installed and what `fastfields-dlpack` puts on its include path, so it is spelled with angle brackets like any other @@ -241,7 +247,8 @@ pushpull's fully-static order×bound compile is nightly configuration and there is no `-iquote` anywhere — so this is about saying which category a dependency is in, not about lookup. `tools/normalise-include-delimiters.py --check` enforces it, and also checks - the converse: every quoted include must resolve beside its includer. + the converse: every quoted include must resolve beside its includer. It runs + in CI on every push, in the `lint (source conventions)` job. - **`#pragma once` on line 1 of every header — no `#ifndef` include guards.** Line 1 with no exception, licence and provenance comments included; they keep their text and sit one line lower. **`include/fastfields/core/dlpack.h` is @@ -260,7 +267,8 @@ pushpull's fully-static order×bound compile is nightly the `FF_*_MAX_NBATCH` / `FF_AUTOCAST_PINNED_HOST` build knobs all stay. Enforced by `tools/normalise-header-guards.py --check`, which also applies the convention and audits that no guard macro is tested from another file — - the one way deleting a `#define` could change what compiles. + the one way deleting a `#define` could change what compiles. It runs in CI on + every push, in the `lint (source conventions)` job. - `include/fastfields/core/dlpack.h` is vendored upstream code: do not edit it, and it is skipped by `codespell` (see `.codespellrc`). It is the only verbatim third-party file in the tree.