Skip to content
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

Merged
merged 2 commits into from
Feb 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,4 @@ database, etc..
See our guides on
[installing & getting started](https://containertoolbx.org/install/) with
Toolbox and [Linux distro support](https://containertoolbx.org/distros/).

100 changes: 0 additions & 100 deletions completion/bash/toolbox

This file was deleted.

41 changes: 41 additions & 0 deletions completion/generate_completions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
#!/usr/bin/env python3
#
# Copyright © 2022 Ondřej Míchal
# Copyright © 2022 Red Hat Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

import os
import subprocess
import sys

if len(sys.argv) != 3:
print('{}: wrong arguments'.format(sys.argv[0]), file=sys.stderr)
print('Usage: {} [SOURCE DIR] [COMPLETION TYPE]'.format(sys.argv[0]), file=sys.stderr)
print()
print("SOURCE DIR is path to the Toolbox Go source code")
print("COMPLETION TYPE is either 'bash', 'zsh' or 'fish'")
sys.exit(1)

source_dir = sys.argv[1]
completion_type = sys.argv[2]

try:
os.chdir(source_dir)
output = subprocess.run(['go', 'run', '.', 'completion', completion_type], check=True)
except subprocess.CalledProcessError as e:
print('{}: go run returned non-zero exit status {}'.format(sys.argv[0], e.returncode), file=sys.stderr)
sys.exit(e.returncode)

sys.exit(0)
36 changes: 36 additions & 0 deletions completion/meson.build
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
generate_completions_program = find_program('generate_completions.py')

if bash_completion.found()
bash_comp_dir = bash_completion.get_pkgconfig_variable('completionsdir')
else
bash_comp_dir = get_option('datadir') / 'bash-completion' / 'completions'
message('bash-completion not found: using', get_option('prefix') / bash_comp_dir, 'as a falback install directory')
endif

if fish.found()
fish_comp_dir = fish.get_pkgconfig_variable('completionsdir')
else
fish_comp_dir = get_option('datadir') / 'fish' / 'completions'
message('fish not found: using', get_option('prefix') / fish_comp_dir, 'as a fallback install directory')
endif

completion_bash = custom_target('bash-completion',
capture: true,
command: [generate_completions_program, meson.global_source_root() / 'src', 'bash'],
install: true,
install_dir: bash_comp_dir,
output: 'toolbox')

completion_zsh = custom_target('zsh-completion',
capture: true,
command: [generate_completions_program, meson.global_source_root() / 'src', 'zsh'],
install: true,
install_dir: get_option('datadir') / 'zsh' / 'site_functions',
output: '_toolbox')

completion_fish = custom_target('fish-completion',
capture: true,
command: [generate_completions_program, meson.global_source_root() / 'src', 'fish'],
install: true,
install_dir: fish_comp_dir,
output: 'toolbox.fish')
11 changes: 4 additions & 7 deletions meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ shellcheck = find_program('shellcheck', required: false)
skopeo = find_program('skopeo', required: false)

bash_completion = dependency('bash-completion', required: false)
fish = dependency('fish', required: false)

migration_path_for_coreos_toolbox = get_option('migration_path_for_coreos_toolbox')
profiledir = get_option('profile_dir')
Expand All @@ -32,13 +33,6 @@ if tmpfilesdir == '' or not fs.exists('/run/.containerenv')
endif
endif

if bash_completion.found()
install_data(
'completion/bash/toolbox',
install_dir: bash_completion.get_variable(pkgconfig: 'completionsdir')
)
endif

if not skopeo.found()
message('Running system tests requires Skopeo for OCI image manipulation.')
endif
Expand Down Expand Up @@ -70,5 +64,8 @@ subdir('data')
subdir('doc')
subdir('profile.d')
subdir('src')
if get_option('install_completions')
subdir('completion')
endif

meson.add_install_script('meson_post_install.py')
7 changes: 7 additions & 0 deletions meson_options.txt
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,10 @@ option(
description: 'Directory for system-wide tmpfiles.d(5) files',
type: 'string',
)

option(
'install_completions',
Copy link
Member

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 on bash-completion and fish, 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:

The following dependencies enable various optional features:
  * bash-completion

Then we would have to test those layers.

Generally speaking, we shouldn't be adding build options and fallbacks without well-defined use-cases.

Copy link
Member

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.

Copy link
Member

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. :)

I'd rather go with the approach of having an explicit switch for completions
instead of having to require said shells.

There are three groups of people building Toolbx:

  • Downstream distributors shipping toolbox binaries
  • Continuous integration
  • Developers hacking on the sources

For 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 for flatpak and podman on my Fedora 36 even though I don't have the fish and zsh 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? :)

Copy link
Member

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.

In other words, I don't think Podman is doing the right thing. :)

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.

Copy link
Member

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

description: 'Install bash, zsh and fish command completions',
type: 'boolean',
value: true,
)
Loading