bitcoin wrapper: respect CMAKE_INSTALL_BINDIR/LIBEXECDIR #36106

pull ryanofsky wants to merge 2 commits into bitcoin:master from ryanofsky:pr/wrapexec changing 6 files +113 −6
  1. ryanofsky commented at 6:28 PM on August 27, 2026: contributor

    Problem: On Arch Linux the installed bitcoin command doesn't work: bitcoin -m node, bitcoin chainstate, and similar commands all fail with execvp failed to execute ... No such file or directory (#35785). Arch installs the programs the wrapper runs into a different directory than the wrapper looks in.

    Solution: Make the wrapper look for those programs in the directory they were actually installed to, rather than a hardcoded one. Small change to the wrapper (src/bitcoin.cpp), plus build-system changes to pass it the configured install directories, and a functional test for the installed layout. Details are in the commit messages.

    Fixes #35785. Built from earlier PRs #36037, #35789, and #36085.

  2. test: characterize installed bitcoin wrapper libexec lookup
    Add a functional test for the installed-layout executable lookup in the
`bitcoin` wrapper. When installed in a `bin/` directory the wrapper looks
for internal binaries like `bitcoind` in a sibling directory under the
same prefix; this path is never exercised by the normal build-tree tests
because every binary sits next to the wrapper there.

The test builds a fake install prefix under tmpdir, copies the wrapper
into `bin/` and `bitcoind` into an internal directory (not next to the
wrapper), and runs the wrapper by absolute path with PATH cleared so a
lookup miss cannot be satisfied by an unrelated system binary.

This characterizes current behavior — the wrapper resolves binaries from a
hardcoded `libexec/` directory — so a following commit can change it to
honor CMAKE_INSTALL_LIBEXECDIR and verify the result.

Adapted from https://github.com/bitcoin/bitcoin/pull/36037.

Co-Authored-By: Avecci-Claussen <cto@greybound.tech>
Co-Authored-By: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
    aa10273765
  3. build: respect CMAKE_INSTALL_BINDIR/LIBEXECDIR in bitcoin wrapper
    Switch from hardcoded "bin"/"libexec" strings to CMAKE_INSTALL_BINDIR and
