build: Introduce CMake-based build system #30454

hebasto · 2024-07-16T14:55:55Z

This PR introduces a new CMake-based build system, which is a drop-in replacement for the current Autotools-based build system.

ML announcement: https://groups.google.com/g/bitcoindev/c/hgKkfQWzrTo

As discussed during the recent CoreDev meetup in April, the switch from Autotools to CMake is intended to happen as soon as possible after branching 28.x off, which means that 29.0 will be built using CMake.

This PR branch is essentially the staging branch, with every change reviewed and tested by a group of contributors, including (in alphabetical order):

Reviewing in a separate staging repo was suggested in #27060 (comment).

The accompanying changes to the OSS-Fuzz project are available in hebasto/oss-fuzz#8.

Please refer to the build options parity table. The "auto" value is no longer available; non-default values must be specified explicitly. Additionally, the new default values have been chosen to suit the everyday build experience for the majority of developers.

System requirements for using the CMake-based build system:

CMake >= 3.22 (if not available in your system's repository, it can be downloaded from https://cmake.org/download/)
a build tool of your choice:
- any Make (GNU Make is no longer a requirement); GNU Make is still required to build depends
- Ninja (https://ninja-build.org/)
- MSBuild
- Xcode

A note for Windows users: The default installation of the latest version of MSVC 17.10.4 includes both CMake 3.28.3 and the vcpkg package manager).

We, the build system developers, kindly ask reviewers to refrain from making suggestions that are not directly related to the migration process or can be implemented separately. Bugs in the scripts and errors in the updated documentation should be the focus of this PR. Please be advised that comments not aligned with this PR's goal may be ignored.

Thank you all for your understanding.

DrahtBot · 2024-07-16T14:55:58Z

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	vasild, TheCharlatan, maflcko, sipsorcery, pablomartin4btc, i-am-yuvi, theuni, fanquake
Concept ACK	Sjors
Stale ACK	whitslack

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:

#30717 (rpc: Add test-only RPCs under -test=<option> flag by Prabhat1308)
#30685 (build: Mark x86_64-linux-gnu release binaries as CET-enabled by hebasto)
#30634 (ci: Use clang-19 from apt.llvm.org by maflcko)
#30619 (doc: Change nproc in docs to getconf _NPROCESSORS_ONLN by l0rinc)
#30527 (Bump python minimum supported version to 3.10 by maflcko)
#30465 (depends: Set CMAKE_SYSTEM_VERSION for CMake builds by hebasto)
#30043 (net: Replace libnatpmp with built-in PCP+NATPMP implementation by laanwj)
#29881 (guix: use GCC 13 to builds releases by fanquake)
#29868 (Reintroduce external signer support for Windows by hebasto)
#25573 ([POC] guix: produce a fully -static-pie bitcoind by fanquake)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#10102 (Multiprocess bitcoin by ryanofsky)

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 · 2024-07-16T15:08:44Z

The only differences from the staging branch are:

I'd say the section can be removed (or moved to a separate comment), because when this will be merged, I presume many more differences will accumulate. Even looking at the outstanding ports (https://github.com/bitcoin/bitcoin/issues?q=label%3A%22Needs+CMake+port%22+is%3Aclosed) right now, there are some.

maflcko · 2024-07-16T15:18:15Z

Please refer to the build options parity table.

Missing link/reference?

hebasto · 2024-07-16T15:23:13Z

Please refer to the build options parity table.

Missing link/reference?

Thanks! Added.

theuni · 2024-07-16T15:52:41Z

src/crypto/sha256.cpp

@@ -2,8 +2,6 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.

-#include <config/bitcoin-config.h> // IWYU pragma: keep


In commit ff36846

Could you remind me what's going on here, please? I assume these aren't needed because we're meant to be adding compile definitions to specific libs. But from what I can tell, we're not actually adding them to sha256.cpp? So... is this currently using an un-optimized sha2?

Generally I'd feel more comfortable turning these into opt-outs, I think, to avoid accidentally building without optims.

For the sake of review/sanity, I think for this commit it's worth documenting (in the commit message) exactly which defines are affected and where in CMake they've been added.

But from what I can tell, we're not actually adding them to sha256.cpp? So... is this currently using an un-optimized sha2?

Please note the PUBLIC keyword, which propagates the definition as a usage requirement:

bitcoin/src/crypto/CMakeLists.txt

Line 125 in 2b7c0f4

target_compile_definitions(bitcoin_crypto_arm_shani PUBLIC ENABLE_ARM_SHANI)

and

bitcoin/src/crypto/CMakeLists.txt

Line 128 in 2b7c0f4

target_link_libraries(bitcoin_crypto PRIVATE bitcoin_crypto_arm_shani)

where that usage requirement is actually applied.

The same approach works for other definitions used in src/crypto/sha256.cpp.

For the sake of review/sanity, I think for this commit it's worth documenting (in the commit message) exactly which defines are affected and where in CMake they've been added.

Sure thing! I'll update the commit message during the next branch update.

One can see that they are indeed disabled now in https://corecheck.dev/bitcoin/bitcoin/pulls/30454 (coverage report + benchmarks).

Shouldn't autotools be nuked in this pull request, assuming that cmake and autotools are apparently incompatible, as designed now?

Please see hebasto#269.

@theuni @maflcko

Can you confirm please that the recent push resolved this issue?

Hmm https://corecheck.dev/bitcoin/bitcoin/pulls/30454 is down again

Hmm https://corecheck.dev/bitcoin/bitcoin/pulls/30454 is down again

It is up now :)

Hmm https://corecheck.dev/bitcoin/bitcoin/pulls/30454 is down again

It is up now :)

Nice. The output there looks better now. But I haven't yet tried locally.

ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh

src/crypto/CMakeLists.txt

hebasto · 2024-07-16T20:16:19Z

A couple of functional tests-related commits have been split to #30463, as suggested by @fanquake offline.

maflcko · 2024-07-17T08:32:56Z

which means that 29.0 will be built using CMake.

Could add the 29.0 milestone?

ci/test/00_setup_env_native_fuzz_with_msan.sh

hebasto · 2024-07-17T08:48:38Z

which means that 29.0 will be built using CMake.

Could add the 29.0 milestone?

Done.

d318c4e depends: bump libmultiprocess for CMake fixes (Cory Fields) Pull request description: Broken out of bitcoin#30454 . Bumped [even further](bitcoin@4883197#r1684802528) after bitcoin-core/libmultiprocess#98 was merged upstream. hebasto Presumably this approach works now with the CMake branch? ACKs for top commit: ryanofsky: Code review ACK d318c4e. Tree-SHA512: 4b5491f73c0063d09e8339829cd831b1f4c441dd7b55a22037c9337c80cce19bb00a3e5cf925efa77d6d4e89ab45482f40f5799bc14948f8cabcbad3c3549430

7231c76 qt: Replace deprecated LogPrintf with LogInfo in GUIUtil::LogQtInfo() (Hennadii Stepanov) b3d3ae0 qt, build: Drop `QT_STATICPLUGIN` macro (Hennadii Stepanov) Pull request description: Broken out of bitcoin#30454. Our `QT_STATICPLUGIN` macro is effectively equivalent to the Qt's `QT_STATIC` macro. It is easy to see in the `_BITCOIN_QT_IS_STATIC` macro implementation: https://github.com/bitcoin/bitcoin/blob/ebd82fa9fae13d5a7a395800295dd7fd34185b58/build-aux/m4/bitcoin_qt.m4#L269-L292 No need to handle both macros. ACKs for top commit: maflcko: re-ACK 7231c76 TheCharlatan: ACK 7231c76 Tree-SHA512: abbf21859b7ac2aaf47c5b0e075403e4cc9bc540b1565d23f51650b8932dde314586aca67fd4ed5daadebc89268baf8c18f65348fa2b836078ac24543c14cfd6

…nced local variable" 44f0878 test: Fix MSVC warning C4101 "unreferenced local variable" (Hennadii Stepanov) 5d25a82 univalue, refactor: Convert indentation tabs to spaces (Hennadii Stepanov) Pull request description: This PR is split from bitcoin#30454 and addresses MSVC warning [C4101](https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4101) "unreferenced local variable". The current MSVC build system in the master branch skips building univalue tests, so it is not affected. No behaviour changes. ACKs for top commit: kevkevinpal: utACK [44f0878](bitcoin@44f0878) maflcko: ACK 44f0878 theuni: trivial ACK 44f0878. Tree-SHA512: 661d3b40ddb4f7915de7a65ccb27a24da88ae499ce03c036099007260b0597e83738f1a3a420985b51f798ee309ade32988c6d78f4ffed401099b175a0b2025b

8c935e6 depends: Fix CMake-generated `libevent*.pc` files (Hennadii Stepanov) Pull request description: Broken out of bitcoin#30454. This is a backport of the merged upstream PR: libevent/libevent#1622. Note that after bitcoin#29835 we might end up dropping pkg-config and using the installed CMake files directly, but that depends on whether or not enough distros actually ship those files. Either way, having fixed up .pc files won't hurt. ACKs for top commit: hebasto: ACK 8c935e6. fanquake: ACK 8c935e6 Tree-SHA512: 259c2ad78fb9e90370a7205dc71c40acda1a872f6509435133bc1c4c2c3de57366e80679aa083e13ed85e7966883dc470c0147ee171a2ed0171a18cd5ffc99b3