build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes

[maflcko]

The bitcoin-config.h includes have issues:

• The header is incompatible with iwyu, because symbols may be defined or not defined. So the IWYU pragma: keep is needed to keep the include when a symbol is not defined on a platform. Compare the previous discussion in lint: Check for missing bitcoin-config.h includes #29408 (comment)
• Guarding the includes by HAVE_CONFIG_H is verbose and brittle. Now that all build config dependencies have been removed from low level headers, the benefits are questionable, and the guard can be removed. The linter could also be tricked by guarding the include by #if defined(HAVE_C0NFIG_H) (O replaced by 0). Compare the previous discussion in refactor: bitcoin-config.h includes cleanup #29404 (comment) .

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, hebasto, achow101

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:

• #29868 (Reintroduce external signer support for Windows by hebasto)
• #28167 (init: Add option for rpccookie permissions (replace 26088) by willcl-ark)

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.

[maflcko]

Currently a draft, so that the conflicting pulls can be merged first, which this pull depends on.

[fanquake]

Want to rebase this now?

[DrahtBot]

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is 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.

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

_{Debug: https://github.com/bitcoin/bitcoin/runs/22559980125}

[TheCharlatan]

git grep 'HAVE_CONFIG_H' reveals one left over define in build-aux/m4/bitcoin_qt.m4. Was that left on purpose?

[maflcko]

Thanks, removed as well. (Not sure why it would be needed for mocs at all)

[TheCharlatan]

ACK fab657b

[maflcko]

cc for cmake @hebasto @theuni

[theuni]

I can't say I like this as I don't really agree that it's harmful as-is. Can't IWYU simply define HAVE_CONFIG_H, and we add the pragmas as well?

I'm not hugely opposed either though. My preference would be to maintain the status quo here, but ultimately I don't care that much.

I would like to finish removing macros from low-level files though, and since this PR gives us a list to look though I'll go ahead and thought-dump here:

I think we should:

• Make all macros in crypto opt-out rather than opt-in, so they're build-able outside of our tree but may require turning features off
• Pass those defines explicitly rather than using bitcoin-config.h (we already do this with some sha2 defines)
• The same for ENABLE_TRACING for util/trace.h: make it DISABLE_TRACING and pass it explicitly where needed.
• Where possible, change the apis in low-level headers to be runtime errors (for ex runCommand and RegisterSignerRPCCommands) to avoid the need for the macros in the headers.

I can open a PR to do the above.

Either way, those things are independent of this PR, so no need to block.

[maflcko]

Can't IWYU simply define HAVE_CONFIG_H, and we add the pragmas as well?

Sorry for putting the two changes in the same scripted diff, but they are unrelated. Now that HAVE_CONFIG_H is no longer included in low-level (standalone) headers, I don't see the need for it anymore. If there is a header that can be built standalone, and needs HAVE_CONFIG_H, let me know. I think iwyu already has HAVE_CONFIG_H defined, so only the pragma change is needed to fix the iwyu suggestions for this header. I am happy to drop the HAVE_CONFIG_H change, but unless there is still a reason to keep HAVE_CONFIG_H, it seems less brittle to me to drop it.

About your other suggested (unrelated) cleanups: There is also a discussion about moving from #ifdef to #if (#16419). If this was done for all symbols in the config header, the iwyu pragma would no longer be needed, I presume. Though, I won't be working on this myself. I mostly care about the changes in this pull request. If someone reworks the build system to make this pull request obsolete, I wouldn't mind :)

[hebasto]

How is it supposed to automatically catch cases when code changes make #include <config/bitcoin-config.h> unneeded in a header or source file?

[maflcko]

How is it supposed to automatically catch cases when code changes make #include <config/bitcoin-config.h> unneeded in a header or source file?

Yes, it is supposed to catch those. This is already the case and unrelated to this pull request. You can try it yourself. The output will be:

^^^
None of the files use a symbol declared in the bitcoin-config.h header. However, they are including
the header. Consider removing the unused include.
            
^---- ⚠️ Failure generated from build config includes check!

[hebasto]

While testing this PR I faced the problem that running linters locally alters the ownership of some files in src to the root user. How to avoid such behaviour?

[maflcko]

I didn't add the docker image, nor is it modified in this pull request. If you have questions about docker or the linter usage, a separate issue may be more appropriate.

edit: If you want to run locally, my recommendation would be to use the test/lint/test_runner.

[hebasto]

How is it supposed to automatically catch cases when code changes make #include <config/bitcoin-config.h> unneeded in a header or source file?

Yes, it is supposed to catch those. This is already the case and unrelated to this pull request. You can try it yourself. The output will be:

^^^
None of the files use a symbol declared in the bitcoin-config.h header. However, they are including
the header. Consider removing the unused include.
            
^---- ⚠️ Failure generated from build config includes check!

Of course, I've tried.

Consider the following diff:

$ git diff
diff --git a/src/base58.cpp b/src/base58.cpp
index cf5d62f164..96aa9255de 100644
--- a/src/base58.cpp
+++ b/src/base58.cpp
@@ -4,6 +4,8 @@
 
 #include <base58.h>
 
+#include <config/bitcoin-config.h> // IWYU pragma: keep
+
 #include <hash.h>
 #include <uint256.h>
 #include <util/strencodings.h>
diff --git a/src/base58.h b/src/base58.h
index 2f4d0b74b1..f4dc16d307 100644
--- a/src/base58.h
+++ b/src/base58.h
@@ -14,6 +14,8 @@
 #ifndef BITCOIN_BASE58_H
 #define BITCOIN_BASE58_H
 
+#include <config/bitcoin-config.h> // IWYU pragma: keep
+
 #include <span.h>
 
 #include <string>

The linter silently removes the added #include <config/bitcoin-config.h> // IWYU pragma: keep lines with no message.

To elaborate my concerns, adding // IWYU pragma: keep precludes the IWYU from notifying us about the cases when the config/bitcoin-config.h is no longer required, right?

[maflcko]

The linter silently removes the added #include <config/bitcoin-config.h> // IWYU pragma: keep lines with no message.

Again, this is not about the linter. This is an issue with the docker setup, which I didn't write, and which is not changed in this pull request. If the issue persists in the lint/test_runner, then let me know. Otherwise, this is a separate issue.

To elaborate my concerns, adding // IWYU pragma: keep precludes the IWYU from notifying us about the cases when the config/bitcoin-config.h is no longer required, right?

The whole point of this pull request is that iwyu can not be used for this type of check.

[hebasto]

ACK fa4673c, I have reviewed the code and it looks OK. Tested on Ubuntu 23.10.

[maflcko]

Rebased (should be trivial to re-ACK)

[TheCharlatan]

ACK fa09451

[hebasto]

re-ACK fa09451, only rebased since my recent review (timedata.cpp removed in #29623).

[maflcko]

Is this waiting for someone's review, or are two ACKs sufficient?

[achow101]

ACK fa09451