ci: Treat IWYU violations as errors #26763

hebasto · 2022-12-28T18:17:13Z

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

DrahtBot · 2022-12-28T18:17:15Z

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.

src/util/string.cpp

src/primitives/transaction.h

fanquake · 2022-12-29T10:11:47Z

src/compat/compat.h

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


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.

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

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

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

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

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.

#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:

#ifdef WIN32 #include <winsock2.h> #else #include <sys/socket.h> #endif

…sk back to "ASan..." one afc6052 ci: Move `--enable-c++20` from "tidy" task back to "ASan..." one (Hennadii Stepanov) Pull request description: This PR reverts cc7335e from bitcoin/bitcoin#25528 partially. C++20 has introduced some new headers, and it is premature to consider them when using the IWYU tool. Required for bitcoin/bitcoin#26763 and bitcoin/bitcoin#26766. Related discussions: - bitcoin/bitcoin#25528 (comment) - bitcoin/bitcoin#26763 (comment) ACKs for top commit: MarcoFalke: review only ACK afc6052 Tree-SHA512: 9c1d3317d0f7a94d46d1e442da1a91669cd9ed1e62a41579edc96e1f5447551b536d32eeb222caf277e85228482753be545e0a874208cb24f9a8491fce3b6d9f

hebasto · 2022-12-29T20:29:31Z

Rebased 682a87e -> 9b7443d (pr26763.02 -> pr26763.03) on top of the #26770.

ajtowns

Concept ACK

ajtowns · 2023-01-03T05:10:31Z

src/chain.h

 #include <vector>

+namespace Consensus {
+struct Params;


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

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:

$ git grep -e "struct Params;" src/node/blockstorage.h:struct Params; src/node/miner.h:namespace Consensus { struct Params; }; src/node/transaction.h:struct Params; src/txdb.h:struct Params; src/validation.h:struct Params; src/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?

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

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?

src/versionbits.h

hebasto · 2023-01-04T10:50:51Z

Updated 9b7443d -> a697950 (pr26763.03 -> pr26763.04):

rebased
dropped non-required changes

maflcko · 2023-01-04T11:05:37Z

src/test/fuzz/util/net.cpp

@@ -21,6 +21,7 @@
 #include <cstdlib>
 #include <cstring>
 #include <thread>
+#include <unordered_map>


Looks wrong as well?

Well, in FuzzedSock::WaitMany() function

bitcoin/src/test/fuzz/util/net.cpp

Lines 332 to 339 in 4717a5a

bool FuzzedSock::WaitMany(std::chrono::milliseconds timeout, EventsPerSock& events_per_sock) const

{

for (auto& [sock, events] : events_per_sock) {

(void)sock;

events.occurred = m_fuzzed_data_provider.ConsumeBool() ? events.requested : 0;

}

return true;

}

the type EventsPerSock is actually an std::unordered_map:

bitcoin/src/util/sock.h

Line 218 in 2ec9782

using EventsPerSock = std::unordered_map<std::shared_ptr<const Sock>, Events, HashSharedPtrSock, EqualSharedPtrSock>;

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

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.

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.

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

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

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

hebasto · 2023-01-04T13:12:20Z

Updated a697950 -> ba0780b (pr26763.04 -> pr26763.05, diff):

addressed @MarcoFalke's comment

hebasto · 2023-01-17T15:02:12Z

Needs rebase

Rebased ba0780b -> 077376c (pr26763.05 -> pr26763.06).

maflcko · 2023-01-17T20:57:38Z

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 · 2023-01-18T12:07:03Z

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 · 2023-01-18T13:36:47Z

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:

>>> Fixing #includes in 'dbwrapper.h'
@@ -11,20 +11,19 @@
 #include <serialize.h>
 #include <span.h>
 #include <streams.h>
-
 #include <leveldb/db.h>
 #include <leveldb/iterator.h>
 #include <leveldb/options.h>
 #include <leveldb/slice.h>
 #include <leveldb/status.h>
 #include <leveldb/write_batch.h>
-
 #include <cstddef>
 #include <cstdint>
 #include <exception>
 #include <stdexcept>
 #include <string>
 #include <vector>
+#include <optional>

 namespace leveldb {
 class Env;
>>> Fixing #includes in 'kernel/context.cpp'
@@ -3,13 +3,10 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.

 #include <kernel/context.h>
-
 #include <crypto/sha256.h>
 #include <key.h>
 #include <logging.h>
-#include <pubkey.h>
 #include <random.h>
-
 #include <string>


IWYU 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 · 2023-01-18T13:50:45Z

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 · 2023-01-18T14:57:12Z

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 · 2023-01-19T09:05:33Z

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

It doesn't help, unfortunately.

hebasto · 2023-01-26T15:48:01Z

Rebased and drafted.

maflcko · 2023-01-26T15:56:08Z

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.

jonatack · 2023-01-26T16:06:30Z

ci/test/06_script_b.sh

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


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.

maflcko · 2023-01-31T11:27:57Z

CI fails with

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

hebasto · 2023-01-31T12:59:43Z

CI fails

Thanks! Fixed.

DrahtBot · 2023-02-02T17:31:45Z

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

hebasto force-pushed the 221228-iwyu branch from a63ceab to b55e240 Compare December 28, 2022 18:17

DrahtBot added the Tests label Dec 28, 2022

This was referenced Dec 28, 2022

tidy: add include-what-you-use #24831

Merged

ci: Use clang-15 and IWYU v0.19 in "tidy" task #26766

Closed

maflcko reviewed Dec 29, 2022

View reviewed changes

src/util/string.cpp Outdated Show resolved Hide resolved

fanquake reviewed Dec 29, 2022

View reviewed changes

hebasto mentioned this pull request Dec 29, 2022

ci: run USDT interface tests in the CI #25528

Merged

hebasto force-pushed the 221228-iwyu branch from b55e240 to 682a87e Compare December 29, 2022 12:53

hebasto mentioned this pull request Dec 29, 2022

ci: Move --enable-c++20 from "tidy" task back to "ASan..." one #26770

Merged

hebasto force-pushed the 221228-iwyu branch from 682a87e to 9b7443d Compare December 29, 2022 20:26

ajtowns reviewed Jan 3, 2023

View reviewed changes

hebasto force-pushed the 221228-iwyu branch from 9b7443d to a697950 Compare January 4, 2023 10:47

maflcko reviewed Jan 4, 2023

View reviewed changes

hebasto force-pushed the 221228-iwyu branch from a697950 to ba0780b Compare January 4, 2023 13:09

maflcko added the Needs rebase label Jan 17, 2023

DrahtBot removed the Needs rebase label Jan 17, 2023

hebasto force-pushed the 221228-iwyu branch from ba0780b to 077376c Compare January 17, 2023 15:00

DrahtBot mentioned this pull request Jan 21, 2023

build: Add CMake-based build system #25797

Closed

16 tasks

This was referenced Jan 21, 2023

indexes: Stop using node internal types and locking cs_main, improve sync logic #24230

Draft

refactor: Remove c_str from util/check #26960

Merged

DrahtBot added the Needs rebase label Jan 26, 2023

hebasto force-pushed the 221228-iwyu branch from 077376c to e3d73a2 Compare January 26, 2023 15:47

DrahtBot removed the Needs rebase label Jan 26, 2023

jonatack reviewed Jan 26, 2023

View reviewed changes

hebasto force-pushed the 221228-iwyu branch from e3d73a2 to 2445fe1 Compare January 26, 2023 16:24

hebasto added 2 commits January 31, 2023 12:21

iwyu: Fix IWYU violations

e65223c

ci: Treat IWYU violations as errors

8b817a2

hebasto force-pushed the 221228-iwyu branch from 2445fe1 to 8b817a2 Compare January 31, 2023 12:21

hebasto mentioned this pull request Feb 1, 2023

ci: Print iwyu patch in git diff format #27012

Merged

DrahtBot added the Needs rebase label Feb 2, 2023

hebasto closed this Feb 2, 2023

hebasto mentioned this pull request Aug 14, 2023

ci: Move tidy to persistent worker #28214

Merged

bitcoin locked and limited conversation to collaborators Feb 2, 2024

	bool FuzzedSock::WaitMany(std::chrono::milliseconds timeout, EventsPerSock& events_per_sock) const
	{
	for (auto& [sock, events] : events_per_sock) {
	(void)sock;
	events.occurred = m_fuzzed_data_provider.ConsumeBool() ? events.requested : 0;
	}
	return true;
	}

ci: Treat IWYU violations as errors #26763

ci: Treat IWYU violations as errors #26763

Uh oh!

Conversation

hebasto commented Dec 28, 2022

Uh oh!

DrahtBot commented Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Dec 29, 2022

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Jan 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jan 4, 2023

Uh oh!

hebasto commented Jan 17, 2023

Uh oh!

maflcko commented Jan 17, 2023

Uh oh!

fanquake commented Jan 18, 2023

Uh oh!

hebasto commented Jan 18, 2023

Uh oh!

maflcko commented Jan 18, 2023

Uh oh!

fanquake commented Jan 18, 2023

Uh oh!

hebasto commented Jan 19, 2023

Uh oh!

hebasto commented Jan 26, 2023

Uh oh!

maflcko commented Jan 26, 2023

Uh oh!

jonatack Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

DrahtBot commented Dec 28, 2022 •

edited

Loading

hebasto Jan 3, 2023 •

edited

Loading

jonatack Jan 26, 2023 •

edited

Loading