ci, iwyu: Fix warnings in src/kernel and treat them as errors
#33779
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33779. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
9a87311 to
a8a33bc
Compare
|
There are no conflicts with other contributors' PRs. Friendly ping @l0rinc @maflcko @ryanofsky @willcl-ark who reviewed #31308, and @TheCharlatan, the kernel expert :) |
|
code review ACK a8a33bc |
willcl-ark
left a comment
There was a problem hiding this comment.
ACK a8a33bc
Indeed a good time to get this change in :)
|
review ACK a8a33bc 🐮 Show signatureSignature: |
|
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: The compile commands are configured for clang-21 and I'm using iwyu v0.25. It also seems like we are not configuring yet |
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 |
Something I was thinking about while review the first part of this PR was whether using CMake's 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 It also probably wouldn't help the repro issues @TheCharlatan notes.
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. |
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? |
|
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. |
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.
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.
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. |
With such a job, it is possible to copy the output and pipe it through |
|
ACK a8a33bc |
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: IWYU edited 873 files on your behalf.
+ git --no-pager diff
diff --git a/src/addrdb.cpp b/src/addrdb.cpp
index 129bbf2..5ecfbf6 100644
--- a/src/addrdb.cpp
+++ b/src/addrdb.cpp
@@ -4,20 +4,14 @@
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include <bitcoin-build-config.h> // IWYU pragma: keep
-
#include <addrdb.h>
-
#include <addrman.h>
#include <chainparams.h>
-#include <clientversion.h>
#include <common/args.h>
#include <common/settings.h>
-#include <cstdint>
#include <hash.h>
#include <logging.h>
#include <logging/timer.h>
-#include <netbase.h>
-#include <netgroup.h>
#include <random.h>
#include <streams.h>
#include <tinyformat.h>
@@ -26,6 +20,23 @@
#include <util/fs_helpers.h>
#include <util/syserror.h>
#include <util/translation.h>
+#include <errno.h>
+#include <stdio.h>
+#include <cstdint>
+#include <algorithm>
+#include <array>
+#include <exception>
+#include <map>
+#include <span>
+#include <stdexcept>
+#include <string>
+
+#include "net_types.h"
+#include "protocol.h"
+#include "serialize.h"
+#include "uint256.h"
+#include "util/result.h"
+#include "util/time.h"
namespace { |
A separate job running IWYU on a compilation database could be quite fast, as we’d only need to build the |
I don't think this diff is "ready to apply"? Headers like 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:
|
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)
Yeah, good points. The sorting is also wrong, which may be good to fix in iwyu, or in clang-format (c.f. #32813 (comment)) |
Done in #33810. |
|
Moved this to draft while we first figure out #33810. |
a8a33bc to
232483a
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
232483a to
d5a62a2
Compare
|
It could make sense to go with #33810 first, but this lgtm: re-ACK d5a62a2 🚢 Show signatureSignature: |
56750c4 iwyu, clang-format: Sort includes (Hennadii Stepanov) 2c78814 ci: Add IWYU job (Hennadii Stepanov) 94e4f04 cmake: Fix target name (Hennadii Stepanov) 0f81e00 cmake: Make `codegen` target dependent on `generate_build_info` (Hennadii Stepanov) 73f7844 iwyu: Add patch to prefer C++ headers over C counterparts (Hennadii Stepanov) 7a65437 iwyu: Add patch to prefer angled brackets over quotes for includes (Hennadii Stepanov) Pull request description: 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](#33779 (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. ACKs for top commit: maflcko: review ACK 56750c4 🌄 sedited: ACK 56750c4 Tree-SHA512: af15326b6d0c5d1e11346ac64939644936c65eb9466cd1a17ab5da347d39aef10f7ab33b39fbca31ad291b0b4b54639b147b24410f4f86197e4a776049882694
d5a62a2 to
8f2ac6f
Compare
|
Rebased and undrafted. |
|
Has #34079 (comment) been reported upstream? |
UPDATE: See include-what-you-use/include-what-you-use#1886. |
maflcko
left a comment
There was a problem hiding this comment.
8f2ac6f 🍯
Show signature
Signature:
untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: 8f2ac6f7efb7e5963436fd59462631888fe8afa3 🍯
WmSu1uh1MmK1km0uU7PCiDe6qvuFJn6vQDZuhXR2o2ovMY3KpXg9izWY29/uzlAXLHeJ1ebkkWztDy/jzhiyBg==
src/util/translation.h
Outdated
| /** Mark a bilingual_str as untranslated */ | ||
| inline bilingual_str Untranslated(std::string original) { return {original, original}; } | ||
| template <std::size_t N> | ||
| inline bilingual_str Untranslated(const char (&original)[N]) { return Untranslated(std::string{original}); } |
There was a problem hiding this comment.
not sure about this. I don't think we should be adding c++ code as an rough alternative to IWYU pragma export. Also, I am starting to think we should get rid of the pragma export to reduce the list of includes, because it doesn't interact well with letting iwyu minimize the include list.
There was a problem hiding this comment.
not sure about this. I don't think we should be adding c++ code as an rough alternative to IWYU pragma export.
Not needing IWYU pragma: export is just a nice side effect. The actual goal of this change is to enforce explicit conversions and make the translation module self-contained.
There was a problem hiding this comment.
I don't think it is maintainable to add a redundant string literal overload for every function in the whole codebase that accepts a std::string as any arg.
There was a problem hiding this comment.
I agree that the current approach is not ideal.
A better alternative for this specific case would be switching the parameter type to std::string_view. This solves the efficiency and IWYU issues without requiring a separate overload. WDYT?
Regarding the rest of the codebase, it might be best to evaluate cases individually. There are some tricky spots, such as using Assume() inside a macro, that might not fit a 'one size fits all' rule.
There was a problem hiding this comment.
A better alternative for this specific case would be switching the parameter type to
std::string_view. This solves the efficiency and IWYU issues without requiring a separate overload. WDYT?
I don't see an efficiency problem here, and i fail to see how either this patch, or string_view would solve it.
If you wanted to make it more efficient, you could write:
inline bilingual_str Untranslated(std::string original) { return {original, std::move(original)}; }However, I am not sure if this is worth it.
There was a problem hiding this comment.
I don't see an efficiency problem here, and i fail to see how either this patch, or string_view would solve it.
I'm not saying that there is a problem that should be solved on its own. But wouldn't that patch reduce the total number of heap allocations where SSO is not involved?
There was a problem hiding this comment.
The use of IWYU pragma export should be limited to cases where you don't want clients to import headers that you consider implementation details, like when you have thousand lines of templated code that you split across multiple files for better maintainability.
<string> is definitely a public header and including the header should be the only way to get std::string. Hence, #include <string> // IWYU pragma export is always wrong and should be verboten.
Whether it is worth avoiding #include <string> where std::string is only used indirectly, is debatable. I would just keep it simple. IWYU is not perfect and we have unnecessary includes anyway where allocators and type traits are used indirectly. I would just drop cd95fbd.
There was a problem hiding this comment.
I guess I agree on the pragma export. If someone wanted to revert that, the diff would be:
diff --git a/src/util/strencodings.h b/src/util/strencodings.h
index 0106385804..b070e5b8c4 100644
--- a/src/util/strencodings.h
+++ b/src/util/strencodings.h
@@ -9,7 +9,7 @@
#ifndef BITCOIN_UTIL_STRENCODINGS_H
#define BITCOIN_UTIL_STRENCODINGS_H
-#include <crypto/hex_base.h> // IWYU pragma: export
+#include <crypto/hex_base.h>
#include <span.h>
#include <util/string.h>
@@ -20,8 +20,8 @@
#include <cstdint>
#include <limits>
#include <optional>
-#include <string> // IWYU pragma: export
-#include <string_view> // IWYU pragma: export
+#include <string>
+#include <string_view>
#include <system_error>
#include <type_traits>
#include <vector>
diff --git a/src/util/string.h b/src/util/string.h
index 330c2a2a61..2578e530af 100644
--- a/src/util/string.h
+++ b/src/util/string.h
@@ -12,8 +12,8 @@
#include <cstring>
#include <locale>
#include <sstream>
-#include <string> // IWYU pragma: export
-#include <string_view> // IWYU pragma: export
+#include <string>
+#include <string_view>
#include <vector>
namespace util {(and then fixup the iwyu complaints)
There was a problem hiding this comment.
Thank you for sharing your thoughts!
The use of
IWYU pragma exportshould be limited...
I guess I agree on the pragma export.
Please see #34319.
8f2ac6f to
a5a8c41
Compare
|
Rebased. The recent feedback from @purpleKarrot and @maflcko has been addressed. |
|
review ACK a5a8c41 🍱 Show signatureSignature: |
Now seems like a good time to update the includes in
src/kernel.