-
Notifications
You must be signed in to change notification settings - Fork 35.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: bitcoin-config.h includes cleanup #29404
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
3fc5395
to
70171e0
Compare
Removed the include from //! function will be undefined in builds where ENABLE_WALLET is false. That was the only example of a false-positive that I managed to find. |
I validated the patch by running: The script misses a few of the cases, because some subset of the Maybe a tidy plugin doing a full text search during the pre-processor hook could work, which would be kind of similar to the process you already posted here. So I guess we could add a little rust lint job for this instead? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include them manually with the exception of some files in crypto:
unrelated question: Is there a reason they need to use the flag at all? The files are never compiled and linked when the flag isn't set, so making them appear "empty" when the flag isn't set is not needed?
@@ -3,6 +3,10 @@ | |||
// Distributed under the MIT software license, see the accompanying | |||
// file COPYING or http://www.opensource.org/licenses/mit-license.php. | |||
|
|||
#if defined(HAVE_CONFIG_H) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated question: Is there any reason for this check? I understand that it is possible to leave the symbol unset and then set any symbols over the command line, but is anyone doing this, or is it realistic that anyone will ever do it?
If not, it could make sense to unconditionally include the config header?
Otherwise, I could see a bug silently sneak in when there is a typo in this line, such as #if defined(_HAVE_CONFIG_H)
(or any other intentional or accidental typo)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several headers (serialization comes to mind) that are very handy to use just by copying them out of our tree and using them as-is without a buildsystem. I do this quite often myself. I'd hate to give it up, but it's also become a goal of mine to remove the need for any autoconf defines for low-level files like that. Not a hard goal, just a nice-to-have. And with c++20 we're nearly there. See #29263 for a PR that does a good bit in this regard.
It would also be a nice property for libbitcoinkernel though :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was that removing the guard doesn't preclude to copy the header and use it elsewhere, instead it makes it obvious that some features are disabled, or may be misdetected (which could lead to bugs). The overhead of removing a single line of code after creating the copy seems worth it to notify the user of this fact? Also, the overall overhead of all users copying a few headers and having to remove a single line in some of them should be much less than the overhead on Bitcoin Core having to add at least two lines of code for each such include manually. (For example, IWYU doesn't do it itself).
So it seems like a win-win to me, but no strong opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless there's an objection, I'd follow up with this in a follow-up?
Concept ACK |
Yeah, I agree. If there's some historical reason for this, I've forgotten it. It makes sense to me to make it either optionally compiled or optionally opted in (or out) via a define, but I don't see the need for both. I do seem to remember some tools ( |
Made this into a scripted-diff here: TheCharlatan@e4186f7 There are no differences with the patch in this PR, besides a single whitespace issue. Feel free to pick the commit and make this into a scripted-diff as well, though I'm not sure if it is worth it, since the script is a bit dense. |
Concept ACK 70171e0. |
@TheCharlatan saw this and said "hold my beer". Amazing! |
3e0ddb0
to
a12fb57
Compare
a12fb57
to
f8aee6c
Compare
Replaced my commit with @TheCharlatan's artwork above. It doesn't work in the c-i environment because Next I tried prepending with a bare Sadly, I'm not sure it's worth continuing with this. Even if it does work, the verify script would have to modify the user's tree which isn't very nice :( |
Could just replace the first command then. Not as nice, but for the purpose of this PR, reviewers can run the mentioned command manually: TheCharlatan@b78e3ce (also applies some fixes, because the verify script uses |
@TheCharlatan Sure, agreed it's better than nothing. Not quite self-contained, but given that a few of us have independently arrived at what the correct diff should be, it's easy to tell if it's correct. Will take that commit. |
-BEGIN VERIFY SCRIPT- regex_string='^(?!//).*(AC_APPLE_UNIVERSAL_BUILD|BOOST_PROCESS_USE_STD_FS|CHAR_EQUALS_INT8|CLIENT_VERSION_BUILD|CLIENT_VERSION_IS_RELEASE|CLIENT_VERSION_MAJOR|CLIENT_VERSION_MINOR|COPYRIGHT_HOLDERS|COPYRIGHT_HOLDERS_FINAL|COPYRIGHT_HOLDERS_SUBSTITUTION|COPYRIGHT_YEAR|ENABLE_ARM_SHANI|ENABLE_AVX2|ENABLE_EXTERNAL_SIGNER|ENABLE_SSE41|ENABLE_TRACING|ENABLE_WALLET|ENABLE_X86_SHANI|ENABLE_ZMQ|HAVE_BOOST|HAVE_BUILTIN_CLZL|HAVE_BUILTIN_CLZLL|HAVE_BYTESWAP_H|HAVE_CLMUL|HAVE_CONSENSUS_LIB|HAVE_CXX20|HAVE_DECL_BE16TOH|HAVE_DECL_BE32TOH|HAVE_DECL_BE64TOH|HAVE_DECL_BSWAP_16|HAVE_DECL_BSWAP_32|HAVE_DECL_BSWAP_64|HAVE_DECL_FORK|HAVE_DECL_FREEIFADDRS|HAVE_DECL_GETIFADDRS|HAVE_DECL_HTOBE16|HAVE_DECL_HTOBE32|HAVE_DECL_HTOBE64|HAVE_DECL_HTOLE16|HAVE_DECL_HTOLE32|HAVE_DECL_HTOLE64|HAVE_DECL_LE16TOH|HAVE_DECL_LE32TOH|HAVE_DECL_LE64TOH|HAVE_DECL_PIPE2|HAVE_DECL_SETSID|HAVE_DECL_STRERROR_R|HAVE_DEFAULT_VISIBILITY_ATTRIBUTE|HAVE_DLFCN_H|HAVE_DLLEXPORT_ATTRIBUTE|HAVE_ENDIAN_H|HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR|HAVE_FDATASYNC|HAVE_GETENTROPY_RAND|HAVE_GETRANDOM|HAVE_GMTIME_R|HAVE_INTTYPES_H|HAVE_LIBADVAPI32|HAVE_LIBCOMCTL32|HAVE_LIBCOMDLG32|HAVE_LIBGDI32|HAVE_LIBIPHLPAPI|HAVE_LIBKERNEL32|HAVE_LIBOLE32|HAVE_LIBOLEAUT32|HAVE_LIBSHELL32|HAVE_LIBSHLWAPI|HAVE_LIBUSER32|HAVE_LIBUUID|HAVE_LIBWINMM|HAVE_LIBWS2_32|HAVE_MALLOC_INFO|HAVE_MALLOPT_ARENA_MAX|HAVE_MINIUPNPC_MINIUPNPC_H|HAVE_MINIUPNPC_UPNPCOMMANDS_H|HAVE_MINIUPNPC_UPNPERRORS_H|HAVE_NATPMP_H|HAVE_O_CLOEXEC|HAVE_POSIX_FALLOCATE|HAVE_PTHREAD|HAVE_PTHREAD_PRIO_INHERIT|HAVE_STDINT_H|HAVE_STDIO_H|HAVE_STDLIB_H|HAVE_STRERROR_R|HAVE_STRINGS_H|HAVE_STRING_H|HAVE_STRONG_GETAUXVAL|HAVE_SYSCTL|HAVE_SYSCTL_ARND|HAVE_SYSTEM|HAVE_SYS_ENDIAN_H|HAVE_SYS_PRCTL_H|HAVE_SYS_RESOURCES_H|HAVE_SYS_SELECT_H|HAVE_SYS_STAT_H|HAVE_SYS_SYSCTL_H|HAVE_SYS_TYPES_H|HAVE_SYS_VMMETER_H|HAVE_THREAD_LOCAL|HAVE_TIMINGSAFE_BCMP|HAVE_UNISTD_H|HAVE_VM_VM_PARAM_H|LT_OBJDIR|PACKAGE_BUGREPORT|PACKAGE_NAME|PACKAGE_STRING|PACKAGE_TARNAME|PACKAGE_URL|PACKAGE_VERSION|PTHREAD_CREATE_JOINABLE|QT_QPA_PLATFORM_ANDROID|QT_QPA_PLATFORM_COCOA|QT_QPA_PLATFORM_MINIMAL|QT_QPA_PLATFORM_WINDOWS|QT_QPA_PLATFORM_XCB|QT_STATICPLUGIN|STDC_HEADERS|STRERROR_R_CHAR_P|USE_ASM|USE_BDB|USE_DBUS|USE_NATPMP|USE_QRCODE|USE_SQLITE|USE_UPNP|_FILE_OFFSET_BITS|_LARGE_FILES)' exclusion_files=":(exclude)src/minisketch :(exclude)src/crc32c :(exclude)src/secp256k1 :(exclude)src/crypto/sha256_arm_shani.cpp :(exclude)src/crypto/sha256_avx2.cpp :(exclude)src/crypto/sha256_sse41.cpp :(exclude)src/crypto/sha256_x86_shani.cpp" git grep --perl-regexp --files-with-matches "$regex_string" -- '*.cpp' $exclusion_files | xargs git grep -L "bitcoin-config.h" | while read -r file; do line_number=$(awk -v my_file="$file" '/\/\/ file COPYING or https?:\/\/www.opensource.org\/licenses\/mit-license.php\./ {line = NR} /^\/\// && NR == line + 1 {while(getline && /^\/\//) line = NR} END {print line+1}' "$file"); sed -i "${line_number}i\\\\n\#if defined(HAVE_CONFIG_H)\\n#include <config/bitcoin-config.h>\\n\#endif" "$file"; done; git grep --perl-regexp --files-with-matches "$regex_string" -- '*.h' $exclusion_files | xargs git grep -L "bitcoin-config.h" | while read -r file; do sed -i "/#define.*_H/a \\\\n\#if defined(HAVE_CONFIG_H)\\n#include <config/bitcoin-config.h>\\n\#endif" "$file"; done; for file in $(git grep --files-with-matches 'bitcoin-config.h' -- '*.cpp' '*.h' $exclusion_files); do if ! grep -q --perl-regexp "$regex_string" $file; then sed -i '/HAVE_CONFIG_H/{N;N;N;d;}' $file; fi; done; -END VERIFY SCRIPT- The first command creates a regular expression for matching all bitcoin-config.h symbols in the following form: ^(?!//).*(AC_APPLE_UNIVERSAL_BUILD|BOOST_PROCESS_USE_STD_FS|...|_LARGE_FILES). It was generated with: ./autogen.sh && printf '^(?!//).*(%s)' $(awk '/^#undef/ {print $2}' src/config/bitcoin-config.h.in | paste -sd "|" -) The second command holds a list of files and directories that should not be processed. These include subtree directories as well as some crypto files that already get their symbols through the makefile. The third command checks for missing bitcoin-config headers in .cpp files and adds the header if it is missing. The fourth command checks for missing bitcoin-config headers in .h files and adds the header if it is missing. The fifth command checks for unneeded bitcoin-config headers in sources files and removes the header if it is unneeded.
f8aee6c
to
9d1dbbd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK.
So I guess we could add a little ... lint job for this instead?
Agree. Without a linter, the code might be drifted back in its current state.
It would be interesting where this changes the code. I presume the places are:
Also, there is an iwyu false-positive in the CI output:
|
ACK 9d1dbbd 🚪 Show signatureSignature:
|
At least for now, this one should be coming from |
Ah, good point. So it looks like right now this is a refactor, and shouldn't change the binary on any platform? If you agree, you can add the |
PR title an description updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 9d1dbbd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 9d1dbbd, I have reviewed the code and it looks OK.
Note there are at least two defines missing from the scriptied diff ( |
It is covered in #29408, in any case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 9d1dbbd
…tcoin-config.h includes fa09451 Add lint check for bitcoin-config.h include IWYU pragma (MarcoFalke) dddd40b scripted-diff: Add IWYU pragma keep to bitcoin-config.h includes (MarcoFalke) Pull request description: 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 (comment) . ACKs for top commit: achow101: ACK fa09451 TheCharlatan: ACK fa09451 hebasto: re-ACK fa09451, only rebased since my recent [review](#29494 (review)) (`timedata.cpp` removed in #29623). Tree-SHA512: 47cb973f7f24bc625acc4e78683371863675d186780236d55d886cf4130e05a78bb04f1d731aae7088313b8e963a9677cc77cf518187dbd99d776f6421ca9b52
As mentioned in #26924 (comment) and #29263 (comment), it is currently not safe to remove
bitcoin-config.h
includes from headers because some unrelated file might be depending on it.See also #26972 for discussion.
Solve this by including the file directly everywhere it's required, regardless of whether or not it's already included by another header.
There should be no functional change here, but it will allow us to safely remove includes from headers in the future.
I'm afraid it's a bit tedious to reproduce these commits, but it's reasonably straightforward:Edit: See note below
Edit: I'll keep this old description for posterity, but the manual approach has been replaced with a scripted diff from TheCharlatan