build: install shell completions via cmake #34721

willcl-ark commented at 2:33 PM on March 3, 2026: member

Packagers currently need to copy shell completion scripts from contrib/completions/ into the appropriate system directories. Install the scripts alongside their corresponding binaries with cmake --install, including when installing a binary by component.

Targets declare their available bash and fish scripts explicitly, following the existing HAS_MANPAGE pattern. This avoids deciding what to install based on file existence at configure time.

Bash scripts install to ${datadir}/bash-completion/completions/; fish scripts install to ${datadir}/fish/vendor_completions.d/. Both are enabled by default. Use -DINSTALL_BASH_COMPLETIONS=OFF or -DINSTALL_FISH_COMPLETIONS=OFF to disable either shell’s scripts.

DrahtBot added the label Build system on Mar 3, 2026

DrahtBot commented at 2:33 PM on March 3, 2026: 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/34721.

Reviews

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

Type	Reviewers
ACK	caesrcd
Concept ACK	w0xlt, hebasto
Stale ACK	ryanofsky, BrandonOdiwuor, 151henry151

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

kpcyrd commented at 3:28 PM on March 3, 2026: none

I tested this patch and it works well for me :smiley_cat:

DrahtBot added the label CI failed on Mar 3, 2026

w0xlt commented at 3:52 PM on March 3, 2026: contributor

Concept ACK

hebasto commented at 3:57 PM on March 3, 2026: member

Concept ACK.

fanquake commented at 4:08 PM on March 3, 2026: member

Will we start maintaining these, if we plan on installing them? #33747, #33402, #33385. There doesn't seem to be any interest historically from contributors.

DrahtBot removed the label CI failed on Mar 3, 2026

willcl-ark commented at 11:45 AM on March 4, 2026: member

Will we start maintaining these, if we plan on installing them?

Fair shout. The alternative is to not maintain them and but to then also not ship them at all. My preference would be to maintain and install them.

I have reviewed #33385 as I think this looks in the best shape to move forward, barring author response time.

caesrcd commented at 6:30 PM on March 4, 2026: contributor

Concept ACK

in CMakeLists.txt:180 in 0a2d2cd116

 176 | @@ -177,6 +177,7 @@ option(BUILD_FUZZ_BINARY "Build fuzz binary." OFF)
 177 |  option(BUILD_FOR_FUZZING "Build for fuzzing. Enabling this will disable all other targets and override BUILD_FUZZ_BINARY." OFF)
 178 |  
 179 |  option(INSTALL_MAN "Install man pages." ON)
 180 | +option(INSTALL_COMPLETIONS "Install shell completion scripts." ON)

luke-jr commented at 7:09 AM on March 10, 2026:

Users are at least somewhat unlikely to use multiple shells, so it probably makes sense to have a separate option for each one

purpleKarrot commented at 9:45 AM on March 10, 2026: contributor

I don't like the direction the install_binary_component command is evolving. My concerns are:

if(EXISTS ${...}): The problem is that adding a file may change the behavior of install, but only after cmake was manually rerun. Adding the file will not trigger the reconfiguration by itself. This leads to flakiness (similar to file(GLOB)). CMakeLists.txt files should always explicitly describe the source tree. Things like if(EXISTS) and file(GLOB) should be reserved for cmake -P scripts.
option(INSTALL_COMPLETIONS): This is a build option, that does not affect the build, hence the wrong tool for the job (so is INSTALL_MAN). Better: Use fine grained install components, to let the user decide at install time what to install. The 1:1 relationship of target and component is not helpful.

The whole install_binary_component command should be inlined at the call sites and then removed. This makes the CMakeLists.txt much more declarative:

For targets that are installed to the default location, don't specify the location but use the default.
For targets that have manpages and shell completions, install them. For targets that don't, just don't.

Any "convenience" wrapper should be killed before it attracts more and more responsibilities.

BrandonOdiwuor commented at 8:23 AM on March 11, 2026: contributor

Concept ACK

Installing shell completions automatically with cmake --install is more convenient. Tested on Ubuntu 24.04 with bash.

--prefix and --component install

in cmake/module/InstallBinaryComponent.cmake:33 in 0a2d2cd116

  27 | @@ -28,4 +28,21 @@ function(install_binary_component component)
  28 |        COMPONENT ${component}
  29 |      )
  30 |    endif()
  31 | +  if(INSTALL_COMPLETIONS)
  32 | +    set(bash_comp ${PROJECT_SOURCE_DIR}/contrib/completions/bash/${target_name}.bash)
  33 | +    if(EXISTS ${bash_comp})

ryanofsky commented at 9:17 PM on April 6, 2026:

In commit "build: install shell completions via cmake" (0a2d2cd11655b75b629992acb54dd2a572759fb0)

This seems ok, but strictly speaking this is inconsistent with the approach used for manpages above. The manpage approach uses an explicit HAS_MANPAGE keyword like install_binary_component(bitcoind HAS_MANPAGE), while this is automatically deciding whether to install files based on file existence.

The manpage approach seems a ilttle cleaner but also harder to maintain so I'm not sure which approach is better, but maybe it's worth considering using HAS_BASH_COMPLETEION, HAS_FISH_COMPLETION keywords here to be consistent?

hebasto commented at 4:01 PM on July 27, 2026:

The manpage approach seems a ilttle cleaner but also harder to maintain so I'm not sure which approach is better, but maybe it's worth considering using HAS_BASH_COMPLETEION, HAS_FISH_COMPLETION keywords here to be consistent?

I agree.

ryanofsky approved

ryanofsky commented at 9:18 PM on April 6, 2026: contributor

Code review ACK 0a2d2cd11655b75b629992acb54dd2a572759fb0

DrahtBot requested review from BrandonOdiwuor on Apr 6, 2026

DrahtBot requested review from hebasto on Apr 6, 2026

DrahtBot requested review from caesrcd on Apr 6, 2026

sedited commented at 8:59 AM on July 21, 2026: contributor

@BrandonOdiwuor can you take another look here?

BrandonOdiwuor commented at 6:55 AM on July 24, 2026: contributor

ACK 0a2d2cd11655b75b629992acb54dd2a572759fb0

I think installing bash (and fish) completions is a useful feature. I tested the functionality in my previous review and it worked as expected for the tools that have completion scripts.

That said, I share the same concerns with the approach raised above. This is not consistent with how manpages are handled, where a HAS_MANPAGE variable is passed to dictate installation.

In commit "build: install shell completions via cmake" (0a2d2cd)

This seems ok, but strictly speaking this is inconsistent with the approach used for manpages above. The manpage approach uses an explicit HAS_MANPAGE keyword like install_binary_component(bitcoind HAS_MANPAGE), while this is automatically deciding whether to install files based on file existence.

The manpage approach seems a ilttle cleaner but also harder to maintain so I'm not sure which approach is better, but maybe it's worth considering using HAS_BASH_COMPLETEION, HAS_FISH_COMPLETION keywords here to be consistent?

151henry151 commented at 6:40 AM on July 27, 2026: contributor

Tested ACK 0a2d2cd11655b75b629992acb54dd2a572759fb0

Debian 13, BUILD_GUI=OFF. Installed to a prefix:

$ find /tmp/btc-pfx -type f \( -path '*bash-completion*' -o -path '*fish/vendor*' \) | sort
/tmp/btc-pfx/share/bash-completion/completions/bitcoin-cli
/tmp/btc-pfx/share/bash-completion/completions/bitcoind
/tmp/btc-pfx/share/bash-completion/completions/bitcoin-tx
/tmp/btc-pfx/share/fish/vendor_completions.d/bitcoin-cli.fish
/tmp/btc-pfx/share/fish/vendor_completions.d/bitcoind.fish
/tmp/btc-pfx/share/fish/vendor_completions.d/bitcoin-tx.fish
/tmp/btc-pfx/share/fish/vendor_completions.d/bitcoin-util.fish
/tmp/btc-pfx/share/fish/vendor_completions.d/bitcoin-wallet.fish

