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

Pipenv provides incorrect completion if current shell does not match login shell #1762

Closed
QasimK opened this issue Mar 16, 2018 · 35 comments
Closed

Comments

@QasimK
Copy link

QasimK commented Mar 16, 2018

It seems as though Pipenv does not correctly detect the fish shell when determining what to output for pipenv --completion. This is a regression that appeared within the last couple of weeks for me.

$ python -m pipenv.help output

Pipenv version: '11.8.0'

Pipenv location: '/home/qasim/.local/lib/python3.6/site-packages/pipenv'

Python location: '/usr/bin/python'

Other Python installations in PATH:

  • 2.7: /usr/bin/python2.7

  • 2.7: /usr/bin/python2.7

  • 3.6: /usr/bin/python3.6m

  • 3.6: /usr/bin/python3.6

  • 3.6.4: /usr/bin/python

  • 2.7.14: /usr/bin/python2

  • 3.6.4: /usr/bin/python3

PEP 508 Information:

{'implementation_name': 'cpython',
 'implementation_version': '3.6.4',
 'os_name': 'posix',
 'platform_machine': 'x86_64',
 'platform_python_implementation': 'CPython',
 'platform_release': '4.15.9-1-ARCH',
 'platform_system': 'Linux',
 'platform_version': '#1 SMP PREEMPT Sun Mar 11 17:54:33 UTC 2018',
 'python_full_version': '3.6.4',
 'python_version': '3.6',
 'sys_platform': 'linux'}

System environment variables:

  • COLORTERM
  • DBUS_SESSION_BUS_ADDRESS
  • DESKTOP_AUTOSTART_ID
  • DESKTOP_SESSION
  • DISPLAY
  • GDMSESSION
  • GDM_LANG
  • GNOME_DESKTOP_SESSION_ID
  • HOME
  • LANG
  • LOGNAME
  • MAIL
  • MOZ_PLUGIN_PATH
  • PATH
  • PWD
  • SESSION_MANAGER
  • SHELL
  • SHLVL
  • SSH_AUTH_SOCK
  • TERM
  • USER
  • USERNAME
  • VTE_VERSION
  • WINDOWID
  • WINDOWPATH
  • XAUTHORITY
  • XDG_CURRENT_DESKTOP
  • XDG_MENU_PREFIX
  • XDG_RUNTIME_DIR
  • XDG_SEAT
  • XDG_SESSION_DESKTOP
  • XDG_SESSION_ID
  • XDG_SESSION_TYPE
  • XDG_VTNR
  • PIP_PYTHON_PATH
  • PYTHONUNBUFFERED

Pipenv–specific environment variables:

Debug–specific environment variables:

  • PATH: /usr/local/bin:/usr/local/sbin:/usr/bin:/usr/lib/jvm/default/bin:/usr/bin/site_perl:/usr/bin/vendor_perl:/usr/bin/core_perl:/home/qasim/.local/bin
  • SHELL: /bin/bash
  • LANG: en_GB.UTF-8
  • PWD: /home/qasim

Expected result

I'd expect the completion to complete without errors :)

Actual result

When attempting to auto-complete on the first Pipenv command:

qasim@qasim-desktop ~> pipenv - (line 1): Illegal command name “_pipenv_completion()”
begin;  _pipenv_completion() {     local IFS=$'\t'     COMPREPLY=( $( env COMP_WORDS="${COMP_WORDS[*]}" \                    COMP_CWORD=$COMP_CWORD \                    _PIPENV_COMPLETE=complete-bash $1 ) )     return 0 }  complete -F _pipenv_completion -o default pipenv 
        ^
from sourcing file -
	called on line 60 of file /usr/share/fish/functions/eval.fish

in function “eval”
	called on line 1 of file ~/.config/fish/completions/pipenv.fish

from sourcing file ~/.config/fish/completions/pipenv.fish
	called on standard input

in command substitution
	called on standard input

source: Error while reading file “-”

It seems as though this is the bash code being output from <???> rather than the fish code.

Steps to replicate
  1. pip install --user --upgrade pipenv
  2. Setup fish completions inside ~/.config/fish/completions/pipenv.fish: eval (pipenv --completion)
  3. Enter fish and type Pipenv and then try to auto-complete something
