From bdbf58a118ac50166e9f666e3cd2e74325b9c2bb Mon Sep 17 00:00:00 2001 From: David Cozens Date: Thu, 28 May 2026 20:22:16 +0100 Subject: [PATCH] ci: S24.13 switch IWYU lanes to advisory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step-level continue-on-error on the Run include-what-you-use step in both analyze-iwyu and analyze-iwyu-freertos-plustcp. Findings still upload via the existing iwyu-report* artifacts; release prep is now responsible for sweeping them. docs/local-checks.md Tier B refreshed to drop IWYU as a required pre-push step. Story originally scoped as "replace IWYU with clang-tidy misc-include-cleaner" — survey revealed misc-include-cleaner's built-in stdlib mapping has ~75% false-positive rate on POSIX extension symbols (CLOCK_REALTIME, SOL_SOCKET, timeval, etc.) with no IWYU-style mapping file support to filter. Switching IWYU to advisory delivers the throughput win the original swap was meant to deliver, without trading one FP surface for another. Branch protection unchanged — step-level continue-on-error keeps the job reporting success on findings, so required-checks still satisfied. Closes #468 Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/ci.yml | 12 ++++++-- DEVLOG.md | 66 ++++++++++++++++++++++++++++++++++++++++ docs/local-checks.md | 63 ++++++++++++++++++-------------------- 3 files changed, 106 insertions(+), 35 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a73406e6..95328c8d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -459,8 +459,14 @@ jobs: - name: Configure run: cmake --preset iwyu - - name: Run include-what-you-use + # IWYU is advisory: the two-pass "should add / should remove" output is + # hard to interpret correctly under time pressure, and the per-PR + # friction it causes outweighs the steady-state value (S24.13 / E24). + # Findings still surface via the uploaded artifact below; release prep + # is responsible for clearing them. + - name: Run include-what-you-use (advisory) shell: bash + continue-on-error: true run: set -o pipefail && cmake --build --preset iwyu --target iwyu 2>&1 | tee build/iwyu/iwyu-output.txt - name: Upload iwyu output @@ -526,8 +532,10 @@ jobs: - name: Configure run: cmake --preset iwyu -DCMAKE_C_COMPILER=clang-19 -DCMAKE_CXX_COMPILER=clang++-19 - - name: Run include-what-you-use + # Advisory per S24.13 — see analyze-iwyu above for rationale. + - name: Run include-what-you-use (advisory) shell: bash + continue-on-error: true run: set -o pipefail && cmake --build --preset iwyu --target iwyu 2>&1 | tee build/iwyu/iwyu-output.txt - name: Upload iwyu output diff --git a/DEVLOG.md b/DEVLOG.md index eeb372a2..8b8071b8 100644 --- a/DEVLOG.md +++ b/DEVLOG.md @@ -1,5 +1,71 @@ # Dev Log +## 2026-05-28 — S24.13 IWYU lanes switched to advisory + +Pivot story under E24. Started out scoped as "replace IWYU with +clang-tidy's `misc-include-cleaner`, diff-scoped" — followed the +proven `analyze-tidy*` lane pattern S24.11 established (clang-tidy in +the cpputest-freertos image) so the swap would have been a +configuration-only change with no separate lane. Halfway through the +finding survey, misc-include-cleaner turned out to have substantial +stdlib-mapping gaps on POSIX extensions: `CLOCK_REALTIME` not mapped +to ``, `SOL_SOCKET` / `SO_KEEPALIVE` not mapped to +``, `timeval` not mapped to ``. About 75 % +of the findings on `Platform/Posix/` were false positives the +two-knob check (`IgnoreHeaders` + `DeduplicateFindings`) couldn't +filter cleanly. Confirmed with a minimal repro: no combination of +`` + `` satisfies the check for `CLOCK_REALTIME`. + +The original motivation for the swap was IWYU's per-PR friction +(two-pass output, container drift, line-shift cascades). The simpler +move that actually buys us throughput is to keep IWYU and stop it +blocking PRs. Two `continue-on-error: true` additions on the IWYU +`Run` steps in `.github/workflows/ci.yml`. cppcheck stays required — +S24.12 just audited its suppressions and the FP rate is low. + +`docs/local-checks.md` updated: Tier B drops the IWYU-pre-push +requirement and `clang-format -i` over touched files is now Tier B's +sole expensive step. Release prep picks up the IWYU artifact sweep. + +Reverted the partial misc-include-cleaner work I'd already started +(eight Core/Source files with newly-added `` and +`SolidSyslogPrival.h` includes plus a `Tests/.clang-tidy` +`IgnoreHeaders` entry for CppUTest) — those were legitimate +improvements but stopped being in scope once the story shape changed. +The misc-include-cleaner investigation, plus a note on the standalone +`clang-include-cleaner` tool (which does support IWYU-style mapping +files and may close the POSIX gap), goes back into E24's candidate +ideas to revisit if we want to retry the swap later. + +### What stays + +- `analyze-iwyu` / `analyze-iwyu-freertos-plustcp` lanes still run on + every PR. +- Both lanes still upload `iwyu-report` / `iwyu-report-freertos-plustcp` + artifacts. +- The `iwyu` CMake preset and the `ENABLE_IWYU` plumbing stay — the + local-run recipe in `docs/local-checks.md` still works. +- Branch protection list unchanged. Step-level `continue-on-error` + makes the job report success on findings, so the required-check + contract is still satisfied without a UI edit. + +### What changed + +- The IWYU step in both lanes is renamed `Run include-what-you-use (advisory)` + and gets `continue-on-error: true`. +- `docs/local-checks.md` Tier B narrative drops IWYU as a required + step and gains an "(optional, advisory)" subsection. + +### Open follow-ups + +- Future story under E24 — investigate `clang-include-cleaner` + standalone (mapping-file support) as a replacement candidate. Land + it via a feature-branch experiment that runs side-by-side with + advisory IWYU for one PR cycle, only promote if FP rate clears + ~95 %. +- Release-prep checklist gains "sweep the IWYU artifact" — will land + alongside whatever release-prep doc owns checklists. + ## 2026-05-28 — S28.05 SolidSyslogLwipRawTcpStream Fifth story in E28 (#465, parent #439). TCP stream adapter for the diff --git a/docs/local-checks.md b/docs/local-checks.md index b3fcc11f..5be16c86 100644 --- a/docs/local-checks.md +++ b/docs/local-checks.md @@ -10,9 +10,14 @@ when, so the wait stays under a few minutes and CI catches the rest. | Tier | When | What | Wall-clock | |---|---|---|---| | **A** — fast feedback | Every commit on the branch | `cmake --build --preset debug --target junit` for whatever preset matches the diff (gcc / clang / freertos-host) | ~30–60 s | -| **B** — pre-push | First push to the branch and any push that changes production source | A + `iwyu` + format reflowed includes + `misra_renumber.py` | ~3–5 min | +| **B** — pre-push | First push to the branch and any push that changes production source | A + format reflowed includes + `misra_renumber.py` | ~2–3 min | | **C** — none | — | — | — | -| **CI** — everything else | After push | `tidy`, `sanitize`, `coverage`, Windows, BDD, integration, FreeRTOS host/cross, IWYU on cpputest-clang, MISRA on cpputest | runs in parallel; results in ~10–15 min | +| **CI** — everything else | After push | `tidy`, `sanitize`, `coverage`, Windows, BDD, integration, FreeRTOS host/cross, advisory IWYU, MISRA on cpputest | runs in parallel; results in ~10–15 min | + +**IWYU is advisory** (S24.13 / E24). The lanes still run on every PR and +the report is uploaded as an artifact, but findings no longer fail the +build. Sweep the IWYU artifact when you do a release cleanup; do not +treat it as a per-PR blocker. Format-on-save in the editor handles formatting per-edit, so no separate `analyze-format` step locally. If you skip an editor with format-on-save, @@ -20,32 +25,41 @@ add a `clang-format -i` sweep over touched files to Tier A. ## Path-gating Tier B -Tier B does the expensive bits (IWYU, MISRA), so scope it to what changed: +Tier B does MISRA-line-drift cleanup, so scope it to what changed: - **Touched only `Tests/`, `Bdd/Targets/`, `docs/`, `cmake/`, or `*.md`** — skip Tier B entirely. Push and let CI run. - **Touched any `Core/Source/`, `Platform/*/Source/`, or public-header file** - — run Tier B against the container that compiles that tree (see below). + — run `clang-format -i` over touched files and + `scripts/misra_renumber.py --apply` to update the suppressions. ## Running Tier B -The IWYU lane runs against clang's parser. cpputest-freertos and cpputest -both ship the same IWYU binary now (clang_19 branch), so pick the container -whose env vars include the trees you touched: +### MISRA — fix line-number drift + +When edits shift production lines, `misra_suppressions.txt` entries go +stale. Fix in one step: + +```bash +# In any container that has cppcheck (all of them do): +scripts/misra_renumber.py # show proposed renumbers +scripts/misra_renumber.py --apply # write back updated suppressions +``` + +The script bails on genuine new findings (mismatched counts per +rule+file) — those need manual review. See the script's docstring. + +### IWYU (optional, advisory) -### Touched only Core / Posix / Windows / OpenSSL trees +If you want a local look before push, the lane is still wired: ```bash -# Inside the gcc devcontainer (Ctrl+Shift+B-able), or: docker compose -f .devcontainer/docker-compose.yml run --rm clang \ bash -c 'cmake --preset iwyu && cmake --build --preset iwyu --target iwyu' ``` -Read the full IWYU output — head/tail truncation hides findings. After -fixing IWYU's complaints, format the file (`clang-format -i path`) — the -include reorder can put forward-decls on lines that need reflowing. - -### Touched any FreeRTOS / Plus-TCP / lwIP / MbedTLS / FatFs tree +For FreeRTOS / Plus-TCP / lwIP / MbedTLS / FatFs trees, use `freertos-host` +with the clang-19 overrides instead: ```bash docker compose -f .devcontainer/docker-compose.yml run --rm freertos-host \ @@ -54,23 +68,8 @@ docker compose -f .devcontainer/docker-compose.yml run --rm freertos-host \ && cmake --build --preset iwyu --target iwyu' ``` -The `-DCMAKE_C_COMPILER=clang-19` overrides match the -`analyze-iwyu-freertos-plustcp` CI lane — the gcc base image carries -`clang-19` but no `clang` alternative. - -### MISRA — after IWYU, fix line-number drift - -When IWYU's include reorder shifts production lines, `misra_suppressions.txt` -entries go stale. Fix in one step: - -```bash -# In any container that has cppcheck (all of them do): -scripts/misra_renumber.py # show proposed renumbers -scripts/misra_renumber.py --apply # write back updated suppressions -``` - -The script bails on genuine new findings (mismatched counts per -rule+file) — those need manual review. See the script's docstring. +CI runs both lanes advisory — findings appear in the `iwyu-report` and +`iwyu-report-freertos-plustcp` artifacts and don't block the build. ## What CI runs and you should not run locally @@ -78,8 +77,6 @@ rule+file) — those need manual review. See the script's docstring. - Windows MSVC + BDD + integration — depend on tools you may not have - BDD-linux-syslog-ng, BDD-windows-otel, BDD-freertos-qemu — heavy multi-container stacks -- The cpputest-clang IWYU lane and the cpputest-freertos IWYU/tidy lanes — - CI runs both; you only need one locally for the trees you touched If CI surfaces a finding you missed locally, fix in another commit on the same branch — cheaper than running every CI lane on every push.