Bash only ships for cli/d/tx (no scripts for util/wallet). Fish covered all five binaries I installed; bitcoin-qt.fish is in-tree but correctly stayed out with the GUI off. -DINSTALL_COMPLETIONS=OFF still installs man pages, just no completions. DESTDIR staging behaves the same. Sourced the installed bash completions — tab-completing from -help works (~138 flags for bitcoind).

I can reproduce the if(EXISTS) issue: add contrib/completions/bash/bitcoin-util.bash after configuring and install just skips it, no warning. Only matters when someone adds a new completion file in-tree. Explicit HAS_BASH_COMPLETION / HAS_FISH_COMPLETION (like HAS_MANPAGE) would be cleaner.

On fanquake's maintenance point: these mostly pull options/commands from -help at completion time rather than hardcoding a full list (bitcoin-cli.bash does keep a small table of per-RPC arg hints). Shipping them isn't signing up to hand-maintain a flag table. Whether the project wants to own them at all is still a separate question.

hebasto commented at 3:40 PM on July 27, 2026: member

It looks like this comment is still unaddressed.

DrahtBot requested review from hebasto on Jul 27, 2026

hebasto commented at 4:00 PM on July 27, 2026: member

I don't like the direction the install_binary_component command is evolving. My concerns are:

if(EXISTS ${...}): The problem is that adding a file may change the behavior of install, but only after cmake was manually rerun. Adding the file will not trigger the reconfiguration by itself. This leads to flakiness (similar to file(GLOB)). CMakeLists.txt files should always explicitly describe the source tree. Things like if(EXISTS) and file(GLOB) should be reserved for cmake -P scripts.

I agree. On the master branch, we have a single use case, which will eventually go away once we switch to the test discovery approach. I suggest avoiding introducing more use cases for if(EXISTS ${...}).

option(INSTALL_COMPLETIONS): This is a build option, that does not affect the build, hence the wrong tool for the job (so is INSTALL_MAN). Better: Use fine grained install components, to let the user decide at install time what to install. The 1:1 relationship of target and component is not helpful.

I'm not convinced that using fine-grained install components is better because, for example, installing everything except manpages would be a headache due to limitations of the command syntax.

The whole install_binary_component command should be inlined at the call sites and then removed. This makes the CMakeLists.txt much more declarative:

For targets that are installed to the default location, don't specify the location but use the default.

For targets that have manpages and shell completions, install them. For targets that don't, just don't.

Any "convenience" wrapper should be killed before it attracts more and more responsibilities.

Many large projects, such as LLVM and Qt, use convenience wrappers extensively. In my opinion, they improve code maintainability as well,

DrahtBot requested review from hebasto on Jul 27, 2026

ryanofsky commented at 5:59 PM on September 14, 2026: contributor

@willcl-ark there seem to be more reasons given here to switch away from if(EXISTS ${...}) to explicit keywords folllowing HAS_MANPAGE pattern. There are also other questions that could be addressed, but it seems like this is the only potentially blocking one?

build: install shell completions via cmake

Packagers currently need to copy shell completion scripts manually.
Install them with their corresponding binaries so component installs include
the matching scripts.

Declare the available scripts at each target call site. This follows the
manpage pattern and avoids silently omitting scripts added after CMake was
configured. Allow bash and fish installation to be selected independently.

91b96bef34

willcl-ark force-pushed on Sep 14, 2026

willcl-ark commented at 8:19 PM on September 14, 2026: member

Thanks for the reviews (and the nudge!).

I replaced the if(EXISTS) checks with explicit HAS_BASH_COMPLETION and HAS_FISH_COMPLETION keywords. I also split the install option into INSTALL_BASH_COMPLETIONS and INSTALL_FISH_COMPLETIONS, both enabled by default.

Also included is the (new-ish) bitcoin.bash.

caesrcd commented at 9:42 PM on September 14, 2026: contributor

Tested ACK 91b96bef3465eba2c4710a3b85c77ceb5809e142

Arch Linux with bash, cmake -B build -DINSTALL_FISH_COMPLETIONS=OFF:

DrahtBot requested review from BrandonOdiwuor on Sep 14, 2026

DrahtBot requested review from ryanofsky on Sep 14, 2026