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

Broken shell completion - Bash #962

Closed
DanHam opened this issue Apr 18, 2022 · 10 comments · Fixed by #1688
Closed

Broken shell completion - Bash #962

DanHam opened this issue Apr 18, 2022 · 10 comments · Fixed by #1688
Assignees
Labels
bug Something isn't working

Comments

@DanHam
Copy link
Contributor

DanHam commented Apr 18, 2022

What happened:

I get an error when I attempt to source the generated Bash shell completions:

$ source <(syft completion bash)
complete: usage: complete [-abcdefgjksuv] [-pr] [-DEI] [-o option] [-A action] [-G globpat] [-W wordlist] [-F function] [-C command] [-X filterpat] [-P prefix] [-S suffix] [name ...]

How to reproduce it (as minimally and precisely as possible):

$ source <(syft completion bash)

Anything else we need to know?:

Environment:

  • Output of syft version: syft 0.44.1

This also occurs with 0.44.0 and 0.43.2

  • OS (e.g: cat /etc/os-release or similar):
$ cat /etc/os-release
NAME="Fedora Linux"
VERSION="35 (Cinnamon)"
ID=fedora
VERSION_ID=35
VERSION_CODENAME=""
PLATFORM_ID="platform:f35"
PRETTY_NAME="Fedora Linux 35 (Cinnamon)"
ANSI_COLOR="0;38;2;60;110;180"
LOGO=fedora-logo-icon
CPE_NAME="cpe:/o:fedoraproject:fedora:35"
HOME_URL="https://fedoraproject.org/"
DOCUMENTATION_URL="https://docs.fedoraproject.org/en-US/fedora/f35/system-administrators-guide/"
SUPPORT_URL="https://ask.fedoraproject.org/"
BUG_REPORT_URL="https://bugzilla.redhat.com/"
REDHAT_BUGZILLA_PRODUCT="Fedora"
REDHAT_BUGZILLA_PRODUCT_VERSION=35
REDHAT_SUPPORT_PRODUCT="Fedora"
REDHAT_SUPPORT_PRODUCT_VERSION=35
PRIVACY_POLICY_URL="https://fedoraproject.org/wiki/Legal:PrivacyPolicy"
VARIANT="Cinnamon"
VARIANT_ID=cinnamon
@DanHam DanHam added the bug Something isn't working label Apr 18, 2022
@spiffcs spiffcs added this to OSS Apr 27, 2022
@spiffcs spiffcs moved this to In Progress in OSS Apr 27, 2022
@spiffcs spiffcs self-assigned this Apr 27, 2022
@spiffcs
Copy link
Contributor

spiffcs commented Apr 27, 2022

