doc: clarify local IWYU workflow and pragmas #35152

l0rinc commented at 2:13 PM on April 24, 2026: contributor

Problem

This was prompted by #34435 (review), where it was not clear to me how (and where) exceptional IWYU cases should be documented.

Fix

This PR documents the IWYU CI wrapper as the reproducible local entrypoint.

The developer notes now recommend reducing suspected IWYU false positives to a minimal upstream reproducer, treat IWYU pragma as a narrow workarounds, and ask for nearby rationale comments on non-obvious IWYU pragma use. An example comment was also added.

The IWYU patch comment is also updated to point at the current clang_22 include picker reference.

Reproducer

Create a dummy commit on top that adds an unused include, then run the command from the developer notes. Without the dummy commit, the command should pass.

<details><summary>IWYU demo commit</summary>

diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
--- a/src/kernel/bitcoinkernel.cpp	(revision c92b329e7b7d49476b5977d26c24d7c4982c6024)
+++ b/src/kernel/bitcoinkernel.cpp	(revision ad2c5ba2ba69156e77061c1e6c098b725c28f322)
@@ -43,6 +43,7 @@
 #include <functional>
 #include <list>
 #include <memory>
+#include <vector>
 #include <span>
 #include <stdexcept>
 #include <string>

</details>

[!NOTE] After repeated failing runs, docker container rm -f ci_native_iwyu may be needed because the local CI wrapper can leave the detached container running when the inner test command fails.

DrahtBot renamed this:
~~doc: clarify local IWYU workflow and pragmas~~
doc: clarify local IWYU workflow and pragmas
on Apr 24, 2026

DrahtBot added the label Docs on Apr 24, 2026

DrahtBot commented at 2:14 PM on April 24, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

in ci/test/03_test_script.sh:213 in c92b329e7b outdated

 207 | @@ -208,6 +208,13 @@ if [ "${RUN_TIDY}" = "true" ]; then
 208 |  fi
 209 |  
 210 |  if [[ "${RUN_IWYU}" == true ]]; then
 211 | +  # Optionally narrow IWYU to files changed on the current branch vs origin/master.
 212 | +  if [[ "${IWYU_FILTER_MODIFIED:-}" == "1" ]]; then
 213 | +    mapfile -t MODIFIED < <(git --git-dir="${BASE_ROOT_DIR}/.git_ci_backup" diff --name-only origin/master...HEAD)

maflcko commented at 2:29 PM on April 24, 2026:

origin/master may be stale:

$ git log origin/master -1
commit 44dbf918d3ab073c3d0d1d78627e1201af582d3c
Date:   Tue Mar 22 08:58:30 2022 +0000

Maybe just use the last merge commit, assuming that that must be clean?

l0rinc commented at 2:40 PM on April 24, 2026:

just use the last merge commit

Isn't that basically a linear search?

You mean something like:

    LAST_MERGE_COMMIT="$(git --git-dir="${BASE_ROOT_DIR}/.git_ci_backup" rev-list --max-count=1 --merges HEAD)"
    mapfile -t MODIFIED < <(git --git-dir="${BASE_ROOT_DIR}/.git_ci_backup" diff --name-only "${LAST_MERGE_COMMIT}...HEAD")

Isn't that just more complicated?

maflcko commented at 2:49 PM on April 24, 2026:

I think if someone is setting IWYU_FILTER_MODIFIED they'd run it on a pull request after it failed CI, so they are only interested in the files modified by the pull request, not all files modified since they cloned the repo.

LAST_MERGE_COMMIT="$(git --git-dir="${BASE_ROOT_DIR}/.git_ci_backup" rev-list --max-count=1 --merges HEAD)"

Yeah, that looks right.

l0rinc commented at 3:04 PM on April 24, 2026:

Done, thanks, it still works locally with the mentioned reproducer.

maflcko approved

maflcko commented at 2:30 PM on April 24, 2026: member

