ci: Treat IWYU violations as errors

hebasto commented at 6:17 pm on December 28, 2022: member

Fixes the #24831 (review):

Not sure if this is setting the return code to non-zero if it fails?

hebasto force-pushed on Dec 28, 2022

DrahtBot commented at 6:17 pm on December 28, 2022: 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
Concept ACK	ajtowns, MarcoFalke

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:

#27012 (ci: Print iwyu patch in git diff format by MarcoFalke)
#25797 (build: Add CMake-based build system by hebasto)

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.

DrahtBot added the label Tests on Dec 28, 2022

in src/util/string.cpp:7 in b55e240701 outdated

3@@ -4,6 +4,7 @@
4 
5 #include <util/string.h>
6 
7+#include <map>

maflcko commented at 8:43 am on December 29, 2022:

This is wrong. Maybe use libc++ instead of libstdc++? Though, that will probably be wrong as well, see https://github.com/include-what-you-use/include-what-you-use/issues/1110#issuecomment-1245370908

hebasto commented at 11:06 am on December 29, 2022:

What about

0--- a/src/util/string.cpp
1+++ b/src/util/string.cpp
2@@ -11,5 +11,5 @@
3 void ReplaceAll(std::string& in_out, const std::string& search, const std::string& substitute)
4 {
5     if (search.empty()) return;
6-    in_out = std::regex_replace(in_out, std::regex(search), substitute);
7+    in_out = std::regex_replace(in_out, std::regex(search.data()), substitute);
8 }

?

Looking into bits/regex.h makes me believe it won’t introduce any regressions.

fanquake commented at 11:21 am on December 29, 2022:

There is a problem if we are refactoring code for no other reason than to make a buggy tool happy.

hebasto commented at 12:55 pm on December 29, 2022:

https://github.com/bitcoin/bitcoin/blob/682a87e0db1fbddf20c40ab6bce9c70cad62e87a/src/util/string.cpp#L9-L10

in src/primitives/transaction.h:25 in b55e240701 outdated

21@@ -22,6 +22,9 @@
22 #include <tuple>
23 #include <utility>
24 #include <vector>
25+#if __cplusplus >= 202002L

fanquake commented at 10:10 am on December 29, 2022:

Not a fan of adding stuff like this just to appease a tool, which happens to be getting run under a non-default compilation mode (--enable-c++20).

hebasto commented at 12:56 pm on December 29, 2022:

See 89bcb1a856a563013062c2c129012498e6b8b1f9

fanquake commented at 2:24 pm on December 29, 2022:

I don’t think we want to just drop the option without adding it to a different CI (and not a Windows build).

hebasto commented at 2:27 pm on December 29, 2022:

I don’t think we want to just drop the option without adding it to a different CI (and not a Windows build).

Agree. Also #25528 (review)

Getting ready a proposal.

hebasto commented at 6:47 pm on December 29, 2022:

Getting ready a proposal.

#26770

hebasto commented at 8:30 pm on December 29, 2022:

Thanks! Updated.

in src/compat/compat.h:28 in b55e240701 outdated

24@@ -25,10 +25,10 @@
25 #include <fcntl.h>
26 #include <sys/mman.h>
27 #include <sys/select.h>
28-#include <sys/socket.h>
29+#include <sys/socket.h> // IWYU pragma: export

fanquake commented at 10:11 am on December 29, 2022:

What symbols are these needed for (here and below), and what are the trade-offs between adding the pragma, and actually adding the include where it is used? We should probably have some rationale as to wether we are going to further include/“export” everything out of compat, or actually add includes to source files.

hebasto commented at 9:20 pm on December 29, 2022:

What symbols are these needed for (here and below)…

0#include <sys/socket.h>                    // for socklen_t, size_t, ssize_t

0#include <netinet/in.h>                    // for in6_addr, in_addr, s6_addr

Both headers are non-Windows.

… and what are the trade-offs between adding the pragma, and actually adding the include where it is used?

Platform-specific headers require to be guarded with #ifdef ... #endif macros. So adding them in every place, where they are required, is not very DRY.

We should probably have some rationale as to wether we are going to further include/“export” everything out of compat, or actually add includes to source files.

My suggestion is to “export” platform-specific headers out of compat.h when IWYU points at them.

fanquake commented at 10:07 am on December 30, 2022:

#include <sys/socket.h> …. size_t, ssize_t

Is this a bug? size_t/ssize_t do not come from socket.h.

So adding them in every place, where they are required, is not very DRY.

