Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cmd: Added shell completion command #840
cmd: Added shell completion command #840
Changes from all commits
199e2cb
d032172
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umm... do we need a new build flag for completions?
With this pull request, we now have several subtly different knobs for having shell completions. First, there's the
install_completions
build flag, then we have the optional dependencies onbash-completion
andfish
, but we have some fallbacks in case they are missing. Documenting those three layers is going to be needlessly more difficult in comparison to what we had before:Then we would have to test those layers.
Generally speaking, we shouldn't be adding build options and fallbacks without well-defined use-cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My aim with providing fallbacks was to make sure we're always installing in the "correct" locations. But looking at what Podman does, we could probably just drop the fallbacks and hard-code the paths reducing the number of layers.
I'd rather go with the approach of having an explicit switch for completions instead of having to require said shells.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't be correct if we are guessing. If a shell's completion machinery offers an API for the location, then we should be using only that. eg., how can we know that the right location for fish is
/usr/share/fish/completions
or/usr/share/fish/vendor_completions.d
? Any random downstream distributor or a new release of the shell can switch the location and we would be in trouble.In other words, I don't think Podman is doing the right thing. :)
There are three groups of people building Toolbx:
toolbox
binariesFor the first, it's acceptable to require the shells as a build dependency so that we can use the appropriate API to get the location for completions. However, it doesn't mean that the shells need to be run-time dependencies of
toolbox
. eg., I have fish and zsh completions installed forflatpak
andpodman
on my Fedora 36 even though I don't have thefish
andzsh
RPMs.The continuous integration case seems to be the same as the first.
Developers hacking on Toolbx, who aren't specifically working on the completions, don't need to worry about the completions at all. They likely don't even want to, because their running shells won't pick up the newly installed completions from a random prefix. Even more so if their host OS is Fedora Silverblue because they would be hacking inside a Toolbx container. So, it's fine if the build skips over the completions because the necessary build dependencies were missing.
If a developer is indeed hacking on the completions, then it's reasonable to expect them to have the necessary shells installed. Otherwise how would they test their changes? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some more digging.
Another incorrect example is Flatpak. :)
A good example to emulate would be systemd:
We should default to using the shell's API to detect the location for installing the completions, but have a build flag to override that. Just like we do for
tmpfiles_dir
.This is particularly relevant for Z shell, because it doesn't have an API and the Internet says that Debian and Fedora use different locations. A build flag for the location can also optimize the build when there's an alternate API for the location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried this in #1123