lgtm

I haven't reviewed the mapfile Bash, and the jq, but otherwise this looks fine.

in doc/developer-notes.md:556 in c92b329e7b

 555 |  1. Facade headers. For example, see [`compat/compat.h`](/src/compat/compat.h).
 556 |  2. Drop-in replacement headers. For example, see [`util/time.h`](/src/util/time.h).
 557 |  3. Presenting a complete interface across multiple headers.
 558 |  
 559 | -A comment explaining the rationale is required for every use of `IWYU pragma: export`.
 560 | +For non-obvious uses of either pragma, add a nearby source comment explaining why it is needed.

fanquake commented at 2:35 PM on April 24, 2026:

I think you can drop "Non-obvious", as that's vague, and any use of either of these is basically a upstream bug, or should have rationale.

l0rinc commented at 2:44 PM on April 24, 2026:

Done, thanks.

l0rinc force-pushed on Apr 24, 2026

DrahtBot added the label CI failed on Apr 24, 2026

hebasto commented at 2:48 PM on April 24, 2026: member

Concept ACK.

l0rinc force-pushed on Apr 24, 2026

DrahtBot removed the label CI failed on Apr 24, 2026

in doc/developer-notes.md:536 in f425d05690

 530 | @@ -531,22 +531,29 @@ The generated coverage report can be accessed at `build/coverage_report/index.ht
 531 |  The [`include-what-you-use`](https://github.com/include-what-you-use/include-what-you-use) tool (IWYU)
 532 |  helps to enforce the source code organization [policy](#source-code-organization) in this repository.
 533 |  
 534 | -To ensure consistency, it is recommended to run the IWYU CI job locally rather than running the tool directly.
 535 | +To reproduce the IWYU CI job locally, run:
 536 | +```bash
 537 | +env -i HOME="$HOME" PATH="$PATH" USER="$USER" IWYU_FILTER_MODIFIED=1 FILE_ENV="./ci/test/00_setup_env_native_iwyu.sh" ./ci/test_run_all.sh || echo "IWYU failed"

hebasto commented at 8:03 AM on April 27, 2026:

c95a6c8db25a961af66e2955e86d797801afa06f

Is there a specific reason for omitting the MAKEJOBS environment variable?

l0rinc commented at 5:50 PM on April 28, 2026:

None, just wasn't aware it's required, added it in latest push, thanks.

hebasto commented at 3:29 PM on April 29, 2026:

While not strictly required, it improves performance by utilising iwyu_tool.py's parallelism. I don't think MAKEJOBS is commonly set in most users' environments. I meant something like this: https://github.com/bitcoin/bitcoin/blob/cef58341a0b2b06ca4ebfe8590cd8e93b09ce72a/ci/README.md?plain=1#L65

l0rinc commented at 4:22 PM on April 29, 2026:

Forgot this in last push - applied, thanks.

in ci/test/03_test_script.sh:218 in f425d05690

 213 | +    LAST_MERGE_COMMIT="$(git --git-dir="${BASE_ROOT_DIR}/.git_ci_backup" rev-list --max-count=1 --merges HEAD)"
 214 | +    mapfile -t MODIFIED < <(git --git-dir="${BASE_ROOT_DIR}/.git_ci_backup" diff --name-only "${LAST_MERGE_COMMIT}...HEAD")
 215 | +    jq --arg root "${BASE_ROOT_DIR}/" 'map(select(.file | ltrimstr($root) | IN($ARGS.positional[])))' "${BASE_BUILD_DIR}/compile_commands.json" --args "${MODIFIED[@]}" > "${BASE_BUILD_DIR}/compile_commands_modified.json"
 216 | +    mv "${BASE_BUILD_DIR}/compile_commands_modified.json" "${BASE_BUILD_DIR}/compile_commands.json"
 217 | +  fi
 218 | +

hebasto commented at 8:36 AM on April 27, 2026:

f425d056906afab6bf74f0a522a2170a274c0745

I'm concerned that IWYU_FILTER_MODIFIED could be misused. Instead of using it to get a minimal reproducer for an IWYU bug, someone might use it as a quick way to verify header changes. This approach could be risky since the entire codebase isn't fully covered by enforced IWYU checks yet. Some source files might currently only compile due to transitive inclusions, which leads to the following compilation failure.

l0rinc commented at 5:49 PM on April 28, 2026:

Done, let me know what you think.

hebasto commented at 3:32 PM on April 29, 2026:

Looks good.

hebasto commented at 8:40 AM on April 27, 2026: member

Would it be possible to address this comment in this PR as well? Something like the following should work:

--- a/src/node/blockstorage.h
+++ b/src/node/blockstorage.h
@@ -18,6 +18,8 @@
 #include <streams.h>
 #include <sync.h>
 #include <uint256.h>
+// IWYU incorrectly suggests removing this header.
+// See https://github.com/include-what-you-use/include-what-you-use/issues/2014.
 #include <util/byte_units.h> // IWYU pragma: keep
 #include <util/expected.h>
 #include <util/fs.h>

in doc/developer-notes.md:556 in f425d05690

 555 |  1. Facade headers. For example, see [`compat/compat.h`](/src/compat/compat.h).
 556 |  2. Drop-in replacement headers. For example, see [`util/time.h`](/src/util/time.h).
 557 |  3. Presenting a complete interface across multiple headers.
 558 |  
 559 | -A comment explaining the rationale is required for every use of `IWYU pragma: export`.
 560 | +For either pragma, add a nearby source comment explaining why it is needed.

hebasto commented at 10:29 AM on April 28, 2026:

Cross-posting @purpleKarrot's comment here:

There are cases where annotations are self-explanatory, so "A comment explaining the rationale is required" might be weakened to "... is recommended".

l0rinc commented at 5:47 PM on April 28, 2026:

Added doc for associated, to use IWYU pragmas sparingly and softened source-comment recommendation, added you and @purpleKarrot as coauthors, thanks.

fanquake commented at 4:05 PM on April 29, 2026:

I think the new wording is fine. However I would say that basically all pragma usage we've seen so far, has either been hiding bugs in our own code, or bugs upstream in IWYU; hence the pushback on adding any usage that hasn't been documented / properly reported.

l0rinc force-pushed on Apr 28, 2026

hebasto commented at 3:32 PM on April 29, 2026: member

Approach ACK 5326cb121cc037ed96f030680474eae5114025af.

ci: update IWYU patch reference

Point the patch comment at the `clang_22` stdlib C include map now used by the native IWYU job.

7c7cec4567

in doc/developer-notes.md:547 in 5326cb121c

hebasto commented at 3:47 PM on April 29, 2026:

What about headers without associated source files? IWYU processes them only via the -check_also option: https://github.com/bitcoin/bitcoin/blob/cef58341a0b2b06ca4ebfe8590cd8e93b09ce72a/ci/test/03_test_script.sh#L224

l0rinc commented at 4:02 PM on April 29, 2026:

Thanks for the comments, I have dropped the last commit.

l0rinc force-pushed on Apr 29, 2026

doc: clarify IWYU workflow

Document the CI wrapper as the reproducible IWYU entrypoint instead of suggesting ad hoc native runs.
Also describe how to handle suspected false positives, explain when local `IWYU pragma` workarounds are appropriate, and add an example rationale to an existing pragma.

Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-authored-by: Daniel Pfeifer <daniel@pfeifer-mail.de>

d084bc88be

l0rinc force-pushed on Apr 29, 2026

DrahtBot added the label CI failed on Apr 29, 2026

hebasto approved

hebasto commented at 4:24 PM on April 29, 2026: member

ACK d084bc88bebe75484e319e0cdbf72a656edebfa3.

DrahtBot removed the label CI failed on Apr 29, 2026