python-poetry / poetry-plugin-shell

MIT License
0 stars 1 forks source link

Avoid using pexpect in poetry shell #18

Open kislyuk opened 3 years ago

kislyuk commented 3 years ago

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 commented 1 year ago

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:

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 commented 1 year ago

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

dimbleby commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.