-
Notifications
You must be signed in to change notification settings - Fork 38.6k
build: Introduce CMake-based build system #30454
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
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. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
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. |
Missing link/reference? |
Thanks! Added. |
| // 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 |
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.
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.
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.
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.
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.
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?
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.
Please see hebasto#269.
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 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 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.
Hmm https://corecheck.dev/bitcoin/bitcoin/pulls/30454 is down again
It is up now :)
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.
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.
Could add the 29.0 milestone? |
Done. |
…Make changes 7ebc458 qt: doc: adapt outdated binary paths to CMake changes (Sebastian Falbesoner) Pull request description: Adapt the qt-related instances of outdated binary paths to `./build/bin/...` (see [#30454](#30454) and the more recently merged [#31161](#31161)). According to `$ git grep src/qt.*bitcoin` there should be no more left to address. ACKs for top commit: maflcko: lgtm ACK 7ebc458 Sjors: utACK 7ebc458 fanquake: ACK 7ebc458 hebasto: ACK 7ebc458. Tree-SHA512: 8cd6579fdf209ec4ee3c4c9cfb94cb11d5d5115068d31613d356ca1303214dc4461580535c2d3f2773f373a4271e9a82df25596d8369eef8235822f7030d88bd
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
9556061 code style: update .editorconfig file (Sebastian Falbesoner) Pull request description: Updates the .editorconfig file, first introduced in 2021 (see PR bitcoin#21123, commit 7a135d5) w.r.t. following changes: - consider Rust .rs files (relevant since bitcoin#28076, commit bbbbdb0) - reflect build system change to CMake (bitcoin#30454, bitcoin#30664) - add setting for bare Makefile still used for depends builds Can be tested e.g. by using the editorconfig-vim plugin (https://github.com/editorconfig/editorconfig-vim). The PR is made under the assumption that the file is still considered useful, especially for new contributors. If people feel like that's not the case anymore, the alternative is to delete it, obviously. Top commit has no ACKs. Tree-SHA512: 8406b1caf31e310f7e17c607d97beac583481e71b4425e0be2bbd8207096aa374a70151b58aae5fdb648ef5ff5c7e1d0a2949e6de3355bdd2009d8353ee24af0
04b647f doc: update `dependencies.md` (Kittywhiskers Van Gogh) 5046964 depends: add patches included with Qt 5.15.19 (Kittywhiskers Van Gogh) 0286d78 depends: Qt 5.15.18 (Kittywhiskers Van Gogh) Pull request description: ## Motivation Bitcoin switched to Qt 6.x in [bitcoin#30997](bitcoin#30997), upgrading from Qt 5.15.16 to Qt 6.7.3. This transition was enabled by, alongside a series of changes to GUI code, migrating the build system to CMake (see [bitcoin#30454](bitcoin#30454)). While efforts have been undertaken to bridge the gap between the pre-transition Autotools infrastructure and our infrastructure, migration is complicated by two factors: * The need for significant OOO backports to bridge the gap * Our divergence from upstream's GUI implementation that will require manual assessment to follow best Qt 6 practices This means that the timeline to migration is medium-term at best but in the meanwhile, the latest OSS version of Qt is 5.15.18 ([source](https://lists.qt-project.org/pipermail/announce/2025-October/000592.html)) and between 5.15.16 and 5.15.19 (the as-of-this-writing, still proprietary release of Qt, [source](https://www.qt.io/blog/commercial-lts-qt-5.15.19-released)), mitigations for vulnerabilities have been included ([source](https://wiki.qt.io/List_of_known_vulnerabilities_in_Qt_products)). While Qt 5.15.19 OSS is not available, critical patches shipped with them are. This pull request updates our Qt depends to the latest available OSS release (v5.15.18) and includes the patches included in the 5.15.19 release. ## Additional Information * Guidance on patch application has been taken from the `qt@5` Homebrew formula ([source](https://github.com/Homebrew/homebrew-core/blob/013dad6a9c390da76aacba8249afa47d4c807a85/Formula/q/[email protected])). * The patches themselves have been sourced from Qt ([source](https://download.qt.io/archive/qt/5.15/)) and modified to fit the build's directory structure, it should remain identical otherwise. *`clang_18_libpng.patch` has been dropped as it is already included in v5.15.18 ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 04b647f UdjinM6: light ACK 04b647f Tree-SHA512: 02e497e32561fcd91c4a08cf389a522cec14b8fc98cd3b5c4bc108d829dc32009c8ade52651e4f56d0d43b9cbad2d2cff9a6ec95e1bfc5a4b2167c0a8d47e791
includes: - 6ce50fd (only some changes to `depends/README.md`)
, bitcoin#30584, bitcoin#31982, bitcoin#32086, bitcoin#31998, bitcoin#32505, bitcoin#32568, bitcoin#32690, bitcoin#32731, bitcoin#32716, bitcoin#32837, bitcoin#32266, bitcoin#30095, bitcoin#30137, bitcoin#33580, partial bitcoin#30454 (build backports: part 4) 8e79c7a fix: allow `dbghelp.dll` for PE targets (Kittywhiskers Van Gogh) f97d3d6 fix: skip `EXPORTED_SYMBOLS` validation on ELF targets (Kittywhiskers Van Gogh) d5dffb6 merge bitcoin#33580: Use $(package)_file_name when downloading from the fallback (Kittywhiskers Van Gogh) b7febbe merge bitcoin#30137: Remove `--enable-threadlocal` (Kittywhiskers Van Gogh) dcf67c7 merge bitcoin#30095: avoid using thread_local variable that has a destructor (Kittywhiskers Van Gogh) 4e57d1a merge bitcoin#32266: Avoid `warning: "_FORTIFY_SOURCE"` redefined for `libevent` (Kittywhiskers Van Gogh) 6e66ef8 merge bitcoin#32837: fix libevent `_WIN32_WINNT` usage (Kittywhiskers Van Gogh) 61f2a23 merge bitcoin#32716: Override host compilers for FreeBSD and OpenBSD (Kittywhiskers Van Gogh) ca52975 merge bitcoin#32731: Build `qt` package for FreeBSD hosts (Kittywhiskers Van Gogh) ee9e934 build: check against `$host` instead of `TARGET_OS` in stacktrace search (Kittywhiskers Van Gogh) 44d32a3 merge bitcoin#32690: fix multiprocess build on OpenBSD (apply capnp patch, correct SHA256SUM command) (Kittywhiskers Van Gogh) 8d90c3c merge bitcoin#32568: use "mkdir -p" when installing xproto (Kittywhiskers Van Gogh) d4bc0aa merge bitcoin#32505: bump to latest config.guess and config.sub (Kittywhiskers Van Gogh) 6020cdc merge bitcoin#31998: patch around PlacementNew issue in capnp (Kittywhiskers Van Gogh) 00350a0 merge bitcoin#32086: Shuffle depends instructions and recommend modern make for macOS (Kittywhiskers Van Gogh) c8e27a2 merge bitcoin#31982: rename libmultiprocess repository (Kittywhiskers Van Gogh) 86d0a27 merge bitcoin#30584: Make default `host` and `build` comparable (Kittywhiskers Van Gogh) e6d6d17 merge bitcoin#31840: add missing Darwin objcopy (Kittywhiskers Van Gogh) 8d887c3 merge bitcoin#31626: Use base system's `sha256sum` utility on FreeBSD (Kittywhiskers Van Gogh) b443c14 partial bitcoin#30454: Introduce CMake-based build system (Kittywhiskers Van Gogh) 67aa238 merge bitcoin#31100: remove dependency install instructions from win docs (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6918 * Dependency for #6927 * [dash#6966](#6966) broke Guix build post-compilation binary verification for ELF and PE binaries as * For PE binaries, with improved `libbacktrace` detection, we are now building the capability in Windows binaries and that introduces a dependency on `dbghelp.dll` ([source](https://github.com/dashpay/dash/blob/edcb9f265b63693a8e684bd22fba5555434eff62/configure.ac)) that wasn't in the `PE_ALLOWED_LIBRARIES` allowlist, it has since been added. * For ELF binaries, now that we use `-export-dynamic` for symbol preservation, the `EXPORTED_SYMBOLS` check now fails. As the diagnostic value of retaining these symbols far exceeds the file size reduction, we have opted to comment out the test entirely. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: re-utACK 8e79c7a Tree-SHA512: 08f904dfcbf4ece848fe805395d78b3615ff40ed5d4df3afa73576de8960ccee5c70547495aefba54cd7b01dceefe0933831640484b0996468189da265dc711b
04b647f doc: update `dependencies.md` (Kittywhiskers Van Gogh) 5046964 depends: add patches included with Qt 5.15.19 (Kittywhiskers Van Gogh) 0286d78 depends: Qt 5.15.18 (Kittywhiskers Van Gogh) Pull request description: ## Motivation Bitcoin switched to Qt 6.x in [bitcoin#30997](bitcoin#30997), upgrading from Qt 5.15.16 to Qt 6.7.3. This transition was enabled by, alongside a series of changes to GUI code, migrating the build system to CMake (see [bitcoin#30454](bitcoin#30454)). While efforts have been undertaken to bridge the gap between the pre-transition Autotools infrastructure and our infrastructure, migration is complicated by two factors: * The need for significant OOO backports to bridge the gap * Our divergence from upstream's GUI implementation that will require manual assessment to follow best Qt 6 practices This means that the timeline to migration is medium-term at best but in the meanwhile, the latest OSS version of Qt is 5.15.18 ([source](https://lists.qt-project.org/pipermail/announce/2025-October/000592.html)) and between 5.15.16 and 5.15.19 (the as-of-this-writing, still proprietary release of Qt, [source](https://www.qt.io/blog/commercial-lts-qt-5.15.19-released)), mitigations for vulnerabilities have been included ([source](https://wiki.qt.io/List_of_known_vulnerabilities_in_Qt_products)). While Qt 5.15.19 OSS is not available, critical patches shipped with them are. This pull request updates our Qt depends to the latest available OSS release (v5.15.18) and includes the patches included in the 5.15.19 release. ## Additional Information * Guidance on patch application has been taken from the `qt@5` Homebrew formula ([source](https://github.com/Homebrew/homebrew-core/blob/013dad6a9c390da76aacba8249afa47d4c807a85/Formula/q/[email protected])). * The patches themselves have been sourced from Qt ([source](https://download.qt.io/archive/qt/5.15/)) and modified to fit the build's directory structure, it should remain identical otherwise. *`clang_18_libpng.patch` has been dropped as it is already included in v5.15.18 ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 04b647f UdjinM6: light ACK 04b647f Tree-SHA512: 02e497e32561fcd91c4a08cf389a522cec14b8fc98cd3b5c4bc108d829dc32009c8ade52651e4f56d0d43b9cbad2d2cff9a6ec95e1bfc5a4b2167c0a8d47e791
Bitcoin Core replaced the Autotools-based build system with a new CMake-based build system in bitcoin/bitcoin#30454. Without cmake and capnproto, running `cmake -B build` fails. I used commit e685172 from Bitcoin Core to test this.
Bitcoin Core replaced the Autotools-based build system with a new CMake-based build system in bitcoin/bitcoin#30454. Without cmake and capnproto, running `cmake -B build` fails. I used commit e685172 from Bitcoin Core to test this.
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:
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.