Comment 1 William Sjöblom 2022-04-13 09:04:10 CEST

I had a look at different tools for providing shell completions in a
declarative manner based on the argument parser object.

• Ahead-of-time completion script generators
  • shtab <https://github.com/iterative/shtab>

    Completion script generation for argparse. Rather complex but
    noticeably faster than the alternatives below for obvious
    reasons. Supports both bash and zsh. Could possibly benefit
    from more granular phase separation complexity-wise.

• Completion performed by the script itself
  • argcomplete <https://pypi.org/project/argcomplete/>

    Completion for argparse. Seems to be relatively widely used by
    individuals using many argparse but seldom bundled with other
    packages. Has good test coverage. Only first-class support for bash.
    Relatively slow but gives the programmer an easy time providing
    rich completions.

  • optcomplete <https://furius.ca/optcomplete/>

    Completion for optparse (the library currently used). Supports only
    Python 2 and does not look very promising at a first glance as the
    code is both unmaintained and littered with TODOs and has close to
    zero test coverage. Nonetheless it may have some good parts/ideas
    and is relatively compact. Supports both bash and zsh.


So, the main trade-off that has to be made is between performance/low
coupling and extendability/completion richness. The approach of spinning
up a python interpreter for every completion also makes supporting 
multiple different shells easier. Also, note that we are running
M.2 drives in our workstations and that the python start-up time on our
machines are very fast compared to others when reasoning about the
overhead of spinning up a python interpreter for every completion call.

Comment 9 Samuel Mannehed 2024-05-03 17:13:00 CEST

Planned flow for bash completion:
=================================

1. When in a terminal using bash, after you have written, for example,
   "$ tl-setup " and press <TAB>, bash will call its built-in function for
   bash completion.

2. Bash's completion function will check this directory for matching scripts:
   /usr/share/bash-completion/completions/

3. Bash will then find the matching completion script in that directory,
   it will also named "tl-setup" in this example.

4. These completion files are simple wrappers which contains 2 lines, first is
   to source the file /opt/thinlinc/share/optcomplete/bash_complete for the
   wrapper function called _optcomplete(), and then runs "complete -F
   _optcomplete <scriptname>".

5. This _optcomplete() function is a wrapper around the real command, i.e. the
   real tl-setup in this example. It will set a number of environment variables
   which we need later. Then run the actual tool in question. This means the
   actual tool will run every time we're autocompleting.

   This wrapper function typically has a particular shape [1], but can be
   modified to one's needs, as we have [2].

6. The actual tool (tl-setup in this example) uses ThinLinc's option parser.
   Our option parser will use the Python module "optcomplete" to produce a list
   of completions. It needs to use the environment variables set by the wrapper
   function in the previous step in order to do so.

7. It's coded in optcomplete to exit after it's done with its thing and not run
   the actual command. In our example, this means tl-setup will not actually
   start, it will exit as soon as we have the list of arguments. This works
   using the env var OPTPARSE_AUTO_COMPLETE=1 which is set in the
   _optcomplete() wrapper function.


So, the steps required to achieve bash completion are the following:
--------------------------------------------------------------------

1. Use Python's optparse module to parse your arguments. This is now the
   case since, as of bug 7889, all our tools use ThinLinc's option parser
   (which, in turn, uses Python's optparse).

2. Install the open source "optcomplete" module from Martin Blais [3].

3. Extend our ThinLinc option parser to use the optcomplete module.

4. Since subcommands are handled differently than optcomplete predicts,
   one has to account for that manually before invoking optcomplete's
   autocomplete() in our parse_args().

5. Install an _optcomplete() bash function [2] that can set the required
   environment variables and call the corresponding real Python tool (e.g
   /opt/thinlinc/sbin/tl-setup).

6. When installing our tools, we must make sure there are files in the system
   directory /usr/share/bash-completion/completions/ with corresponding names,
   e.g. /usr/share/bash-completion/completions/tl-setup. These bash-completion
   files should call our _optcomplete() bash function.

[1]: https://devmanual.gentoo.org/tasks-reference/completion/index.html
[2]: attachment 1191 [details]
[3]: https://github.com/blais/optcomplete

Comment 29 Tobias 2024-05-13 16:33:18 CEST

We've discovered that tab completion of dynamic options like agent names or
active usernames is problematic. This kind of information requires root
privileges to acquire, which is fine when running a script the normal way, e.g.

> sudo tl-notify --user tester "Attention all users"

However, when tab completing dynamic options--the username in this example--it's
being run as the base user. In other words, one has to be logged in as root to
tab complete dynamic options.

This appears to be the standard, that non-privileged users cannot tab themselves
to sensitive information.

Thus we probably have to live with this limitation and stress this root
requirement in the documentation somewhere.

Comment 30 Tobias 2024-05-14 14:49:33 CEST

All scripts--for which it is relevant--should be using our optionparser now (bug 7889), and exhibit working tab completions with proper specific completions for each options and arguments.

This should be verified for the following scripts:

• tl-limit-printers
• tl-notify
• tl-env
• tl-ldap-certalias
• tl-mount-localdrives
• tl-session-param
• tl-support
• tl-umount-localdrives
• tlctl
• tl-setup
• tl-sso-password
• tl-sso-token-passphrase
• tl-config (hivetool)

Comment 34 Tobias 2024-05-15 13:25:43 CEST

The problem mentioned in comment #29 has been broken out into a separate bug 8344.

Comment 36 Frida Flodin 2024-05-16 09:34:50 CEST

Tab competition on a fresh RHEL 9 does not work. You need to have the package 'bash-completion' installed for it to work. This package is included when installing the group "Server with GUI" so in practice it should be there. Should this be our responsibility? To clarify, tab completion does not work for any commands in this case, not only ours.

Comment 41 Frida Flodin 2024-05-17 09:23:44 CEST

(In reply to Frida Flodin from comment #36)
> Tab competition on a fresh RHEL 9 does not work. You need to have the
> package 'bash-completion' installed for it to work. This package is included
> when installing the group "Server with GUI" so in practice it should be
> there. Should this be our responsibility? To clarify, tab completion does
> not work for any commands in this case, not only ours.

After a quick discussion, we concluded that we should not do anything about this. This is the expected behavior. Some users might want bash-completion to be off due to, for example, performance or preference.

Comment 59 Alexander Zeijlon 2024-05-24 09:28:24 CEST

When working on adding unit tests for the completer functions for tl-session-param, we noticed that our option parser's handling of arguments is inconsistent between a "normal" parsing attempt during runtime, and an attempt to complete a partially written command.

More specifically, the lines below are all valid commands that run as expected when typed in the terminal without attempting to use tab-completion:
> tl-session-param --eval=MAC=/client_params/hwaddr
> tl-session-param --eval MAC=/client_params/hwaddr
> tl-session-param -e MAC=/client_params/hwaddr
But when e.g. typing a partial command and then, in this case, trying to tab-complete on the session path, no completion is made:
> tl-session-param --eval=MAC=/client_params/hw [TAB][TAB]
If you then begin the argument to --eval with a quote-character, tab-completion works.
> tl-session-param --eval="MAC=/client_params/hw [TAB][TAB]
> tl-session-param --eval="MAC=/client_params/hwaddr"
This is both unexpected and undocumented behavior. We should be able to tab-complete without having to quote the argument to an option.

Comment 61 Alexander Zeijlon 2024-05-24 14:00:14 CEST

It looks like we have made assumptions about how tab-completion works that are incorrect. The problem, as it is right now, is that the completion functionality in bash parses the partially completed command line differently from what we expect.

When [TAB] is pressed, bash allows a function to be run, specific for the command that is provided on the command line, and in the context of this function, one has access to a set of variables that are meant to be used to calculate possible completions.

The variables are:
COMP_LINE       The current command line.
COMP_POINT      The index of the cursor position in the current command line.
COMP_WORDBREAKS A string of characters that will be treated as separators.
COMP_WORDS      A bash-array containing the command line split according to
                COMP_WORDBREAKS.
COMP_CWORD      The index of the current word in COMP_WORDS, the cursor is on.
COMP_TYPE       The type of completion being attempted; [TAB], [!] or [?].

In particular, the array COMP_WORDS is important for us here, since we are looking in it in order to be able to make assumptions about which option we are attempting to provide completions for. And in the case that does not work in comment 59, the wrong part of COMP_LINE gets interpreted as an argument to tl-session-param.

The failed tab-completion attempt:
> tl-session-param --eval=MAC=/client_params/hw [TAB][TAB]
results in this COMP_WORDS:
> [ 'tl-session-param', '--eval', '=', 'MAC', '=', '/client_params/hw' ]
While the successful tab-completion attempt:
> tl-session-param --eval="MAC=/client_params/hw [TAB][TAB]
results in this COMP_WORDS:
> [ 'tl-session-param', '--eval', '=', '"MAC=/client_params/hw' ]
Hence, in the failed attempt, it looks like bash has split COMP_LINE too greedily on equal-signs. This difference in behavior is not something we expected, and we need to handle it in a robust way.

Comment 62 Alexander Zeijlon 2024-05-27 08:49:54 CEST

If we omit '=' from COMP_WORDBREAKS, it looks we get a bit closer to what we want to see.
> COMP_WORDBREAKS=${COMP_WORDBREAKS//=}
> ...
> [ 'tl-session-param', '--eval=MAC=/client_params/hw' ]
This output could be easier to look at in Python, since we then know that the option can be joined with its argument by '=' in some cases.

Comment 63 Alexander Zeijlon 2024-05-27 09:55:31 CEST

One other option would be to make use of the bash-completion package that provide a set of utility-functions for handling and simplifying completion.



I experimented some with its _init_completion-function. The result of a call to _init_completion is four variables

The variables:
cur    The current word under the cursor (all characters of the word until the
       cursor position).
prev   The previous word before the cursor.
words  An array containing the words until and including the word pointed at by
       cursor (Does not include words after the word pointed at by the cursor).
cword  Index into the words array to the word under the cursor (including
       characters after the cursor position).

When called with '-s', for splitting long options, you get a similar effect as the one seen in comment 62.

Test with '=':
> tl-session-param --eval=MAC=/client_params/hw [TAB][TAB]
Results in:
> cur = 'MAC=/client_params/hw'
> prev = '--eval'
> words = [ 'tl-session-param', '--eval=MAC=/client_params/hw' ]
> cword = 1
Test with ' ':
> tl-session-param --eval MAC=/client_params/hw [TAB][TAB]
Results in:
> cur = 'MAC=/client_params/hw'
> prev = '--eval'
> words = [ 'tl-session-param', '--eval', 'MAC=/client_params/hw' ]
> cword = 2

This is even closer to what we want to do. In both cases, $cur and $prev are pointing to the wanted words in the input.

Comment 64 Alexander Zeijlon 2024-05-27 10:02:21 CEST

_init_completion also outputs a boolean variable, $split, which tells you if $cur and $prev was split from the same word in $words.

Comment 65 Alexander Zeijlon 2024-05-27 10:26:49 CEST

A side note:

We might be able to bypass some of bash's built in completion behavior by providing COMP_LINE as input to Python during the completion attempt.

E.g.:
> ... script-name ${COMP_LINE#* }
At first glance, this may sound like a good idea, since we then could benefit from Python's own parsing of command line arguments to sys.argv. But some cases may be difficult to handle, such as if the cursor is not at the end of the line, in which case we are not interested in attempting to complete on the last word in sys.argv.

Comment 66 Samuel Mannehed 2024-05-27 14:03:17 CEST

> * Tab completion should work in other common shells like zsh, fish, etc.

This was moved to bug 8353.

Comment 89 Linn 2024-05-31 15:26:38 CEST

There has been a slight change in how tl-config reports errors. The output below shows how the format of the warning has changed:
> # Before bug 7867
> sudo /opt/thinlinc/bin/tl-config -a /vsmagent/logging
> file:///opt/thinlinc/etc/conf.d/vsmagent.hconf: line 96: Syntax error: line does not end with ]
> 
> # After bug 7867
> sudo /opt/thinlinc/bin/tl-config -a /vsmagent/logging/
> sys:1: Warning: file:///opt/thinlinc/etc/conf.d/vsmagent.hconf: line 96: Syntax error: line does not end with ]
The warnings are very similar, but the second one has a little bit of extra noise by printing "sys:1: Warning:". The noise is added by warnings.warn(), that we now use to print these warnings instead of printing them directly to stderr.

By default, warnings.warn() has even more noise in its output, but by setting the stacklevel to a high number we shouldn't exceed, we are able to bypass most of the noise by ignoring the info included in the stack trace.

Comment 111 Linn 2024-06-07 09:30:03 CEST

This bug will be tested at the same time as bug 7889.

Comment 112 Linn 2024-06-07 09:38:10 CEST

Tested scripts tl-sso-password and tl-sso-token-passphrase. These have the same arguments and functionality, except that the former is for passwords and the latter is for smart cards.

Both scripts have the following flags (none of the flags takes arguments). Tabbing works well for all of them.
>  -c	--check
>  -h	--help
>  -r	--remove
>  	--version

Comment 113 Tobias 2024-06-07 10:18:25 CEST

There's currently a bug in tab completion appearing when tab completing a word in the middle of a finished line starting with sudo, e.g. completing --user in

> sudo tl-notify --user user1 "the message"

generates a traceback,

>Traceback (most recent call last):
>  File "/opt/thinlinc/sbin/tl-notify", line 150, in <module>
>    iiI1111II ( )
>  File "/opt/thinlinc/sbin/tl-notify", line 104, in iiI1111II
>    ( iiI1 , IiII11i11II ) = I11IIII1i1I . parse_args ( )
>  File "/opt/thinlinc/modules/thinlinc/optionparse.py", line 50, in parse_args
>    self.tab_complete()
>  File "/opt/thinlinc/modules/thinlinc/optionparse.py", line 314, in tab_complete
>    cwords = oc.get_comp_words()
>  File "/opt/thinlinc/modules/thinlinc/optcomplete.py", line 239, in get_comp_words
>    if os.environ[cw] != '':
>  File "/usr/lib64/python3.9/os.py", line 679, in __getitem__
>    raise KeyError(key) from None
>KeyError: 'COMP_WORD_2'

For some reason this only appears when the line starts with sudo, but works perfectly fine otherwise.

Comment 114 Tobias 2024-06-07 11:15:54 CEST

The bug mentioned in comment #113 seems to stem from problem with bash array indexing.  With sudo in the beginning, bash appears to add an empty string to $COMP_WORDS inbetween the existing words, resulting in their indices being bumped.

However, this empty string is pruned from the array when looping over its content. Its element and index is gone, such that
> for n in "${!COMP_WORDS[@]}"; do
>     echo "COMP_WORDS[${n}]: ${COMP_WORDS[${n}]}"
> done
prints
> COMP_WORDS[0]: tl-notify
> COMP_WORDS[1]: --user
> COMP_WORDS[3]: user1
> COMP_WORDS[4]: themsg
We can solve this by looping over its content as presented by bash, and performing the indexing ourselves.

Comment 115 Linn 2024-06-07 11:27:03 CEST

Tested tl-support, tab complete works for all flags. Flag -u/--user has default value 'support', that value is also possible to tab complete.
> -h 	--help
> -p	--port
> -u	--user
> 	--version
I see the issue mentioned in comment 113, but since this happens in shared code the fix for it should affect all scripts, and hence it's enough to test it in later scripts.

Comment 117 Tobias 2024-06-07 13:54:51 CEST

The issue mentioned in comment #113 should be resolved now.

Comment 118 Tobias 2024-06-07 16:08:06 CEST

Tested tab completion for tl-setup using server build #3619 on RHEL9.

The options -a/--answers-from and -g/--generate-answers properly employs their file completer.

Comment 119 Samuel Mannehed 2024-06-07 17:02:20 CEST

Tested tab completion for tl-mount-localdrives and tl-umount-localdrives using server build 3623 on RHEL9.

Writing `tl-mount-localdrives ` or `tl-umount-localdrives ` and then tabbing correctly doesn't show any completions. This is different from how it worked in 4.16.0 where it would tab complete files in the current folder, which could be confusing.

After adding a '-' it is possible to tab complete all the options as expected. No completion is added after the first option until another '-' is typed. Looks good.

Stepping back on the commandline and completing in the middle also works well.

Comment 124 Alexander Zeijlon 2024-06-10 16:40:46 CEST

Tested tab-completion for tl-session-param using server build 3627 on RHEL 9.

Tab-completing on options works as one would expect where typing:
> tl-session-param -[TAB][TAB]
presents the user with all available options, both long and short versions.

When an option is fully completed, the cursor is placed one space after the option, such that the user can attempt to tab-complete on its argument.

Argument completion:
====================

-a/--all-entries expects a path into the directory/tree structure for the running session that is not a path to a specific value, but instead a path to a subtree. E.g. /client_params/ but not /client_params/hwaddr.

The completer function presents the user with only valid such paths.

Without any specific option provided, tab-completion will present the user with paths to specific values, e.g. to /client_params/hwaddr etc.

The completer function presents the user with only valid such paths.

-e/--eval expects its argument to be of the format VAR=/path/to/value. Since the format contains a mandatory '=', completion will not work, and is disabled until we have handled the COMP_WORDBREAKS issue. For now, this option uses NoneCompleter which means that it will not attempt to complete at all.

Comment 125 Tobias 2024-06-10 16:41:48 CEST

Tested tl-config using build #3628 on RHEL9.

All options are properly completing according to their specified completers.

The --eval option has had its completer removed since it wasn't working properly with the '=' sign present.

Comment 126 Linn 2024-06-13 15:49:56 CEST

Tested tl-ldap-certalias with Jenkins build 3630, tab complete works for all flags. None of the flags take any argument.
> -d	--debug
> -h 	--help
> -s	--simulate
> -v	--verbose
> 	--version

Comment 127 Alexander Zeijlon 2024-06-13 17:05:58 CEST

Tested tl-env using build #3630 on RHEL9.

The only option that expects an argument, --session-number, properly suggests all available display numbers for the user running the command, no display numbers are presented if the user has no running sessions.

Comment 128 Frida Flodin 2024-06-14 11:27:19 CEST

Tested tlctl using build #3632 on RHEL9. Tested all this as root due to bug 8344.

Tab completion works for:
✅ Commands "load" and "session" (not "help" bug 8365)
✅ Options -h, --help, --version
✅ Subcommand "load list", "session terminate" etc etc
✅ All options for subcommands
✅ The specific completion of SUBCLUSTER, AGENT, USERNAME and DISPLAY after the corresponding options.

Comment 130 Alexander Zeijlon 2024-06-14 13:22:58 CEST

Tested tl-notify using build #3630 on RHEL9.

The only option that expects an argument, --user, properly suggests all available users that currently have sessions running, when tl-notify is used as root.

When the tl-notify instead is used as a non-root user, it only tab-completes to the user itself.

Comment 131 Samuel Mannehed 2024-06-14 13:49:29 CEST

Tested tl-limit-printers using build 3630 on RHEL9. I also compared with 4.16.0.

The command doesn't expect any arguments and only has three options (--help, --verbose, and --version). Tab completion is disabled unless you start with a '-', this is an improvement compared to 4.16.0 where you got completions of the files in the current folder. Completion after '-' works well for all 3 options.

Comment 132 Alexander Zeijlon 2024-06-17 10:37:27 CEST

> MUST:
> * Tab completion should work in the most common shell (bash).
Tab-completion works in bash.
> * Tab completion should work for static options and subcommands for our
>   most common command line tools.
Tab-completion works for static options and subcommands for all our scripts that use our option parser.
> * Tab completion suggestions should feel reasonably fast (appear in less
>   than a second).
Tab-completion feels responsive and there is no apparent delay when attempting to complete.

> SHOULD:
> * Tab completion should work for static options and sub commands for all
>   our command line tools.
Tab-completion works for static options and subcommands for all our scripts that use our option parser.
> * Tab completion should work for common dynamic options (tl-config
>   configuration variables and folders).
Tab-completion works in most cases, but there are exceptions. Completion will stop working in cases where there are invalid characters on the command line. See bug 8354.

> COULD:
> * Tab completion should work for dynamic options like agent names and
>   logged-in usernames.
Tab-completion works for dynamic options.

Comment 133 Samuel Mannehed 2024-06-17 11:55:30 CEST

In reply to Alexander Zeijlon from comment #132)
> > SHOULD:
> > * Tab completion should work for static options and sub commands for all
> >   our command line tools.
> Tab-completion works for static options and subcommands for all our scripts
> that use our option parser.
Not all our command line tools have completion as of this bug, but the remaining cases were broken out to new bugs:

 * Wrapper commands — bug 8348
 * ThinLinc client — bug 8349
 * tl-memberof-group — bug 8350

(In reply to Alexander Zeijlon from comment #132)
> > * Tab completion should work for common dynamic options (tl-config
> >   configuration variables and folders).
> Tab-completion works in most cases, but there are exceptions. Completion
> will stop working in cases where there are invalid characters on the command
> line. See bug 8354.
> 
> > COULD:
> > * Tab completion should work for dynamic options like agent names and
> >   logged-in usernames.
> Tab-completion works for dynamic options.
Note that parts of this were broken out to new bugs:

 * Dynamic completion info is still missing in one case — bug 8350
 * Many dynamic options require root permissions — bug 8344

Comment 135 Alexander Zeijlon 2024-06-20 12:20:13 CEST

We are not consistent with other command line tools in how they handle completion of options.

It seems like the most common way to handle completion of options is to only suggest long options. This makes sense since short options are already only one key press away from being complete.

Long options are also self documenting, and hence fits better in the context of tab-completion where we want to provide meaningful output to our users.

Reopening.

Comment 137 Alexander Zeijlon 2024-06-20 12:32:11 CEST

We are no longer completing on short options.

Comment 139 Tobias 2024-06-26 09:06:33 CEST

Tested the added feature mentioned in comment #137--to not complete on short options--with server build #3640 on RHEL9.

Tested various ThinLinc scripts including our only one with subparsers: tlctl. We seem to consistently only get long options suggested by the completer.

Working as intended. Closing.