I don't think we want these PUBLIC? (I also don't think IWYU should be driving build system changes)

As stated in the commit message, std::map does not allow incomplete types. settings.h therefore requires univalue.h to be included. That is a PUBLIC dependency by definition.

At the moment, this dependency is not expressed explicitly. The consequence is that all clients that include settings.h also have to include univalue.h and link bitcoin_common. IWYU does not introduce a new dependency, it just makes it explicit that the dependency exists already.

Expressing the actual architecture explicitly should be seen as a good thing and we can expect to see more of that in the future. FILE_SET will make it explicit which target a header file belongs to, which is also something that is not expressed in the buildsystem at the moment. Chances are, that we realize that the actual architecture does not match our expectations. We should not blame the tools for this.

settings.h therefore requires univalue.h to be included. That is a PUBLIC dependency by definition.

I don't follow this -- settings.cpp is in bitcoin_common not bitcoin_node, so if the settings to univalue dependency did mean univalue should be publicly exposed, that would only affect bitcoin_common, not bitcoin_node. But just because univalue is used as the internal implementation for settings, and header information needs to be available to everyone using settings, that doesn't mean the implementation logic needs to be a PUBLIC export even for bitcoin_common, as far as I can see.

If I'm missing something, can you spell out the logic here?

@hebasto / @purpleKarrot can either of you follow up here?

If we do want to make this change (including univalue.h properly in that header, satisfying IWYU and making the header easier to use), then I think the PUBLIC approach is the way to go, rather than adding PRIVATE univalue links to every user of common that doesn't already have it.

It's not clear to me whether that's a good or a bad idea (does it add univalue code to libs/executables where it's not needed?). IWYU is "correct" in the sense that as-is the dependencies are a code-smell -- if you include this header and try to use it, without also including univalue.h you'll get a bug (and you also can't include univalue.h without adding at least a PRIVATE dep on univalue for the module). So if we don't want to add univalue to every thing that uses common, we should at least add a pragma to document that this isn't ideal code, perhaps something like:

// Users of this header need to explicitly #include <univalue.h>
class Univalue; // IWYU pragma: keep

In commit "Include <univalue.h> instead of forward declaring UniValue" (b1c654b)

I have the same question as AJ. I don't see why bitcoin_node needs to change here. The bitcoin_common change above should already propagate the univalue dependency to consumers of bitcoin_common.

Changing bitcoin_common from a PRIVATE to PUBLIC dependency of bitcoin_node seems like a separate interface change, only needed if bitcoin_node public headers expose bitcoin_common headers/types. Is there a specific header requiring this?

Doesn't look like there's been a response to the questions in this thread. Even if this was discussed offline, or similar, given that multiple reviewers have all asked the same question, it seems like there should be a response here.

I don't follow this -- settings.cpp is in bitcoin_common not bitcoin_node, so if the settings to univalue dependency did mean univalue should be publicly exposed, that would only affect bitcoin_common, not bitcoin_node. But just because univalue is used as the internal implementation for settings, and header information needs to be available to everyone using settings, that doesn't mean the implementation logic needs to be a PUBLIC export even for bitcoin_common, as far as I can see.

I agree. The changes to the univalue dependency do not imply promoting bitcoin_common from PRIVATE to PUBLIC in bitcoin_node.

Let's take a step back, and look at the bitcoin_node's consumers. Skipping GUI and tests, there are two of them: bitcoind and bitcoin-node. Both share the same source file src/bitcoind.cpp. Looking at its included headers, one can clearly see that it directly uses symbols from four static libraries: bitcoin_clientversion, bitcoin_common, bitcoin_node and bitcoin_util. So they are part of bitcoind's build specification.

This can be achieved either (1) by explicitly specifying those libraries in target_link_libraries(bitcoind ...) or (2) by propagating them as usage requirements in target_link_libraries(bitcoin_node PUBLIC ...).

Also note that these dependencies are not reflected in our design documentation (also see #35167 (comment)).

In commit "Include <univalue.h> instead of forward declaring UniValue" (b1c654b)

I have the same question as AJ. I don't see why bitcoin_node needs to change here. The bitcoin_common change above should already propagate the univalue dependency to consumers of bitcoin_common.

Changing bitcoin_common from a PRIVATE to PUBLIC dependency of bitcoin_node seems like a separate interface change, only needed if bitcoin_node public headers expose bitcoin_common headers/types. Is there a specific header requiring this?

In commit "ci, iwyu: Fix warnings in src/common and treat them as errors" (28712de)

Is is right that util/check.h is needed on win32? I don't see Assume/Assert being used in this file, but I do see assert being used. Also it looks like includes are out of order here and I'd be surprised if IWYU was suggesting changes for win32 in any case, so something seems odd here. Would be good to mention in commit message if any changes not suggested by IWYU are being made here.

Is is right that util/check.h is needed on win32? I don't see Assume/Assert being used in this file, but I do see assert being used... Would be good to mention in commit message if any changes not suggested by IWYU are being made here.

Yes, we use <util/check.h> as a drop-in replacement for <cassert>. I've updated the commit message to explicitly mention these manual header replacements:

ci, iwyu: Enforce warning-free `src/common`

Fix all IWYU warnings in `src/common` and treat them as errors in the
CI.

Additionally, ensure that our drop-in header replacements are used
instead of system headers:
- `<util/check.h>` instead of `<cassert>`
- `<util/time.h>` instead of `<chrono>`

Also it looks like includes are out of order here...

The headers here:

are sorted by groups first: project headers, system headers, and then platform headers. They are sorted lexicographically within each group. The same approach is used across the entire codebase.