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

Avoid using pexpect in poetry shell #18

Open
kislyuk opened this issue Jun 25, 2021 · 5 comments
Open

Avoid using pexpect in poetry shell #18

kislyuk opened this issue Jun 25, 2021 · 5 comments

Comments

@kislyuk
Copy link

kislyuk commented Jun 25, 2021

poetry shell has a surprising behavior of spawning a pexpect wrapper which proxies I/O streams between the user and the application. This makes poetry shell unsuitable as a general purpose shell configuration tool: aside from the performance implications, pexpect does not deal correctly with programs that directly access the tty to interact with the terminal (and was never intended to be a full blown terminal application proxy).

It would be much better if poetry shell could simply spawn the shell via os.exec() after configuring the environment, to avoid having to proxy anything. This would be more consistent with user expectations and would prevent applications from breaking when running inside poetry shell.

@neersighted
Copy link
Member

FWIW this is very much possible, but it runs into the problems of Pipenv's 'fancy shell.' On the Pipenv side of things, they meant to make fancy the default, but so many users have their shell misconfigured that it's a crapshoot if it works.

Specifically, the issue is thus:

  • Poetry sets VIRTUAL_ENV and PATH
  • A user modifies their PATH by prepending in ~/.bashrc/~/.zshrc, which runs on every interactive session.
  • In the new shell, the virtual env is no longer first in the PATH and the user is very frustrated with Poetry.

In order for this model to work, the user's shell configuration needs to be well-behaved and only prepend in the root shell. Typically, but not always, said root shell is --login/-l. Most of the time modifying PATH in ~/.bash_profile/~/.zprofile is sufficient, but this has tradeoffs like requiring a new login session to make changes on Linux (as most of the time the user session is started as the child of a login shell, and new terminals do not run new login shells).

There are also some situations in which $SHLVL = 0 is better to check, but those are a bit niche.


I personally would like to see this, as it would reduce a lot of complexity and make many things simpler in Poetry. However, the current imperfect usage of pexpect is because most users cannot be bothered to understand the finer points of shell configuration and blame Poetry if things don't "just work." There is a reason that the (detested by many users, hence the existence of poetry shell) source /path/to/activate/deactivate method used by 'plain' virtual environments is still the standard.

All this being said, I wouldn't mind a breaking change in a major version where we go ahead and make poetry shell when we detect a badly-behaved user configuration an error, and tell the user they must fix their shell config or be stuck using source /path/to/activate only.

@neersighted
Copy link
Member

(would love some opinions on this from @python-poetry/core as well)

@dimbleby
Copy link

dimbleby commented Oct 23, 2022

personally I never use poetry shell and consider it one of the parts of poetry that, if I were king of the world, would be removed altogether. More trouble than it's worth IMO.

I don't expect this view to carry the day.

@Secrus
Copy link
Member

Secrus commented Oct 24, 2022

I generally agree with dimbleby, but that would be a big change in CLI API. Overall, the shell handling needs redesign anyway. The way I see it, I don't really understand why we need pexpect and not just source /path/to/venv/activate (with maybe some extra flags like --no-create to not create venv if none is present or --force to force activate venv)

@neersighted
Copy link
Member

neersighted commented Oct 24, 2022

We can't just source /path/to/script as we can't control the parent shell. There are two options: source /path/to/script in a child shell which 'just works' more or less (but now we have to pass I/O and signals, which as we're discussing, is fraught), or spawn a new shell and attempt to set the variables that activate sets in the environment ourselves.

This is also fraught as our changes run before the user's shell configuration and thus are subject to being clobbered by poorly configured shells.

There is a third option that will be less obvious to users but is the 'best' in terms of consistency: poetry shell - in the style of pyenv init -.

That is to say, in order to alter the current environment, we print shell script to stdout. poetry shell would output something like "Run poetry shell - | source instead!" and poetry shell - would write the contents of the activate script to stdout (picking the correct interpreter), which would make it equivalent to manual venv activation.

I don't expect this to be popular as an alternative either; however I do think that we can no longer treat poetry shell as a second-class citizen. It suffers from benign neglect as most maintainers/power users do not use it, and consider it "somewhat broken but good enough for newbies who haven't moved on to better workflows."

It seems that that attitude is no longer tenable as too many new users get cut by the sharp edges and compromises of poetry shell -- therefore I think that a breaking change that rewrites the semantics in some way would be beneficial.

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

No branches or pull requests

4 participants