@DanHam we recently merged a PR that addresses this bug. If you run from main (source <(go run cmd/syft/main.go completion bash) there should be no more error.
Screen Shot 2022-04-27 at 5 47 45 PM

@DanHam
Copy link
Contributor Author

DanHam commented Apr 30, 2022

@spiffcs

Did #965 make its way into v0.48.0 of syft? If so, I'm afraid the issue is still not fixed.

$ syft version
Application:        syft
Version:            0.45.0
JsonSchemaVersion:  3.2.2
BuildDate:          2022-04-29T15:47:45Z
GitCommit:          36973021fad57baf443ffa88181394b87ce109a0
GitDescription:     v0.45.0
Platform:           linux/amd64
GoVersion:          go1.18.1
Compiler:           gc

$ source <(syft completion bash)
complete: usage: complete [-abcdefgjksuv] [-pr] [-DEI] [-o option] [-A action] [-G globpat] [-W wordlist] [-F function] [-C command] [-X filterpat] [-P prefix] [-S suffix] [name ...]

While I'm sure the command you posted above works, it requires Go to be installed. The command then downloads may GB's of Go packages and takes many minutes to complete. As such, this is not really a practical solution for many (all?) end users.

I'm assuming you meant this as a temporary work around until this is fixed?

@spiffcs
Copy link
Contributor

spiffcs commented May 4, 2022

I'm so sorry @DanHam for the miscommunication in my comment.

#965 did make it into the latest release.

The command run in the above comment was just running syft off of the development branch before compiling. Users should not have to have to have Golang installed to use the program.

It looks like the latest release is generating a completion file:
Screen Shot 2022-05-04 at 2 02 00 PM

When I switch my shell from fish => bash-3.2 I get completion to load correctly.
Screen Shot 2022-05-04 at 2 06 18 PM

I also ran the help command and tried out a couple of the different recommendations:
Screen Shot 2022-05-04 at 2 08 04 PM

The completion file itself being generated does seem to have a few bugs atm and I'll take a look at that portion when I have some more time.

It looks like on your end your session is disliking:

if [[ $(type -t compopt) = "builtin" ]]; then
    complete -o default -F __start_
else
    complete -o default -o nospace -F __start_
fi

complete command output on my current machine

complete: usage: complete [-abcdefgjksuv] [-pr] [-o option] [-A action] [-G globpat] [-W wordlist] [-P prefix] [-S suffix] [-X filterpat] [-F function] [-C command] [name ...]

It looks like there might be some disparity between the versions of complete being used since the above help printout differs slightly from what you posted.

Running fedora:35 locally i get type -t compopt == bultin for the check above
complete -o default -F __start_ then fails.

I tried it out by writing a short no-op completion script

# dumb.bash
__start_()
{
    local cur prev words cword split
    local nouns=()
    local noun_aliases=()
}

complete -o default -F __start_

source dumb.bash
complete: usage: complete [-abcdefgjksuv] [-pr] [-DEI] [-o option] [-A action] [-G globpat] [-W wordlist] [-F function] [-C command] [-X filterpat] [-P prefix] [-S suffix] [name ...]

^ Same failure you're seeing which I could not replicate on my local OSX distribution.

I think the problem is with -o default but man complete doesn't give me much to work with on fedora:35 atm in the docker container. I can continue debugging later to see if others have solved this for your distribution across other tools.

@DanHam
Copy link
Contributor Author

DanHam commented May 5, 2022

I'm so sorry @DanHam for the miscommunication in my comment.

Not a problem. Thank you for taking the time to look into this for us.

The completion file itself being generated does seem to have a few bugs atm and I'll take a look at that portion when I have some more time.

Great - hopefully we'll see a fix for this soon.

Thanks again for all the time you've spent looking into this (and improving syft in general).

@julien-carsique-sonarsource

The complete command is missing the name, even if it should be optional according to the documentation.
The bug is resolved by adding at the beginning

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

and editing the end with

if [[ $(type -t compopt) = "builtin" ]]; then
    complete -o default -F __start_ $PROG
else
    complete -o default -o nospace -F __start_ $PROG
fi
unset PROG

I don't know if it's a problem in github.com/spf13/cobra or with the called options.
Tested with GNU bash, version 5.0.17

@spiffcs spiffcs moved this from In Progress (Actively Resolving) to Triage (Comments or Progress Made) in OSS May 26, 2022
@tgerla tgerla removed the status in OSS Feb 16, 2023
@tgerla
Copy link
Contributor

tgerla commented Mar 2, 2023

Hi @DanHam, sorry for the very long delay replying to this ticket. Did you happen to see @julien-carsique-sonarsource's comment above with a patch for the completion script? It sounds like the full fix may require a change to the upstream library (cobra) which creates the completion script. Thanks for any updates if you have them!

@tgerla tgerla moved this to Awaiting Response in OSS Mar 2, 2023
@DanHam
Copy link
Contributor Author

DanHam commented Mar 16, 2023

Hi @tgerla,

I did see the comment and explanation from @julien-carsique-sonarsource - thanks Julien!

Rather than installing the completion script in e.g. /usr/share/bash-completion/completions, I normally just source the generated completion script in my .bashrc: source <(syft completion bash)

Following Julien's explanation of the root cause above, I was able to get completions working using the following in my .bashrc:

source <(syft completion bash | sed -e '/complete -o default/ {/"syft"/ !s/$/ syft/}')

The sed command just adds 'syft' to the end of the complete commands at the end of the completion script. Completion then works as expected

@tgerla tgerla removed the status in OSS Mar 16, 2023
@tgerla tgerla assigned tgerla and unassigned spiffcs Mar 16, 2023
@tgerla
Copy link
Contributor

tgerla commented Mar 16, 2023

Hi @DanHam, thank you for confirming. I think the best thing we can do in the short term is update the documentation with this workaround. We'll go ahead and put this issue in the backlog. Thanks again!

@tgerla tgerla moved this to Backlog in OSS Mar 16, 2023
@cschug
Copy link

cschug commented Mar 21, 2023

I'm not sure if spf13/cobra is used correctly here. While the workarounds given above lead to syntactically correct shell code I'm not convinced it is the correct fix. I'm not a Go developer but by just looking at the source code it looks like the shell code for Bash completion is generated in a gigantic fmt.Sprintf() call, see here and basically everywhere (and that's a lot of places!) this bracketed expression %[1]s is being expanded to an empty string.

At the last block we were talking about above

if [[ $(type -t compopt) = "builtin" ]]; then
    complete -o default -F __start_%[1]s %[1]s
else
    complete -o default -o nospace -F __start_%[1]s %[1]s
fi

this is most obvious and leads to the syntax error. But for correctness I guess also all other placed need to be expanded correctly. If I for a short test just hardcode an additional Use in here like

diff --git a/cmd/syft/cli/commands.go b/cmd/syft/cli/commands.go
index cc80b97a..39b1b404 100644
--- a/cmd/syft/cli/commands.go
+++ b/cmd/syft/cli/commands.go
@@ -55,6 +55,7 @@ func New() (*cobra.Command, error) {

        // rootCmd is currently an alias for the packages command
        rootCmd := &cobra.Command{
+               Use:           "syft",
                Short:         packagesCmd.Short,
                Long:          packagesCmd.Long,
                Args:          packagesCmd.Args,

it looks to me it has some positive effect on properly rendering the Bash shell expansion code. But as said, I'm not a Go developer and do not really see what side-effects and other implications that has. So I'm not really sure what I'm doing, but would suggest to dig in that direction.

@DanHam
Copy link
Contributor Author

DanHam commented Mar 22, 2023

@cschug is 100% right! While our workarounds 'work' they are definitely not the right fix.

After reading the comment above I did a bit of testing locally to try and fix this. Although I wouldn't consider myself a Go developer either, everything seemed to be fixed / working as expected so I took the plunge and opened a PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants