build: Add CMake-based build system (1 of N) #27060

pull hebasto wants to merge 2 commits into bitcoin:master from hebasto:230208-cmake-A changing 3 files +159 −0
  1. hebasto commented at 10:54 am on February 8, 2023: member

    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

    1. Integration with developer tools like clang-tidy, IWYU etc.

    2. CTest tool with a bunch of useful features, including coverage analysis, integration with sanitizers and memory check tools like valgrind.

    3. CPack, a powerful packaging tool.

    4. Integration with IDEs.

    5. The concept of a “target with assosiated properties” is better than Autotools’ global variables in many ways (robustness, readability, maintanability).

    6. [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.

    1. Native Windows support. No longer need to maintain build_msvc subdirectory.

    2. 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:

    1. CMake is a native build system of Qt 6.

    2. [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:

    • merge all CMake code by v25.0 and test it
    • switch to the CMake-based build system by v26.0

    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:

     0#  - 3.11: add_library() and add_executable() commands can now be called without any sources
     1#          and will not complain as long as sources are added later via the target_sources()
     2#          command.
     3#  - 3.11: The target_*() commands learned to set the INTERFACE properties on Imported Targets.
     4#  - 3.12: The add_compile_definitions() command was added to set preprocessor definitions
     5#          at directory level. This supersedes add_definitions().
     6#  - 3.12: If the CONFIGURE_DEPENDS flag is specified, CMake will add logic to the main
     7#          build system check target to rerun the flagged GLOB commands at build time.
     8#  - 3.12: The target_link_libraries() command now supports Object Libraries.
     9#  - 3.12: A new $<TARGET_EXISTS:...> generator expression has been added.
    10#  - 3.12: A new FindPython3 module has been added to provide a new way to locate python
    11#          environments.
    12#  - 3.13: The install(TARGETS) command learned to install targets created outside the current
    13#          directory.
    14#  - 3.13: The target_link_options() command was created to specify link options for targets
    15#          and their dependents.
    16#  - 3.13: The target_link_libraries() command may now be called to modify targets created
    17#          outside the current directory.
    

    Notes for reviewers

    To configure a build system, one can run in their local repo root:

    • on Linux / *BSD / macOS:
    0cmake -S . -B  build
    
    • on Windows:
    0cmake -G "Visual Studio 17 2022" -A x64 -S . -B build
    

    To re-configure, it is recommended to use the clean build tree, for example:

    0rm -rf build
    

    For now, the only artifact we explicitly create in the build tree is a bitcoin-config.h header:

    0cat build/src/config/bitcoin-config.h
    
  2. DrahtBot commented at 10:54 am on February 8, 2023: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept NACK fanquake, theuni, adam2k
    Stale ACK theStack, jarolrod, willcl-ark, TheCharlatan, vasild

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label Build system on Feb 8, 2023
  4. hebasto commented at 11:01 am on February 8, 2023: member
  5. hebasto renamed this:
    build: Add CMake-based build system
    build: Add CMake-based build system (1 of N)
    on Feb 8, 2023
  6. hebasto force-pushed on Feb 8, 2023
  7. in CMakeLists.txt:16 in 44810a4918 outdated
    11+  cmake_policy(SET CMP0092 NEW)
    12+endif()
    13+if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.20)
    14+  # MSVC RTTI flag /GR is not added to CMAKE_CXX_FLAGS by default.
    15+  cmake_policy(SET CMP0117 NEW)
    16+endif()
    


    vasild commented at 8:30 am on February 9, 2023:
    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?)

    hebasto commented at 8:48 am on February 9, 2023:

    I guess some laggish linux distro still ships with 3.13?

    Debian 10 Buster.


    hebasto commented at 9:05 am on February 9, 2023:

    nit: CMake 3.13 is from Nov 2018.

    The draft supports even CMake 3.10 which is shipped with Ubuntu Bionic :)


    hebasto commented at 8:22 pm on February 9, 2023:
    Related: today’s IRC meeting log.
  8. in CMakeLists.txt:28 in a520b40276 outdated
    20@@ -21,7 +21,11 @@ project("Bitcoin Core"
    21   LANGUAGES CXX C ASM
    22 )
    23 
    24-if(MSVC)
    25+# Configurable options.
    26+# When adding a new option, end the <help_text> with a full stop for consistency.
    27+option(CXX20 "Enable compilation in C++20 mode." OFF)
    28+
    29+if(CXX20 OR MSVC)
    


    vasild commented at 8:32 am on February 9, 2023:

    nit: this would enable C++20 unconditionally on MSVC. Maybe the option should not exist on MSVC? For Windows users, it would be strange to have an option which does nothing and when CXX20=OFF, then we still compile in C++20 mode.

    C++17 seems to be supported on MSCV in configure.ac.


    hebasto commented at 8:55 am on February 9, 2023:

    C++17 seems to be supported on MSCV in configure.ac.

    MSVC build configuration is defined in the build_msvc directory, not in configure.ac.

    And C++20 for MSVC was introduced in 88ec5d40dcf5d9f95217b123b48203b2f334c0a1 from #25472.


    hebasto commented at 9:02 am on February 9, 2023:

    Maybe the option should not exist on MSVC?

    Thanks! Going to fix this.


    hebasto commented at 11:15 am on February 9, 2023:

    Maybe the option should not exist on MSVC?

    Thanks! Going to fix this.

    Updated.

  9. in src/CMakeLists.txt:5 in 044ff46008 outdated
    0@@ -0,0 +1,7 @@
    1+# Copyright (c) 2023 The Bitcoin Core developers
    2+# Distributed under the MIT software license, see the accompanying
    3+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
    4+
    5+configure_file(${CMAKE_SOURCE_DIR}/cmake/bitcoin-config.h.in config/bitcoin-config.h @ONLY)
    


    vasild commented at 8:42 am on February 9, 2023:
    This would generate the config file in config/bitcoin-config.h whereas with autotools it would be in src/config/bitcoin-config.h. I guess this is ok (as long as the proper include flags are passed)?

    hebasto commented at 8:59 am on February 9, 2023:

    It generates ${CMAKE_CURRENT_BINARY_DIR}/config/bitcoin-config.h, which is equivalent to ${CMAKE_BINARY_DIR}/src/config/bitcoin-config.h.

    The related include_directories() goes two lines below :)

  10. vasild approved
  11. vasild commented at 8:44 am on February 9, 2023: contributor

    ACK 044ff46008645ee7b77575d7d68ac12dc6b99d14

    The first two commits may as well be squashed into one. Some non-blocker questions below.

  12. hebasto force-pushed on Feb 9, 2023
  13. hebasto force-pushed on Feb 9, 2023
  14. hebasto commented at 11:15 am on February 9, 2023: member

    Updated 044ff46008645ee7b77575d7d68ac12dc6b99d14 -> 1d4cec0f80d01a06418b59b1563da72996a7bd79 (pr27060.02 -> pr27060.03, diff):

    The first two commits may as well be squashed into one.

    Squashed.

  15. vasild approved
  16. vasild commented at 11:16 am on February 9, 2023: contributor
    ACK 1d4cec0f80d01a06418b59b1563da72996a7bd79
  17. TheCharlatan approved
  18. TheCharlatan commented at 5:51 pm on February 9, 2023: contributor

    ACK 1d4cec0f80d01a06418b59b1563da72996a7bd79

    And to be clear, also approach ACK for doing this piecemeal and the targeted timeline.

  19. theStack approved
  20. theStack commented at 1:50 am on February 10, 2023: contributor

    ACK 1d4cec0f80d01a06418b59b1563da72996a7bd79

    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.

  21. jarolrod approved
  22. jarolrod commented at 5:08 am on February 10, 2023: member

    ACK 1d4cec0f80d01a06418b59b1563da72996a7bd79

    tested on macOS 12.6 M1, cannot currently test for windows.

  23. in cmake/bitcoin-config.h.in:21 in 1d4cec0f80 outdated
    16+
    17+/* Minor version. */
    18+#define CLIENT_VERSION_MINOR @CLIENT_VERSION_MINOR@
    19+
    20+/* Version build. */
    21+#define CLIENT_VERSION_BUILD @CLIENT_VERSION_BUILD@
    


    maflcko commented at 1:35 pm on February 10, 2023:

    Any reason to change the order and at the same the time the docstrings? This makes it harder to review the diff:

     0diff --git a/tmp/a b/tmp/b
     1index 13d6b32ec..bd846a352 100644
     2--- a/tmp/a
     3+++ b/tmp/b
     4@@ -1,442 +1,44 @@
     5-/* src/config/bitcoin-config.h.  Generated from bitcoin-config.h.in by configure.  */
     6-/* src/config/bitcoin-config.h.in.  Generated from configure.ac by autoheader.  */
     7+// Copyright (c) 2023 The Bitcoin Core developers
     8+// Distributed under the MIT software license, see the accompanying
     9+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
    10 
    11 #ifndef BITCOIN_CONFIG_H
    12-
    13 #define BITCOIN_CONFIG_H
    14 
    15-/* Define if building universal (internal helper macro) */
    16-/* #undef AC_APPLE_UNIVERSAL_BUILD */
    17+/* Define to the full name of this package. */
    18+#define PACKAGE_NAME "Bitcoin Core"
    19 
    20-/* Define this symbol if type char equals int8_t */
    21-/* #undef CHAR_EQUALS_INT8 */
    22+/* Define to the version of this package. */
    23+#define PACKAGE_VERSION "24.99.0"
    24 
    25-/* Version Build */
    26-#define CLIENT_VERSION_BUILD 0
    27-
    28-/* Version is release */
    29-#define CLIENT_VERSION_IS_RELEASE false
    30-
    31-/* Major version */
    32+/* Major version. */
    33 #define CLIENT_VERSION_MAJOR 24
    34 
    35-/* Minor version */
    36+/* Minor version. */
    37 #define CLIENT_VERSION_MINOR 99
    38 
    39-/* Copyright holder(s) before %s replacement */
    40-#define COPYRIGHT_HOLDERS "The %s developers"
    41+/* Version build. */
    42+#define CLIENT_VERSION_BUILD 0
    43 
    44-/* Copyright holder(s) */
    45-#define COPYRIGHT_HOLDERS_FINAL "The Bitcoin Core developers"
    46-
    47-/* Replacement for %s in copyright holders string */
    48-#define COPYRIGHT_HOLDERS_SUBSTITUTION "Bitcoin Core"
    49-
    50-/* Copyright year */
    51-#define COPYRIGHT_YEAR 2023
    52-
    53-/* Define this symbol to build code that uses ARMv8 SHA-NI intrinsics */
    54-/* #undef ENABLE_ARM_SHANI */
    55-
    56...
    

    hebasto commented at 2:34 pm on February 10, 2023:
    Thanks! Updated.
  24. in CMakeLists.txt:42 in 1d4cec0f80 outdated
    38+# When adding a new option, end the <help_text> with a full stop for consistency.
    39+include(CMakeDependentOption)
    40+cmake_dependent_option(CXX20 "Enable compilation in C++20 mode." OFF "NOT MSVC" ON)
    41+
    42+if(CXX20)
    43+  set(CMAKE_CXX_STANDARD 20)
    


    maflcko commented at 1:42 pm on February 10, 2023:
    I changed this to 26 for testing, but didn’t run into an issue on cmake 3.13. Is this expected?

    hebasto commented at 1:46 pm on February 10, 2023:

    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?


    maflcko commented at 1:48 pm on February 10, 2023:
     0# grep CMAKE_CXX_STANDARD CMakeLists.txt && cmake --version && cmake   -S . -B ./build7  
     1  set(CMAKE_CXX_STANDARD 26)
     2  set(CMAKE_CXX_STANDARD 26)
     3set(CMAKE_CXX_STANDARD_REQUIRED ON)
     4cmake version 3.13.4
     5
     6CMake suite maintained and supported by Kitware (kitware.com/cmake).
     7-- The CXX compiler identification is GNU 8.3.0
     8-- The C compiler identification is GNU 8.3.0
     9-- The ASM compiler identification is GNU
    10-- Found assembler: /usr/bin/cc
    11-- Check for working CXX compiler: /usr/bin/c++
    12-- Check for working CXX compiler: /usr/bin/c++ -- works
    13-- Detecting CXX compiler ABI info
    14-- Detecting CXX compiler ABI info - done
    15-- Detecting CXX compile features
    16-- Detecting CXX compile features - done
    17-- Check for working C compiler: /usr/bin/cc
    18-- Check for working C compiler: /usr/bin/cc -- works
    19-- Detecting C compiler ABI info
    20-- Detecting C compiler ABI info - done
    21-- Detecting C compile features
    22-- Detecting C compile features - done
    23
    24
    25Configure summary
    26=================
    27Preprocessor defined macros ........... 
    28C compiler ............................ /usr/bin/cc
    29CFLAGS ................................ 
    30C++ compiler .......................... /usr/bin/c++
    31CXXFLAGS .............................. 
    32Common compile options ................ 
    33Common link options ................... 
    34Build type:
    35 - CMAKE_BUILD_TYPE ................... 
    36 - CFLAGS ............................. 
    37 - CXXFLAGS ........................... 
    38 - LDFLAGS for executables ............ 
    39 - LDFLAGS for shared libraries ....... 
    40
    41
    42-- Configuring done
    43-- Generating done
    44-- Build files have been written to: /bitcoin-core/build7
    

    hebasto commented at 2:15 pm on February 10, 2023:

    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:

    0--- a/CMakeLists.txt
    1+++ b/CMakeLists.txt
    2@@ -89,3 +89,5 @@ else()
    3   message(" - LDFLAGS for shared libraries ....... ${CMAKE_SHARED_LINKER_FLAGS_RELEASE}")
    4 endif()
    5 message("\n")
    6+
    7+add_library(test_lib OBJECT src/addrdb.cpp)
    
  25. hebasto force-pushed on Feb 10, 2023
  26. hebasto commented at 2:34 pm on February 10, 2023: member

    Updated 1d4cec0f80d01a06418b59b1563da72996a7bd79 -> f9e1e7264f925690db74abd3efe7f1a3e10f2542 (pr27060.03 -> pr27060.04, diff):

  27. willcl-ark commented at 2:42 pm on February 10, 2023: contributor

    I notice in the generated file build/CMakeCache.txt that I have the following:

    0//Choose the type of build, options are: None Debug Release RelWithDebInfo
    1// MinSizeRel ...
    2CMAKE_BUILD_TYPE:STRING=
    

    Should this be set to Release by default so that we pick up Release optimisations?

  28. hebasto commented at 2:45 pm on February 10, 2023: member

    @willcl-ark

    I notice in the generated file build/CMakeCache.txt that I have the following:

    0//Choose the type of build, options are: None Debug Release RelWithDebInfo
    1// MinSizeRel ...
    2CMAKE_BUILD_TYPE:STRING=
    

    Should this be set to Release by default so that we pick up Release optimisations?

    You are correct! Those commits are coming :) See 290f1e36ddf697c0825aa3340ff50ebc83a3b572 and 9d8bf0b1302651e825081ce8186822b510b3b83c from #25797.

  29. willcl-ark commented at 2:48 pm on February 10, 2023: contributor

    You are correct! Those commits are coming :) See 290f1e3 and 9d8bf0b from #25797.

    Ah I see. In that case LGTM!

  30. willcl-ark approved
  31. willcl-ark commented at 2:49 pm on February 10, 2023: contributor
    ACK f9e1e7264
  32. DrahtBot requested review from jarolrod on Feb 10, 2023
  33. DrahtBot requested review from TheCharlatan on Feb 10, 2023
  34. DrahtBot requested review from theStack on Feb 10, 2023
  35. DrahtBot requested review from vasild on Feb 10, 2023
  36. fanquake commented at 3:10 pm on February 10, 2023: member

    merge all CMake code by v25.0

    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.

    and test it

    Developers should at-least be testing, and becoming comfortable with CMake before this is merged, not after.

    switch to the CMake-based build system by v26.0

    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).

  37. hebasto commented at 3:23 pm on February 10, 2023: member

    I don’t understand why it’s being split up

    It is split up because #25797 diff seems unreasonable large for reviewing.

    How many steps in this going to be broken into?

    I’d say not “how many steps” but “how large diff for every step”. To be done, things must be doable.

    switch to the CMake-based build system by v26.0

    What does this mean?

    Use CMake-based build system for a release.

    When are we removing Autotools?

    Any time developers are comfortable with.

    We aren’t going to support two build systems in this repository at the same time.

    Good.

  38. theuni commented at 3:41 pm on February 10, 2023: member

    merge all CMake code by v25.0

    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.

    +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?

  39. vasild approved
  40. vasild commented at 4:35 pm on February 10, 2023: contributor

    ACK f9e1e7264f925690db74abd3efe7f1a3e10f2542

    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.

  41. hebasto commented at 6:17 pm on February 10, 2023: member

    @theuni

    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?

    but not under threat

    Sorry, but I did not mean any threats.

  42. TheCharlatan commented at 9:32 pm on February 10, 2023: contributor

    ACK f9e1e7264f925690db74abd3efe7f1a3e10f2542

    I was about to post what @vasild just suggested and second his approach.

  43. theuni commented at 4:25 pm on February 13, 2023: member

    @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.

  44. in CMakeLists.txt:39 in a46abb9f25 outdated
    22+)
    23+
    24+# Configurable options.
    25+# When adding a new option, end the <help_text> with a full stop for consistency.
    26+include(CMakeDependentOption)
    27+cmake_dependent_option(CXX20 "Enable compilation in C++20 mode." OFF "NOT MSVC" ON)
    


    theuni commented at 4:48 pm on February 13, 2023:
    Why is c++20 turned on automatically for MSVC? Afaics autotools doesn’t do that.

    hebasto commented at 5:58 pm on February 13, 2023:

    Why is c++20 turned on automatically for MSVC? Afaics autotools doesn’t do that.

    #27060 (review):

    MSVC build configuration is defined in the build_msvc directory, not in configure.ac.

    And C++20 for MSVC was introduced in 88ec5d40dcf5d9f95217b123b48203b2f334c0a1 from #25472.


    theuni commented at 6:58 pm on February 13, 2023:
    Thanks. I don’t understand that change, but I’ll take my complaints to that PR :)

    hebasto commented at 7:20 pm on February 13, 2023:
    It originates from #24531.
  45. in CMakeLists.txt:7 in a46abb9f25 outdated
    0@@ -0,0 +1,76 @@
    1+# Copyright (c) 2023 The Bitcoin Core developers
    2+# Distributed under the MIT software license, see the accompanying
    3+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
    4+
    5+cmake_minimum_required(VERSION 3.13)
    6+
    7+if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.15)
    


    theuni commented at 5:10 pm on February 13, 2023:

    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”.


    hebasto commented at 6:01 pm on February 13, 2023:

    https://cmake.org/cmake/help/latest/manual/cmake-policies.7.html:

    Policies in CMake are used to preserve backward compatible behavior across multiple releases.


    theuni commented at 7:10 pm on February 13, 2023:

    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:

    0if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.20)
    1  set(CMAKE_POSITION_INDEPENDENT_CODE ON)
    2endif()
    

    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?


    hebasto commented at 7:22 pm on February 13, 2023:

    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?


    hebasto commented at 2:44 pm on February 14, 2023:

    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.

  46. in CMakeLists.txt:27 in f9e1e7264f outdated
    16+endif()
    17+
    18+project("Bitcoin Core"
    19+  VERSION 24.99.0
    20+  DESCRIPTION "Bitcoin client software"
    21+  LANGUAGES CXX C ASM
    


    theuni commented at 5:37 pm on February 13, 2023:

    Note that this ends up setting:

    0C compiler ............................ /usr/bin/cc
    1...
    2C++ 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:

    0export PATH=/opt/clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04/bin:$PATH
    1cmake -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.


    hebasto commented at 6:09 pm on February 13, 2023:
    0export PATH=/opt/clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04/bin:$PATH
    1cmake -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.

    0cmake -S . -B ./build2 \
    1  -DCMAKE_C_COMPILER=/opt/clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang \
    2  -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.


    theuni commented at 7:03 pm on February 13, 2023:
    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.

    vasild commented at 1:48 pm on February 14, 2023:
    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.
  47. in CMakeLists.txt:59 in f9e1e7264f outdated
    45+  set(CMAKE_CXX_STANDARD 17)
    46+endif()
    47+set(CMAKE_CXX_STANDARD_REQUIRED ON)
    48+set(CMAKE_CXX_EXTENSIONS OFF)
    49+
    50+set(CMAKE_POSITION_INDEPENDENT_CODE ON)
    


    theuni commented at 5:39 pm on February 13, 2023:
    Does this need CMP0083 ?

    hebasto commented at 7:15 pm on February 13, 2023:
    Thanks! Fixed.

    hebasto commented at 2:43 pm on February 14, 2023:
    UPD: Now it is covered by the version range in the cmake_minimum_required() command.
  48. hebasto force-pushed on Feb 13, 2023
  49. hebasto commented at 7:14 pm on February 13, 2023: member

    Updated f9e1e7264f925690db74abd3efe7f1a3e10f2542 -> 75c4333e484ffea433f32b48ca7d4a1dc819bb36 (pr27060.04 -> pr27060.05, diff):

  50. in CMakeLists.txt:107 in 75c4333e48 outdated
    102+endif()
    103+message("\n")
    104+if(configure_warnings)
    105+    message("  ******\n")
    106+    foreach(warning IN LISTS configure_warnings)
    107+      message("  WARNING: ${warning}\n")
    


    vasild commented at 2:01 pm on February 14, 2023:

    nit:

    0      message(WARNING "${warning}")
    

    https://cmake.org/cmake/help/latest/command/message.html


    hebasto commented at 2:41 pm on February 14, 2023:
    Thanks! Updated.
  51. vasild approved
  52. vasild commented at 2:02 pm on February 14, 2023: contributor
    ACK 75c4333e484ffea433f32b48ca7d4a1dc819bb36
  53. DrahtBot requested review from willcl-ark on Feb 14, 2023
  54. cmake: Add root `CMakeLists.txt` file 0a0ab6c1ea
  55. cmake: Add `config/bitcoin-config.h` support 12758d3f69
  56. hebasto force-pushed on Feb 14, 2023
  57. hebasto commented at 2:35 pm on February 14, 2023: member

    Updated 75c4333e484ffea433f32b48ca7d4a1dc819bb36 -> 12758d3f69c4900f0d23df06e1ad746417428f3a (pr27060.05 -> pr27060.06):

    • rebased on the top of the recent changes in the build system (#27016)
    • cmake_policy() commands replaced with version range in the cmake_minimum_required() one
    • documented choice of versions
    • added a note suggested by @theuni
    • addressed @vasild’s comment
    • second commit diff minimized
  58. adam2k commented at 5:43 pm on February 14, 2023: none

    This is great! ACK: https://github.com/bitcoin/bitcoin/commit/12758d3f69c4900f0d23df06e1ad746417428f3a

    I see from the NACK comments that the reviewers seem to mostly be pushing back on this portion:

    Roadmap

    There are two goals:

    • merge all CMake code by v25.0 and test it
    • switch to the CMake-based build system by v26.0

    In the spirit of having a complete git working history in the master branch, I understand the viewpoint.

    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 ACK reviewers this PR does not add anything about cmake within the docs.

  59. fanquake commented at 9:08 pm on February 14, 2023: member

    seem to mostly be pushing back on this portion:

    There are multiple reasons to push back on this. For example:

    1. The parent PR (#25797) suffers from a lack of high-level review. While there are lots of “CMake good, Autotools bad” type comments, along with some developers checking out the branch and seeing if it works on their machine, there is very little review in regards to the actual implementation; which I think still has room for improvement, for example:

      • Re-implementing a large amount of the legacy Autotools infra we are trying to move away from, inside CMake itself. Is this a good idea, or even necessary? Wouldn’t now be the exact right time to drop some of the cruft, such as, checking for whether stdlib.h exists at configure time? Do we need to port all of this over, if yes, are we keeping it forever?

      • Re-implementing the current Autotools option handling/awkwardness, inside CMake. Changing build systems, when you’re going to break everything anyway, might be the perfect time for us to introduce some more sane defaults, and possibly cull some legacy options, rather than carrying all the cruft (including auto-enabling things based on installed libs, rather than more explicit, opt-in options/defaults) over into the new build system (where we may just have to deprecate/remove it later anyway). See the recently removed UPNP/NATPMP default setting options as an example of something that I don’t think would have made sense to port over.

    2. There is no urgency to do this. The benefits for developers/users in the short term, are little (less so when nothing is going to work until some point when more PRs are merged). I don’t understand why there is some sense of “we need to start merging this NOW” being created, or why it makes so much of a difference if we instead wait the 2 months, until the 25.x branch-off, to do a clean integration, with CMake in, Autotools out, for 26.x. Trying to rush this in now, in chunks, will almost undoubtedly lead to a partially working CMake build system being in 25.x, which seems pointless, and confusing. I think lack of CMake being mentioned in any docs does not matter; if there is a CMakeLists.txt present in the root of the repo, developers will assume it works (otherwise why would it be there).

    3. Comments like:

      When are we removing Autotools?

      Any time developers are comfortable with.

    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 26.x, then there should be mention of when we are removing Autotools (it should be prior to that?), otherwise what is the alternative to having 2 build systems at that point. As I’ve mentioned multiple times already, given the existence of release branches, that are maintained over multiple year periods, as a project, we are already going to be maintaining both systems for some period (which, for example, will result in more complicated backporting etc), and there is no reason to drag that out any longer than it need be, by having two available for use over some period (and removing any impetus for anyone to actually switch).

  60. hebasto commented at 3:08 pm on February 15, 2023: member

    @fanquake

    3. Comments like: > When are we removing Autotools?

        > > Any time developers are comfortable with.
    

    are pretty hand wavy, and do not establish any sort of plan.

    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 :smile:). 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”.


    1. The parent PR (#25797) suffers from a lack of high-level review.

    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 (review). I mean, when system introspection code will be added, there must be no diff between an Autotools-generated bitcoin-config.h and CMake-generated one.


    2. There is no urgency to do this.

    True. No code must be merged before it has been reviewed and tested properly.


    ~Will we be OK with this PR as a “reviewing stage”? I mean, adding more commits after reviewing and ACKing of previous ones, with optional squashing of already reviewed commits, and rebasing on top of the recent changes in the current build system in the master branch.~

    ~Doing in such a way we can get a reviewed CMake-based build system implementation ready to be merged at some moment in the future.~

    UPDATE: Reviewing is welcome on that staging branch.

    This PR is moved to https://github.com/hebasto/bitcoin/pull/5.

    Also see a couple of comment below.

  61. theuni commented at 6:10 pm on February 15, 2023: member

    Will we be OK with this PR as a “reviewing stage”? I mean, adding more commits after reviewing and ACKing of previous ones, with optional squashing of already reviewed commits, and rebasing on top of the recent changes in the current build system in the master branch.

    Doing in such a way we can get a reviewed CMake-based build system implementation ready to be merged at some moment in the future.

    My comment above:

    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.

    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.

  62. hebasto commented at 6:31 pm on February 15, 2023: member

    @theuni

    Thank you!

    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.

    Like that?

  63. theuni commented at 6:38 pm on February 15, 2023: member

    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.

  64. hebasto commented at 6:54 pm on February 15, 2023: member

    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

    Just in case, I’ve made an announcement in #bitcoin-core-builds IRC channel.

  65. hebasto referenced this in commit 3bc91d27ad on Feb 16, 2023
  66. fanquake commented at 10:39 am on February 17, 2023: member
    I think we can close this now that review is happening elsewhere, and we’ve still got the parent PR.
  67. hebasto closed this on Feb 17, 2023

  68. hebasto deleted the branch on Feb 27, 2023
  69. bitcoin locked this on Feb 27, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-16 19:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me