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
Stale ACK	l0rinc, willcl-ark, purpleKarrot, maflcko

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

Conflicts

No conflicts as of last run.

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==

sedited 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?

sedited 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.

fanquake commented at 2:21 pm on December 16, 2025: member

Moved this to draft while we first figure out #33810.

fanquake marked this as a draft on Dec 16, 2025

hebasto force-pushed on Dec 16, 2025

DrahtBot added the label CI failed on Dec 16, 2025

DrahtBot commented at 4:20 pm on December 16, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/20272513332/job/58211985526 LLM reason (✨ experimental): Lint failure: includes use quotes instead of brackets, triggering lint-includes.py and non-zero exit from the linter.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

hebasto force-pushed on Dec 16, 2025

DrahtBot removed the label CI failed on Dec 16, 2025

maflcko commented at 9:02 am on December 22, 2025: member

It could make sense to go with #33810 first, but this lgtm:

re-ACK d5a62a288fba2b1d69325623e0640d1f33af58b8 🚢

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: re-ACK d5a62a288fba2b1d69325623e0640d1f33af58b8 🚢
3Ja6xcEDdUUI3zhnDduwv6nFpS5ep2ifXdFKsXJYkTEoXQYxqD06sAuO1NSTf57ZiwYCuG9ru6rhBsMLBhgWgDw==

DrahtBot requested review from l0rinc on Dec 22, 2025

DrahtBot requested review from willcl-ark on Dec 22, 2025

DrahtBot requested review from purpleKarrot on Dec 22, 2025

DrahtBot added the label Needs rebase on Dec 22, 2025

hebasto referenced this in commit 5bbc7c8cc1 on Dec 22, 2025

Add `Untranslated` overload for string literals

This change allows `#include <string>` to be omitted in files where
`Untranslated` is invoked with a string literal.

cd95fbdf12

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

hebasto force-pushed on Dec 22, 2025

hebasto marked this as ready for review on Dec 22, 2025

hebasto commented at 10:49 pm on December 22, 2025: member

Rebased and undrafted.

fanquake commented at 10:51 pm on December 22, 2025: member

Has #34079 (review) been reported upstream?

DrahtBot removed the label Needs rebase on Dec 22, 2025

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