#34721 build: install shell completions via cmake
https://github.com/bitcoin/bitcoin/pull/34721 · · +23/-8 in 4 files, 1 commits · labels: Build system
Goal
- Automatically install bash and fish shell completions alongside binaries during CMake installation
- Eliminates manual copying of completion scripts for source installers and package maintainers
This PR adds CMake installation rules for bash and fish shell completions located in contrib/completions/. It introduces INSTALL_BASH_COMPLETIONS and INSTALL_FISH_COMPLETIONS options and associates completions directly with executable target components using explicit target flags.
Problem: Downstream packagers and users installing Bitcoin Core from source have to manually find and copy shell completion scripts into system vendor directories. Installing binaries by component or prefix omits completion scripts unless manually scripted.
Category: Build and CI (#27 of 55)
P3 · new feature
- P3 because this provides a minor usability improvement for Linux packagers and command-line users
- Automates previously manual completion script installation with minimal ongoing maintenance
- Does not affect release integrity, reproducible builds, or core platform support
P3 because this is a worthwhile usability improvement for Linux packagers and command-line users that requires minimal maintenance. It does not affect release integrity, reproducible builds, or platform support, but automates what was previously a manual copy step.
Membership: Modifies CMakeLists.txt and cmake/module/InstallBinaryComponent.cmake to install shell completions.
Factors: security/stability 0, bug 0, performance 0, user value 2, leverage 0
Reviewability: Ready
- Ready for review now
- Configure-time file checks were replaced with explicit flags and confirmed clean
The author addressed reviewer feedback by replacing configure-time file checks with explicit flags, and recent testing confirms clean operation.
Author status: active
Open concerns:
- purpleKarrot expressed a general preference to eliminate convenience wrappers like install_binary_component in favor of inlining declarative install rules, but hebasto supported keeping the wrapper.
Resolved concerns:
- purpleKarrot, ryanofsky, hebasto, and 151henry151 flagged that checking file existence with if(EXISTS) at configure time is fragile; resolved by switching to explicit HAS_BASH_COMPLETION and HAS_FISH_COMPLETION target flags.
- luke-jr requested splitting the toggle per shell rather than a single monolithic flag; resolved by adding separate options for bash and fish.
Agreement: Strong
- Strong consensus on installing completions via CMake (BrandonOdiwuor, caesrcd, w0xlt, hebasto, ryanofsky)
- Fragile configure-time file checks resolved by adding explicit target flags (purpleKarrot, ryanofsky)
- Open preference to inline install rules instead of using a convenience wrapper (purpleKarrot, hebasto)
Strong consensus on installing shell completions; file-checking objections resolved in latest push.
Multiple reviewers support installing shell completions via CMake. The only blocking technical feedback concerned CMake file-detection hygiene, which the author resolved by adopting explicit target parameters.
- Concept ACKs and Tested ACKs from BrandonOdiwuor, caesrcd, 151henry151, w0xlt, hebasto, and ryanofsky.
- purpleKarrot and ryanofsky objected to if(EXISTS) sniffing; author replaced it with explicit keywords.
- fanquake questioned whether completion scripts would be maintained; 151henry151 pointed out scripts generate options dynamically from help text rather than hardcoded tables.
Review verdicts (DrahtBot): 1 (+3)
- ACK: caesrcd
- Stale ACK: ryanofsky, BrandonOdiwuor, 151henry151
- Concept ACK: w0xlt, hebasto
Files
0 lines under test/bench/ci.
- cmake/module/InstallBinaryComponent.cmake +14/-1
- src/CMakeLists.txt +6/-6
- CMakeLists.txt +2/-0
- src/qt/CMakeLists.txt +1/-1
Card
This PR updates the CMake build system to install bash and fish shell completions when running cmake --install, tying them to specific binary components using explicit target flags. It addresses issue #34714 to eliminate the need for downstream packagers to copy shell completions manually. The PR is ready for review after a push that resolved reviewer concerns about dynamic file detection. It enjoys multiple tested ACKs with no blocking objections.