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

Clashes with gcloud bash/zsh completion _python_argcomplete() #310

Closed
pdecat opened this issue Jul 1, 2020 · 5 comments
Closed

Clashes with gcloud bash/zsh completion _python_argcomplete() #310

pdecat opened this issue Jul 1, 2020 · 5 comments

Comments

@pdecat
Copy link

pdecat commented Jul 1, 2020

Hi,

the _python_argcomplete() shell function defined by Google's gcloud SDK completion is clashing with the one defined by argcomplete: https://issuetracker.google.com/issues/147687442#comment12

A work-around is to patch the gcloud defined function to suffix it and avoid the clash, but that needs to be done on each update:

sudo sed -i.orig 's/_python_argcomplete/_python_argcomplete_gcloud/g' /usr/lib/google-cloud-sdk/completion.bash.inc

I'm basically looking for alternative solutions to definitively fix the issue. Any ideas?

@algorythmic
Copy link

algorythmic commented Sep 10, 2020

The fault here is definitely Google's but for a workaround that you do not have to re-apply after an update, run the sed while sourcing the file.

i.e. in your bashrc, instead of sourcing the completion.bash.inc file, add something like:

. <(sed -E 's/(^| )(_python_argcomplete)(\(\)| )/\1_gcloud\2\3/g' \
    /usr/lib/google-cloud-sdk/completion.bash.inc)

If Google ever fixes the issue by adding their own prefix/suffix to the function name, the sed above should become a no-op so it won't break.

@evanunderscore
Copy link
Collaborator

Thanks for the suggested workaround @algorythmic.

We could also fix this in argcomplete by using a unique function name per executable, which we're actually already doing for fish (though I'm not exactly sure why).

Unfortunately I don't have time to implement and test this, but I could help out with a review if someone wanted to contribute the feature.

@pdecat
Copy link
Author

pdecat commented Sep 23, 2020

I was about to try and implement what's already done for fish but for bash,
and it turns out the recent #288 (comment) and #296 PRs already provides a work-around by adding a -e/--external-argcomplete-script argument to suffix to the bash function name: https://github.com/kislyuk/argcomplete/pull/296/files#diff-bafa75d304c1cec0a49ba03fd491da57R19

That argument can be used as:

eval $(register-python-argcomplete pipx -e pipx)

It generates:

# register-python-argcomplete pipx -e pipx

# Run something, muting output or redirecting it to the debug stream
# depending on the value of _ARC_DEBUG.
# If ARGCOMPLETE_USE_TEMPFILES is set, use tempfiles for IPC.
__python_argcomplete_run() {
    if [[ -z "$ARGCOMPLETE_USE_TEMPFILES" ]]; then
        __python_argcomplete_run_inner "$@"
        return
    fi
    local tmpfile="$(mktemp)"
    _ARGCOMPLETE_STDOUT_FILENAME="$tmpfile" __python_argcomplete_run_inner "$@"
    local code=$?
    cat "$tmpfile"
    rm "$tmpfile"
    return $code
}

__python_argcomplete_run_inner() {
    if [[ -z "$_ARC_DEBUG" ]]; then
        "$@" 8>&1 9>&2 1>/dev/null 2>&1
    else
        "$@" 8>&1 9>&2 1>&9 2>&1
    fi
}

_python_argcomplete_pipx() {
    local IFS=$'\013'
    local SUPPRESS_SPACE=0
    if compopt +o nospace 2> /dev/null; then
        SUPPRESS_SPACE=1
    fi
    COMPREPLY=( $(IFS="$IFS" \
                  COMP_LINE="$COMP_LINE" \
                  COMP_POINT="$COMP_POINT" \
                  COMP_TYPE="$COMP_TYPE" \
                  _ARGCOMPLETE_COMP_WORDBREAKS="$COMP_WORDBREAKS" \
                  _ARGCOMPLETE=1 \
                  _ARGCOMPLETE_SUPPRESS_SPACE=$SUPPRESS_SPACE \
                  __python_argcomplete_run "pipx") )
    if [[ $? != 0 ]]; then
        unset COMPREPLY
    elif [[ $SUPPRESS_SPACE == 1 ]] && [[ "$COMPREPLY" =~ [=/:]$ ]]; then
        compopt -o nospace
    fi
}
complete -o nospace -o default -o bashdefault -F _python_argcomplete_pipx pipx

image

And it no longer clashes with the gcloud completion functions.

@pdecat
Copy link
Author

pdecat commented Sep 23, 2020

Anyway, submitted #320 to always suffix the bash function.

@kislyuk
Copy link
Owner

kislyuk commented Sep 27, 2020

On the face of it, this does not look like a bug in argcomplete. If I understand correctly, gcloud vendors argcomplete and ends up forcing its argcomplete installation on the rest of the system. The bash namespace is global, and we have to accept any interference from other software stomping on our names or incorrectly vendoring us and causing interference between different vendored and unvendored versions of ourselves. Perhaps the solution here is to provide an argcomplete vendoring guide, but I'm not entirely sure what we'd write there.

The solution proposed here and in #320 does not accomplish the goal of namespace isolation either between completion targets or between argcomplete versions (there are still other functions defined in the shellcode - __python_argcomplete_run and __python_argcomplete_run_inner - that share the global namespace).

I'm not at all convinced that isolating between completion targets by creating one completion function per target is the right thing to do (it seems wasteful to load all these different functions into memory if there are a lot of targets).

I am more amenable to isolating between argcomplete versions. Imagining how this would work, ALL shell functions defined by argcomplete would need to have, in their name, the version of argcomplete that installed them (for example, __python_argcomplete_1_2_3_run (and might also possibly need to include the name and/or path to the Python interpreter that installed them).

But I'm not sure if that's necessary or advisable either. I can imagine the solution above causing dependency hell types of issues and broken installations with cryptic errors when someone installs a bunch of completion shellcode and then upgrades Python or argcomplete.

I'd look for guidance to other software that has to deal with global namespacing in shellcode, including other bash-completion modules. I would welcome any pointers on how they deal with this kind of situation, if at all.

I'm closing this bug and PR for now. If you want an update to be considered here, please provide a clear design rationale and background on why it's needed, how it will prevent a problem like this from reoccurring in the future, how it will behave when many completion targets are registered, and how it will behave when Python or argcomplete are upgraded.

@kislyuk kislyuk closed this as completed Sep 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants