Fix/bash completion

[grunwmar]

Bash completion of rhc does not work (RHELPLAN-140295)

[GitHub grunwmar repo/branch rhc/fix/bash-completion]
Bugzilla link: https://bugzilla.redhat.com/show_bug.cgi?id=2145198

I hope the text is understandable. Excuseme for quite verbose style, however as many of us are not native speakers of English I just want to be sure that all things are explained and understood well.

Analysis

Urfave/cli

• The urfave/cli package exports apps subcommands and options as a textual list to stdout defined inside instance of its cli.App .
• This is done by calling app with option --generate-bash-completion which exports the list to stdout.
• Inside urfave/cli package there is predefined bash script urfave/cli/<version>/autocomplete/bash_autocomplete which does proper autocompletion using call of the app with the mentioned --generate-bash-completion option, so the script can get subcommands and options to show (the autocompletion calls the app with mentioned option everytime the tab key is pressed to obtain autocompletion).
• This script needs to be distributed to correct system directory "manually" and has to have a name of the app/command i.e. /usr/share/bash-completion/completions/<app_name> . The urfave/cli will not to do it by itself (see documentation).
• Another approach is to call autocompletion script directly from go package urfave/cli/<version>/autocomplete/bash_autocomplete with correctly defined PROG environmental variable in shell session (described later in Summary/Another approach section).
• Both is decribed in documentation on Urfave/cli's GitHub.
• To make autocompletion active, there has to be installed package bash-autocompletion (just by dnf install bash-completion).

RHC

• In the rhc's makefile I have found two things which I consider as the cause of malfunction.
• The Makefile creates a file rhc.bash containing list of subcommands/option for autocompletion by running rhc --generate-bash-completion > rhc.bash (github). But it means that file rhc.bash is not a bash script indeed. Then this file is delivered to the destination where the autoclompletion script should be i.e. to /usr/share/bash-completion/completions/. And of course the corresponding file for rhcd command is somewhere on Arrakis.
• In the nutshell, the Makefile makes and places to directory containing autocompletions a nonsense, but not the autocompletion script for rhc and rhcd commands.

rhc/main: Makefile [row 90]

%.bash: $(GOSRC)
go run $(BUILDFLAGS) -ldflags "$(LDFLAGS)" . --generate-bash-completion > $@

rhc/main: Makefile [row 128]

install: $(BIN) $(DATA)
	pkg-config --modversion dbus-1 || exit 1
	pkg-config --modversion systemd || exit 1
	install -D -m755 ./rhc      $(DESTDIR)$(BINDIR)/$(SHORTNAME)
	install -D -m644 ./rhc.1.gz $(DESTDIR)$(MANDIR)/man1/$(SHORTNAME).1.gz
	install -D -m644 ./rhc.bash $(DESTDIR)$(DATADIR)/bash-completion/completions/$(SHORTNAME)

Solution seems to be simple. Just to take file from urfave/cli/<version>/autocomplete/bash_autocomplete and copy it to /usr/share/bash-completion/completions/ two-times. Once as rhc once as rhcd. Then all works nicely (if bash-completion are installed).

Solution

I choose approach to just place the file
urfave/cli/<version>/autocomplete/bash_autocomplete to rhc project as rhc.bash and removing corresponding part of Makefile (rhc/main: Makefile [row 89]).

Summary

I am not fully satisfied with this solution, mainly because the urfave_cli_path.sh script I added has to be marked as executable. I found, that Urfave/cli.App loads information about the path to is bash_autocomplete to App object, but this variable is not exposed and I was unable to reach it from rhc code, although to be able to get the path direclty from Urfave/cli.App instance would be much much nicer way to solve the problem.

Another approach

using

export PROG=rhc
source urfave/cli/<version>/autocomplete/bash_autocomplete

export PROG=rhcd
source urfave/cli/<version>/autocomplete/bash_autocomplete

still requres the fill the correct <verstion> placeholder and therefore usage of something like urfave_cli_path.sh, i.e. still is needed to get correct path to the bash_autocomplete file.

Another minor issue is, that when running make install the command is complaining about missing dbus-1 package:

...
Package dbus-1 was not found in the pkg-config search path.
Perhaps you should add the directory containing `dbus-1.pc'
to the PKG_CONFIG_PATH environment variable
Package 'dbus-1', required by 'virtual:world', not found
make: *** [Makefile:125: install] Error 1

This could be easily fixed by running dnf install dbus-devel.

---

Changes were tested on RHEL 8.6 and on RHEL 9.2

[jirihnidek]

Thanks for the PR 👍 I have few questions, comments and requests.

[jirihnidek]

I would not do it in this way. This is upstream repository for rhc client. We should not have here any changes that are specific for downstream. The upstream repository of rhcd is yggdrasil. Thus I would just not add this line to this patch.

[jirihnidek]

We should introduce similar change to yggdrasil upstream repository. Note: yggdrasil uses meson build system.

[grunwmar]

Ok, I did not know this! Thx!

[jirihnidek]

I think that it is not necessary to have special file with this script, because it is used only in Makefile.

Or is this script useful for anything else?

If not, then I think that we should "move" the whole script (or the logic of the script) into the Makefile.

Reference: https://www.gnu.org/software/make/manual/html_node/One-Shell.html

[jirihnidek]

Is it actually necessary to use the latest version of the bash script from urfave Go package? The script is very simple:

#! /bin/bash

: ${PROG:=$(basename ${BASH_SOURCE})}

_cli_bash_autocomplete() {
  if [[ "${COMP_WORDS[0]}" != "source" ]]; then
    local cur opts base
    COMPREPLY=()
    cur="${COMP_WORDS[COMP_CWORD]}"
    if [[ "$cur" == "-"* ]]; then
      opts=$( ${COMP_WORDS[@]:0:$COMP_CWORD} ${cur} --generate-bash-completion )
    else
      opts=$( ${COMP_WORDS[@]:0:$COMP_CWORD} --generate-bash-completion )
    fi
    COMPREPLY=( $(compgen -W "${opts}" -- ${cur}) )
    return 0
  fi
}

complete -o bashdefault -o default -o nospace -F _cli_bash_autocomplete $PROG
unset PROG

Couldn't we just copy the script to our repository? I'm not enforcing. I'm just asking :-).

[grunwmar]

Yes, I've later discussed as better option to have the script included directly exactly because it is simple and probably will not be changed in future.

[jirihnidek]

LGTM. I have only small request. Can you please rebase and squash 8 commits into one commit?

There are many pages how to do that. E.g. here: https://stackoverflow.com/questions/5189560/how-do-i-squash-my-last-n-commits-together

I do usually this:

git reset --soft HEAD~<num_of_commits_to_be_squashed>
git commit
git push --force origin <current_branch>

You can also extend git commit with --reedit-message=<commit_hash>, when you already has some good commit message and you don't want to write commit message again.

[jirihnidek]

Note:

We will have to do another improvement in another PR after this PR will be merged.

IMHO It will be also necessary to adjust https://github.com/RedHatInsights/rhc/blob/main/util.go#L52, because bash completion script suggest hidden command line options and it is not want we want, because these hidden command line option are not documented and it could be confusing. Hidden command line options are intended only for testing purpose.

[jirihnidek]

LGTM. I think we can merge it as it is.

[subpop]

LGTM. This is similar to how I corrected it in yggdrasil: RedHatInsights/yggdrasil@eebef45