python-poetry / poetry

Python packaging and dependency management made easy
https://python-poetry.org
MIT License
31.14k stars 2.25k forks source link

improve reliability of `poetry shell` configuring $PATH properly #497

Closed seansfkelley closed 5 years ago

seansfkelley commented 5 years ago

Issue

From https://github.com/sdispater/poetry/issues/198#issuecomment-430742299.

Preface: I'm not sure if this can be realistically solved by Poetry or if you're too much at the mercy of each person's shell configuration. But here goes.

Certain shell configurations interact poorly with poetry shell; see https://github.com/sdispater/poetry/issues/198#issuecomment-424016403 and https://github.com/sdispater/poetry/issues/172 for examples. Resolutions to the issue tend to be ad-hoc workarounds that involve spelunking in your shell configuration, see https://github.com/sdispater/poetry/issues/198#issuecomment-430742299 and https://github.com/sdispater/poetry/issues/172#issuecomment-401554154 respectively.

The root of the issue seems to be that Poetry attaches the virtualenv to the beginning of $PATH, then spawns the shell, which may override that configuration by attaching more things to the beginning of $PATH. If possible, modifying $PATH after the shell initializes ought to give a better result that will work by default on more systems. I don't recall seeing this issue with pipenv shell, but admittedly I didn't dig as deep while I was using it as my primary tool.

timonbimon commented 5 years ago

also ran into this problem (that pyenv prepends its path to $PATH after poetry does). The fix suggested by @seansfkelley (modifying $PATH after the shell initializes) would probably help :)

itsthejoker commented 5 years ago

I wrote my own solution to this. It's a hack and a mildly kludgy one, but it does work and I'm happy with it.

Requirements to implement as-is:

In ~/.config/fish/config.fish, add this to the bottom so that it runs after pyenv:

function fix_shell
    eval (python ~/fish_scripts/fix_poetry_path.py)
end

fix_shell

Adjust the above script path to whatever you want. The python script:

import os
import sys

def fix_path():
    if os.environ.get('POETRY_ACTIVE', None) is None:
        sys.exit(0)
    syspath = os.environ.get('PATH').split(':')
    # poetry always puts venvs in the local directory, so we just need
    # the first hit with .venv
    venv_index = None
    for item in syspath:
        if '.venv' in item:
            venv_index = syspath.index(item)
            break
    if venv_index is None:
        print('Cannot rearrange path; no *.venv entry found!')
        sys.exit(1)

    venv = syspath.pop(venv_index)
    syspath.insert(0, venv)

    return ':'.join(syspath)

if __name__ == '__main__':
    print('export PATH={}'.format(fix_path()))
eblume commented 5 years ago

I have hit this same issue, and I have realized that you can fix this (at least in fish) by guarding pyenv initialization with a check for the environment variable POETRY_ACTIVE.

I've included a PR to document my fix: https://github.com/sdispater/poetry/pull/678

The code is (roughly)*:

if not set -q POETRY_ACTIVE
    status --is-interactive; and source (pyenv init -|psub)
    status --is-interactive; and source (pyenv virtualenv-init -|psub)
end
betafcc commented 5 years ago

I was curious about the inconsistency of this command so I followed the bread crumbs, the OP points the right reason, but here is my walkthrough, which may be useful:

TL;DR

don't use subprocess.call('/bin/bash') AFTER setting the venv variables, it will in fact inherit the environ but the child will run the rc files after, which may or may not overshadow relevant variables, depending on the user configuration

instead, do:

  1. create the shell in a child process
  2. send source command via stdin
  3. bring up child's interaction to user

This can be done via Popen, but a reliable and portable way is using pexpect.spawn.sendline followed by pexpect.spawn.interact

Explanation

1. Chasing the source lines

1 - poetry shell calls poetry.console.main which calls Application().run(), which uses cleo's BaseApplication and wtv and finally gets to poetry.console.commands.shell.ShellCommand

2 - ShellCommand.handle runs self.env.execute(Shell.get().path)

Shell.get().path is meant to return the path of the current-running shell binary, probably no problem there

In [1]: from poetry.utils.shell import Shell
In [2]: Shell.get().path
Out[2]: '/bin/bash'  # also works in zsh and even in sh

3 - self.env.execute is the next step

self.env is created via Env.create_venv(self.poetry.file.parent, o, self.poetry.package.name) which creates VirtualEnv which finally contains the execute method that is run:

    def execute(self, bin, *args, **kwargs):
        with self.temp_environ():
            os.environ["PATH"] = self._updated_path()
            os.environ["VIRTUAL_ENV"] = str(self._path)

            self.unset_env("PYTHONHOME")
            self.unset_env("__PYVENV_LAUNCHER__")

            return super(VirtualEnv, self).execute(bin, *args, **kwargs)

You can see it sets enviroment variables and calls super().execute (which is Env.execute defined in the same file)

and there is the problem

    def execute(self, bin, *args, **kwargs):
        bin = self._bin(bin)

        return subprocess.call([bin] + list(args), **kwargs)

2. Whats wrong?

The problem is that the environment variables are set before the shell process is created, which do inherit these variables, but can overshadow in its .rc files execution startup-routine. This will be the case in every shell, maybe you're lucky and your rc files does not overshadows the relevant variables, or maybe you can work around in your rc files to guard a $POETRY_SHELL conditional but that will always be unreliable

Here's how it works:

$ which python
/home/betafcc/.pyenv/shims/python  # .bashrc activated pyenv shim

$ source "$(dirname $(poetry run which python))/activate" # chases the original venv/bin/activate and sources it
(poet-test-py3.7) $ which python
/home/betafcc/.cache/pypoetry/virtualenvs/poet-test-py3.7/bin/python  # virtualenv overshadows pyenv shim

Now notices what happens if I spawn a new bash:

(poet-test-py3.7) $ bash
(poet-test-py3.7) $

As any child process do, all environment variables are inherited, even my PROMPT (via $PS1) is still marked as in the virtualenv the result will be exactly the same if I had called bash via python's subprocess.call:

(poet-test-py3.7) $ exit
(poet-test-py3.7) $ # returns to outer bash
(poet-test-py3.7) $ python -c 'import subprocess; subprocess.call("/bin/bash")'
(poet-test-py3.7) $ # now I'm in python-called bash shell

In both cases, the rc files will be executed as they normally do every time a bash|zsh|wtv process is initiaded, therefore, overshadowing the parent-inherited environment variables:

(poet-test-py3.7) $ which python
/home/betafcc/.pyenv/shims/python

Note that my prompt still inherited the parent $PS1, as I don't have any PS1 setting in my .bashrc, but I do have pyenv's shims activation, which overshadows the venv binaries

3. Solution

In short, the core problem can be reproduced in a fresh shell:

$ source /home/betafcc/.cache/pypoetry/virtualenvs/poet-test-py3.7/bin/activate; \
> bash;
(poet-test-py3.7) $ # does activate the venv
(poet-test-py3.7) $ which python
/home/betafcc/.pyenv/shims/python # but its overshadowed by startup rc sourcing

Ie, 1 - "run the source command", 2 - "start interactive shell",

What should be done is sending the command after the startup routines:

$ # fresh shell
$ bash -i <<< 'source /home/betafcc/.cache/pypoetry/virtualenvs/poet-test-py3.7/bin/activate;
> exec </dev/tty;'
$ source /home/betafcc/.cache/pypoetry/virtualenvs/poet-test-py3.7/bin/activate; exec </dev/tty
(poet-test-py3.7) $
(poet-test-py3.7) $ which python
/home/betafcc/.cache/pypoetry/virtualenvs/poet-test-py3.7/bin/python

Note the resulted lines, it wasn't me who typed the line $ source /home/... it came from stdin, then the control came back to me via exec </dev/tty

Turns out this is the same thing pipenv shell does

    def fork_compat(self, venv, cwd, args):
        from .vendor import pexpect

        # Grab current terminal dimensions to replace the hardcoded default
        # dimensions of pexpect.
        dims = get_terminal_size()
        with temp_environ():
            c = pexpect.spawn(self.cmd, ["-i"], dimensions=(dims.lines, dims.columns))
        c.sendline(_get_activate_script(self.cmd, venv))
        if args:
            c.sendline(" ".join(args))

        # Handler for terminal resizing events
        # Must be defined here to have the shell process in its context, since
        # we can't pass it as an argument
        def sigwinch_passthrough(sig, data):
            dims = get_terminal_size()
            c.setwinsize(dims.lines, dims.columns)

        signal.signal(signal.SIGWINCH, sigwinch_passthrough)

        # Interact with the new shell.
        c.interact(escape_character=None)
        c.close()
        sys.exit(c.exitstatus)

the relevant lines are:

...
            c = pexpect.spawn(self.cmd, ["-i"], dimensions=(dims.lines, dims.columns))
        c.sendline(_get_activate_script(self.cmd, venv))
...
        # Interact with the new shell.
        c.interact(escape_character=None)

Ie, 1 - Create a interactive shell; 2 - queue the source command to be executed via stdin; 3 - bring subprocess interaction to user

betafcc commented 5 years ago

Using pipenv aproach, this probably would do as an almost drop in replacement for the subprocess.call:

import sys
import subprocess

def spawn_venv(cmd, venv_path, args=[], call_kwargs={}):
    if sys.platform == "win32":
        return subprocess.call([bin] + list(args), **call_kwargs)

    # pexpect.spawn doesn't work on windows
    # https://pexpect.readthedocs.io/en/stable/overview.html#windows
    from pexpect import spawn

    shell = spawn(cmd, list(args))

    # tell child process to source venv
    shell.sendline(get_activate_script(cmd, venv_path))

    # give child process interaction to user
    return shell.interact(escape_character=None)

# https://github.com/pypa/pipenv/blob/8707fe52571422ff5aa2905a2063fdf5ce14840b/pipenv/shells.py#L32-L54
def get_activate_script(cmd, venv):
    """Returns the string to activate a virtualenv.
    This is POSIX-only at the moment since the compat (pexpect-based) shell
    does not work elsewhere anyway.
    """
    # Suffix and source command for other shells.
    # Support for fish shell.
    if "fish" in cmd:
        suffix = ".fish"
        command = "source"
    # Support for csh shell.
    elif "csh" in cmd:
        suffix = ".csh"
        command = "source"
    else:
        suffix = ""
        command = "."
    # Escape any spaces located within the virtualenv path to allow
    # for proper activation.
    venv_location = str(venv).replace(" ", r"\ ")
    # The leading space can make history cleaner in some shells.
    return " {2} {0}/bin/activate{1}".format(venv_location, suffix, command)

Then just change subprocess.call([bin] + list(args), **kwargs) to spawn_venv(bin, self._path, args, kwargs)

This is not good enough for a PR because:

  1. you may want to set the dimensions argument in the spawn call and add a event handler for terminal resizing, pipenv uses a backport of shutil.get_terminal_size for that

  2. pipenv provides a cli flag that end's up using subprocess.call normally

  3. I don't know which **kwargs can be used, would need to normalize between subprocess.call and pexpect.spawn

  4. Maybe it breaks the simmetry between Env.execute and Env.run

xsteadfastx commented 5 years ago

I have hit this same issue, and I have realized that you can fix this (at least in fish) by guarding pyenv initialization with a check for the environment variable POETRY_ACTIVE.

I've included a PR to document my fix: #678

The code is (roughly)*:

if not set -q POETRY_ACTIVE
    status --is-interactive; and source (pyenv init -|psub)
    status --is-interactive; and source (pyenv virtualenv-init -|psub)
end
* for me, I also set PYENV_ROOT just above this guard and also update PATH as per pyenv's instructions in this guard.

thank you so much for this workaround. this kept me from dropping poetry on my first try.

dazza-codes commented 5 years ago

Heads up, stay away from PATH and the pyenv model until some shim problems are resolved, see pyenv/pyenv#1112

In a similar theme of avoiding PATH, the latest releases of miniconda no longer modify the ~/.bashrc to add conda to the PATH. The latest releases use something like an /etc/profile.d/conda.sh to load bash functions (check it with type conda for example).

seansfkelley commented 5 years ago

Should this issue be closed in favor of #571? That seems like the proper solution, and there's a lot of knowledge already split between these two tickets.

seansfkelley commented 5 years ago

Closing in favor of #571 which is arguably "more correct" (and what I do manually now, in any case!) to keep discussion in one place.

github-actions[bot] commented 6 months ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.