Skip to content

S24.13: switch IWYU lanes to advisory #468

Description

@DavidCozens

Parent epic: #254 (E24: Code Hygiene)

Switch the two IWYU CI lanes (analyze-iwyu, analyze-iwyu-freertos-plustcp)
to advisory — they still run on every PR and upload their artifacts, but
findings no longer fail the build.

Motivation

A trial swap from IWYU to clang-tidy --checks=misc-include-cleaner
(the originally-scoped story) revealed that misc-include-cleaner's
built-in stdlib mapping has substantial gaps on POSIX extension symbols
(CLOCK_REALTIME, SOL_SOCKET, SO_KEEPALIVE, POSIX timeval, etc.),
producing roughly 75 % false positives on the Platform/Posix/ tree
(10 false positives vs 3 real findings in a 13-finding sample). The
check's only configuration knobs are IgnoreHeaders (per-header global
silencing) and DeduplicateFindings; no IWYU-style mapping file
support. So switching tools didn't actually solve the problem — it
exchanged one accuracy hit for another.

The simpler, higher-throughput move is to keep IWYU but stop it
blocking PRs:

  • Per-PR friction (IWYU's two-pass "should add / should remove" output)
    costs a measurable chunk of every CI cycle.
  • The actual signal — when IWYU is right — is best dealt with in
    batches around release prep, not in dribs and drabs in feature PRs.
  • cppcheck stays required: its false-positive rate is low and S24.12
    just audited its suppressions.

The misc-include-cleaner / clang-include-cleaner standalone /
alternate-tool investigation moves to a separate candidate idea under
E24, surfaced if we revisit the swap.

Architecture boundary

CI workflow only. No production-code or test-code changes. Two
continue-on-error: true additions on the IWYU Run steps in
.github/workflows/ci.yml, plus a docs/local-checks.md refresh to
reflect the new tier shape.

Acceptance criteria

  • analyze-iwyu and analyze-iwyu-freertos-plustcp lanes still
    run on every PR.
  • Both lanes still upload their report artifacts
    (iwyu-report, iwyu-report-freertos-plustcp).
  • Neither lane's findings fail the build going forward.
  • docs/local-checks.md reflects the new advisory status and the
    release-prep sweep responsibility.

Out of scope

  • Replacing IWYU outright — superseded by the conclusion above; the
    alternative-tool investigation goes back into E24's candidate ideas.
  • Making cppcheck advisory — low false-positive rate; staying required.
  • Adjusting branch-protection required checks — IWYU lanes report
    success going forward (via step-level continue-on-error), so the
    required-checks list does not need a UI edit.

Notes for the next pass at this

If we revisit a tool swap, key constraints we discovered:

  • misc-include-cleaner's stdlib mapping covers ISO C cleanly, has gaps
    on POSIX extension macros and structs.
  • It honours the existing // IWYU pragma: keep / : export /
    : private directives, so a swap would not require pragma rewrites.
  • It folds into the existing per-TU clang-tidy compile step (via
    CMAKE_C_CLANG_TIDY / CMAKE_CXX_CLANG_TIDY), so no separate lane
    / preset is needed.
  • The standalone clang-include-cleaner tool (not the clang-tidy
    check) does support IWYU-style mapping files and may close the
    POSIX gap, but that path was not explored in this story.

Metadata

Metadata

Assignees

No one assigned

    Labels

    storyStory issue

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions