Skip to content

feat: install zsh completions - #38

Open
planger wants to merge 2 commits into
mainfrom
feat/zsh-completions
Open

feat: install zsh completions#38
planger wants to merge 2 commits into
mainfrom
feat/zsh-completions

Conversation

@planger

@planger planger commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Problem

make install and the .deb/.rpm packages generated and shipped only the bash completion script, so zsh users got no completion for enclave at all and fell back to filename completion.

The bash side works without any user setup because bash-completion 2.x ships a dynamic loader whose first search directory is a user-level XDG path (${XDG_DATA_HOME:-$HOME/.local/share}/bash-completion/completions). Zsh has no counterpart: its default fpath is compiled in and contains only root-owned directories, with no XDG user entry and no lazy loader.

Change

enclave completion zsh already emitted a correct #compdef script; it was simply never installed. Now it is:

  • make install installs _enclave to ~/.local/share/zsh/site-functions/, and make uninstall removes it.
  • .deb installs to /usr/share/zsh/vendor-completions/, .rpm to /usr/share/zsh/site-functions/. The paths differ because Debian reserves vendor-completions for distro packages and does not keep /usr/share/zsh/site-functions on fpath, while Fedora uses site-functions. Both are on the respective default fpath, so package installs need no user action.
  • scripts/verify-package-assets.sh now asserts that both completions are present in the built package, accepting either zsh layout. Completion files previously had no packaging coverage.
  • scripts/clean-state.sh knows the new install locations.

A user-level install cannot be made zero-config the way the bash one is, since no writable directory is on zsh's default fpath. Rather than editing ~/.zshrc from a build target, make install prints a single hint line with the fpath line to add, guarded by command -v zsh so it stays silent where zsh is absent and can never fail the target. The README documents the same for both shells.

Verification

  • make install-binary and make uninstall into a throwaway HOME: both completion files placed and removed, hint renders $fpath literally, silent and exit 0 when zsh is missing.
  • Installed layout works end to end: with the directory on fpath, compinit registers _comps[enclave]=_enclave, and completion offers subcommands with descriptions, per-subcommand flags (enclave ps -- gives --all, --json, --name, --tool) and enum values (--tool gives the seven tool profiles).
  • debian/rules override_dh_auto_install staged into a temp dir: verify-package-assets.sh passes, including the new assertions, and stages usr/share/zsh/vendor-completions/_enclave.
  • make build, make test, make check-license-headers all pass. The generated zsh script carries the license header, so it is covered by the header check like the bash one.

Known limitations

  • The rpm path was not exercised locally: no rpmbuild on the dev host, and debhelper was missing so make deb-quick could not run either. The packaging CI jobs cover both, and the new verify-package-assets.sh assertion will fail loudly if either path is wrong.
  • Completions are still Linux only. make install on macOS installs no completion for either shell, unchanged by this PR.

make install and the deb/rpm packages shipped only the bash completion
script, so zsh users got no completion at all: zsh does not read
bash-completion/completions.

The packages install into the directory each distro keeps on zsh's default
fpath. A user-level install has no such directory, so make install prints
the one-line fpath hint when zsh is present.

@EclipseSourceAI EclipseSourceAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Autonomous AI review.

This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. Noting why helps, since replies are read and taken into account in follow-up reviews.

Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.

To get an updated review after pushing changes, a maintainer may re-request a review from this account.

Running in Eclipse Enclave, submitted via review-guard-mcp

Installs the already-generated zsh completion script across make install, the .deb, and the .rpm, plus cleanup and packaging-verification coverage. The distro path split (vendor-completions on Debian, site-functions on Fedora) is correct: Debian's default fpath carries the former and deliberately omits /usr/share/zsh/site-functions.

Scope is tight and the tricky part was already handled on main: writeCompletionWithHeader keeps #compdef on line 1 so compinit still registers the file, with a test guarding it. make build, make test, and make lint (shellcheck plus the license-header check) all pass here, and I confirmed make install-binary / make uninstall place and remove both completions, with the zsh hint silent and exit 0 when no zsh is on PATH.

Worth a look: the new assertion in scripts/verify-package-assets.sh accepts either zsh layout, so it does not actually pin the per-distro path the way the PR description suggests. The rest are minor (hardcoded ~/.local/share vs XDG_DATA_HOME, README compinit guidance, fish parity).

Comment thread scripts/verify-package-assets.sh Outdated
Comment thread Makefile Outdated
Comment thread Makefile Outdated
Comment thread README.md Outdated
Comment thread Makefile
verify-package-assets.sh accepted either zsh layout, so a .deb shipping
into site-functions (not on Debian's fpath) or an .rpm into
vendor-completions (dead on Fedora) passed. The expected directory is now
a required argument supplied by each packaging workflow.

make install hardcoded ~/.local/share while clean-state.sh also cleans
$XDG_DATA_HOME, so the two disagreed when it was set.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants