ci: Add IWYU job #33810

pull hebasto wants to merge 6 commits into bitcoin:master from hebasto:251106-force-iwyu-ci changing 9 files +96 −11
  1. hebasto commented at 11:27 pm on November 6, 2025: member

    This PR separates the IWYU checks into its own CI job to provide faster feedback to developers. No other changes are made to the treatment of IWYU warnings. The existing “tidy” CI job will no longer run IWYU.

    See also the discussion of #33779, specifically this comment:

    Maybe a better approach would be to run the enforced sections in a separate, faster job? Some of the linters are already a bit annoying to invoke locally, so I usually just run the lint job. Doing the same for the includes seems fine to me.

    Based on ideas from #32953.

  2. hebasto added the label Tests on Nov 6, 2025
  3. DrahtBot commented at 11:28 pm on November 6, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33810.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK w0xlt, stickies-v

    If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31425 (RFC: Riscv bare metal CI job by sedited)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  4. w0xlt commented at 0:00 am on November 7, 2025: contributor
    Concept ACK
  5. in src/crypto/hex_base.cpp:11 in f72b2f7f58 outdated 
     7@@ -8,7 +8,6 @@
 8 #include <cassert>
 9 #include <cstring>
10 #include <string>
11-#include <tuple>
    maflcko commented at 10:49 am on November 7, 2025:

    I think both CI tasks should use the same config (debian:trixie). Otherwise, if someone tries to reproduce the CI config, they two configs will contradict each other, which doesn’t seem helpful?

    Also, I find it confusing that one CI task is printing the iwyu errors and the other is printing the warnings. What is the goal here? It seems clearer to just print the errors and warnings in one task, like it was done before.

    As mentioned previously, iwyu takes 9 minutes (https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3491592008) and in this pull it takes 30 seconds and you claim that it is faster. However, if the goal is to eventually run it on more stuff, it will take 9 minutes again. So I’d say we should also combine iwyu errors and warnings into one task, to be honest and accurate about the expected long-term runtime of the task.

    hebasto commented at 11:37 am on November 7, 2025:
    Reworked into a single job.
  6. fanquake commented at 11:14 am on November 7, 2025: member

    providing convenient feedback to developers.

    How can we improve the quality of the feedback (https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3496969502)? Getting it faster is nice, but if it can’t be taken and applied directly without fixing the sorting, and the formatting, and in the worse case, changing headers entirely for modernize-deprecated-headers, then it doesn’t seem that much more useful.

  7. hebasto commented at 11:26 am on November 7, 2025: member

    but if it can’t be taken and applied directly without fixing the sorting, and the formatting, and in the worse case, changing headers entirely for modernize-deprecated-headers, then it doesn’t seem that much more useful.

    I disagree. In practice, when a developer works on changes that modify includes, the diff in the includes would typically be just a few lines. Using the diff from the CI job as a hint seems entirely reasonable. I don’t see a strong need to increase the script’s complexity just to produce a diff that can be immediately applied.

  8. hebasto force-pushed on Nov 7, 2025
  9. in ci/test/03_test_script.sh:44 in 9bb013d8e5 
    43@@ -44,7 +44,7 @@ echo "=== END env ==="
44 # Don't apply patches in the tidy job, because it relies on the `git diff`
    maflcko commented at 11:55 am on November 7, 2025: 
    0# Don't apply patches in the iwyu job, because it relies on the `git diff`

    nit, if you re-touch

    hebasto commented at 12:11 pm on November 7, 2025:
    Thanks! Fixed.
  10. maflcko approved
  11. maflcko commented at 12:01 pm on November 7, 2025: member
    lgtm. Seems fine
  12. hebasto renamed this:
    ci: Add fast IWYU job
    ci: Add IWYU job
    on Nov 7, 2025
  13. hebasto force-pushed on Nov 7, 2025
  14. DrahtBot added the label CI failed on Nov 7, 2025
  15. fanquake commented at 12:12 pm on November 7, 2025: member

    just to produce a diff that can be immediately applied.

    The diff can’t be applied at all, how wrong it is varies, and there are no instructions for devs on how to fix it.

  16. maflcko commented at 12:23 pm on November 7, 2025: member

    instructions

    Some basic instructions could be added after the echo "^^^ ⚠️ Failure generated from IWYU", if needed.

  17. hebasto commented at 12:32 pm on November 7, 2025: member

    instructions

    Some basic instructions could be added after the echo "^^^ ⚠️ Failure generated from IWYU", if needed.

    Happy to add them once someone suggests good wording.

  18. DrahtBot removed the label CI failed on Nov 7, 2025
  19. hebasto force-pushed on Nov 7, 2025
  20. hebasto commented at 3:36 pm on November 7, 2025: member
    Rebased on top of the merged bitcoin/bitcoin#33818.
  21. DrahtBot added the label CI failed on Nov 7, 2025
  22. hebasto force-pushed on Nov 7, 2025
  23. fanquake commented at 5:01 pm on November 7, 2025: member
    The TSAN failure is #33318.
  24. in ci/test/03_test_script.sh:214 in bd48a101f9 outdated 
    216@@ -213,7 +217,9 @@ if [ "${RUN_TIDY}" = "true" ]; then
217     echo "^^^ ⚠️ Failure generated from clang-tidy"
218     false
219   fi
220+fi
221 
222+if [ "${RUN_IWYU}" == "true" ]; then
223   # TODO: Consider enforcing IWYU across the entire codebase.
    maflcko commented at 5:42 pm on November 7, 2025:

    Maybe:

    0echo "^^^ ⚠️ Failure generated from IWYU"
1echo
2echo "Some adjustments to the diff may be needed:"
3echo
4echo "* Use the C++ headers: E.g. <cerrno> over <errno.h>"
5echo '* Use #include <__.h> over #include "__.h" for all headers'
6echo "* Sort the headers, so that they are in the right pre-existing section, if"
7echo "  there are any."
    hebasto commented at 5:52 pm on November 7, 2025:
    Thanks! Added.
  25. hebasto force-pushed on Nov 7, 2025
  26. maflcko approved
  27. maflcko commented at 6:11 pm on November 7, 2025: member
    lgtm
  28. DrahtBot removed the label CI failed on Nov 7, 2025
  29. stickies-v commented at 12:52 pm on November 17, 2025: contributor

    Concept ACK, but a little more information in the PR description would be helpful, it’s hard to parse what expected behaviour is - both from the CI, as well as from developers. Does passing iwyu become mandatory for CI? Are we duplicating CI runs or moving it to a separate iwyu job? Should developers change anything in their workflow? Linking the previous discussion is helpful, but doesn’t necessarily describe what’s adopted in this PR.

    Also, style preferences wrt includes should be added to developer-notes.md imo.

  30. DrahtBot added the label Needs rebase on Nov 20, 2025
  31. hebasto force-pushed on Nov 20, 2025
  32. hebasto commented at 5:45 pm on November 20, 2025: member

    Concept ACK, but a little more information in the PR description would be helpful, it’s hard to parse what expected behaviour is - both from the CI, as well as from developers.

    The PR description has been updated.

    Does passing iwyu become mandatory for CI? Are we duplicating CI runs or moving it to a separate iwyu job?

    That isn’t the goal of this PR. Other PRs are dedicated to such changes: #33725 and #33779.

  33. hebasto commented at 5:46 pm on November 20, 2025: member
    Rebased.
  34. DrahtBot removed the label Needs rebase on Nov 20, 2025
  35. in ci/test/03_test_script.sh:247 in 3ead7ab320 
    239@@ -234,6 +240,13 @@ if [ "${RUN_TIDY}" = "true" ]; then
240   run_iwyu "compile_commands_iwyu_errors.json"
241   if ! ( git --no-pager diff --exit-code ); then
242     echo "^^^ ⚠️ Failure generated from IWYU"
243+    echo
244+    echo "Some adjustments to the diff may be needed:"
245+    echo
246+    echo "* Use the C++ headers: E.g. <cerrno> over <errno.h>"
247+    echo '* Use #include <__.h> over #include "__.h" for all headers'
    fanquake commented at 4:57 pm on November 21, 2025:
    I’m wondering, given we are building IWYU from source, can we just patch it, so the output is correct (uses <>); avoiding the need for this fixup.
    hebasto commented at 5:51 pm on December 4, 2025:
    Thanks! Reworked.
  36. hebasto force-pushed on Dec 4, 2025
  37. hebasto commented at 5:51 pm on December 4, 2025: member
    The feedback from @fanquake has been addressed.
  38. iwyu: Add patch to prefer angled brackets over quotes for includes cd907eca6b
  39. iwyu: Add patch to prefer C++ headers over C counterparts 5bd57a7259
  40. cmake: Make `codegen` target dependent on `generate_build_info` 4e75221965
  41. cmake: Fix target name 
    The executable target run by ctest is `mptest`.

This change removes unnecessary steps when building the `codegen`
target.
    02b859b752
  42. ci: Add IWYU job 
    The change in `src/crypto/hex_base.cpp` is because GCC 14 is not
affected by an IWYU bug.
See: https://github.com/include-what-you-use/include-what-you-use/issues/1763.
    f85e96fe89
  43. iwyu, clang-format: Sort includes 73439910a0
  44. in ci/test/03_test_script.sh:237 in 2d54366bd0 
    230@@ -225,6 +231,12 @@ if [ "${RUN_TIDY}" = "true" ]; then
231   run_iwyu "compile_commands_iwyu_errors.json"
232   if ! ( git --no-pager diff --exit-code ); then
233     echo "^^^ ⚠️ Failure generated from IWYU"
234+    echo
235+    echo "Some adjustments to the diff may be needed:"
236+    echo
237+    echo "* Use the C++ headers: E.g. <cerrno> over <errno.h>"
    maflcko commented at 9:18 am on December 5, 2025:

    nit: Looking at the output, I think this is already done correctly?

    If so, sorting can be done as well via:

     0diff --git a/src/.clang-format b/src/.clang-format
 1index c5fcd0b48c..4a1692a4bb 100644
 2--- a/src/.clang-format
 3+++ b/src/.clang-format
 4@@ -97,7 +97,7 @@ ForEachMacros:
 5   - BOOST_FOREACH
 6 IfMacros:
 7   - KJ_IF_MAYBE
 8-IncludeBlocks:   Preserve
 9+IncludeBlocks:   Regroup
10 IncludeCategories:
11   - Regex:           '^<bitcoin-build-config\.h>'
12     Priority:        -1

    and then running:

    0git diff -U0 | ./contrib/devtools/clang-format-diff.py -p1 -i -v
    hebasto commented at 12:23 pm on December 5, 2025:

    nit: Looking at the output, I think this is already done correctly?

    I don’t think so. The generated diffs still suggest the C headers:

    0+#include <errno.h>
    maflcko commented at 1:23 pm on December 5, 2025:

    I see. I guess they could also be fixed. Either with a patch in iwyu, since it is already patched, or with modernize-deprecated-headers clang-tidy-diff, or with a stand-alone mapping script: ./ci/test/modernize-deprecated-headers.py $( git diff --name-only )

    For ref, the mapping is probably:

     0<assert.h>:<cassert>
 1<ctype.h>:<cctype>
 2<errno.h>:<cerrno>
 3<float.h>:<cfloat>
 4<limits.h>:<climits>
 5<locale.h>:<clocale>
 6<math.h>:<cmath>
 7<setjmp.h>:<csetjmp>
 8<signal.h>:<csignal>
 9<stdarg.h>:<cstdarg>
10<stddef.h>:<cstddef>
11<stdio.h>:<cstdio>
12<stdlib.h>:<cstdlib>
13<string.h>:<cstring>
14<time.h>:<ctime>
15<wchar.h>:<cwchar>
16<wctype.h>:<cwctype>
17<uchar.h>:<cuchar>
18<inttypes.h>:<cinttypes>
19<stdint.h>:<cstdint>
  45. hebasto force-pushed on Dec 5, 2025
  46. hebasto commented at 3:48 pm on December 5, 2025: member

    @maflcko

    Your suggestions have been taken. Thank you!


hebasto DrahtBot w0xlt maflcko fanquake stickies-v

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-12-07 09:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me