build: simplify by flattening the dependency graph #30911

pull theuni wants to merge 2 commits into bitcoin:master from theuni:cmake-flatten-dependencies changing 4 files +21 −0
  1. theuni commented at 6:44 pm on September 16, 2024: member

    These changes speed up my build (default config/options/targets) by roughly 10%. I suspect the difference may be more significant in other build configs.

    Before:

    $ time cmake –build build -j24 real 3m26.932s

    After:

    $ time cmake –build build -j24 real 3m7.556s

    Generally they allow for jobservers (either make -jX or ninja) to be better utilized. This can be verified using top while building and looking at the number of compiles running at any given time before/after these changes. Before, it’s easy to observe periods of stalling when only one or two compiles are happening. After these changes, the compiler process count should mostly match the number of jobs given (-jX) until it falls off at the end.


    The first commit sets DEPENDS_EXPLICIT_ONLY for commands which generate our test header files. Without this option, test_bitcoin’s generated headers won’t be built until all of its other dependencies have been built. This introduces a significant stall in the build, though currently only Ninja benefits from this being set, and only CMake >= 3.27 understands it.

    Example from a generated build.ninja:

    Before:

    # Custom command for src/test/data/base58_encode_decode.json.h

    build src/test/data/base58_encode_decode.json.h | ${cmake_ninja_workdir}src/test/data/base58_encode_decode.json.h: CUSTOM_COMMAND /home/cory/dev/bitcoin/src/test/data/base58_encode_decode.json /home/cory/dev/bitcoin/cmake/script/GenerateHeaderFromJson.cmake || libcrc32c.a libcrc32c_sse42.a libleveldb.a libminisketch.a minisketch_clmul src/bitcoin_clientversion src/crypto/libbitcoin_crypto.a src/crypto/libbitcoin_crypto_avx2.a src/crypto/libbitcoin_crypto_sse41.a src/crypto/libbitcoin_crypto_x86_shani.a src/generate_build_info src/libbitcoin_cli.a src/libbitcoin_common.a src/libbitcoin_consensus.a src/libbitcoin_node.a src/secp256k1/src/libsecp256k1.a src/secp256k1/src/secp256k1_precomputed src/test/util/libtest_util.a src/univalue/libunivalue.a src/util/libbitcoin_util.a src/wallet/libbitcoin_wallet.a src/zmq/libbitcoin_zmq.a

    After:

    # Custom command for src/test/data/base58_encode_decode.json.h

    build src/test/data/base58_encode_decode.json.h | ${cmake_ninja_workdir}src/test/data/base58_encode_decode.json.h: CUSTOM_COMMAND /home/cory/dev/bitcoin/src/test/data/base58_encode_decode.json /home/cory/dev/bitcoin/cmake/script/GenerateHeaderFromJson.cmake


    The second commit is more significant. It sets CMAKE_OPTIMIZE_DEPENDENCIES globally, which allows the objects of static libs to be built in parallel when one lib depends on the other. This can be set as a per-lib property, but I don’t see any need for that as we don’t currently have any edge-cases where this wouldn’t be ok. If those should arise, we could always disable on a per-lib basis.

    Edit: turns out this triggers an upstream bug, which I guess can be considered an edge-case until fixed in CMake. I’ve added 2 per-lib opt-outs as a result.

    Example:

    Before:

    # Link the static library src/libbitcoin_cli.a

    build src/libbitcoin_cli.a: CXX_STATIC_LIBRARY_LINKER__bitcoin_cli_RelWithDebInfo src/CMakeFiles/bitcoin_cli.dir/compat/stdin.cpp.o src/CMakeFiles/bitcoin_cli.dir/rpc/client.cpp.o || src/univalue/libunivalue.a

    After:

    # Link the static library src/libbitcoin_cli.a

    build src/libbitcoin_cli.a: CXX_STATIC_LIBRARY_LINKER__bitcoin_cli_RelWithDebInfo src/CMakeFiles/bitcoin_cli.dir/compat/stdin.cpp.o src/CMakeFiles/bitcoin_cli.dir/rpc/client.cpp.o

  2. DrahtBot commented at 6:44 pm on September 16, 2024: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30911.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK l0rinc, hebasto
    Stale ACK willcl-ark

    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:

    • #30901 (cmake: Revamp handling of data files for {test,bench}_bitcoin targets by hebasto)
    • #28792 (Embed default ASMap as binary dump header file by fjahr)

    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.

  3. DrahtBot added the label Build system on Sep 16, 2024
  4. theuni requested review from hebasto on Sep 16, 2024
  5. DrahtBot added the label CI failed on Sep 16, 2024
  6. theuni commented at 7:46 pm on September 16, 2024: member

    Looks like this is hitting a CMake bug: https://gitlab.kitware.com/cmake/cmake/-/issues/24058

    Converting to draft while I investigate.

  7. theuni marked this as a draft on Sep 16, 2024
  8. theuni force-pushed on Sep 16, 2024
  9. willcl-ark commented at 9:22 pm on September 16, 2024: member

    Concept ACK.

    I noticed a bit of a “gap” in our compilation on master the other day."

    screenshot_20240916-221513

    This PR appears to fix it quite nicely:

    image

    I didn’t see quite the same speedup as you though, but will investigate and do some more thorough testing/review tomorrow.

    These tests were also using a default build, run using ninja with -j16.

  10. theuni force-pushed on Sep 16, 2024
  11. theuni force-pushed on Sep 16, 2024
  12. DrahtBot removed the label CI failed on Sep 16, 2024
  13. theuni marked this as ready for review on Sep 17, 2024
  14. theuni force-pushed on Sep 17, 2024
  15. maflcko added the label DrahtBot Guix build requested on Sep 17, 2024
  16. hebasto commented at 8:45 am on September 17, 2024: member

    Concept ACK.

    a39a915c226775396f6505efe77a4aba4d0ae3ad Did you consider setting CMAKE_ADD_CUSTOM_COMMAND_DEPENDS_EXPLICIT_ONLY globally?

    aaf30ef89465c18228b4e46fd1378b9087274cbe

    turns out this triggers an upstream bug, which I guess can be considered an edge-case until fixed in CMake. I’ve added 2 per-lib opt-outs as a result.

    This bug may also affect the crc32c library after switching to the upstream build system.

  17. maflcko commented at 8:49 am on September 17, 2024: member

    … though currently only Ninja benefits from this being set, and only CMake >= 3.27 understands it.

    I wonder if one CI task should be using Ninja (and cmake >= 3.27), if it isn’t too hard to implement. Otherwise this config will remain untested and errors may sneak in to the master branch, only being detected after merge.

  18. in cmake/module/GenerateHeaders.cmake:10 in aaf30ef894 outdated
     1@@ -2,12 +2,19 @@
     2 # Distributed under the MIT software license, see the accompanying
     3 # file COPYING or https://opensource.org/license/mit/.
     4 
     5+if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.27)
     6+  set(DEPENDS_EXPLICIT_OPT DEPENDS_EXPLICIT_ONLY)
     7+else()
     8+  set(DEPENDS_EXPLICIT_OPT)
     9+endif()
    10+
    


    hebasto commented at 8:53 am on September 17, 2024:
    A note for reviewers. The Guix builds are using CMake 3.24.2 (the cmake-minimal package).
  19. hebasto commented at 12:14 pm on September 17, 2024: member

    … though currently only Ninja benefits from this being set, and only CMake >= 3.27 understands it.

    I wonder if one CI task should be using Ninja (and cmake >= 3.27), if it isn’t too hard to implement. Otherwise this config will remain untested and errors may sneak in to the master branch, only being detected after merge.

    Please consider #30915.

  20. theuni commented at 1:51 pm on September 17, 2024: member

    Concept ACK.

    a39a915 Did you consider setting CMAKE_ADD_CUSTOM_COMMAND_DEPENDS_EXPLICIT_ONLY globally?

    Yes, but I didn’t feel as comfortable setting this one globally due to our (for ex) phony deploy targets. I figured we may make some changes in the future that could introduce hard-to-track-down bugs. It’s clear to see that the header generation is self-contained, and afaik that’s the only place that would matter for build performance.

  21. DrahtBot commented at 2:00 pm on September 17, 2024: contributor

    Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

    File commit 9f1aa88d4d9596aeb5220fa30ee3972294b62793(master) commit b492ec55637aacc0ab5c7905ef6a07d67ce19b34(master and this pull)
    SHA256SUMS.part 58b44f5adc71c2a4... 92c2d99d5c2c299b...
    *-aarch64-linux-gnu-debug.tar.gz 55850b93e85c3347... e02fea8f8e1ffe54...
    *-aarch64-linux-gnu.tar.gz fae846aaf9fea0b7... 79fac3429fc6e816...
    *-arm-linux-gnueabihf-debug.tar.gz 0426295872ec43a2... 47a0e42463683e95...
    *-arm-linux-gnueabihf.tar.gz 27a12872b8ada11a... e76b2e19a2001e4b...
    *-arm64-apple-darwin-unsigned.tar.gz 54300b0a29a7998b... 6c00a2baf4bc9c5e...
    *-arm64-apple-darwin-unsigned.zip 562e78cd7591cde8... ca69bb4613fae917...
    *-arm64-apple-darwin.tar.gz 6aa58a8f90301fa9... 7f8160bbfcef1738...
    *-powerpc64-linux-gnu-debug.tar.gz 6c157065ef9eda2d... 6a66652e72b552c1...
    *-powerpc64-linux-gnu.tar.gz 6cb89079528938d2... 354701aec723e97c...
    *-riscv64-linux-gnu-debug.tar.gz d8d21e764b475786... 3b45e46456511ddb...
    *-riscv64-linux-gnu.tar.gz 3d3b46cf6da3a0e1... 4c2f5127865699ae...
    *-x86_64-apple-darwin-unsigned.tar.gz fb603e19fe15e837... 75d9554b1c590680...
    *-x86_64-apple-darwin-unsigned.zip e7320795bfdf4091... 35b98fe93214dccb...
    *-x86_64-apple-darwin.tar.gz 35320ad7bed83b2d... dd77ff7b04a227ac...
    *-x86_64-linux-gnu-debug.tar.gz 1917ce819eb44d06... e5e7b908a9b9e832...
    *-x86_64-linux-gnu.tar.gz f643a9ee6827456b... a7d6fc4322776f68...
    *.tar.gz aca1d4d37426ef68... 11f85372ff9c79ed...
    guix_build.log 15b4512c52f2afb5... 838afc53bda90ced...
    guix_build.log.diff 3a2249ad9e4edb69...
  22. DrahtBot removed the label DrahtBot Guix build requested on Sep 17, 2024
  23. maflcko commented at 3:14 pm on September 17, 2024: member

    It’s clear to see that the header generation is self-contained, and afaik that’s the only place that would matter for build performance.

    Could rebase on current master, now that the header generation is 30x faster, after 2a0949f0977db2dbb7ac452e8d58d413b9526756 ? This should also change the benchmark in the pull description.

  24. theuni force-pushed on Sep 17, 2024
  25. fanquake referenced this in commit 6b97882ab5 on Sep 18, 2024
  26. l0rinc commented at 2:50 pm on September 19, 2024: contributor

    Now that #30888 is merged, the difference is not that dramatic: I’m measuring a 0.5% change:

    latest master 2x

    % ccache --clear && git clean -fxd && cmake -B build && time cmake --build build -j10

    cmake –build build -j10 994.42s user 91.78s system 832% cpu 2:10.47 total cmake –build build -j10 993.43s user 91.40s system 832% cpu 2:10.30 total

    rebased branch 2x

    % ccache --clear && git clean -fxd && cmake -B build && time cmake --build build -j10

    cmake –build build -j10 997.78s user 90.28s system 867% cpu 2:05.49 total cmake –build build -j10 998.51s user 90.64s system 867% cpu 2:05.54 total

    % cmake --version

    cmake version 3.30.2


    But conceptually it seems to be better than before so: ACK 42edb2f4a5900e0b2451a7446d58962c0bf0095d

  27. DrahtBot requested review from willcl-ark on Sep 19, 2024
  28. DrahtBot requested review from hebasto on Sep 19, 2024
  29. willcl-ark commented at 3:14 pm on September 23, 2024: member

    ACK 42edb2f4a5900e0b2451a7446d58962c0bf0095d

    I agree with @l0rinc that this seems conceptually nicer, and also see a much smaller time diff since 30888:

     0# master @ 33adc7521cc8bb24b941d959022b084002ba7c60
     1________________________________________________________
     2Executed in  181.29 secs    fish           external
     3   usr time   38.26 mins  214.00 micros   38.26 mins
     4   sys time    1.49 mins   55.00 micros    1.49 mins
     5
     6
     7# this PR rebased on master @ 33adc7521cc8bb24b941d959022b084002ba7c60
     8________________________________________________________
     9Executed in  179.09 secs    fish           external
    10   usr time   39.35 mins  202.00 micros   39.35 mins
    11   sys time    1.47 mins   56.00 micros    1.47 mins
    

    I also still see a “nicer” build process structure after this PR (using ninja tracing):

    image

  30. DrahtBot added the label CI failed on Oct 17, 2024
  31. DrahtBot removed the label CI failed on Oct 18, 2024
  32. in CMakeLists.txt:46 in 42edb2f4a5 outdated
    39@@ -40,6 +40,11 @@ set(CMAKE_TRY_COMPILE_PLATFORM_VARIABLES
    40   CMAKE_CXX_LINK_EXECUTABLE
    41 )
    42 
    43+# Flatten static lib dependencies.
    44+# Without this, if libfoo.a depends on libbar.a, libfoo's objects can't begin
    45+# to be compiled until libbar.a has been created.
    46+set(CMAKE_OPTIMIZE_DEPENDENCIES TRUE)
    


    hebasto commented at 12:52 pm on October 22, 2024:
    CMake has some variables/properties that must be set before the project() command. And the CMAKE_OPTIMIZE_DEPENDENCIES is not one of them. For the sake of clarity, I suggest to keep only necessary stuff before the project() command.

    theuni commented at 3:10 pm on January 31, 2025:
    Good point. Move it down below language selection, and also now it’s only set if not specified by the user.
  33. hebasto commented at 12:53 pm on October 22, 2024: member

    Tested and benchmarked on Ubuntu 23.10 with CMake 3.27.4 using the following command:

    0$ rm -rf build && cmake -B build -G Ninja --preset dev-mode -DWITH_MULTIPROCESS=OFF -DWITH_CCACHE=OFF && time cmake --build build -j 16
    
    • The master branch @ 6fc4692797121b54de0c54e5b09ee47f322c038a:
    0real	6m44.237s
    
    • 330d16e1aa8fe8d7a5bb755189d0d2991fef8a43:
    0real	6m45.264s
    
    • 42edb2f4a5900e0b2451a7446d58962c0bf0095d
    0real	6m45.859s
    

    I’m OK with the flattened dependency graph, but the expected build time improvement is questionable. At least, the “speed up” phrase should be removed from the PR description and commit messages.

  34. DrahtBot requested review from hebasto on Oct 22, 2024
  35. edilmedeiros commented at 7:54 pm on October 23, 2024: contributor

    I noticed a bit of a “gap” in our compilation on master the other day."

    I have been noticing this “gap” all the time with cmake. I’ll check with this PR.

  36. fanquake added this to the milestone 29.0 on Jan 31, 2025
  37. build: avoid unnecessary dependencies on generated headers
    This prevents the generation of these headers from also depending on the
    dependencies of the libs/binaries which consume them.
    
    Specifically, this prevents generated test headers (such as
    test/data/base58_encode_decode.json.h) from depending on the
    dependencies of test_bitcoin (libcrc32c.a libcrc32c_sse42.a libleveldb.a)
    
    Note that this is currently only relevant for Ninja.
    
    For more detail, see:
    https://cmake.org/cmake/help/latest/command/add_custom_command.html
    c4e498300c
  38. build: simplify dependency graph
    Allow the objects of static libs to be built in parallel rather than serially
    based on their dependency ordering.
    
    For more detail, see:
    https://cmake.org/cmake/help/latest/prop_tgt/OPTIMIZE_DEPENDENCIES.html
    12fa9511b5
  39. theuni force-pushed on Jan 31, 2025
  40. theuni renamed this:
    build: speed up by flattening the dependency graph
    build: simplify by flattening the dependency graph
    on Jan 31, 2025
  41. theuni commented at 3:12 pm on January 31, 2025: member

    Updated to address hebasto’s comment, and removed comments about it being a speedup. Should be good to go now.

    This speeds things up significantly for me in some cases where I’m only making specific targets. I think it should be a general speedup in other cases too, but sure, it’s more about the dependency reduction.

  42. in CMakeLists.txt:78 in 12fa9511b5
    70@@ -71,6 +71,13 @@ set(CMAKE_CXX_EXTENSIONS OFF)
    71 
    72 list(APPEND CMAKE_MODULE_PATH ${PROJECT_SOURCE_DIR}/cmake/module)
    73 
    74+# Flatten static lib dependencies.
    75+# Without this, if libfoo.a depends on libbar.a, libfoo's objects can't begin
    76+# to be compiled until libbar.a has been created.
    77+if (NOT DEFINED CMAKE_OPTIMIZE_DEPENDENCIES)
    78+  set(CMAKE_OPTIMIZE_DEPENDENCIES TRUE)
    


    l0rinc commented at 2:21 pm on February 1, 2025:
    I’m not sure I understand when we’re using ON/OFF and when TRUE/FALSE, but as far as I know both are accepted here, so it’s just a nit

    purpleKarrot commented at 10:02 pm on February 3, 2025:
    1, ON, YES, TRUE, Y, or any non-zero number (including floating point numbers) are treated as true. See: basic-expressions. Sticking to ON/OFF consistently avoids this confusion. But yeah, it is a nit.
  43. l0rinc approved
  44. l0rinc commented at 2:25 pm on February 1, 2025: contributor

    utACK 12fa9511b5fba18e83f88b7b831906595bcf2116

    Compared to previous version, CMAKE_OPTIMIZE_DEPENDENCIES was moved after project and only assigned if user didn’t provide any value.

     0git diff HEAD a6efc4ca368661fb82136ea4d68f3043d058dc48 
     1diff --git a/CMakeLists.txt b/CMakeLists.txt
     2index edfb63926a..86d5ed6448 100644
     3--- a/CMakeLists.txt
     4+++ b/CMakeLists.txt
     5@@ -40,6 +40,11 @@ set(CMAKE_TRY_COMPILE_PLATFORM_VARIABLES
     6   CMAKE_CXX_LINK_EXECUTABLE
     7 )
     8 
     9+# Flatten static lib dependencies.
    10+# Without this, if libfoo.a depends on libbar.a, libfoo's objects can't begin
    11+# to be compiled until libbar.a has been created.
    12+set(CMAKE_OPTIMIZE_DEPENDENCIES TRUE)
    13+
    14 project(BitcoinCore
    15   VERSION ${CLIENT_VERSION_MAJOR}.${CLIENT_VERSION_MINOR}.${CLIENT_VERSION_BUILD}
    16   DESCRIPTION "Bitcoin client software"
    17@@ -71,13 +76,6 @@ set(CMAKE_CXX_EXTENSIONS OFF)
    18 
    19 list(APPEND CMAKE_MODULE_PATH ${PROJECT_SOURCE_DIR}/cmake/module)
    20 
    21-# Flatten static lib dependencies.
    22-# Without this, if libfoo.a depends on libbar.a, libfoo's objects can't begin
    23-# to be compiled until libbar.a has been created.
    24-if (NOT DEFINED CMAKE_OPTIMIZE_DEPENDENCIES)
    25-  set(CMAKE_OPTIMIZE_DEPENDENCIES TRUE)
    26-endif()
    27-
    28 #=============================
    29 # Configurable options
    30 #=============================
    
  45. hebasto commented at 12:44 pm on February 3, 2025: member

    It seems I lack a deep understanding of CMake’s underlying behaviour.

    The comment in c4e498300c7e6b23dc464eca75a2bc9f86270dab states:

    Specifically, this prevents generated test headers (such as test/data/base58_encode_decode.json.h) from depending on the dependencies of test_bitcoin (libcrc32c.a libcrc32c_sse42.a libleveldb.a)

    Note that this is currently only relevant for Ninja.

    And this aligns perfectly with the CMake docs:

    Indicates that the command’s DEPENDS argument represents all files required by the command and implicit dependencies are not required.

    Without this option, if any target uses the output of the custom command, CMake will consider that target’s dependencies as implicit dependencies for the custom command in case this custom command requires files implicitly created by those targets.

    However, I fail to trigger header regeneration in the following scenario on the master branch:

    0$ cmake --version | head -1
    1cmake version 3.30.3
    2$ cmake -G Ninja -B build
    3$ cmake --build build -t test_bitcoin
    4<apply a patch to modify some of test_bitcoin's dependencies>
    5$ cmake --build build -t test_bitcoin  # headers are expected to be regenerated, but they are not
    

    What am I doing wrong?

  46. theuni commented at 7:28 pm on February 3, 2025: member
    0$ cmake -G Ninja -B build
    1$ cmake --build build -t test_bitcoin
    2$ touch src/test/data/bip341_wallet_vectors.json
    3$ cmake --build build -t test_bitcoin
    

    [2/5] cd /home/cory/dev/bitcoin/build/src/test && /usr/bin/cmake -DJSON_SOURCE_PATH=/home/cory/dev/bitcoin/src/test/data/bip341_wallet_vectors.json -DHEADER_PATH=/home/cory/dev/bitcoin/build/src/test/data/bip341_wallet_vectors.json.h -P /home/cory/dev/bitcoin/cmake/script/GenerateHeaderFromJson.cmake

    Why would the headers need to be regenerated unless the .json/.raw files themselves were touched? Afaik they’re self-contained and only depend on their input files.

  47. hebasto commented at 7:34 pm on February 3, 2025: member

    Why would the headers need to be regenerated unless the .json/.raw files themselves were touched?

    The .json/.raw files are explicit dependencies specified via the DEPENDS option. The DEPENDS_EXPLICIT_ONLY option changes the treatment of implicit dependencies.

    UPD.

    Afaik they’re self-contained and only depend on their input files.

    Then what is the following commit message excerpt about:

    This prevents the generation of these headers from also depending on the dependencies of the libs/binaries which consume them.

    ?

  48. theuni commented at 7:48 pm on February 3, 2025: member

    Without that commit, the headers must be generated late in the build, after a bunch of other libs have been built:

    base58_encode_decode.json /home/cory/dev/bitcoin/cmake/script/GenerateHeaderFromJson.cmake || libcrc32c.a libcrc32c_sse42.a libleveldb.a libminisketch.a minisketch_clmul src/bitcoin_clientversion src/crypto/libbitcoin_crypto.a src/crypto/libbitcoin_crypto_avx2.a src/crypto/libbitcoin_crypto_sse41.a src/crypto/libbitcoin_crypto_x86_shani.a src/generate_build_info src/libbitcoin_cli.a src/libbitcoin_common.a src/libbitcoin_consensus.a src/libbitcoin_node.a src/secp256k1/src/libsecp256k1.a src/secp256k1/src/secp256k1_precomputed src/test/util/libtest_util.a src/univalue/libunivalue.a src/util/libbitcoin_util.a src/wallet/libbitcoin_wallet.a src/zmq/libbitcoin_zmq.a

    See? The header now depends on all of those libs. Meaning that test_bitcoin can’t start until (for ex) zmq has been built. That introduces a stall in the build eventually. Imagine if you’re doing a -j50 build and all that remains to be built is test_bitcoin and src/zmq/libbitcoin_zmq.a. Without this change, they can’t be built in parallel, so the tests wait on everything else.

  49. theuni commented at 9:09 pm on February 3, 2025: member

    Actually, @hebasto , what guarantees that the headers are built before the test_bitcoin objects? It seems to work ok but I’m not sure why it doesn’t race?

    Seems the headers should be an interface library that test_bitcoin depends on?

    Edit: nm, from the add_dependencies doc:

    Changed in version 3.9: The Ninja Generators use weaker ordering than other generators in order to improve available concurrency. They only guarantee that the dependencies’ custom commands are finished before sources in start compiling; this ensures generated sources are available.

  50. purpleKarrot commented at 10:11 pm on February 3, 2025: contributor

    I have to dig deeper what “header generation” actually is in this context here. But generally, add_custom_target and add_dependencies are not useful for code generation. You just need a add_custom_command that generates the files and you use those files in an add_library/add_executable/target_sources. As long as the custom command and the library/executable are defined in the same directory, dependencies will be resolved.

    Custom targets are only useful for things that are built explicitly, like make documentation.

  51. theuni commented at 10:17 pm on February 3, 2025: member
    Thanks, yeah, ignore that question. You got it exactly right. We’re covered by the generated headers being an output of an add_custom_command and being added to the target.
  52. hebasto commented at 8:40 am on February 4, 2025: member

    Without that commit, the headers must be generated late in the build, after a bunch of other libs have been built:

    base58_encode_decode.json /home/cory/dev/bitcoin/cmake/script/GenerateHeaderFromJson.cmake || libcrc32c.a libcrc32c_sse42.a libleveldb.a libminisketch.a minisketch_clmul src/bitcoin_clientversion src/crypto/libbitcoin_crypto.a src/crypto/libbitcoin_crypto_avx2.a src/crypto/libbitcoin_crypto_sse41.a src/crypto/libbitcoin_crypto_x86_shani.a src/generate_build_info src/libbitcoin_cli.a src/libbitcoin_common.a src/libbitcoin_consensus.a src/libbitcoin_node.a src/secp256k1/src/libsecp256k1.a src/secp256k1/src/secp256k1_precomputed src/test/util/libtest_util.a src/univalue/libunivalue.a src/util/libbitcoin_util.a src/wallet/libbitcoin_wallet.a src/zmq/libbitcoin_zmq.a

    See? The header now depends on all of those libs. Meaning that test_bitcoin can’t start until (for ex) zmq has been built. That introduces a stall in the build eventually. Imagine if you’re doing a -j50 build and all that remains to be built is test_bitcoin and src/zmq/libbitcoin_zmq.a. Without this change, they can’t be built in parallel, so the tests wait on everything else.

    Thanks! I get it know. I’ve compared the resulting ninja.build files, and it’s clear that the first commit effectively removes order-only dependencies in header build edges.

    UPD. Actually, it was in the PR description :man_facepalming:

  53. hebasto approved
  54. hebasto commented at 8:55 am on February 4, 2025: member
    ACK 12fa9511b5fba18e83f88b7b831906595bcf2116.
  55. in cmake/minisketch.cmake:77 in 12fa9511b5
    73@@ -74,6 +74,9 @@ add_library(minisketch STATIC EXCLUDE_FROM_ALL
    74   ${PROJECT_SOURCE_DIR}/src/minisketch/src/fields/generic_8bytes.cpp
    75 )
    76 
    77+# Workaround for https://gitlab.kitware.com/cmake/cmake/-/issues/24058
    


    fanquake commented at 2:27 pm on February 12, 2025:
    No new movement here, but someone has offered another work around: https://gitlab.kitware.com/cmake/cmake/-/issues/24058#note_1624877

    theuni commented at 5:11 pm on February 12, 2025:
    @hebasto Looking at this again, is there any reason this needs to be an object library? It triggers the bug that necessitates the workaround here. I’m wondering if rather than using our hack or the (better) workaround suggested upstream, if we should just make it an archive library and avoid the issue altogether?

    hebasto commented at 4:29 pm on February 14, 2025:

    @hebasto Looking at this again, is there any reason this needs to be an object library?

    I don’t thin so.

    It triggers the bug that necessitates the workaround here. I’m wondering if rather than using our hack or the (better) workaround suggested upstream, if we should just make it an archive library and avoid the issue altogether?

    From Professional CMake: A Practical Guide 20th Edition:

    Some developers may find object libraries more natural if coming from a background where non-CMake projects defined their targets based on sources or object files rather than a related set of static libraries. In general, however, where there is a choice, static libraries will typically be the more convenient choice in CMake projects.


    hebasto commented at 3:00 pm on February 18, 2025:

    @theuni

    Looking at this again, is there any reason this needs to be an object library? It triggers the bug that necessitates the workaround here. I’m wondering if rather than using our hack or the (better) workaround suggested upstream, if we should just make it an archive library and avoid the issue altogether?

    Please see #31880.

  56. fanquake merged this on Feb 12, 2025
  57. fanquake closed this on Feb 12, 2025


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: 2025-02-22 15:12 UTC

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