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

Use spack commands --format=bash to generate shell completion #14393

Merged
merged 1 commit into from
Jan 23, 2020

Conversation

tgamblin
Copy link
Member

@tgamblin tgamblin commented Jan 6, 2020

fixes #8780

This PR adds a --format=bash option to spack commands to auto-generate the Bash programmable tab completion script. It can be extended to work for other shells.

Progress:

  • Fix bug in superclass initialization in ArgparseWriter
  • Refactor ArgparseWriter (see below)
  • Ensure that output of old --format options remains the same
  • Add ArgparseCompletionWriter and BashCompletionWriter
  • Add --aliases option to add command aliases
  • Standardize positional argument names
  • Tests for spack commands --format=bash coverage
  • Tests to make sure spack-completion.bash stays up-to-date
  • Tests for spack-completion.bash coverage
  • Speed up spack-completion.bash by caching subroutine calls

Things I would like to save for future PRs:

This PR also necessitates a significant refactoring of ArgparseWriter. Previously, ArgparseWriter was mostly a single _write method which handled everything from extracting the information we care about from the parser to formatting the output. Now, _write only handles recursion, while the information extraction is split into a separate parse method, and the formatting is handled by format. This allows subclasses to completely redefine how the format will appear without overriding all of _write.

@JBlaschke
Copy link
Contributor

Thanks, I'll take a look.

It looks like fish completions are defined somewhat differently (but still tediously). I will first try existing bash-to-fish completions generators. Otherwise I can write a SpackFishCompletionsWriter based on your bash one.

I agree that we don't want to maintain 2 parallel 1000+ line scripts, and this looks like the solution.

@adamjstewart
Copy link
Member

@tgamblin This looks great! Some thoughts:

  1. I'm wondering if we should roll this into spack commands --format={bash,zsh,fish} instead of creating a new command.
  2. I think the only way this will actually be helpful is if we also autocomplete things like $(_all_packages). What I'm envisioning is a short spack-completion.{bash,zsh,fish} that mainly defines one or two functions, and then a separate file (completely auto-generated by this command) that gets sourced within it. Otherwise, there is still a lot of manual labor involved in updating the file. Would it be possible to parse the "positional arguments" to determine which autocompletion function to call? We may need to be more consistent with our positional argument naming.

If you want to focus on the concretizer, I would be happy to hack on this over the next couple weeks.

@tgamblin
Copy link
Member Author

tgamblin commented Jan 7, 2020

I'm wondering if we should roll this into spack commands --format={bash,zsh,fish} instead of creating a new command.

Sounds fine to me. I'd call the completion formats bash-completion, zsh-completion, etc., just so it's clear what's being generated.

I think the only way this will actually be helpful is if we also autocomplete things like $(_all_packages)

Yep this is what I meant by "smarts around things like completing package names".

What I'm envisioning is a short spack-completion.{bash,zsh,fish} that mainly defines one or two functions, and then a separate file (completely auto-generated by this command) that gets sourced within it. Otherwise, there is still a lot of manual labor involved in updating the file.

Or just have this command prepend the initial function definitions to the generated output -- that's what I was thinking based on how the bash completion currently looks. Up to you.

Would it be possible to parse the "positional arguments" to determine which autocompletion function to call? We may need to be more consistent with our positional argument naming.

This is exactly what I was thinking. basically if the argument name is specs do one thing and mirrors do another. I was also saying above that the command writer could help by writing hints in the command module -- not sure if you need that or if a naming convention is sufficient.

If you want to focus on the concretizer, I would be happy to hack on this over the next couple weeks.

Yes please!

@tgamblin tgamblin mentioned this pull request Jan 7, 2020
@adamjstewart
Copy link
Member

Question for everyone involved:

Previously, the tab-completion for something like spack [TAB] would run $(spack commands) to generate an up-to-date list of subcommands. With this PR in its current state, we are hard-coding that list of subcommands.

Disadvantage:

  • whenever someone adds/removes a command (infrequent), it won't tab-complete properly

Advantage:

  • spack [TAB] is significantly faster

I think this situation isn't particularly controversial, just because commands are added/removed so infrequently. But what about package names?

Currently, every time you hit spack install [TAB], spack install py-[TAB], we re-run $(spack list). At least on my system, this takes a non-negligible amount of time. How would people feel about hard-coding this list of package names? I've tried to figure out how to cache this information within Bash in the past, but never managed to figure it out. Once we're able to auto-generate the tab completion script, this should be easy to update with every release (or every commit!).

Vote 👍 for faster hard-coding and 👎 for slower but more up-to-date

@greenc-FNAL
Copy link
Member

@adamjstewart The slower, up-to-date methods for command and package completion will include extension commands and packages from configured external repos, respectively. I could go with caching based on ages of configuration files and the modification times on configured packages/ directories, but hard-wiring would seem to introduce restrictions in scope that a user of (say) a remote installation would have to internalize and take account of. One would also want to take account of environments and chains, I would have thought.

@adamjstewart
Copy link
Member

@chissg Correct, hard-coding wouldn't work for commands/packages that only exist in other repositories. Commands like spack find wouldn't be hard-coded, as there is no way to know what will be installed ahead of time. The best we could do is hard-code a list of all package names, not just installed ones.

If anyone can figure out how to cache these functions from within Bash, that would make things a lot faster.

@adamjstewart
Copy link
Member

adamjstewart commented Jan 8, 2020

Okay, I figured out how to cache the results in Bash, I think this will make everyone happy. Basically, you can do something like this:

diff --git a/share/spack/spack-completion.bash b/share/spack/spack-completion.bash
index e1b10f676d..c05b146363 100755
--- a/share/spack/spack-completion.bash
+++ b/share/spack/spack-completion.bash
@@ -87,7 +87,8 @@ function _bash_completion_spack {
     # Make sure function exists before calling it
     if [[ "$(type -t $subfunction)" == "function" ]]
     then
-        COMPREPLY=($($subfunction))
+        $subfunction
+        COMPREPLY=($(compgen -W "$SPACK_COMPREPLY" -- "$cur"))
     fi
 }
 
@@ -718,16 +719,16 @@ function _spack_info {
 function _spack_install {
     if $list_options
     then
-        compgen -W "-h --help --only -u --until -j --jobs --overwrite
+        SPACK_COMPREPLY="-h --help --only -u --until -j --jobs --overwrite
                     --keep-prefix --keep-stage --dont-restage --use-cache
                     --no-cache --cache-only --show-log-on-error --source
                     -n --no-checksum -v --verbose --fake --only-concrete
                     -f --file --clean --dirty --test --run-tests
                     --log-format --log-file --help-cdash -y --yes-to-all
                     --cdash-upload-url --cdash-build --cdash-site
-                    --cdash-track --cdash-buildstamp" -- "$cur"
+                    --cdash-track --cdash-buildstamp"
     else
-        compgen -W "$(_all_packages)" -- "$cur"
+        _all_packages
     fi
 }
 
@@ -1347,7 +1348,11 @@ function _subcommands {
 }
 
 function _all_packages {
-    spack list
+    if [[ -z $SPACK_ALL_PACKAGES ]]
+    then
+        SPACK_ALL_PACKAGES=$(spack list)
+    fi
+    SPACK_COMPREPLY=$SPACK_ALL_PACKAGES
 }
 
 function _all_resource_hashes {

Caching wasn't working before because we were running each function in a subshell, and you can't pass variables from a subshell to a supershell. By running everything in the current shell, we can cache the results of each call to spack list for the rest of that Bash session. I'll wait to update this until I get this PR working and we can auto-generate the whole thing.

@adamjstewart adamjstewart force-pushed the features/autogenerate-completion branch from bc1190a to 0dc19c6 Compare January 8, 2020 18:51
@adamjstewart
Copy link
Member

adamjstewart commented Jan 8, 2020

For the record, if we want to unit test the Bash completion script, we can do something like https://brbsix.github.io/2015/11/29/accessing-tab-completion-programmatically-in-bash/

@adamjstewart adamjstewart changed the title commands: add spack completion command to generate shell completion Use spack commands --format=bash to generate shell completion Jan 8, 2020
Copy link
Member

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

Still need to replace positionals with calls to subfunctions, but this is pretty close now!

lib/spack/llnl/util/argparsewriter.py Show resolved Hide resolved
str: the function definition beginning
"""
name = prog.replace('-', '_').replace(' ', '_')
return '_{0} () {{'.format(name)
Copy link
Member

Choose a reason for hiding this comment

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

I went with the POSIX-compliant function definition so this won't need to be changed for most shells.

lib/spack/spack/test/cmd/commands.py Show resolved Hide resolved
@@ -0,0 +1,194 @@
# Copyright 2013-2020 Lawrence Livermore National Security, LLC and other
Copy link
Member

Choose a reason for hiding this comment

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

How do we get GitHub/vim to recognize this filetype, add a shebang or use .gitattributes?

Copy link
Member

Choose a reason for hiding this comment

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

.gitattributes works for GitHub but not for vim. # vim: set filetype=sh : didn't work for me locally, but maybe I don't have modelines enabled. A shebang worked, but since this script is sourced, I'm not sure if it should have a shebang.

Copy link
Member

Choose a reason for hiding this comment

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

We could also replace the extension with .bash or .sh, but then it isn't clear what the difference is between this file and the actual file you source.

done
}

complete -o bashdefault -o default -F _bash_completion_spack spack
Copy link
Member

Choose a reason for hiding this comment

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

Git's tab completion also uses -o bashdefault which we weren't using before. I tested this with the system Bash on macOS (3.2.57) and Spack-installed Bash (5.0.11).

share/spack/spack-completion.bash Outdated Show resolved Hide resolved
@adamjstewart
Copy link
Member

adamjstewart commented Jan 10, 2020

How do we want to handle the translation of positional args to Bash subroutines? I see three possible avenues:

  1. Decide on a consistent positional arg naming scheme. We could either rely on the option name, dest, or metavar. Right now, we're using a pretty random mix of package, PACKAGE, packages, pkg, spec, specs, constraints, name. I propose the following standardization:

    • Commands that only accept package names use "package"
    • Commands that accept any kind of spec use "spec" or "specs"
    • Commands that require an installed package/spec are prefaced with "installed_"
    • Always lowercase
  2. The other option would be to introduce a module-level variable like level, which we currently use to decide whether a command belongs in the short docs or the long docs. The problem with this is that it works for things like spack install, but not for spack compiler where there are subsubcommands with different completions. I don't see a way to introduce a subparser-level variable, but let me know if I'm missing something.

  3. Screw it, always complete all commands like this with a list of Spack packages, not necessarily installed packages.

@greenc-FNAL
Copy link
Member

greenc-FNAL commented Jan 10, 2020

@adamjstewart I'm in favor of 1, specifically:
Name all the relevant bash completion helper functions such that the metavar can be obtained algorithmically, viz. _ch_<x>s? -> <x> ('s' being an optional plural). That way, there's a rule to be followed when adding new helper functions for other types of completion. In the rare case when a command writer doesn't want completion, they can use X instead, remaining consistency with the semantics of the metavar but disabling completion. This is better than 2 IMO because the latter won't deal with complex subcommands with multiple positional arguments with different completion semantics if I understand you correctly.

@adamjstewart adamjstewart force-pushed the features/autogenerate-completion branch from 9926e86 to ed55a9f Compare January 10, 2020 17:38
@tgamblin
Copy link
Member Author

I also like (1), except this part:

Never make option name plural

Some commands can take multiple specs/package names/etc., and some can only take one. I think that should be understandable from the command line options, and if we're consistent about it users will have an easier time understanding the different commands.

It doesn't have to be relevant for completion, though -- the completion can just know that both of, e.g., installed_spec and installed_specs imply the same type of completion.

@adamjstewart
Copy link
Member

adamjstewart commented Jan 10, 2020

@tgamblin there are a couple of ways to handle the plural thing. For a command like spack find that accepts multiple specs, we currently use nargs=argparse.REMAINDER, which causes the help message to be formatted like:

usage: spack find [-hdplLcfumvMN] [--format FORMAT | --json] [--groups]
                  [--no-groups] [-t TAGS] [--show-full-compiler] [-x | -X]
                  [--deprecated] [--only-deprecated] [--start-date START_DATE]
                  [--end-date END_DATE]
                  ...

list and search installed packages

positional arguments:
  constraint            constraint to select a subset of installed packages

If we change that to nargs='+', we get:

usage: spack find [-hdplLcfumvMN] [--format FORMAT | --json] [--groups]
                  [--no-groups] [-t TAGS] [--show-full-compiler] [-x | -X]
                  [--deprecated] [--only-deprecated] [--start-date START_DATE]
                  [--end-date END_DATE]
                  constraint [constraint ...]

list and search installed packages

positional arguments:
  constraint            constraint to select a subset of installed packages

When we use the former, something like constraints may make sense, but if we use the latter, constraint makes more sense. What do you think about that?

EDIT: In my own code, when I run into this situation, I usually make dest plural and metavar singular so that I access my variables via args.things and the help message contains thing [thing ...].

EDIT: Now I remember why argparse.REMAINDER was necessary, nargs='+' doesn't allow things like hdf5 -mpi because it interprets -mpi as optional flags. So you're right, we will need plurals.

contains 'python' _spack_completions spack extensions ''

# XFAIL: Fails for Python 2.6 because pkg_resources not found?
#contains 'compilers.py' _spack_completions spack test ''
Copy link
Member

Choose a reason for hiding this comment

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

This is likely due to #9034

@adamjstewart
Copy link
Member

adamjstewart commented Jan 20, 2020

How can I see coverage for spack-completion.bash? When I click on the coverage tests, it won't display coverage for spack-completion.bash because there's no data in develop to compare to.

EDIT: nvm, found it: https://codecov.io/gh/spack/spack/src/features%2Fautogenerate-completion/share/spack/spack-completion.bash

Any idea why it doesn't consider function definitions to be covered?

EDIT: This was likely fixed by SimonKagstrom/kcov#258. The version of kcov in Travis is too old, so I may have to swap foo () for foo() unless we want to wait for it to be updated.

Any idea why that one if-statement claims not to be covered?

EDIT: Putting things on a single line gets us to 100% coverage!

@adamjstewart
Copy link
Member

Alright, we now cache all subroutine calls in our tab completion. This makes repeated tab completion of the same command, like spack install [TAB], spack install py-[TAB], spack install py-num[TAB] significantly faster.

Unfortunately, the completion-test.sh don't run any faster because each succeeds and contains call runs in a subshell, so the variables are deleted when the shell is deleted. This means that completion-test.sh is still really slow (takes about 2 minutes). It's a shame to add 2 minutes to all Travis runtimes, but I agree with running unit tests in a subshell to prevent variable leaking.

set -u

# Source setup-env.sh before tests
. share/spack/setup-env.sh
. "$SHARE_DIR/setup-env.sh"
Copy link
Member

Choose a reason for hiding this comment

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

Previously this script only worked if you ran it from $SPACK_ROOT

_config_sections() {
if [[ -z "${SPACK_CONFIG_SECTIONS:-}" ]]
then
SPACK_CONFIG_SECTIONS="compilers mirrors repos packages modules config upstreams"
Copy link
Member

Choose a reason for hiding this comment

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

See #14474

_extensions() {
if [[ -z "${SPACK_EXTENSIONS:-}" ]]
then
SPACK_EXTENSIONS="aspell go-bootstrap go icedtea jdk kim-api lua matlab mofem-cephas octave openjdk perl python r ruby rust tcl yorick"
Copy link
Member

Choose a reason for hiding this comment

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

See #14473

else
compgen -W "regenerate enable disable" -- "$cur"
SPACK_COMPREPLY=""
Copy link
Member

Choose a reason for hiding this comment

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

This is something I still need to fix. The actual positional arg is {regenerate,enable,disable}. If we reimplement this using subcommands, the problem goes away.

else
compgen -W "$(installed_packages)" -- "$cur"
Copy link
Member

Choose a reason for hiding this comment

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

This was a bug, we were missing the leading underscore. Same below for _spack_gpg_sign.

@adamjstewart
Copy link
Member

@tgamblin There are still a few bugs to work out, but this PR should be ready for review. Note that the spack-completion.bash tests increase our coverage by a whole 0.5%, bumping us up to 78% coverage overall.

Copy link
Member Author

@tgamblin tgamblin left a comment

Choose a reason for hiding this comment

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

@adamjstewart: a couple minor change requests and I think this is ready to merge.


def format(self, prog, description, usage,
positionals, optionals, subcommands):
string = self.begin_command(prog)
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be a StringIO

Copy link
Member

Choose a reason for hiding this comment

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

What's the benefit of using StringIO? I've never used it before.

Copy link
Member Author

Choose a reason for hiding this comment

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

usage (str): the command usage
positionals (list): list of positional arguments
optionals (list): list of optional arguments
subcommands (list): list of subcommand parsers
Copy link
Member Author

Choose a reason for hiding this comment

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

These 6 args are kind of begging to be a little ParsedArgs data class so that we don't have to pass all 6 around.

lib/spack/spack/cmd/common/arguments.py Show resolved Hide resolved
@tgamblin
Copy link
Member Author

tgamblin commented Jan 22, 2020

@adamjstewart: when you push this, can you squash the commits into a single one (or a few) by you that I can rebase? If I squash this PR, it'll credit the work to me (as I opened the bare-bones prototype)

tgamblin added a commit that referenced this pull request Jan 23, 2020
This PR adds a `--format=bash` option to `spack commands` to
auto-generate the Bash programmable tab completion script. It can be
extended to work for other shells.

Progress:

- [x] Fix bug in superclass initialization in `ArgparseWriter`
- [x] Refactor `ArgparseWriter` (see below)
- [x] Ensure that output of old `--format` options remains the same
- [x] Add `ArgparseCompletionWriter` and `BashCompletionWriter`
- [x] Add `--aliases` option to add command aliases
- [x] Standardize positional argument names
- [x] Tests for `spack commands --format=bash` coverage
- [x] Tests to make sure `spack-completion.bash` stays up-to-date
- [x] Tests for `spack-completion.bash` coverage
- [x] Speed up `spack-completion.bash` by caching subroutine calls

This PR also necessitates a significant refactoring of
`ArgparseWriter`. Previously, `ArgparseWriter` was mostly a single
`_write` method which handled everything from extracting the information
we care about from the parser to formatting the output. Now, `_write`
only handles recursion, while the information extraction is split into a
separate `parse` method, and the formatting is handled by `format`. This
allows subclasses to completely redefine how the format will appear
without overriding all of `_write`.

Co-Authored-by: Todd Gamblin <tgamblin@llnl.gov>
@tgamblin tgamblin force-pushed the features/autogenerate-completion branch from 572773f to 48f5abd Compare January 23, 2020 01:51
tgamblin added a commit that referenced this pull request Jan 23, 2020
This PR adds a `--format=bash` option to `spack commands` to
auto-generate the Bash programmable tab completion script. It can be
extended to work for other shells.

Progress:

- [x] Fix bug in superclass initialization in `ArgparseWriter`
- [x] Refactor `ArgparseWriter` (see below)
- [x] Ensure that output of old `--format` options remains the same
- [x] Add `ArgparseCompletionWriter` and `BashCompletionWriter`
- [x] Add `--aliases` option to add command aliases
- [x] Standardize positional argument names
- [x] Tests for `spack commands --format=bash` coverage
- [x] Tests to make sure `spack-completion.bash` stays up-to-date
- [x] Tests for `spack-completion.bash` coverage
- [x] Speed up `spack-completion.bash` by caching subroutine calls

This PR also necessitates a significant refactoring of
`ArgparseWriter`. Previously, `ArgparseWriter` was mostly a single
`_write` method which handled everything from extracting the information
we care about from the parser to formatting the output. Now, `_write`
only handles recursion, while the information extraction is split into a
separate `parse` method, and the formatting is handled by `format`. This
allows subclasses to completely redefine how the format will appear
without overriding all of `_write`.

Co-Authored-by: Todd Gamblin <tgamblin@llnl.gov>
@tgamblin tgamblin force-pushed the features/autogenerate-completion branch from 48f5abd to b989e52 Compare January 23, 2020 04:36
This PR adds a `--format=bash` option to `spack commands` to
auto-generate the Bash programmable tab completion script. It can be
extended to work for other shells.

Progress:

- [x] Fix bug in superclass initialization in `ArgparseWriter`
- [x] Refactor `ArgparseWriter` (see below)
- [x] Ensure that output of old `--format` options remains the same
- [x] Add `ArgparseCompletionWriter` and `BashCompletionWriter`
- [x] Add `--aliases` option to add command aliases
- [x] Standardize positional argument names
- [x] Tests for `spack commands --format=bash` coverage
- [x] Tests to make sure `spack-completion.bash` stays up-to-date
- [x] Tests for `spack-completion.bash` coverage
- [x] Speed up `spack-completion.bash` by caching subroutine calls

This PR also necessitates a significant refactoring of
`ArgparseWriter`. Previously, `ArgparseWriter` was mostly a single
`_write` method which handled everything from extracting the information
we care about from the parser to formatting the output. Now, `_write`
only handles recursion, while the information extraction is split into a
separate `parse` method, and the formatting is handled by `format`. This
allows subclasses to completely redefine how the format will appear
without overriding all of `_write`.

Co-Authored-by: Todd Gamblin <tgamblin@llnl.gov>
@tgamblin tgamblin force-pushed the features/autogenerate-completion branch from b989e52 to bc438ee Compare January 23, 2020 04:46
@tgamblin tgamblin merged commit 11f2b61 into develop Jan 23, 2020
@tgamblin tgamblin deleted the features/autogenerate-completion branch January 23, 2020 05:31
@tgamblin tgamblin added this to Todo in Spack v0.14.0 release via automation Feb 23, 2020
@tgamblin tgamblin moved this from Todo to Done in Spack v0.14.0 release Feb 23, 2020
@adamjstewart adamjstewart mentioned this pull request Mar 31, 2020
@haampie
Copy link
Member

haampie commented Jan 6, 2022

This PR broke completion of flags:

$ spack install --ver[tab] zlib
$ spack install - zlib

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Automate the creation of the bash completion file
5 participants