cmake: Move internal binaries from bin/ to libexec/ #31679

pull ryanofsky wants to merge 3 commits into bitcoin:master from ryanofsky:pr/libexec changing 9 files +58 −10
  1. ryanofsky commented at 5:01 pm on January 17, 2025: contributor

    Currently when “make install” or “cmake –install” are run, various internal binaries that are confusing and not typically useful are installed to ${CMAKE_INSTALL_PREFIX}/bin and can wind up on the system PATH. This PR moves internal binaries out of bin/ into libexec/ where they will still be accessible but will not be automatically placed on the PATH or be confused with more useful binaries. The PR also adds an install rule for the bitcoin-chainstate binary.

    After this PR binaries installed to bin/ are:

    • bitcoin-cli
    • bitcoind
    • bitcoin-qt
    • bitcoin-tx
    • bitcoin-util
    • bitcoin-wallet

    And binaries installed to libexec/ are:

    • bench_bitcoin
    • bitcoin-chainstate
    • bitcoin-gui
    • bitcoin-node
    • test_bitcoin
    • test_bitcoin-qt

    In the future if #31375 gets merged, there will be a new bitcoin wrapper executable in bin/ that can be used to call other binaries, and with that present, we could consider moving other binaries from bin/ to libexec/ and recommending that most users should use the wrapper instead of calling the different utilities directly. But this PR should make still make sense without #31375 to organize the binaries better.

    This PR is part of the process separation project.

  2. DrahtBot commented at 5:01 pm on January 17, 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/31679.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK TheCharlatan, jonatack

    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:

    • #31453 (util: detect and warn when using exFAT on MacOS by willcl-ark)
    • #31349 (ci: detect outbound internet traffic generated while running tests by vasild)
    • #30437 (multiprocess: add bitcoin-mine test program by ryanofsky)
    • #28710 (Remove the legacy wallet and BDB dependency by achow101)

    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 Jan 17, 2025
  4. DrahtBot added the label CI failed on Jan 17, 2025
  5. DrahtBot commented at 7:52 am on January 18, 2025: contributor
    CI failed with [17:52:38.033] /ci_container_base/ci/test/03_test_script.sh: line 154: /ci_container_base/ci/scratch/out/bin/test_bitcoin: No such file or directory
  6. luke-jr commented at 3:25 pm on January 23, 2025: member
    libexec is usually used for binaries run by programs rather than people. I don’t think most of the ones you’re moving fit that, and should remain in bin.
  7. Sjors commented at 7:46 am on January 24, 2025: member

    Run by people is a higher bar than “not typically useful”, but could make sense.

    If there are no binaries for which it currently makes sense to put them in libexec, since they’re all called by people, then it seems this PR has be based on #31375.

  8. ryanofsky commented at 5:57 pm on January 28, 2025: contributor

    The idea that an executable placed in libexec/ should be called by other executables is a good rule of thumb but not a requirement of some kind. (And if it were a requirement, it would be satisfied by #31375.)

    The technical difference between binaries installed to bin/ and binaries installed to libexec/ is that binaries in bin/ get exposed on the PATH while binaries in libexec/ don’t.

    There is nothing wrong with binaries in libexec/ being documented and called used by users and packagers and other tools and scripts. Not every executable that a user could conceivably call needs to be present in PATH.

    Packages like postfix have manpages for internal executables installed in libexec/ (anvil, bounce, smtpd, etc). Git subcommands can be called manually from libexec and have manpages of their own, too. I would say postfix and git are pretty much archetypes of packages designed around unix principles and there is nothing novel about installing low-level executables in libexec/.

    With this PR, two sets of binaries are installed to libexec/ if they have been enabled in cmake:

    1. multiprocess binaries bitcoin-node and bitcoin-gui
    2. testing binaries bench_bitcoin test_bitcoin test_bitcoin-qt.

    I can’t think of a reason why multiprocess binaries should be installed on PATH, but if someone wants to make that case I’m ready to hear it. As an alternative to this PR, they could be installed in bin/ with different names like bitoin-node-multiprocess or to a completely new folder.

    For the test and bench binaries I just think it’s confusing and not useful to install these on the PATH. I think this is true today and will be especially true after #31375.

  9. ryanofsky force-pushed on Jan 30, 2025
  10. ryanofsky commented at 3:05 am on January 30, 2025: contributor
    Updated 2a952710661a1990ed1c24b0ebd16de8ac0df87c -> 47a872236e070814ad74922d3f8a653e1c6af968 (pr/libexec.1 -> pr/libexec.2, compare) to fix CI error
  11. DrahtBot removed the label CI failed on Jan 30, 2025
  12. DrahtBot added the label Needs rebase on Feb 12, 2025
  13. ryanofsky force-pushed on Feb 13, 2025
  14. ryanofsky commented at 6:48 pm on February 13, 2025: contributor
    Rebased 47a872236e070814ad74922d3f8a653e1c6af968 -> d2ceb2e0735a2c8343f8316b55fac55323aba62c (pr/libexec.2 -> pr/libexec.3, compare) due to conflict with #31834
  15. DrahtBot removed the label Needs rebase on Feb 13, 2025
  16. cmake: Move internal binaries from bin/ to libexec/ 
    Currently when "make install" or "cmake --install" are run, various internal
binaries that are confusing and not typically useful and installed to
`${CMAKE_INSTALL_PREFIX}/bin` and can wind up on the system PATH. This PR moves
internal binaries out of bin/ into libexec/ where they will still be accessible
but will not be automatically placed on the PATH or be confused with more
useful binaries. The PR also adds an install rule for the bitcoin-chainstate
binary. After this PR binaries installed to bin/ are:

- bitcoin-cli
- bitcoind
- bitcoin-qt
- bitcoin-tx
- bitcoin-util
- bitcoin-wallet

And binaries installed to libexec/ are:

- bench_bitcoin
- bitcoin-gui
- bitcoin-node
- test_bitcoin
- test_bitcoin-qt

In the future if #31375 gets merged, there will be a new `bitcoin` wrapper
executable in bin/ that can be used to call other binaries, and with that
present, we could consider moving other binaries from bin/ to libexec/ and
recommending that most users should use the wrapper instead of calling the
different utilities directly. But this PR should make sense with or without
#31375.
    45bfd97ec7
  17. DrahtBot added the label Needs rebase on Feb 14, 2025
  18. ryanofsky force-pushed on Feb 14, 2025
  19. ryanofsky commented at 3:19 pm on February 14, 2025: contributor
    Rebased d2ceb2e0735a2c8343f8316b55fac55323aba62c -> 45bfd97ec7c9991f41673d79b01277bfd940e64e (pr/libexec.3 -> pr/libexec.4, compare) due to conflict with #31844
  20. DrahtBot removed the label Needs rebase on Feb 14, 2025
  21. TheCharlatan commented at 7:14 pm on April 8, 2025: contributor
    Concept ACK
  22. jonatack commented at 4:37 am on April 9, 2025: member

    Concept ACK

    Would need a release note?

  23. doc: Add release notes describing libexec/ binaries 2f04eff3e9
  24. doc: Add description of installed files to files.md bda7626e98
  25. ryanofsky commented at 1:34 pm on April 14, 2025: contributor

    Would need a release note?

    Thanks! Added release note and files.md documention in new commits.

    Added 2 commits 45bfd97ec7c9991f41673d79b01277bfd940e64e -> bda7626e987ce3d7287f72e6989dedde2c4ab666 (pr/libexec.4 -> pr/libexec.5, compare) with release notes and documentation

  26. TheCharlatan commented at 9:16 pm on April 15, 2025: contributor

    I think some installation packagers run test_bitcoin as part of their installation recipes, or at least I found one such example: https://github.com/epiccurious/bitcoin-core-node-builder/pull/164/files. @hebasto also polled usage of test_bitcoin on social media: https://x.com/hhebasto/status/1729879307768738070. Clearly there aren’t many people doing this, but are we ok with breaking the workflows of the few people that are? You mention that:

    For the test and bench binaries I just think it’s confusing and not useful to install these on the PATH. I think this is true today and will be especially true after #31375.

    What is confusing about keeping them where they are for now? Maybe there is some utility in testing your platform before actually running bitcoin on it.

  27. DrahtBot added the label Needs rebase on May 7, 2025
  28. DrahtBot commented at 2:25 pm on May 7, 2025: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.


ryanofsky DrahtBot luke-jr Sjors TheCharlatan jonatack

Labels
Build system Needs rebase

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