Makefile/rpm: Add shell completions #1938
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds shell completion for bash, zsh, and fish to the bootc RPM package. The implementation is straightforward, generating the completion files during the build and installing them. My review focuses on ensuring the packaging adheres to common standards. I've found a minor issue with the naming of the bash completion file, which doesn't follow the Fedora Packaging Guidelines. I've provided suggestions to correct this.
renner0e
force-pushed
the
rpm-completions
branch
from
44ebb7a to
17224a4
Compare
contrib/packaging/bootc.spec
Outdated
| %endif | ||
| %endif | ||
|
|
||
| for shell in bash zsh fish; do |
There was a problem hiding this comment.
A little bit unfortunate to hardcode the list of shells again here; the Rust code supports others too. But OTOH there's a need to deal with the hardcoded RPM macro dirs for these too.
contrib/packaging/bootc.spec
Outdated
| %endif | ||
|
|
||
| for shell in bash zsh fish; do | ||
| target/release/bootc completion $shell > target/release/bootc.$shell |
There was a problem hiding this comment.
Also: it is a goal that the build process is encapsulated in Makefile. Let's handle this the same way we do man pages, by having it be a make completion say which is part of make install by default.
There was a problem hiding this comment.
i.e. when someone makes a Debian/arch/etc package they don't need to replicate this logic.
There was a problem hiding this comment.
should be addressed with: 97f26f9
I am new to Makefiles, thanks for the patience 🙂
renner0e
force-pushed
the
rpm-completions
branch
from
97f26f9 to
2ddcb24
Compare
Signed-off-by: renner <renner0@posteo.de>
renner0e
force-pushed
the
rpm-completions
branch
from
2ddcb24 to
ddfe5bf
Compare
renner0e
changed the title
| .PHONY: completion | ||
| completion: | ||
| mkdir -p target/completion | ||
| for shell in bash elvish fish powershell zsh; do \ |
| install -d $(DESTDIR)$(prefix)/lib/bootc/install | ||
| install -D -m 0644 -t $(DESTDIR)$(prefix)/share/man/man5 target/man/*.5; \ | ||
| install -D -m 0644 -t $(DESTDIR)$(prefix)/share/man/man8 target/man/*.8; \ | ||
| install -D -m 0644 target/completion/bootc.bash $(DESTDIR)$(prefix)/share/bash-completion/completions/bootc |
There was a problem hiding this comment.
Is not consistent with this list.
There was a problem hiding this comment.
fixed with: eaf920e. I would rather not package elvish and powershell completions in the rpm due to lack of docs and ecosystem around it. I also never used them. I hope that's okay.
Signed-off-by: renner <renner0@posteo.de>
Not packaging those due to the lack of documentation on this. Signed-off-by: renner <renner0@posteo.de>
renner0e
force-pushed
the
rpm-completions
branch
from
e698772 to
eaf920e
Compare
contrib/packaging/bootc.spec
Outdated
| %endif | ||
| %{_unitdir}/* | ||
| %{_mandir}/man*/*bootc* | ||
| %if 0%{?rhel} >= 9 |
There was a problem hiding this comment.
AI: Important: This conditional appears inverted. The commit message says "old versions don't have these macros" but the condition uses explicit paths on RHEL 9+, where the macros (%{bash_completions_dir} etc.) do exist.
If the intent is to use explicit paths on old RHEL that lacks the macros, the condition should be %if 0%{?rhel} && 0%{?rhel} < 9 (old RHEL without macros) with explicit paths in that branch.
Alternatively, since the macros expand to identical paths anyway, consider just always using explicit paths and removing the conditional entirely:
%{_datadir}/bash-completion/completions/bootc
%{_datadir}/zsh/site-functions/_bootc
%{_datadir}/fish/vendor_completions.d/bootc.fish
contrib/packaging/bootc.spec
Outdated
| touch %{?buildroot}/%{_docdir}/bootc/baseimage/base/sysroot/.keepdir | ||
| find %{?buildroot}/%{_docdir} ! -type d -printf '%{_docdir}/%%P\n' | sort > bootcdoclist.txt | ||
|
|
||
| rm -f %{buildroot}/%{_prefix}/share/elvish/lib/bootc.elv |
There was a problem hiding this comment.
AI: (low) Minor inconsistency: using %{_prefix}/share here vs %{_datadir} elsewhere. Could use %{_datadir} for consistency, though functionally equivalent.
Makefile
Outdated
| cargo run --release --package xtask -- manpages | ||
|
|
||
| .PHONY: completion | ||
| completion: |
There was a problem hiding this comment.
AI: The completion target calls target/release/bootc but doesn't depend on bin. If someone runs make completion or make install without first running make bin, this will fail. Consider adding the dependency:
completion: binSigned-off-by: renner <renner0@posteo.de>
3 participants
works 🙂