@techalchemy
Copy link
Member

@QasimK what version of fish are you running?

@techalchemy
Copy link
Member

@QasimK you aren't running fish...

SHELL: /bin/bash

And your ~/.local/bin is the last thing on your path and will be pre-empted by everything else:

PATH: /usr/local/bin:/usr/local/sbin:/usr/bin:/usr/lib/jvm/default/bin:/usr/bin/site_perl:/usr/bin/vendor_perl:/usr/bin/core_perl:/home/qasim/.local/bin

To use completion with bash, use: eval "$(pipenv --completion)"

Closing for now since this seems to be an issue with which shell is running

@QasimK
Copy link
Author

QasimK commented Mar 17, 2018

@techalchemy Thank you for looking into this. I would like to assure you that I am running fish and I hope the following screenshot is sufficient evidence.

image

According to this and this, SHELL: /bin/bash is correct because bash is my login shell.

Thanks for pointing out ~/.local/bin on my path, I've changed that now.

@uranusjr
Copy link
Member

Pipenv uses SHELL to detect what shell you’re in. That’s why it does not work. It’s not the best way to do it, but for now I guess you’ll have to work around it (by changing SHELL temporarily to trick Pipenv into producing the correct output).

@kennethreitz Are you open to modify the option to take an explicit argument (e.g. --completion=fish) or detect the current shell with an additional environ (maybe PIPENV_SHELL)?

@QasimK
Copy link
Author

QasimK commented Mar 17, 2018

An explicit argument would probably be simpler, and I believe that's how pip does it. I'd be happy to create a PR for that

@QasimK
Copy link
Author

QasimK commented Mar 20, 2018

@uranusjr Would you mind re-opening this issue given that it is an issue with Pipenv and not the environment, and given that there is also a potential solution on the table

@uranusjr uranusjr reopened this Mar 20, 2018
@uranusjr uranusjr changed the title Pipenv completions error inside fish shell Pipenv provides incorrect completion if current shell does not match login shell Mar 20, 2018
@sersorrel
Copy link
Contributor

Pipenv uses click-completion, so you can use this instead:

eval (env _PIPENV_COMPLETE=source-fish pipenv)

however, it does seem like this is a Pipenv bug, if $SHELL is not actually meant to reflect the current shell.

@techalchemy
Copy link
Member

@anowlcalledjosh that’s exactly what pipenv expects $SHELL to be. The problem is if you login with some other shell, say bash, then you use run fish, for instance, but not as a login shell (i.e not with fish -l -i)

@techalchemy
Copy link
Member

Overall I would say +1 for the solution proposed by @anowlcalledjosh - closing this because pipenv really expects you to run properly configured login shells when you use it and the documentation does specifically say to specify $SHELL

@QasimK
Copy link
Author

QasimK commented Mar 23, 2018

@techalchemy Not sure if I can be any less blunt, but it seems like Pipenv is using $SHELL incorrectly. It's kind of like creating a program with the documentation that $PATH is to be the location of your documents folder?

Also, I'm not sure if there is any other way to run a "properly configured" fish without changing your default shell (which is not recommended because of the problems that causes):

image

The workaround that @anowlcalledjosh provided works great. I feel that should at least be documented if Pipenv isn't willing to make other changes.

@techalchemy
Copy link
Member

Hm arguably yes we are misusing the shell variable, how else would you suggest we get the users shell on Linux? People typically add completion in their login scripts so we haven’t encountered this. Also completion has been historically slow so I feel most people have just avoided it.

As for documentation, we need plenty of help there so PRs welcome :)

@uranusjr
Copy link
Member

uranusjr commented Mar 23, 2018

Well there are a few workarounds available without complicating the most common workflow. Allowing PIPENV_SHELL to take precedence over SHELL is one. Another would be to simply document a hack like

source $(SHELL=$(which fish) pipenv --completion)

I don’t know how to write fish scripts so this is bash, but you get the idea.

@veggiedefender
Copy link

I hacked around this by adding set SHELL /usr/bin/fish to my ~/.config/fish/config.fish. This probably breaks some other stuff, but I'm not sure what, and things still seem to be working ¯\_(ツ)_/¯

@techalchemy
Copy link
Member

@veggiedefender not only does it not break other stuff but it is recommended and is what you are supposed to have in your $SHELL variable

@QasimK
Copy link
Author

QasimK commented Apr 7, 2018

@techalchemy Earlier, I provided evidence that that is not what you're supposed to have in your $SHELL variable. Do you have any documentation that shows otherwise?

@sersorrel
Copy link
Contributor

Per POSIX, $SHELL should be set as follows:

This variable shall represent a pathname of the user's preferred command language interpreter.

That's all it says (plus a note about behaviour of "utilities" being undefined when $SHELL isn't POSIX-compatible). Fish is certainly your "preferred command language interpreter", so afaict it's totally valid for $SHELL to point to Fish.

@tsiq-oliver
Copy link
Contributor

How else would you suggest we get the users shell on Linux?

Proof of concept:

>>> import os
>>> import psutil
>>> print(psutil.Process(os.getppid()).exe())
/bin/zsh

Obviously, doesn't work in scenarios where there's an intermediate parent.

@uranusjr
Copy link
Member

uranusjr commented Apr 7, 2018

Obviously, doesn't work in scenarios where there's an intermediate parent.

Unfortunately it’s not uncommon for Pipenv to be run with an intermediate parent, especially on Windows.

@uranusjr
Copy link
Member

uranusjr commented Apr 7, 2018

This might be a good way to guess the preferred shell, however. Pipenv can always fall back to SHELL if it fails. I’d be interested to take a look if someone is interested in implementing this.

@techalchemy
Copy link
Member

We aren’t shipping psutil. I literally spent 2 days writing bindings for windows process crawling to cut psutil out of our dependencies because it causes problems for people on windows.

@techalchemy
Copy link
Member

There is really no reason to change this behavior as far as I can tell, we have enough technical complexity as is.

@veggiedefender
Copy link

veggiedefender commented Apr 7, 2018

This would definitely add some complexity but after doing some research, it looks like the only real way of identifying the current shell is to look for the existence of shell-specific environment variables such as $FISH_VERSION, $ZSH_VERSION, $BASH_VERSION, $FCEDIT, and the like. If anything, we could just fall back to using $SHELL if any of those don't exist.

@tsiq-oliver
Copy link
Contributor

tsiq-oliver commented Apr 7, 2018

Sounds like stuck between a rock and a hard place then.

If that's really the case, then the best solution would be to make this (the need to set $SHELL to one's current shell) clear in the docs. I haven't found any mention of $SHELL in the docs thus far.

@sersorrel
Copy link
Contributor

click-completion (the library Pipenv uses for autocomplete) can already try to guess the shell being used, if psutil is available. I think extending click-completion's guessing ability (presumably by environment variables as @veggiedefender mentioned), and then relying on that guessing in Pipenv, would be better than implementing anything new in Pipenv itself.

@uranusjr, it would be a ~3-line change to Pipenv to attempt to automatically detect the shell (via psutil), then fall back to parsing $SHELL if it fails. I can open a PR if you think it's worth it? Or I can open a PR against click-completion to add $SHELL-parsing there, which would simplify the code in Pipenv slightly.

@techalchemy
Copy link
Member

Thanks for the offer @anowlcalledjosh — our current stance is that we aren’t using psutil in pipenv. We literally just removed it and had to vendor pew and write c extension bindings for windows. PSUtil is slow and it breaks regularly for windows users, we just aren’t going to use it. We handle enough special edge cases, if we can’t trust your environment we aren’t really able to be responsible for maintaining logic to handle edge cases.

If there’s an approach that works around this, I would be ok with that. But in my mind the easiest approach is to set your shell variable

@uranusjr
Copy link
Member

uranusjr commented Apr 8, 2018

Also it should be worth some write-up in the documentation. Anyone care to contribute?

@samuela
Copy link

samuela commented Apr 26, 2018

I just ran into this exact problem after installing fish on the README's recommendation. Honestly, I feel like this issue does warrant further discussion given how easy it is to fall into this trap. The README strongly recommends using fish but then things immediately break down once you try to set it up.

Most elegant solution as far as I can tell is to simply have pipenv --completion take an optional parameter specifying which shell you would like the completions for. That way when you set up your .bashrc you just ask for bash completions and if you set up your fish stuff you can just ask for fish completions. I understand that $SHELL can be a tricky beast; this could be a simple workaround.

It seems like people have ragequit out of #2045 but a solution along those lines would be simple and keep everything working smoothly.

@techalchemy
Copy link
Member

Fish is recommended in the docs. Not using bash, then fish, then whatever. And as was described in one of the numerous other issues on this, you can basically do that already anyway

@samuela
Copy link

samuela commented Apr 26, 2018

And as was described in one of the numerous other issues on this, you can basically do that already anyway

@techalchemy I'm not quite sure what you mean here. Yes, it is technically possible to hack together a working installation but no, it's definitely not user friendly. In the mind of a person on macOS following the readme,

  1. I see that using fish is recommended over bash. Ok, that sounds interesting... brew install fish.
  2. Hmm, well it looks like I'm supposed to set up completions so I'll add eval (pipenv --completion) to my ~/.config/fish/completions/pipenv.fish.
  3. Sweet, now let's see what we got here!
bash$ fish
fish~> pipenv -<TAB>
(line 1): Illegal command name “_pipenv_completion()”
begin;  _pipenv_completion() {     local IFS=$'\t'     COMPREPLY=( $( env COMP_WORDS="${COMP_WORDS[*]}" \                    COMP_CWORD=$COMP_CWORD \                    _PIPENV_COMPLETE=complete-bash $1 ) )     return 0 }  complete -F _pipenv_completion -o default pipenv 
        ^
from sourcing file -
	called on line 60 of file /usr/share/fish/functions/eval.fish

in function “eval”
	called on line 1 of file ~/.config/fish/completions/pipenv.fish

from sourcing file ~/.config/fish/completions/pipenv.fish
	called on standard input

in command substitution
	called on standard input

source: Error while reading file “-”

That's just an awful user experience since they followed the exact instructions from the README. And I think the number of people that have commented and filed issues reporting this problem is some indication of its gravity.

@sersorrel
Copy link
Contributor

(argh, pressed submit too early...)

Most elegant solution as far as I can tell is to simply have pipenv --completion take an optional parameter specifying which shell you would like the completions for.

I don't think this is actually possible with Click. You can have --completion, or --completion <shell>, but not both (unless I've missed something in the docs).

@samuela
Copy link

samuela commented Apr 26, 2018

@anowlcalledjosh mmm i see... is there any way to do like --completion and --completion=<shell> or --completion and --completion --completion-shell=<shell>?

@samuela
Copy link

samuela commented Apr 26, 2018

@anowlcalledjosh Ah or what about replacing

@option(
    '--completion',
    is_flag=True,
    default=False,
    help="Output completion (to be eval'd).",
)

with something like

@option(
    '--completion',
    default=None,
    help="Output completion (to be eval'd).",
)

and then if completion is None, guess based on $SHELL, else use the provided shell.

@sersorrel
Copy link
Contributor

Unfortunately that doesn't work – if you then run pipenv, then completion is None, but if you then try to do pipenv --completion, it complains Error: --completion option requires an argument.

It looks like pallets/click#549 is tracking the lack of this feature in Click.

@samuela
Copy link

samuela commented Apr 26, 2018

@anowlcalledjosh oh right i misunderstood the docs. but it seems like --completion-shell=<shell> would still work

@hmaarrfk
Copy link

I'm just going to chime in and say that it is probably legitimate to keep the user's SHELL variable to a more common shell, like bash, or zsh as many other large pieces of software are not correctly configured to run off a fish SHELL. I see fish as a cool interactive shell, but legacy software still requires bash.

Personally, I set gnome-terminal to run the command fish instead of my SHELL and I've found that to be nice. But I ran into this issue, and was quite frustrated.

@pypa pypa locked as resolved and limited conversation to collaborators May 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants