cmake: add a component for each binary #31844

pull theuni wants to merge 4 commits into bitcoin:master from theuni:cmake-install-components changing 10 files +48 −46
  1. theuni commented at 8:39 pm on February 11, 2025: member

    This makes it possible to build/install only the desired binaries regardless of the configuration. For consistency, the component names match the binary names. Kernel and GUI have been renamed.

    Additionally it fixes #31762 by installing only the manpages for the configured targets (and includes them in the component installs for each).

    Also fixes #31745.

    Alternative to #31765 which is (imo) more correct/thorough.

    Can be tested using (for ex):

    0$ cmake -B build
1$ cmake --build build -t bitcoind -t bitcoin-cli
2$ cmake --install build --component bitcoind
3$ cmake --install build --component bitcoin-cli
  2. DrahtBot commented at 8:39 pm on February 11, 2025: 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/31844.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hebasto, TheCharlatan, stickies-v
    Stale ACK s373nZ

    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:

    • #31765 (cmake: Install man pages for configured targets only by hebasto)
    • #30595 (kernel: Introduce initial C header API by TheCharlatan)

    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 Feb 11, 2025
  4. DrahtBot added the label CI failed on Feb 11, 2025
  5. DrahtBot commented at 10:03 pm on February 11, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/37053681658

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  6. ci: don't try to install for a fuzz build 
    Currently the manpages are installed, but that is a bug. An upcoming commit
will avoid installing manpages for targets that aren't configured, which
removes the "install" target for fuzz builds.
    fb0546b1c5
  7. theuni force-pushed on Feb 11, 2025
  8. cmake: use per-target components for bitcoin-qt and bitcoin-gui 
    This makes the usage consistent with the next commit, which will add a
per-target component for each binary.
    0264c5d86c
  9. theuni force-pushed on Feb 11, 2025
  10. DrahtBot removed the label CI failed on Feb 11, 2025
  11. fanquake added this to the milestone 29.0 on Feb 12, 2025
  12. s373nZ commented at 3:26 pm on February 12, 2025: none

    ACK fb0546b1c5ebb858605bef4c9fa001782e0ab213

    Tested lightly by building and installing for targeted components bitcoind and bitcoin-cli as described in the PR description.

    Should be mentioned this also closes #31745.

  13. theuni commented at 7:20 pm on February 12, 2025: member
    @maflcko Mind taking a look at the first commit here to see if you’d prefer a different approach?
  14. TheCharlatan approved
  15. TheCharlatan commented at 7:43 am on February 13, 2025: contributor
    ACK 686ebcfe93efa9ecca9b94e78da23a36c3b01f90
  16. maflcko commented at 8:02 am on February 13, 2025: member

    @maflcko Mind taking a look at the first commit here to see if you’d prefer a different approach?

    The first commit lgtm. Anything should be fine in the fuzz ci task as long as it compiles and runs the fuzz executable.

  17. s373nZ commented at 10:23 am on February 13, 2025: none
    ACK 686ebcfe93efa9ecca9b94e78da23a36c3b01f90
  18. in src/kernel/CMakeLists.txt:135 in 686ebcfe93 outdated 
    132 
133 configure_file(${PROJECT_SOURCE_DIR}/libbitcoinkernel.pc.in ${PROJECT_BINARY_DIR}/libbitcoinkernel.pc @ONLY)
134-install(FILES ${PROJECT_BINARY_DIR}/libbitcoinkernel.pc DESTINATION "${CMAKE_INSTALL_LIBDIR}/pkgconfig" COMPONENT Kernel)
135+install(FILES ${PROJECT_BINARY_DIR}/libbitcoinkernel.pc DESTINATION "${CMAKE_INSTALL_LIBDIR}/pkgconfig" COMPONENT libbitcoinkernel)
136 
137 include(GNUInstallDirs)
    stickies-v commented at 3:33 pm on February 13, 2025:
    nit: I think this is now duplicated and can be removed?
    theuni commented at 5:41 pm on February 13, 2025:

    Hmm, IIUC it’s always been superfluous as it should’ve been needed for the .pc install above, so it must’ve always coming in from the top level anyway.

    Will remove.

    theuni commented at 6:10 pm on February 13, 2025:

    Heh, testing shows GNUInstallDirs is actually coming in from secp and we don’t need it at all in our project. That’s pretty gross of CMake…

    But still, I’ll remove this one and leave the other one there for self-documentation.

  19. hebasto commented at 3:54 pm on February 13, 2025: member

    Concept ACK.

    In addition to installation, components are also intended for use in packaging via cpack. However, Bitcoin Core does not currently utilise it.

  20. stickies-v approved
  21. stickies-v commented at 5:13 pm on February 13, 2025: contributor

    tACK 686ebcfe93efa9ecca9b94e78da23a36c3b01f90

    Being able to specify install components for all targets is helpful, and this implementation seems very clean. Part of me annoyed is that the consistency isn’t there for kernel (bitcoinkernel target and libbitcoinkernel component), but it seems like that’s conventional for libraries so I’ll suck it up.

  22. DrahtBot requested review from hebasto on Feb 13, 2025
  23. theuni commented at 5:33 pm on February 13, 2025: member

    @stickies-v Thanks for pointing that out, that was accidental! I actually didn’t notice the difference. I think we should fix it so that they’re aligned instead of having a single outlier.

    Though.. would it make sense to rename the target to libbitcoinkernel instead of changing the component to bitcoinkernel? I think the former would be more obvious?

    Edit: nm, can’t do that as CMake will rename the resulting file to liblibbitcoinkernel.so.

  24. cmake: add and use install_binary_component 
    Add a separate component for each binary for fine-grained installation options.

Also install the man pages for only for the targets enabled.
    2e0c92558e
  25. cmake: rename Kernel component to bitcoinkernel for consistency 9b033bebb1
  26. theuni force-pushed on Feb 13, 2025
  27. theuni commented at 6:28 pm on February 13, 2025: member

    Addressed @stickies-v’s review.

    Only change of interest is changing the name of the Kernel component to bitcoinkernel rather than libbitcoinkernel as it was before.

  28. hebasto commented at 9:36 pm on February 13, 2025: member

    @stickies-v Thanks for pointing that out, that was accidental! I actually didn’t notice the difference. I think we should fix it so that they’re aligned instead of having a single outlier.

    Though.. would it make sense to rename the target to libbitcoinkernel instead of changing the component to bitcoinkernel? I think the former would be more obvious?

    Edit: nm, can’t do that as CMake will rename the resulting file to liblibbitcoinkernel.so.

    But we can:

    0add_custom_target(libbitcoinkernel)
1add_dependencies(libbitcoinkernel bitcoinkernel)
  29. hebasto approved
  30. hebasto commented at 10:22 pm on February 13, 2025: member

    ACK 9b033bebb18dfd609c02736292f37cc6589fcc8d.

    I have a slight preference for the approach mentioned above.

  31. DrahtBot requested review from stickies-v on Feb 13, 2025
  32. DrahtBot requested review from TheCharlatan on Feb 13, 2025
  33. TheCharlatan approved
  34. TheCharlatan commented at 8:49 am on February 14, 2025: contributor
    Re-ACK 9b033bebb18dfd609c02736292f37cc6589fcc8d
  35. Sjors commented at 10:34 am on February 14, 2025: member
    Tested 9b033bebb18dfd609c02736292f37cc6589fcc8d a bit on arm macOS 15.3.
  36. stickies-v approved
  37. stickies-v commented at 1:07 pm on February 14, 2025: contributor
    re-ACK 9b033bebb18dfd609c02736292f37cc6589fcc8d
  38. fanquake merged this on Feb 14, 2025
  39. fanquake closed this on Feb 14, 2025
  40. s373nZ commented at 1:22 pm on February 14, 2025: none

    re-ACK https://github.com/bitcoin/bitcoin/commit/9b033bebb18dfd609c02736292f37cc6589fcc8d

    Consider the prefix name feedback a nit.

  41. theuni commented at 4:50 pm on February 14, 2025: member

    @stickies-v Thanks for pointing that out, that was accidental! I actually didn’t notice the difference. I think we should fix it so that they’re aligned instead of having a single outlier. Though.. would it make sense to rename the target to libbitcoinkernel instead of changing the component to bitcoinkernel? I think the former would be more obvious? Edit: nm, can’t do that as CMake will rename the resulting file to liblibbitcoinkernel.so.

    But we can:

    0add_custom_target(libbitcoinkernel)
1add_dependencies(libbitcoinkernel bitcoinkernel)

    Clever :)

    Want to PR that along with a comment along the lines of “Add a convenience libbitcoinkernel target as a synonym for bitcoinkernel” and I’ll give it a quick ACK?

  42. hebasto commented at 5:10 pm on February 14, 2025: member

    Want to PR that along with a comment along the lines of “Add a convenience libbitcoinkernel target as a synonym for bitcoinkernel” and I’ll give it a quick ACK?

    #31869.

  43. hebasto commented at 5:21 pm on February 14, 2025: member

    ACK 9b033be.

    My apologies not very solid testing, but this branch seems not always working:

     0$ cmake -B build -G Ninja -DBUILD_UTIL_CHAINSTATE=ON -DBUILD_SHARED_LIBS=OFF
 1$ cmake --build build --target bitcoinkernel
 2$ cmake --install build --component bitcoinkernel --prefix /home/hebasto/INSTALL
 3-- Install configuration: "RelWithDebInfo"
 4CMake Error at build/src/kernel/cmake_install.cmake:46 (file):
 5  file INSTALL cannot find
 6  "/home/hebasto/dev/bitcoin/build/src/crypto/libbitcoin_crypto.a": No such
 7  file or directory.
 8Call Stack (most recent call first):
 9  build/src/cmake_install.cmake:172 (include)
10  build/cmake_install.cmake:57 (include)

    UPD. This behaviour is not introduced in this PR and is observed @ 7bbd761e816b713ac88c87f8b0045ea2f958ecbf.


theuni DrahtBot s373nZ TheCharlatan maflcko stickies-v hebasto Sjors

Labels
Build system

Milestone
29.0

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 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me