Skip to content

ci: S24.13 switch IWYU lanes to advisory #469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DavidCozens merged 1 commit into from
May 28, 2026
Merged

ci: S24.13 switch IWYU lanes to advisory #469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
66 changes: 66 additions & 0 deletions DEVLOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 `<time.h>`, `SOL_SOCKET` / `SO_KEEPALIVE` not mapped to
`<sys/socket.h>`, `timeval` not mapped to `<sys/time.h>`. 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
`<time.h>` + `<sys/time.h>` 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 `<stdint.h>` 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.
Comment on lines +48 to +50

@coderabbitai coderabbitai Bot May 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Document consistency risk: branch-protection note may conflict with CI docs.

This says branch protection is unchanged with advisory step behavior, but docs/ci.md still describes IWYU as a failing gate. Please add a follow-up note/link here (or update the CI docs in the same change) to avoid conflicting guidance for contributors.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@DEVLOG.md` around lines 48 - 50, The DEVLOG entry that reads "Branch
protection list unchanged. Step-level `continue-on-error` makes the job report
success on findings..." conflicts with existing CI documentation that still
treats IWYU as a failing gate; either add a follow-up note/link in that DEVLOG
entry pointing to the CI documentation and explicitly stating IWYU is now
advisory under step-level continue-on-error, or update the CI docs to reflect
the new IWYU advisory behavior so both places are consistent (look for the exact
DEVLOG sentence and the IWYU/CI docs references to patch).


### 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
Expand Down
63 changes: 30 additions & 33 deletions docs/local-checks.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,42 +10,56 @@ 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,
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 \
Expand All @@ -54,32 +68,15 @@ 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

- `tidy`, `sanitize`, `coverage` — minutes each, all gated by CI
- 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.
Loading