#36106 bitcoin wrapper: respect CMAKE_INSTALL_BINDIR/LIBEXECDIR
| Reviewability | Reviews | Agreement | Size | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| #36106 bitcoin wrapper: respect CMAKE_INSTALL_BINDIR/LIBEXECDIR | Ready | 1 | Strong | 42 + 71 tests | |||||||||
|
|
|
|
| |||||||||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||||||||
Goal
Threads CMAKE_INSTALL_BINDIR and CMAKE_INSTALL_LIBEXECDIR through bitcoin-build-config.h into the bitcoin wrapper (src/bitcoin.cpp). Replaces hardcoded 'bin' and 'libexec' directory lookups with configured build-time paths and adds configure-time validation checks and functional tests for installed directory layouts.
Problem: On distributions such as Arch Linux that set CMAKE_INSTALL_LIBEXECDIR to 'lib', the installed bitcoin wrapper command cannot find internal executables like bitcoind or bitcoin-node, failing immediately with execvp 'No such file or directory'.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- nothing: all objections and suggestions are settled and latest push has been tested
Agreement
Strong
substantive support, no open objection (hebasto, cyb3ralbert, cyb3ralbert)
Thread notes: cyb3ralbert verified the valgrind fix across multiple configurations and provided a Tested ACK; the objection is now accepted.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| hebasto (member) | objection, correctness | Functional test fails when CMAKE_INSTALL_BINDIR is customized | fixed · fix pushed · Author passed CMAKE_INSTALL_BINDIR to the functional test in 50450a06 | changed | yes | 2026-09-01: 'The new test fails when the user specifies `CMAKE_INSTALL_BINDIR`' Status from: 2026-09-09: ryanofsky: 'Good catch, fixed by passing CMAKE_INSTALL_BINDIR to the test now.' |
| hebasto (member) | suggestion, maintenance | Document the limitation on CMAKE_INSTALL_BINDIR and CMAKE_INSTALL_LIBEXECDIR values if strictly necessary | fixed · fix pushed · Author expanded comment on static assert to note bindir limitation in 50450a06 | answered | yes | 2026-09-01: 'If this is strictly necessary, it should be probably documented.' Status from: 2026-09-09: ryanofsky: 'expanded the comment on the static assert to note the bindir limitation' |
| hebasto (member) | suggestion, approach | Perform check for unset or empty CMake variables earlier at configure stage | fixed · fix pushed · Author added CMake checks for empty values in 50450a06 | answered | yes | 2026-09-01: 'Shouldn't the check for unset or empty CMake variables be performed earlier, at the configuration stage?' Status from: 2026-09-09: ryanofsky: 'added some cmake checks for empty values' |
| hebasto (member) | suggestion, maintenance | Remove redundant re-inclusions of GNUInstallDirs to keep code clean | fixed · fix pushed · Author removed re-inclusion from cmake/module/InstallBinaryComponent.cmake in 50450a06 | answered | yes | 2026-09-01: 'maybe consider removing the now-redundant ones to keep the code clean.' Status from: 2026-09-09: ryanofsky: 'removed from cmake/module/InstallBinaryComponent.cmake in the latest push' |
| hebasto (member) | suggestion, approach | Place test installation prefix directly under self.options.tmpdir rather than using the node's datadir | fixed · fix pushed · Author moved test installation prefix directly under tmpdir in 50450a06 | answered | yes | 2026-09-01: 'Maybe place it directly under `self.options.tmpdir`?' Status from: 2026-09-09: ryanofsky: 'Good idea, that does seem better and done in latest push' |
| cyb3ralbert (contributor) | suggestion, approach | Set CMAKE_INSTALL_LIBEXECDIR=lib in BITCOIN_CONFIG in a CI job to cover the wrapper change | answered · Author agreed with the idea but prefers to leave CI configuration changes to a follow-up | answered | yes | 2026-09-02: '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.' Status from: 2026-09-09: ryanofsky: 'I would definitely leave it for a followup because CI configuration changes need a different kind of review' |
| cyb3ralbert (contributor) | suggestion, approach | Prefix wrapper invocation with binaries.valgrind_cmd in run_wrapper so the new checks run under --valgrind | fixed · fix pushed · Author added valgrind_cmd in 50450a06 | answered | yes | 2026-09-02: 'run_wrapper calls subprocess.run([wrapper, ...]) directly, bypassing binaries.valgrind_cmd, so the new checks are skipped under --valgrind' Status from: 2026-09-09: ryanofsky: 'Thanks! Added that here as well' |
| cyb3ralbert (contributor) | objection, correctness | tool_bitcoin.py fails under --valgrind because PATH is blanked before the valgrind binary is located | accepted · fix pushed · cyb3ralbert confirmed the fix with tACK after testing | changed | yes | 2026-09-10: 'The last push did introduce one regression, though: `tool_bitcoin.py` fails under `--valgrind`.' Status from: 2026-10-01: cyb3ralbert: 'The valgrind issue from my comment (10.09.2026) is fixed' |
Support:
- purpleKarrot (member) (2026-08-29): Looks good. But it also has AI co-authors. [not substantive]
- hebasto (member) (Concept ACK, 2026-09-01): Concept ACK. [not substantive]
- hebasto (member) (Approach ACK, 2026-09-01) [both]: Approach ACK dfc786c75ec3cdb18b824547bf586642c8bda6b5. Tested also with the `cmake --install` command's `--prefix` option.
- cyb3ralbert (contributor) (2026-09-02) [tested]: The fix works — I built the branch and ran the #35785 scenario to verify it.
- cyb3ralbert (contributor) (Tested ACK, 2026-10-01) [tested: test/functional/tool_bitcoin.py]: The valgrind issue from my comment (10.09.2026) is fixed: https://github.com/bitcoin/bitcoin/pull/36106#issuecomment-5617007631
Participants:
- purpleKarrot (member, support, 1 statements 2026-08-29 to 2026-08-29): commented looks good while noting AI co-authors
- hebasto (member, objection, 5 statements 2026-09-01 to 2026-09-01): Concept and Approach ACK, but reported test failure with custom CMAKE_INSTALL_BINDIR and raised CMake suggestions
- cyb3ralbert (contributor, objection, 3 statements 2026-09-02 to 2026-10-01): tested against Arch Linux layout, reported valgrind regression, and confirmed fix with tACK
Files
71 lines under test/bench/ci.
- test/functional/tool_bitcoin.py +69/-0
- src/bitcoin.cpp +18/-5
- CMakeLists.txt +14/-0
- cmake/bitcoin-build-config.h.in +10/-0
- test/config.ini.in +2/-0
- cmake/module/InstallBinaryComponent.cmake +0/-1
Card
The short text the ranking pass compares across PRs.
PR 36106 fixes an issue where the bitcoin wrapper cannot execute underlying binaries like bitcoind or bitcoin-node on distributions with non-default install directories such as Arch Linux (issue #35785). It replaces hardcoded 'bin' and 'libexec' directory paths in src/bitcoin.cpp with build-time values from CMAKE_INSTALL_BINDIR and CMAKE_INSTALL_LIBEXECDIR, accompanied by configure-time sanity checks and functional test coverage. Review has been constructive, with an Approach ACK from hebasto and extensive testing from cyb3ralbert, and all requested test and build adjustments have been incorporated.
Data
thread read through 2026-10-01 (15 statements), head bd3d5cf57e
labels: none
Categories
Build and CI
P3 · platform fix
Fixes CMake installation path handling for distributions that customize libexec paths. On systems like Arch Linux that set CMAKE_INSTALL_LIBEXECDIR to 'lib', installed wrapper builds currently cannot locate internal binaries and fail at runtime.
Membership: Modifies CMakeLists.txt and cmake/bitcoin-build-config.h.in to properly handle and validate CMAKE_INSTALL_BINDIR and CMAKE_INSTALL_LIBEXECDIR installation paths.
Factors: security/stability 0, bug 1, performance 0, user value 1, leverage 0
IPC / multiprocess
P2 · bug fix (assessed alone as P3; Raised to P2 for consistency with #36105: both are wrapper breakages on real install targets, and this one makes the wrapper fail outright on common distro layouts.)
Resolves a runtime binary discovery failure in the bitcoin wrapper on platforms with non-standard install paths. According to the PR description, on distributions like Arch Linux, the wrapper 'cannot find internal executables like bitcoind or bitcoin-node, failing immediately with execvp'.
Membership: Touches src/bitcoin.cpp, the multiprocess wrapper executable, and its build integration.
Factors: security/stability 0, bug 1, performance 0, user value 1, leverage 0
Tools and scripts
P2 · bug fix (assessed alone as P3; Raised to P2 and moved up: the wrapper is simply unusable on distros with non-default libexec layout (issue #35785), the same class of supported-platform breakage as #36105, so the two must share a band.)
Fixes execution failure of the bitcoin wrapper command when installed with custom directory layouts. Without this fix, the wrapper cannot invoke underlying executables on distributions using non-default install directories.
Membership: Changes src/bitcoin.cpp and adds test/functional/tool_bitcoin.py to test the bitcoin wrapper tool.
Factors: security/stability 0, bug 1, performance 0, user value 1, leverage 0