build: Add CMake-based build system (1 of N) #27060
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. ConflictsNo conflicts as of last run. |
|
Friendly ping people who commented on #25797: @achow101 |
hebasto
changed the title
hebasto
force-pushed
the
230208-cmake-A
branch
from
7580ed8
to
044ff46
Compare
CMakeLists.txt
Outdated
| cmake_minimum_required(VERSION 3.13) | ||
|
|
||
| if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.15) | ||
| # MSVC runtime library flags are selected by the CMAKE_MSVC_RUNTIME_LIBRARY abstraction. | ||
| cmake_policy(SET CMP0091 NEW) | ||
| # MSVC warning flags are not in CMAKE_<LANG>_FLAGS by default. | ||
| cmake_policy(SET CMP0092 NEW) | ||
| endif() | ||
| if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.20) | ||
| # MSVC RTTI flag /GR is not added to CMAKE_CXX_FLAGS by default. | ||
| cmake_policy(SET CMP0117 NEW) | ||
| endif() |
There was a problem hiding this comment.
nit: CMake 3.13 is from Nov 2018. Is there any reason to support that? Why not just cmake_minimum_required(VERSION 3.20) and remove those ifs? (I guess some laggish linux distro still ships with 3.13?)
There was a problem hiding this comment.
I guess some laggish linux distro still ships with 3.13?
Debian 10 Buster.
There was a problem hiding this comment.
nit: CMake 3.13 is from Nov 2018.
The draft supports even CMake 3.10 which is shipped with Ubuntu Bionic :)
There was a problem hiding this comment.
Related: today's IRC meeting log.
hebasto
force-pushed
the
230208-cmake-A
branch
2 times, most recently
from
12620c9
to
1d4cec0
Compare
|
Updated 044ff46 -> 1d4cec0 (pr27060.02 -> pr27060.03, diff):
Squashed. |
There was a problem hiding this comment.
ACK 1d4cec0
And to be clear, also approach ACK for doing this piecemeal and the targeted timeline.
There was a problem hiding this comment.
ACK 1d4cec0
Tested with OpenBSD 7.2 and cmake 3.24.2, verified that the generated src/config/bitcoin-config.h file in the build directory is a subset of the one generate by autotools. Can't relate to the MSVC-related changes, since I don't have a Windows build environment available.
| cmake_dependent_option(CXX20 "Enable compilation in C++20 mode." OFF "NOT MSVC" ON) | ||
|
|
||
| if(CXX20) | ||
| set(CMAKE_CXX_STANDARD 20) |
There was a problem hiding this comment.
I changed this to 26 for testing, but didn't run into an issue on cmake 3.13. Is this expected?
There was a problem hiding this comment.
I changed this to 26 for testing, but didn't run into an issue on cmake 3.13. Is this expected?
Mind providing the exact command?
There was a problem hiding this comment.
# grep CMAKE_CXX_STANDARD CMakeLists.txt && cmake --version && cmake -S . -B ./build7
set(CMAKE_CXX_STANDARD 26)
set(CMAKE_CXX_STANDARD 26)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
cmake version 3.13.4
CMake suite maintained and supported by Kitware (kitware.com/cmake).
-- The CXX compiler identification is GNU 8.3.0
-- The C compiler identification is GNU 8.3.0
-- The ASM compiler identification is GNU
-- Found assembler: /usr/bin/cc
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
Configure summary
=================
Preprocessor defined macros ...........
C compiler ............................ /usr/bin/cc
CFLAGS ................................
C++ compiler .......................... /usr/bin/c++
CXXFLAGS ..............................
Common compile options ................
Common link options ...................
Build type:
- CMAKE_BUILD_TYPE ...................
- CFLAGS .............................
- CXXFLAGS ...........................
- LDFLAGS for executables ............
- LDFLAGS for shared libraries .......
-- Configuring done
-- Generating done
-- Build files have been written to: /bitcoin-core/build7
There was a problem hiding this comment.
The CMAKE_CXX_STANDARD variable just holds a default value for CXX_STANDARD target property, which CMake will actually check.
To see such behavior, please consider applying a diff as follows:
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -89,3 +89,5 @@ else()
message(" - LDFLAGS for shared libraries ....... ${CMAKE_SHARED_LINKER_FLAGS_RELEASE}")
endif()
message("\n")
+
+add_library(test_lib OBJECT src/addrdb.cpp)
hebasto
force-pushed
the
230208-cmake-A
branch
from
1d4cec0
to
f9e1e72
Compare
|
Updated 1d4cec0 -> f9e1e72 (pr27060.03 -> pr27060.04, diff):
|
|
I notice in the generated file build/CMakeCache.txt that I have the following: Should this be set to |
You are correct! Those commits are coming :) |
NACK merging this prior to the 25.x branch off. I don't understand why it's being split up, and we're now trying to merge changes that aren't even usable to compile Core? (also resulting in the confusion above). How many steps in this going to be broken into? What is the reason to get this merged now.
Developers should at-least be testing, and becoming comfortable with CMake before this is merged, not after.
What does this mean? When are we removing Autotools? We aren't going to support two build systems in this repository at the same time. That is just going to draw out and overcomplicate what is already going to be a long and drawn-out (and breaking) process of supporting both (via release branches). |
It is split up because #25797 diff seems unreasonable large for reviewing.
I'd say not "how many steps" but "how large diff for every step". To be done, things must be doable.
Use CMake-based build system for a release.
Any time developers are comfortable with.
Good. |
+1 this take. NACK for incomplete/parallel buildsystems in the v25 branch. I'd really like to review this PR (and the others) thoroughly and thoughtfully now that you consider them merge-ready, but not under threat of "whatever parts of this that are ready are being merged during this release cycle, better fix up the chunks as best we can first". Without everyone dogfooding at once, I'm afraid we might not catch high-level problems before moving on to the next chunk in the series to review. Merging for v25 would mean maintaining parallel changes for that cycle. I think that's just a recipe for disaster. IMO the most straightforward way forward would be to merge at the beginning of a release cycle (v26 if it's ready by then) and deprecate autotools at the same time, potentially even removing it before tag. That way everyone switches at once and new features don't have to be shoved into the old buildsystem, it would simply be frozen in time. So... what's the rush? I realize that this is a monster to maintain, but it's just the nature of such a big change. Speaking more helpfully and practically, this change may be big enough to warrant an out-of-the-ordinary workflow: maybe either working on a CMake branch in the Core repo, or hooking CI up to an additional repo. Since (I believe) the desire to merge for v25 comes from the desire for more eyes and testing, maybe providing a more "official" way of trying out CMake pre-merge would be enough? |
There was a problem hiding this comment.
ACK f9e1e72
The parent PR is too huge to review. If that is to be merged at once, then this is literally asking to merge it without proper review. Thus, it makes sense to me to split it. Yes, this means in the intermediate steps it is not going to be possible to build using CMake. I think that is ok.
If that is going to help, maybe put everything cmake related in a subdirectory like e.g. ./cmake_incomplete/CMakeLists.txt and/or requiring that it is run like cmake -D I_UNDERSTAND_THIS_IS_INCOMPLETE_UNMAINTAINED_AND_NOT_OFFICIALLY_SUPPORTED_CMAKE_BUILD_FOR_DEVS_ONLY=1 and aborting with an appropriate message otherwise?
We aren't going to support two build systems in this repository at the same time.
I agree that better be avoided. Merging this and subsequent PRs does not mean that a CMake build system is going to be supported - doc/build* still mentions autotools solely and nothing about CMake.
|
Thank you for your helpful comment. If I understand you correctly, you are suggesting to follow the same path as #2943 did, right? If so, should that huge unstructured patch touch (a) Guix-related stuff, (b) CI-related stuff? Or Guix and CI changes should go separately?
Sorry, but I did not mean any threats. |
|
@hebasto Yes. I'm going to review/test the CMake work (starting with this PR) in earnest this week. Until then, I'll hold off on further commenting/judgement. I do stand by my comment on timing though. Getting this merged "as-is" for v25 is a bad idea imo. I suggest that we create a staging branch/repo for reviewing and merging chunks at a time, with the goal of merging from staging to master after v26 branch. Similar to the workflow (though not necessarily the timing) @sipa used for segwit. |
| # Configurable options. | ||
| # When adding a new option, end the <help_text> with a full stop for consistency. | ||
| include(CMakeDependentOption) | ||
| cmake_dependent_option(CXX20 "Enable compilation in C++20 mode." OFF "NOT MSVC" ON) |
There was a problem hiding this comment.
Why is c++20 turned on automatically for MSVC? Afaics autotools doesn't do that.
There was a problem hiding this comment.
Why is c++20 turned on automatically for MSVC? Afaics autotools doesn't do that.
MSVC build configuration is defined in the build_msvc directory, not in configure.ac.
There was a problem hiding this comment.
Thanks. I don't understand that change, but I'll take my complaints to that PR :)
There was a problem hiding this comment.
It originates from #24531.
CMakeLists.txt
Outdated
|
|
||
| cmake_minimum_required(VERSION 3.13) | ||
|
|
||
| if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.15) |
There was a problem hiding this comment.
This introduces the possibility of a departure from Autotools behavior, and is THE reason I was so opposed to switching to CMake for so long. After the "Autotools Hell" years, it mostly froze in time and (i'm oversimplifying) modules/features became locked-in. That means, in practice, you don't have to worry about whether autoconf's m4's are new enough for feature X.
CMake, however, takes us back in time in this regard. We now have to worry about its version and the feature-set that it provides.
To state it plainly: Different build output based on the installed cmake version is possible and would be a significant and problematic departure for us.
From what I can tell, the changes here are fine because they're surface-level. But IMO we need a note to the effect of: "Changes which affect binary results may not be quietly gated by CMake version".
There was a problem hiding this comment.
https://cmake.org/cmake/help/latest/manual/cmake-policies.7.html:
Policies in CMake are used to preserve backward compatible behavior across multiple releases.
There was a problem hiding this comment.
Understood for policies. But that's not to mean that it's not possible to misuse or misunderstand, which is why I'm suggesting the note. For example, one could imagine something naive/broken like:
if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.20)
set(CMAKE_POSITION_INDEPENDENT_CODE ON)
endif()which would provide better binaries for builders with newer CMake.
Maybe the note could say (if it's true?) that only cmake_policy should be set as a result of CMAKE_VERSION testing?
There was a problem hiding this comment.
But IMO we need a note to the effect of: "Changes which affect binary results may not be quietly gated by CMake version".
Ah, you mean a comment in CMake's code, right?
There was a problem hiding this comment.
But IMO we need a note to the effect of: "Changes which affect binary results may not be quietly gated by CMake version".
Ah, you mean a comment in CMake's code, right?
Thanks! Updated.
| project("Bitcoin Core" | ||
| VERSION 24.99.0 | ||
| DESCRIPTION "Bitcoin client software" | ||
| LANGUAGES CXX C ASM |
There was a problem hiding this comment.
Note that this ends up setting:
C compiler ............................ /usr/bin/cc
...
C++ compiler .......................... /usr/bin/c++
Which is different from what autotools would find. cc/c++ are usually symlinks to a valid compiler, but they don't always exist.
For ex, this would actually break my current workflow because my current llvm+clang toolchain did not ship with these links. So I'd see surprising results with a build like:
export PATH=/opt/clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04/bin:$PATH
cmake -S . -B ./build2
Rather than picking up /opt/clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang, it finds /usr/bin/cc.
I'll have to think about that a bit. On one hand it makes sense to use the default machinery provided by CMake as it's what CMake users will expect, however it also means a behavioral change that may go unnoticed by seasoned Core builders.
There was a problem hiding this comment.
export PATH=/opt/clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04/bin:$PATH cmake -S . -B ./build2Rather than picking up
/opt/clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang, it finds/usr/bin/cc.
cmake -S . -B ./build2 \
-DCMAKE_C_COMPILER=/opt/clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang \
-DCMAKE_CXX_COMPILER=/opt/clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang++
Or use CC and CXX environment variables. The latter works both for Autotools and CMake.
There was a problem hiding this comment.
Of course. I was simply pointing out that in this case, ./configure && make wouldn't map to cmake -S . -B ./build && make -C build as one might expect. So it's a question of expectations of current build parity vs CMake-ness.
There was a problem hiding this comment.
Do autotools really pick /foo/bin/clang (assuming /foo/bin is in $PATH)? I tried that but for me it picked /usr/bin/c++ nevertheless, even though I had a clang executable in a directory earlier than /usr/bin in $PATH.
| set(CMAKE_CXX_STANDARD_REQUIRED ON) | ||
| set(CMAKE_CXX_EXTENSIONS OFF) | ||
|
|
||
| set(CMAKE_POSITION_INDEPENDENT_CODE ON) |
There was a problem hiding this comment.
UPD: Now it is covered by the version range in the cmake_minimum_required() command.
hebasto
force-pushed
the
230208-cmake-A
branch
from
f9e1e72
to
75c4333
Compare
|
Updated f9e1e72 -> 75c4333 (pr27060.04 -> pr27060.05, diff): |
hebasto
force-pushed
the
230208-cmake-A
branch
from
75c4333
to
12758d3
Compare
|
Updated 75c4333 -> 12758d3 (pr27060.05 -> pr27060.06):
|
|
This is great! ACK: 12758d3 I see from the
In the spirit of having a complete git working history in the Are there any other pieces (without overly complicating things) that can also be pulled in from into this PR to make this more "complete"? If it's not possible then I agree it's not ideal, but it will make future review easier and to the point of other |
There are multiple reasons to push back on this. For example:
are pretty hand wavy, and do not establish any sort of plan. They sound suspiciously like we'll end up with 2 build systems, in the tree, at the same time, which is the one thing we want to avoid. If the plan is to start using CMake in |
My comment is "pretty hand wavy" for the only reason -- I have no experience of introducing a new build system for a project like Bitcoin Core (#2943 happened before I ran my first Bitcoin Core node 😄). That's why I'm asking you to share your vision of the optimal way to do it, which will avoid all drawbacks you've mentioned multiple times already. Anyway, a discussion "how to introduce a new build system into the project" can be separated from another one "how a new build system is implemented".
Well, the "100% feature parity" requirement was actively discussed among developers (@dongcarl, @theuni, @ryanofsky) there:
And, I think (probably mistakenly), it is implicitly expected in @MarcoFalke's #27060 (comment). I mean, when system introspection code will be added, there must be no diff between an Autotools-generated
True. No code must be merged before it has been reviewed and tested properly.
UPDATE: Reviewing is welcome on that staging branch. This PR is moved to hebasto#5. Also see a couple of comment below. |
My comment above:
Again, I suggest a staging repo for review. It could just be your personal repo where you PR to a branch. Review happens in chunks (as you're doing now) but to that branch, that way we don't have the temporary clutter in the master repo. Then, once all the chunks have been individually merged and reviewed, it is submitted as one giant pr to master where it can be tested in full before final merge. This is essentially the Dictator and Lieutenants Workflow This would allow us to collaborate on PR chunks and keep track of issues, all without disrupting the main repo, or attracting cheerleading. |
|
Thank you!
Like that? |
Exactly, thank you! So, right, I'd like to request that everyone who has been involved in reviewing (technically) the other CMake PRs to come along with me to reviewing over at @hebasto's staging repo. Since CMake is apparently so urgent, I'm sure we'll have no trouble recruiting review over there :p Maybe it won't end up working, but I think it's worth a shot. It's at least preferable to trying to do everything 1 PR here. |
Just in case, I've made an announcement in #bitcoin-core-builds IRC channel. |
e7fb823 cmake: Add `config/bitcoin-config.h` support (Hennadii Stepanov) bc25dda cmake: Add root `CMakeLists.txt` file (Hennadii Stepanov) Pull request description: This is a first PR into a staging branch as suggested in bitcoin#27060 (comment). This PR: 1. Establishes the minimum required CMake [version](https://gitlab.kitware.com/cmake/community/-/wikis/CMake-Versions-on-Linux-Distros). As it looks non-productive to support Ubuntu Bionic as a build platform, version [3.13](https://cmake.org/cmake/help/latest/release/3.13.html) has been chosen. 2. Creates a `bitcoin-config.h` header in the build tree. ## Notes for reviewers To configure a build system, one can run in their local repo root: - on Linux / *BSD / macOS: ```sh cmake -S . -B build ``` - on Windows: ```cmd cmake -G "Visual Studio 17 2022" -A x64 -S . -B build ``` To re-configure, it is recommended to use the clean build tree, for example: ```sh rm -rf build ``` For now, the only artifact we explicitly create in the build tree is a `bitcoin-config.h` header: ```sh cat build/src/config/bitcoin-config.h ``` ACKs for top commit: vasild: ACK e7fb823 TheCharlatan: ACK [e7fb823](e7fb823) theuni: So, sure, ACK e7fb823 to keep things moving. But I'm hoping 2/N is less boilerplate and more skeletal :) Tree-SHA512: cfbc775832b4758dc42a55c7f04ac50bd7136136007d68e8969d16f78bd557ad1eaeac6bed8622e4aa94d9249afbdd5be3d690ef01e7b97cca05f04b6fa115de
|
I think we can close this now that review is happening elsewhere, and we've still got the parent PR. |
This is split from #25797 which is a full implementation with an Autotools feature parity and has more than 80 commits.
A bit rehashed list of benefits of a CMake-based build system over an Autotools-based one:
General benefits
Integration with developer tools like clang-tidy, IWYU etc.
CTest tool with a bunch of useful features, including coverage analysis, integration with sanitizers and memory check tools like valgrind.
CPack, a powerful packaging tool.
Integration with IDEs.
The concept of a "target with assosiated properties" is better than Autotools' global variables in many ways (robustness, readability, maintanability).
[social] A great community. CMake is actively developed.
The Bitcoin Core specific benefits
A preliminary note. Two arguments (a) "nobody cares about X platform" and (b) "nobody cares about Y binary" are out of this PR scope, as long as the X platform is promised to be supported by the Bitcoin Core project, and the Y binary is a part of the Bitcoin Core project.
Native Windows support. No longer need to maintain
build_msvcsubdirectory.Correctness with Windows DLLs out-of-the-box. In comparison, the current build system has 3 (three!) hacks of libtool. And it is still broken:
libbitcoinkernel: Buildingmingw-w64dll's broken #25008CMake is a native build system of Qt 6.
[social] It is expected that more current and new developers are/will be able/willing to review/contribute/maintain CMake code rather Autotools stuff.
Roadmap
There are two goals:
Implementation details
In particular, this PR establishes the minimum required CMake version. As it looks non-productive to support Ubuntu Bionic as a build platform, version 3.13 has been chosen. Benefits of v3.13 over v3.10 which affect our CMake code directly are as follow:
Notes for reviewers
To configure a build system, one can run in their local repo root:
cmake -S . -B buildcmake -G "Visual Studio 17 2022" -A x64 -S . -B buildTo re-configure, it is recommended to use the clean build tree, for example:
For now, the only artifact we explicitly create in the build tree is a
bitcoin-config.hheader: