ci, iwyu: Fix warnings in `src/kernel` and treat them as errors

hebasto commented at 5:03 pm on November 4, 2025: member

Now seems like a good time to update the includes in src/kernel.

DrahtBot commented at 5:03 pm on November 4, 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/33779.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc, willcl-ark, maflcko, purpleKarrot

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33725 (ci, iwyu: Treat warnings as errors for src/init and src/policy by hebasto)
#30214 (refactor: Improve assumeutxo state representation by ryanofsky)

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.

ci, iwyu: Fix warnings in `src/kernel` and treat them as errors a8a33bc0c0

hebasto force-pushed on Nov 4, 2025

DrahtBot added the label CI failed on Nov 4, 2025

DrahtBot removed the label CI failed on Nov 4, 2025

hebasto marked this as ready for review on Nov 4, 2025

hebasto commented at 11:41 pm on November 4, 2025: member

There are no conflicts with other contributors’ PRs.

Friendly ping @l0rinc @maflcko @ryanofsky @willcl-ark who reviewed #31308, and @TheCharlatan, the kernel expert :)

hebasto added the label Refactoring on Nov 4, 2025

l0rinc commented at 11:43 pm on November 4, 2025: contributor

code review ACK a8a33bc0c0a11093418debc36db8ac63bf90e687

willcl-ark approved

willcl-ark commented at 10:46 am on November 5, 2025: member

ACK a8a33bc0c0a11093418debc36db8ac63bf90e687

Indeed a good time to get this change in :)

maflcko commented at 1:47 pm on November 5, 2025: member

review ACK a8a33bc0c0a11093418debc36db8ac63bf90e687 🐮

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK a8a33bc0c0a11093418debc36db8ac63bf90e687 🐮
3G2GSehSG/6BqX/m6sCiIHGIarE7ISW3dV0VxpX5fcM2EJ8hZOVOcNXSZRr7oRnrNufO91NK8KYEdhrrIaMbJAg==

TheCharlatan commented at 1:53 pm on November 5, 2025: contributor

I tried to reproduce what the CI is doing locally. Having to run the full CI either on push or locally just to fix the includes is not ideal. The following command seems to produce different results:

0/home/user/Downloads/include-what-you-use/iwyu_tool.py -p /home/user/bitcoin/build_dev_mode_clang/compile_commands.json src/kernel/chainparams.cpp -- -Xiwyu --cxx17ns -Xiwyu --mapping_file=/home/user/bitcoin/contrib/devtools/iwyu/bitcoin.core.imp -Xiwyu --max_line_length=160
1
2(/home/user/bitcoin/src/kernel/chainparams.h has correct #includes/fwd-decls)
3
4/home/user/bitcoin/src/kernel/chainparams.cpp should add these lines:
5#include <functional>                  // for identity, equal_to

The compile commands are configured for clang-21 and I’m using iwyu v0.25. It also seems like we are not configuring yet for the kernel library headers and implementations.

maflcko commented at 2:07 pm on November 5, 2025: member

different results:

I wonder if it comes from different libstdc++ header versions used. Generally, I could imagine it being difficult to achieve fully reproducible and consistent results across all build configurations. If relying on the CI (locally or remote) is too tedious, then it probably doesn’t make sense to treat iwuy suggestions as errors.

Possibly we could spin up a separate CDash to track them externally (outside this project) as warnings or errors? cc @willcl-ark @purpleKarrot

willcl-ark commented at 2:15 pm on November 5, 2025: member

Having to run the full CI either on push or locally just to fix the includes is not ideal.

Something I was thinking about while review the first part of this PR was whether using CMake’s CXX_INCLUDE_WHAT_YOU_USE ${iwyu_path} to run this as another configuration “natively” with cmake. The issue here is it’s project-wide, and you have to set SKIP_LINTING on those files you don’t want it run on. Then we could perhaps provide a preset to run iwyu or something.

I tried implementing that but either must be set per file (verbose) or on a per-target basis, and the resulting cmake ended up being messier than the jq filtering we do currently, so I gave up experimenting with that.

It also probably wouldn’t help the repro issues @TheCharlatan notes.

Possibly we could spin up a separate CDash to track them externally (outside this project) as warnings or errors

Would be possible relatively easily, I think, if there’s interest.

maflcko commented at 2:22 pm on November 5, 2025: member

Possibly we could spin up a separate CDash to track them externally (outside this project) as warnings or errors

Would be possible relatively easily, I think, if there’s interest.

Nice. I was thinking that maybe the CI in this repo could have an auth token to push the iwyu for all builds to the dashboard. This way, it could be easier to check the warnings/error for each pull request, without it being a merge blocker. Also, there could be regular “fix iwyu” pull requests to bring the warnings/errors down, with an easy way to check via the dashboard as well.

fanquake commented at 2:22 pm on November 5, 2025: member

Would be possible relatively easily, I think, if there’s interest.

I’m wondering what problem we are trying to solve. Externalizing this would just lead to random PRs to this repo, to “fix” includes, which can’t be verified here? If developers aren’t actively engaged in actually maintaining includes and no CI will ever fail, that sounds like endless churn. It’s also unclear why this needs to be in some other dashboard, if we can have the infra/result determined here already?

TheCharlatan commented at 2:26 pm on November 5, 2025: contributor

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.

maflcko commented at 2:40 pm on November 5, 2025: member

I’m wondering what problem we are trying to solve.

I think the problem is that it is inherently impossible to reproduce an iwyu run locally without the exact CI config. Maybe this is fine, and nothing needs to be fixed. Though, if devs think that relying exactly on the CI is too tedious, then a dashboard could seem like a nice solution.

endless churn

I think iwyu will mean churn, regardless of how it is enforced, if it is run on more actively changed modules of the codebase.

I guess this gives a third alternative: Only enforce IWYU pre branch-off, so that there is ideally only a single “fix” pull per release and all released versions are iwyu-clean. Post branch-off the IWYU enforcement could be turned-off, so that developers can just continue to develop normally, like before, without having to worry about tedious IWYU fixups for every single pull request and commit.

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.

Looking at the runtimes here, it seems iwyu takes 9 minutes alone, so with the configure overhead, it will likely be hard to get under 10 minutes. It is less than the 19 minutes the full task takes with clang-tidy, but there is probably a limit to how fast it can be made.

purpleKarrot commented at 8:01 am on November 6, 2025: contributor

Maybe a better approach would be to run the enforced sections in a separate, faster job?

With such a job, it is possible to copy the output and pipe it through fix_includes.py locally. Full reproducibility is nice to have (and can be achieved with a dev container that contains all the necessary tools in the right version), but for a development workflow it is also possible to ignore IWYU (just rely on clangd’s recommendations to add/remove includes) and then fix deviations using the described approach when the PR job fails.

purpleKarrot commented at 8:03 am on November 6, 2025: contributor

ACK a8a33bc0c0a11093418debc36db8ac63bf90e687

maflcko commented at 8:18 am on November 6, 2025: member

With such a job, it is possible to copy the output and pipe it through fix_includes.py locally.

For reference, if devs want to copy the output, the job already has the full diff, ready to apply. So there is no need to take an extra step. Example https://github.com/bitcoin/bitcoin/actions/runs/19076699870/job/54494252522?pr=33779#step:9:41045:

 0IWYU edited 873 files on your behalf.
 1
 2+ git --no-pager diff
 3diff --git a/src/addrdb.cpp b/src/addrdb.cpp
 4index 129bbf2..5ecfbf6 100644
 5--- a/src/addrdb.cpp
 6+++ b/src/addrdb.cpp
 7@@ -4,20 +4,14 @@
 8 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 9 
10 #include <bitcoin-build-config.h> // IWYU pragma: keep
11-
12 #include <addrdb.h>
13-
14 #include <addrman.h>
15 #include <chainparams.h>
16-#include <clientversion.h>
17 #include <common/args.h>
18 #include <common/settings.h>
19-#include <cstdint>
20 #include <hash.h>
21 #include <logging.h>
22 #include <logging/timer.h>
23-#include <netbase.h>
24-#include <netgroup.h>
25 #include <random.h>
26 #include <streams.h>
27 #include <tinyformat.h>
28@@ -26,6 +20,23 @@
29 #include <util/fs_helpers.h>
30 #include <util/syserror.h>
31 #include <util/translation.h>
32+#include <errno.h>
33+#include <stdio.h>
34+#include <cstdint>
35+#include <algorithm>
36+#include <array>
37+#include <exception>
38+#include <map>
39+#include <span>
40+#include <stdexcept>
41+#include <string>
42+
43+#include "net_types.h"
44+#include "protocol.h"
45+#include "serialize.h"
46+#include "uint256.h"
47+#include "util/result.h"
48+#include "util/time.h"
49 
50 namespace {

hebasto commented at 11:49 am on November 6, 2025: member

Having to run the full CI either on push or locally just to fix the includes is not ideal.

Something I was thinking about while review the first part of this PR was whether using CMake’s CXX_INCLUDE_WHAT_YOU_USE ${iwyu_path} to run this as another configuration “natively” with cmake. The issue here is it’s project-wide, and you have to set SKIP_LINTING on those files you don’t want it run on. Then we could perhaps provide a preset to run iwyu or something.

A separate job running IWYU on a compilation database could be quite fast, as we’d only need to build the codegen target (though it requires a fix for the Qt code). That’s a plan for the future.

fanquake commented at 12:15 pm on November 6, 2025: member

For reference, if devs want to copy the output, the job already has the full diff, ready to apply.

I don’t think this diff is “ready to apply”? Headers like errno.h are wrong, and should be <cerrno> (this is enforced by modernize-deprecated-headers in the tidy job)? Includes like #include "net_types.h" are also wrong, and should be #include <net_types.h>?

Given that developers working on this code, have said that this change will likely make development more difficult, should be reason enough to fix the developer workflows/tooling (or at least decide on a the fix) before doing anything here; especially given that the value add of “fixing includes” is trending towards 0, compared to developers actually getting work done/wasting time fighting with CIs. This problem will only grow as this starts to cover more of the codebase, and effect more devs, so it seems like a good time to figure out a solution.

Currently solutions include:

Add a new dashboard somewhere. This needs to be created, and maintained (externally) by someone, and doesn’t seem to solve the problem of developers being able to easily run the same checks locally. Its not clear if that dashboard would post immediately applyable diffs to PRs (updated on every push), when CI turns red here, or developers would need to check the dashboard to find the diff to fix their PR?
Add a new (faster) CI job here. If the code for this CI job can be easily ran locally (like the lint job), and produce a diff that can be immediately applied (see above for why current output does not seem to be that) then this could be a good solution, assuming the diff is easily surfaced.
Leave things as they are. Developers can open a PR, and see if the CI turns green. If not, they can scroll through ~68'000 lines of CI output, to extract a diff, potentially (see above) modify the diff so it can be applied, then push again.

maflcko commented at 12:27 pm on November 6, 2025: member

Leave things as they are. Developers can open a PR, and see if the CI turns green. If not, they can scroll through ~68'000 lines of CI output, to extract a diff, potentially (see above) modify the diff so it can be applied, then push again.

The massive output is just the iwyu warning/debug output. On real pull requests that run into errors, the diff should be smaller. (Should be easy to test by pushing this pr’s ci changes without the code changes)

I don’t think this diff is “ready to apply”? Headers like errno.h are wrong, and should be <cerrno> (this is enforced by modernize-deprecated-headers in the tidy job)? Includes like #include "net_types.h" are also wrong, and should be #include <net_types.h>?

Yeah, good points. The sorting is also wrong, which may be good to fix in iwyu, or in clang-format (c.f. #32813 (review))

hebasto commented at 11:31 pm on November 6, 2025: member

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.

Done in #33810.

ci, iwyu: Fix warnings in src/kernel and treat them as errors #33779

Code Coverage & Benchmarks

Reviews

Conflicts

ci, iwyu: Fix warnings in `src/kernel` and treat them as errors #33779