CMAKE_INSTALL_LIBEXECDIR in the installed-layout executable lookup, so the
wrapper works on distributions that configure non-default install
directories. Previously `bitcoin -m node`, `bitcoin chainstate`, and
similar subcommands failed with "execvp failed to execute" on Arch Linux,
which sets CMAKE_INSTALL_LIBEXECDIR=lib (#35785).

Thread both values through bitcoin-build-config.h. Include GNUInstallDirs
at the top level of CMakeLists.txt before the subdirectories that
substitute these values into generated files, and add configure-time
FATAL_ERROR guards for empty values (complement to the compile-time
static_asserts). Remove the now-redundant include(GNUInstallDirs) from
InstallBinaryComponent.cmake. Pass BINDIR and LIBEXECDIR through
test/config.ini.in so test_installed_layout() verifies the actual
configured directories rather than hardcoded names, skipping the check
when either is absolute. Fix run_wrapper() in the test to prepend
binaries.valgrind_cmd.

Fixes #35785.

The fix and test build on #36037, #35789, and #36085.

Co-Authored-By: Avecci-Claussen <cto@greybound.tech>
Co-Authored-By: Andrew Barnes <bortstheboat@gmail.com>
Co-Authored-By: riba2534 <riba2534@qq.com>
Co-Authored-By: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-Authored-By: cyb3ralbert <cyberalbert@protonmail.ch>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
    50450a06dc
  4. DrahtBot commented at 6:28 PM on August 27, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline and AI policy for information on the review process.

    Type Reviewers
    Approach ACK hebasto

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #36190 (util: preserve Windows child exit status -1 by Bortlesboat)
    • #36105 (bitcoin wrapper: Fix Windows exec so wrapper waits for child process by ryanofsky)
    • #36022 (test: add coverage for bitcoin wrapper argument handling by cyb3ralbert)
    • #32387 (ipc: add windows support by ryanofsky)

    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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  5. ryanofsky force-pushed on Aug 27, 2026
  6. DrahtBot added the label CI failed on Aug 27, 2026
  7. ryanofsky commented at 7:54 PM on August 27, 2026: contributor

    Updated 94c64d40c8e5b51122c39af9d781e360752aec11 -> dfc786c75ec3cdb18b824547bf586642c8bda6b5 (pr/wrapexec.1 -> pr/wrapexec.2, compare)<!-- end --> to fix tool_bitcoin.py installed-layout check failing in CI: pass a writable -datadir so 'bitcoin node -version' doesn't depend on the default datadir ($HOME/.bitcoin) being writable https://github.com/bitcoin/bitcoin/actions/runs/33103647138/job/98627629393

  8. DrahtBot removed the label CI failed on Aug 27, 2026
  9. purpleKarrot commented at 3:18 AM on August 29, 2026: contributor

    Looks good. But it also has AI co-authors.

  10. hebasto commented at 11:52 AM on September 1, 2026: member

    Concept ACK.

    ... it also has AI co-authors.

    From the project's AI Policy:

    Do not include agents as ... co-authors of your commits...

  11. in src/bitcoin.cpp:25 in dfc786c75e 
      17 | @@ -18,6 +18,12 @@
  18 |  #include <tinyformat.h>
  19 |  #include <vector>
  20 |  
  21 | +// These are used to locate installed executables relative to the wrapper and
  22 | +// must not be empty, so a missing GNUInstallDirs value fails at compile time
  23 | +// rather than silently breaking the installed-layout lookup in ExecCommand().
  24 | +static_assert(BITCOIN_BINDIR[0] != '\0', "BITCOIN_BINDIR must not be empty");
  25 | +static_assert(BITCOIN_INTERNAL_LIBEXECDIR[0] != '\0', "BITCOIN_INTERNAL_LIBEXECDIR must not be empty");
    hebasto commented at 2:03 PM on September 1, 2026:

    1. This limits the user's choice of values for CMAKE_INSTALL_BINDIR and CMAKE_INSTALL_LIBEXECDIR, whereas the CMake docs do not impose any such restriction. If this is strictly necessary, it should be probably documented.

    2. Shouldn't the check for unset or empty CMake variables be performed earlier, at the configuration stage?

    ryanofsky commented at 1:11 AM on September 9, 2026:

    re: #36106 (review)

    Thanks, I expanded the comment on the static assert to note the bindir limitation and added some cmake checks for empty values. It's very likely there are more checks and improvements that can be made. This PR is just closing the biggest gap by starting to use these configuration values which were ignored before.

  12. in CMakeLists.txt:643 in dfc786c75e outdated 
     636 | @@ -637,6 +637,11 @@ if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.29)
 637 |    set(CMAKE_SKIP_TEST_ALL_DEPENDENCY FALSE)
 638 |  endif()
 639 |  
 640 | +# Populate CMAKE_INSTALL_BINDIR, CMAKE_INSTALL_LIBEXECDIR, etc. before the
 641 | +# subdirectories below, which substitute these values into generated files
 642 | +# (test/config.ini and src/bitcoin-build-config.h).
 643 | +include(GNUInstallDirs)
    hebasto commented at 2:06 PM on September 1, 2026:

    While re-inclusions of GNUInstallDirs are harmless, maybe consider removing the now-redundant ones to keep the code clean.

    ryanofsky commented at 1:12 AM on September 9, 2026:

    re: #36106 (review)

    While re-inclusions of GNUInstallDirs are harmless, maybe consider removing the now-redundant ones to keep the code clean.

    Thanks, removed from cmake/module/InstallBinaryComponent.cmake in the latest push

  13. in test/functional/tool_bitcoin.py:113 in dfc786c75e 
     108 | +        # prefix constructed here, so skip the check in that case.
 109 | +        libexecdir = self.config["environment"]["LIBEXECDIR"]
 110 | +        if os.path.isabs(libexecdir):
 111 | +            self.log.info("Skipping installed-layout check; CMAKE_INSTALL_LIBEXECDIR is absolute")
 112 | +            return
 113 | +        prefix = self.nodes[0].datadir_path / "fake-prefix"
    hebasto commented at 2:09 PM on September 1, 2026:

    Using the node's datadir for the installation prefix seems counter-intuitive to me. Maybe place it directly under self.options.tmpdir?

    ryanofsky commented at 1:12 AM on September 9, 2026:

    re: #36106 (review)

    Using the node's datadir for the installation prefix seems counter-intuitive to me. Maybe place it directly under self.options.tmpdir?

    Good idea, that does seem better and done in latest push

  14. hebasto commented at 2:10 PM on September 1, 2026: member

    Approach ACK dfc786c75ec3cdb18b824547bf586642c8bda6b5.

    Tested also with the cmake --install command's --prefix option.

    The new test fails when the user specifies CMAKE_INSTALL_BINDIR:

    $ cmake -B build -DCMAKE_INSTALL_BINDIR="nib"
$ cmake --build build
$ ./build/test/functional/tool_bitcoin.py
2026-09-01T13:53:13.898723Z TestFramework (INFO): PRNG seed is: 6212645667151046958
2026-09-01T13:53:13.899141Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_0y4u82jq
2026-09-01T13:53:13.899475Z TestFramework (INFO): Ensure bitcoin node command invokes bitcoind by default
2026-09-01T13:53:13.903417Z TestFramework (INFO): Ensure bitcoin -M invokes bitcoind
2026-09-01T13:53:13.907124Z TestFramework (INFO): Ensure bitcoin -M does not accept -ipcbind
2026-09-01T13:53:13.910846Z TestFramework (INFO): Ensure bitcoin -m invokes bitcoin-node
2026-09-01T13:53:13.914668Z TestFramework (INFO): Ensure bitcoin -m does accept -ipcbind
2026-09-01T13:53:13.918495Z TestFramework (INFO): Ensure bitcoin accepts -ipcbind by default
2026-09-01T13:53:13.922104Z TestFramework (INFO): Ensure bitcoin recognizes -ipcbind in config file
2026-09-01T13:53:13.926035Z TestFramework (INFO): Ensure installed wrapper finds internal binaries in configured libexec/
2026-09-01T13:53:14.046585Z TestFramework (ERROR): Unexpected exception:
Traceback (most recent call last):
  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/test_framework.py", line 145, in main
    self.run_test()
    ~~~~~~~~~~~~~^^
  File "/home/hebasto/dev/bitcoin/./build/test/functional/tool_bitcoin.py", line 92, in run_test
    self.test_installed_layout()
    ~~~~~~~~~~~~~~~~~~~~~~~~~~^^
  File "/home/hebasto/dev/bitcoin/./build/test/functional/tool_bitcoin.py", line 143, in test_installed_layout
    assert_equal(result.returncode, 0)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/util.py", line 94, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(1 == 0)
2026-09-01T13:53:14.098127Z TestFramework (INFO): Not stopping nodes as test failed. The dangling processes will be cleaned up later.
2026-09-01T13:53:14.098499Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_0y4u82jq
2026-09-01T13:53:14.098707Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_0y4u82jq/test_framework.log
2026-09-01T13:53:14.099086Z TestFramework (ERROR): 
2026-09-01T13:53:14.099412Z TestFramework (ERROR): Hint: Call /home/hebasto/dev/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_0y4u82jq' to consolidate all logs
2026-09-01T13:53:14.099593Z TestFramework (ERROR): 
2026-09-01T13:53:14.099752Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2026-09-01T13:53:14.099975Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2026-09-01T13:53:14.100138Z TestFramework (ERROR):
  15. cyb3ralbert commented at 9:30 AM on September 2, 2026: contributor

    The fix works — I built the branch and ran the #35785 scenario to verify it. I did find two gaps in the test coverage, though: one that seems worth fixing here, and one that could be a follow-up.

    test_installed_layout derives other_dir = "lib" if libexecdir != "lib" else "libexec", which at the default CMAKE_INSTALL_LIBEXECDIR=libexec is exactly the pair the old wrapper hardcodes:

    CMAKE_INSTALL_LIBEXECDIR wrapper result
    libexec (default) this branch pass
    libexec (default) before this change pass
    lib this branch pass
    lib before this change fail — AssertionError: not(1 == 0)

    CMAKE_INSTALL_LIBEXECDIR does not appear anywhere under ci/ or .github/, so every job builds with the GNUInstallDirs default (libexec) and none of them can tell the two wrappers apart — a regression to a hardcoded directory would stay green. The lib value is what Arch actually uses (-DCMAKE_INSTALL_LIBEXECDIR=lib in its PKGBUILD), so adding it to BITCOIN_CONFIG in one of the existing ci/test/00_setup_env_*.sh jobs would make the test you already wrote cover the change. That part could go in a follow-up.

    The smaller one: run_wrapper calls subprocess.run([wrapper, ...]) directly, bypassing binaries.valgrind_cmd, so the new checks are skipped under --valgrind — 25 ms and 63 ms against ~2 s for each neighbouring check in the same run. The same point was raised on #36022 in this file and fixed there by prefixing self.nodes[0].binaries.valgrind_cmd; it matters here because that command carries --trace-children=yes, which is "Needed for 'bitcoin' wrapper".

  16. ryanofsky force-pushed on Sep 9, 2026
  17. ryanofsky commented at 1:30 AM on September 9, 2026: contributor

    <!-- begin push-3 -->

    Updated dfc786c75ec3cdb18b824547bf586642c8bda6b5 -> 50450a06dceffe6d86cef529c77d81be18b7852b (pr/wrapexec.2 -> pr/wrapexec.3, compare)<!-- end --> with review suggestions.

    re: hebasto #36106#pullrequestreview-5079009323

    The new test fails when the user specifies CMAKE_INSTALL_BINDIR:

    Good catch, fixed by passing CMAKE_INSTALL_BINDIR to the test now.

    re: cyb3ralbert #36106 (comment)

    The lib value is what Arch actually uses (-DCMAKE_INSTALL_LIBEXECDIR=lib in its PKGBUILD), so adding it to BITCOIN_CONFIG in one of the existing ci/test/00_setup_env_*.sh jobs would make the test you already wrote cover the change. That part could go in a follow-up.

    Yeah it is hard to have good test coverage for build-time configuration code, but I agree it could be a good idea to change one of the CI job configurations to try setting nonstandard directories and make sure things don't break.

    I would definitely leave it for a followup because CI configuration changes need a different kind of review and probably a different set of reviewers than this code change.

    fixed there by prefixing self.nodes[0].binaries.valgrind_cmd

    Thanks! Added that here as well

Contributors
ryanofskyDrahtBotpurpleKarrothebastocyb3ralbert
Linked (view graph)
#35785 launcher does not respect CMAKE_INSTALL_LIBEXECDIR#35789 build: respect CMAKE_INSTALL_LIBEXECDIR in bitcoin launcher#36037 build: honor CMAKE_INSTALL_LIBEXECDIR in wrapper#36085 build: respect CMAKE_INSTALL_BINDIR and CMAKE_INSTALL_LIBEXECDIR in the launcher
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: 2026-09-09 07:56 UTC