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

maflcko commented at 6:39 pm on February 27, 2024: member

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 #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 #29404 (review) .

DrahtBot commented at 6:39 pm on February 27, 2024: contributor

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.

DrahtBot renamed this:
~~build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes~~
build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes
on Feb 27, 2024

DrahtBot added the label Build system on Feb 27, 2024

maflcko commented at 6:39 pm on February 27, 2024: member

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

DrahtBot added the label Needs rebase on Feb 28, 2024

maflcko force-pushed on Feb 29, 2024

DrahtBot removed the label Needs rebase on Feb 29, 2024

fanquake commented at 4:21 pm on March 1, 2024: member

Want to rebase this now?

DrahtBot added the label Needs rebase on Mar 1, 2024

maflcko marked this as ready for review on Mar 12, 2024

maflcko force-pushed on Mar 12, 2024

DrahtBot removed the label Needs rebase on Mar 12, 2024

maflcko force-pushed on Mar 12, 2024

DrahtBot added the label CI failed on Mar 13, 2024

DrahtBot commented at 11:08 am on March 13, 2024: contributor

🚧 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

maflcko force-pushed on Mar 13, 2024

TheCharlatan commented at 11:42 am on March 13, 2024: contributor

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

maflcko force-pushed on Mar 13, 2024

maflcko commented at 11:54 am on March 13, 2024: member

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

TheCharlatan approved

TheCharlatan commented at 12:30 pm on March 13, 2024: contributor

ACK fab657b191c997506dd32fe7a1efc0657c206426

maflcko commented at 1:17 pm on March 13, 2024: member

cc for cmake @hebasto @theuni

DrahtBot removed the label CI failed on Mar 13, 2024

theuni commented at 6:42 pm on March 25, 2024: member

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 commented at 7:47 am on March 26, 2024: member

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 (https://github.com/bitcoin/bitcoin/issues/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 :)

DrahtBot added the label Needs rebase on Apr 1, 2024

maflcko force-pushed on Apr 4, 2024

DrahtBot removed the label Needs rebase on Apr 4, 2024

DrahtBot added the label Needs rebase on Apr 5, 2024

maflcko force-pushed on Apr 9, 2024

DrahtBot removed the label Needs rebase on Apr 9, 2024

TheCharlatan approved

TheCharlatan commented at 7:10 am on April 10, 2024: contributor

Re-ACK ffff3d2bae599d872bae184c4d7c2b0dacfc6e26

DrahtBot added the label Needs rebase on Apr 28, 2024

maflcko force-pushed on Apr 29, 2024

DrahtBot removed the label Needs rebase on Apr 29, 2024

hebasto commented at 1:27 pm on April 29, 2024: member

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 commented at 2:29 pm on April 29, 2024: member

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:

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

hebasto commented at 3:06 pm on April 29, 2024: member

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 commented at 3:11 pm on April 29, 2024: member

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 commented at 3:18 pm on April 29, 2024: member

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:
0^^^
1None of the files use a symbol declared in the bitcoin-config.h header. However, they are including
2the header. Consider removing the unused include.
3            
4^---- ⚠️ Failure generated from build config includes check!

Of course, I’ve tried.

Consider the following diff:

 0$ git diff
 1diff --git a/src/base58.cpp b/src/base58.cpp
 2index cf5d62f164..96aa9255de 100644
 3--- a/src/base58.cpp
 4+++ b/src/base58.cpp
 5@@ -4,6 +4,8 @@
 6 
 7 #include <base58.h>
 8 
 9+#include <config/bitcoin-config.h> // IWYU pragma: keep
10+
11 #include <hash.h>
12 #include <uint256.h>
13 #include <util/strencodings.h>
14diff --git a/src/base58.h b/src/base58.h
15index 2f4d0b74b1..f4dc16d307 100644
16--- a/src/base58.h
17+++ b/src/base58.h
18@@ -14,6 +14,8 @@
19 #ifndef BITCOIN_BASE58_H
20 #define BITCOIN_BASE58_H
21 
22+#include <config/bitcoin-config.h> // IWYU pragma: keep
23+
24 #include <span.h>
25 
26 #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 commented at 3:20 pm on April 29, 2024: member

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 approved

hebasto commented at 3:35 pm on April 29, 2024: member

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

DrahtBot requested review from TheCharlatan on Apr 29, 2024

DrahtBot added the label Needs rebase on Apr 30, 2024

scripted-diff: Add IWYU pragma keep to bitcoin-config.h includes

-BEGIN VERIFY SCRIPT-
 perl -0777 -pi -e 's/#if defined\(HAVE_CONFIG_H\)\n#include <config\/bitcoin-config.h>.*\n#endif.*\n/#include <config\/bitcoin-config.h> \/\/ IWYU pragma: keep\n/g' $( git grep -l '#include <config/bitcoin-config.h>' )
-END VERIFY SCRIPT-

dddd40ba82

Add lint check for bitcoin-config.h include IWYU pragma

Also, remove the no longer needed, remaining definitions and checks of
HAVE_CONFIG_H.

fa09451f8e

maflcko force-pushed on May 1, 2024

maflcko commented at 6:37 am on May 1, 2024: member

Rebased (should be trivial to re-ACK)

DrahtBot removed the label Needs rebase on May 1, 2024

TheCharlatan approved

TheCharlatan commented at 7:51 pm on May 1, 2024: contributor

ACK fa09451f8e6799682d7e7c863f25334fd1c7dce3

DrahtBot requested review from hebasto on May 1, 2024

hebasto approved

hebasto commented at 9:06 pm on May 2, 2024: member

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

maflcko commented at 1:38 pm on May 7, 2024: member

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

achow101 commented at 6:03 pm on May 7, 2024: member

ACK fa09451f8e6799682d7e7c863f25334fd1c7dce3

achow101 merged this on May 7, 2024

achow101 closed this on May 7, 2024

maflcko deleted the branch on May 7, 2024

hebasto added the label Needs CMake port on May 7, 2024

hebasto commented at 1:47 pm on May 21, 2024: member

Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/203.

hebasto removed the label Needs CMake port on May 21, 2024

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

Code Coverage

Reviews

Conflicts