Isn’t adding every header, everywhere it is used, the entire point of IWYU though?

This could also lead to having a single bloated compat.h, which when included everywhere, mitigates one of the other supposed benefit of IWYU, which is removing unneeded headers to reducing preprocessing time.

maflcko commented at 10:15 am on December 30, 2022:

It just seems more convenient to include compat.h or socks.h when using the Sock wrapper, instead of having to deal with low-level system includes everywhere Sock is used. I guess an alternative to the iwyu pragma would be to also wrap the low-level types, but not sure if this is actually better.

hebasto commented at 10:29 am on December 30, 2022:

#include <sys/socket.h> …. size_t, ssize_t

Is this a bug? size_t/ssize_t do not come from socket.h.

Perhaps, it is. But it is unrelated, as the socket.h header is required to provide the socklen_t symbol.

So adding them in every place, where they are required, is not very DRY.

Isn’t adding every header, everywhere it is used, the entire point of IWYU though?

This could also lead to having a single bloated compat.h, which when included everywhere, mitigates one of the other supposed benefit of IWYU, which is removing unneeded headers to reducing preprocessing time.

That is not what I meant. The compat.h header, which includes platform-specific headers, allows to avoid repetition of code like that:

0#ifdef WIN32
1#include <winsock2.h>
2#else
3#include <sys/socket.h>
4#endif

hebasto force-pushed on Dec 29, 2022

maflcko referenced this in commit 65de8eeeca on Dec 29, 2022

hebasto force-pushed on Dec 29, 2022

hebasto commented at 8:29 pm on December 29, 2022: member

Rebased 682a87e0db1fbddf20c40ab6bce9c70cad62e87a -> 9b7443dc27ac9b3188a2a8ace51478970b98b3b7 (pr26763.02 -> pr26763.03) on top of the bitcoin/bitcoin#26770.

in src/chain.h:25 in 9b7443dc27 outdated

21+#include <cstdint>
22+#include <string>
23 #include <vector>
24 
25+namespace Consensus {
26+struct Params;

ajtowns commented at 5:10 am on January 3, 2023:

Why drop the #include <consensus/params.h> just to manually forward declare the struct? This seems like a backwards step. Seems better to either leave it as is, or move GetBlockProofEquivalentTime back to pow.cpp so that chain.h doesn’t need Consensus::Params at all (partially reverting #7311 ; GBPET() is used in ConnectBlock() so it isn’t really separate from consensus functionality for us).

hebasto commented at 8:40 am on January 3, 2023:

Why drop the #include <consensus/params.h> just to manually forward declare the struct?

Although forward declarations have their own pros and cons, this code base seems to lean in their favour:

0$ git grep -e "struct Params;"
1src/node/blockstorage.h:struct Params;
2src/node/miner.h:namespace Consensus { struct Params; };
3src/node/transaction.h:struct Params;
4src/txdb.h:struct Params;
5src/validation.h:struct Params;
6src/zmq/zmqpublishnotifier.cpp:struct Params;

This seems like a backwards step.

Mind elaborating?

As a question like that has been raised, should we document our preferences in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md?

ajtowns commented at 1:01 am on January 4, 2023:

Mind elaborating?

If we’ve got a dependency from one module to another (“validation” depends on “consensus/params”) then we should just include the header file; if we’re avoiding a dependency (eg making “chain” just store chain data independent of whatever the consensus parameters might be), then we shouldn’t be entangling them by referencing the “unrelated” module’s definitions.

Some of those cases don’t even avoid the dependency at all; eg validation.h includes chainparams.h which includes consensus/params.h anyway.

It makes the dependencies slightly harder to track: forward declarations are harder to search for than a plain #include; it also violates the DRY-principle, though not in a very big way.

The benefit is that not including the full header can reduce compilation time in the cases where you can avoid including a complicated header (eg, one that pulls in boost multiindex or that pulls in lots of other headers). That probably makes sense if you’re avoiding pulling in txmempool.h (which validation.h does), but even then any effort’s probably better spent in improving our modularisation…

hebasto commented at 11:04 am on January 4, 2023:

The changes in src/chain.h appeared non-required for this PR, and they have been dropped.

The discussion about forward declaration vs header including could be continued elsewhere, maybe in a dedicated issue, couldn’t it?

in src/versionbits.h:9 in 9b7443dc27 outdated

5@@ -6,6 +6,7 @@
6 #define BITCOIN_VERSIONBITS_H
7 
8 #include <chain.h>
9+#include <consensus/params.h>

ajtowns commented at 5:15 am on January 3, 2023:

I’m surprised deploymentstatus.h doesn’t also need to include consensus/params.h, given it defines (non-template) inline functions that call Consensus::ValidDeployment()

hebasto commented at 8:14 am on January 3, 2023:

According to https://api.cirrus-ci.com/v1/task/5220634955874304/logs/ci.log, the deploymentstatus.h is not parsed by IWYU now.

ajtowns commented at 5:27 am on January 3, 2023: contributor

Concept ACK

hebasto force-pushed on Jan 4, 2023

hebasto commented at 10:50 am on January 4, 2023: member

Updated 9b7443dc27ac9b3188a2a8ace51478970b98b3b7 -> a6979504e36d75bfe68215b99d8bd7651843db53 (pr26763.03 -> pr26763.04):

rebased
dropped non-required changes

in src/test/fuzz/util/net.cpp:24 in a6979504e3 outdated

20@@ -21,6 +21,7 @@
21 #include <cstdlib>
22 #include <cstring>
23 #include <thread>
24+#include <unordered_map>

maflcko commented at 11:05 am on January 4, 2023:

Looks wrong as well?

hebasto commented at 12:26 pm on January 4, 2023:

Well, in FuzzedSock::WaitMany() function https://github.com/bitcoin/bitcoin/blob/4717a5aa31bc9cdb5406f4161f4c600e5c2659c0/src/test/fuzz/util/net.cpp#L332-L339 the type EventsPerSock is actually an std::unordered_map: https://github.com/bitcoin/bitcoin/blob/2ec97825e70882262ee2b7d53dc1def788a60162/src/util/sock.h#L218

maflcko commented at 12:45 pm on January 4, 2023:

Right, so it would need to include util/sock.h instead to get EventsPerSock?

hebasto commented at 1:13 pm on January 4, 2023:

Thanks! Updated.

Some relevant IWYU’s docs: https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhatIsAUse.md#re-exporting.

maflcko commented at 1:17 pm on January 4, 2023:

This will force iwyu to remove #include <unordered_map> if util/sock.h is included, but an unrelated code piece needs the unordered map include?

Not sure which is preferable. I am just explaining the tradeoffs.

hebasto commented at 1:22 pm on January 4, 2023:

What about // IWYU pragma: no_include <unordered_map> in src/test/fuzz/util/net.cpp?

maflcko commented at 1:26 pm on January 4, 2023:

Seems worse (combining all downsides), compared to just including the file

hebasto commented at 1:30 pm on January 4, 2023:

FWIW, I lean to just following IWYU’s suggestion and including a header. It won’t make a translation unit bigger anyway.

hebasto force-pushed on Jan 4, 2023

hebasto commented at 1:12 pm on January 4, 2023: member

Updated a6979504e36d75bfe68215b99d8bd7651843db53 -> ba0780b756d446fbdcb2102f0b39f4bf516ffe74 (pr26763.04 -> pr26763.05, diff):

addressed @MarcoFalke’s comment

maflcko added the label Needs rebase on Jan 17, 2023

DrahtBot removed the label Needs rebase on Jan 17, 2023

hebasto force-pushed on Jan 17, 2023

hebasto commented at 3:02 pm on January 17, 2023: member

Needs rebase

Rebased ba0780b756d446fbdcb2102f0b39f4bf516ffe74 -> 077376c805c620ddb1a893bff4cdabc51ef44313 (pr26763.05 -> pr26763.06).

maflcko commented at 8:57 pm on January 17, 2023: member

Concept ACK. Seems like a good attempt to reduce review on the include list or avoid shipping a release with missing includes (on Linux fwiw).

However, the CI output might be too cumbersome to act on. It would be nice if it also printed a nice diff to copy (or if there was a documented way to create one locally).

fanquake commented at 12:07 pm on January 18, 2023: member

However, the CI output might be too cumbersome to act on. It would be nice if it also printed a nice diff to copy (or if there was a documented way to create one locally).

Yea. I think in it’s current state, this change will mostly just annoy contributors, for not much gain. Even more so if the CI is giving them nonsensical output (IWYU bugs), and it’s not clear what should be done (add the wrong include, add a IWYU pragma inline, add a workaround to our .imp file etc).

hebasto commented at 1:36 pm on January 18, 2023: member

However, the CI output might be too cumbersome to act on. It would be nice if it also printed a nice diff to copy (or if there was a documented way to create one locally).

It is possible to leverage the fix_includes.py tool which produces output as follows:

 0>>> Fixing #includes in 'dbwrapper.h'
 1@@ -11,20 +11,19 @@
 2 #include <serialize.h>
 3 #include <span.h>
 4 #include <streams.h>
 5-
 6 #include <leveldb/db.h>
 7 #include <leveldb/iterator.h>
 8 #include <leveldb/options.h>
 9 #include <leveldb/slice.h>
10 #include <leveldb/status.h>
11 #include <leveldb/write_batch.h>
12-
13 #include <cstddef>
14 #include <cstdint>
15 #include <exception>
16 #include <stdexcept>
17 #include <string>
18 #include <vector>
19+#include <optional>
20
21 namespace leveldb {
22 class Env;
23>>> Fixing #includes in 'kernel/context.cpp'
24@@ -3,13 +3,10 @@
25 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
26
27 #include <kernel/context.h>
28-
29 #include <crypto/sha256.h>
30 #include <key.h>
31 #include <logging.h>
32-#include <pubkey.h>
33 #include <random.h>
34-
35 #include <string>
36
37
38IWYU edited 2 files on your behalf.

I think in it’s current state, this change will mostly just annoy contributors, for not much gain. Even more so if the CI is giving them nonsensical output (IWYU bugs), and it’s not clear what should be done (add the wrong include, add a IWYU pragma inline, add a workaround to our .imp file etc).

The current state, in the master branch, is worse because using IWYU is a no-op effectively, but consumes resources.

In case IWYU produces “nonsensical output” for any particular source file or header, the latter could just be skipped from analysis temporarily.

maflcko commented at 1:50 pm on January 18, 2023: member

Looks like that would need --blank_lines set to leave the new lines? Also, it would be good to have a single patch to copy-paste for all files, and not patches for each file separated by debug logs.

fanquake commented at 2:57 pm on January 18, 2023: member

The current state, in the master branch, is worse because using IWYU is a no-op effectively, but consumes resources.

I disagree, it’s not worse, because if you don’t care about it, you don’t have to engage / fix anything, and the resource usage/overhead is basically 0. It’s also useful enough that if someone does want to use the tool / run it locally, they can.

hebasto commented at 9:05 am on January 19, 2023: member

Looks like that would need --blank_lines set to leave the new lines?

It doesn’t help, unfortunately.

DrahtBot added the label Needs rebase on Jan 26, 2023

hebasto force-pushed on Jan 26, 2023

hebasto commented at 3:48 pm on January 26, 2023: member

Rebased and drafted.

DrahtBot removed the label Needs rebase on Jan 26, 2023

maflcko commented at 3:56 pm on January 26, 2023: member

It is possible to leverage the fix_includes.py tool

Could probably make sense to add this to the CI, to allow easy copy-pasting from Cirrus or from a local run.

in ci/test/06_script_b.sh:76 in e3d73a205c outdated

71@@ -72,7 +72,7 @@ if [ "${RUN_TIDY}" = "true" ]; then
72           " src/util/syserror.cpp"\
73           " src/util/threadinterrupt.cpp"\
74           " src/zmq"\
75-          " -p . ${MAKEJOBS} -- -Xiwyu --cxx17ns -Xiwyu --mapping_file=${BASE_BUILD_DIR}/bitcoin-$HOST/contrib/devtools/iwyu/bitcoin.core.imp"
76+          " -p . ${MAKEJOBS} -- -Xiwyu --error -Xiwyu --cxx17ns -Xiwyu --mapping_file=${BASE_BUILD_DIR}/bitcoin-$HOST/contrib/devtools/iwyu/bitcoin.core.imp"

jonatack commented at 4:06 pm on January 26, 2023:

Light concept ACK if this change reduces review work and unburdens the review process, which seems more important than the potential annoyance it may add to opening a PR. Edit: I would likely find that this change makes working on a PR easier.

hebasto force-pushed on Jan 26, 2023

maflcko commented at 11:27 am on January 31, 2023: member

CI fails with

0should remove these lines:
1- #include <threadsafety.h>  // lines 9-9

iwyu: Fix IWYU violations e65223cb87

ci: Treat IWYU violations as errors 8b817a2a80

hebasto force-pushed on Jan 31, 2023

hebasto commented at 12:59 pm on January 31, 2023: member

CI fails

Thanks! Fixed.

DrahtBot added the label Needs rebase on Feb 2, 2023

DrahtBot commented at 5:31 pm on February 2, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

hebasto closed this on Feb 2, 2023

bitcoin locked this on Feb 2, 2024

ci: Treat IWYU violations as errors #26763

Reviews

Conflicts