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

Poetry launches wrong shell on all shells on Windows #3537

Closed
1 task
matthewdeanmartin opened this issue Jan 3, 2021 · 7 comments
Closed
1 task

Poetry launches wrong shell on all shells on Windows #3537

matthewdeanmartin opened this issue Jan 3, 2021 · 7 comments
Labels
kind/bug Something isn't working as expected

Comments

@matthewdeanmartin
Copy link

matthewdeanmartin commented Jan 3, 2021

  • [x ] I am on the latest Poetry version.

  • [ x] I have searched the issues of this repo and believe that this is not a duplicate.

  • If an exception occurs when executing a command, I executed it again in debug mode (-vvv option).

  • OS version and name: Windows 10

  • Poetry version: 1.1.4

  • ** Happens even with a empty, default pyproject.toml.

Issue

Poetry runs WSL linux instead of git-bash, mingw, or cygwin.

First shellingham incorrectly detects the shell as ('bash', 'bash.exe'). On windows in \System32\ is WSL linux bash.exe, which is on the PATH everywhere, so mingw, git-bash, cygwin all are doing tricks to pretend to be bash, but actually running "C:\Program Files\Git\git-bash.exe" or what have you.

Next, poetry just runs "bash.exe" which launches WSL & doesn't even launch WSL correctly, it dumps you to a promptless shell with no indication that anything has happened, a less persistent person would assume everything had crashed. So it isn't so much that poetry knowingly launched WSL, it launches WSL wrong.

    def activate(self, env):  # type: (VirtualEnv) -> None
        if WINDOWS:
            return env.execute(self.path)

Pipenv on the other hand never screws up pipenv shell so I look at its source code. Pipenv doesn't know or care what tricks were done to keep windows from launching the wrong bash, doesn't need to and I think poetry shouldn't need to either.

sys.exit(subprocess.call(args, shell=True, universal_newlines=True))

ref: https://github.com/pypa/pipenv/blob/cda15b3b30e04e038ee286bced6c47a311f1e0ec/pipenv/shells.py
This exploits subprocess.call's ability to launch a new child shell, however the parent shell was launched.

Aside: I even tried running poetry inside of WSL linux but performance is so bad because of WSLs IO problems that WSL might as well be an unsupported OS.

It would be a good thing to support more operating systems than just bare metal mac & Linux. I can't switch to Poetry until it supports windows, mac, alpine, AWS Linux and so on.

@matthewdeanmartin matthewdeanmartin added kind/bug Something isn't working as expected status/triage This issue needs to be triaged labels Jan 3, 2021
@TBBle
Copy link
Contributor

TBBle commented Jan 4, 2021

That does look like a bug in shellingham, if it's failing to find the full path to the bash.exe, and just returning it without a path. It would be better off returning a failure, then the fallback to $SHELL would kick-in and be closer to doing the right thing. (Except that on Windows, it falls back to $COMSPEC, so you'd get cmd.exe...)

Edit: It's a known limitation of shellingham on Windows:

Currently the command only contains the executable name on Windows, even if the command is invoked by the full path. This may change in the future.

To be clear, you're running Poetry from inside bash, so the detection is at least finding the right kind of shell? If this was happening from PowerShell or cmd, that would be very different, and certainly isn't replicating for me here. (poetry shell has other issues under Windows, though, i.e. #2030).


Simply using subprocess.call(args, shell=True, universal_newlines=True) on Windows always gets you cmd (see the source) because it uses $COMPSEC, so it's not actually able to "launch a new child shell, however the parent shell was launched". In fact, on Windows, pipenv normally should not be using shell=True because it's just inserting a new copy of CMD needlessly into the process tree.

Perhaps the reason pipenv works here is that by using shell=True, it's somehow picking up a $PATH change that isn't found without it, that puts the right bash.exe before WSL's bash.exe?


I was curious, and reproduced this under MSYS2. And indeed, adding shell=True to the subprocess.Popen call fixes the problem.

I was able to replicate the problem simply by running

py -3 -c 'import subprocess; subprocess.Popen(["bash.exe"]).communicate()'

but not with

python -c 'import subprocess; subprocess.Popen(["bash.exe"]).communicate()'

So the problem is that the current code works fine if using MSYS-installed Python in MSYS, but not if using Windows-installed Python. And the Python.exe is the one associated with the Poetry venv (in my case, since I used pipx), not the environment where you're currently running.

So I suspect this only affects users who install Poetry from outside MSYS, but try to use it from within MSYS. I've definitely hit issues with crossing MSYS and non-MSYS for Python, because Python does MSYS detection, but sometimes it's finding the install state, not the runtime state.

(As a practical example, messing with pipx inside and outside MSYS damaged my pipx install, and I had to delete it and reinstall it and the packages it was managing. Only a minor inconvenience, but shows that Python doesn't well-support using the same install both inside and outside MSYS)

shell=True seems like a working workaround in this case.

@j20232
Copy link

j20232 commented Jan 6, 2021

I'm also caught this error on the following environment.

  • OS: Windows 10
  • Poetry version: 1.1.4

When I run $poetry shell on Git Bash or MSYS2, Poetry launches WSL.

I think this problem is NOT limited to MSYS...

@TBBle
Copy link
Contributor

TBBle commented Jan 6, 2021

Git Bash is an instance of MSYS2.

I expect this will affect any MSYS or Cygwin-style environment, as they are generally not the "bash.exe" in the $PATH, unless Poetry is run using the Python from within that environment.

That would require a separate Poetry install into a separate POETRY_HOME, because of the way it binds to Python.

Since you're clearly in a position to test, can you confirm that editing the subprocess.Popen call in utils/env.py in your install to include shell=True as a keyword parameter fixes the issue?

@matthewdeanmartin
Copy link
Author

I have verified that if I add shell=True, e.g

def activate(self, env):  # type: (VirtualEnv) -> None
    if WINDOWS:
        # verified, if you add shell=True, you get a new copy of current shell
        return env.execute(self.path, shell=True)

Then I get correct behavior. This is the output with and without the shell=True

matth@DESKTOP-FANFFGU MINGW64 ~/GitHub/poetry-master
$ which python
/c/Users/matth/AppData/Local/Programs/Python/Python38/python

# Without shell = True
matth@DESKTOP-FANFFGU MINGW64 ~/GitHub/poetry-master
$ python -m poetry shell
Spawning shell within C:\Users\matth\AppData\Local\pypoetry\Cache\virtualenvs\poetry-J1Xjz0Lw-py3.8
whoami
mmartin
exit

# With shell = True
matth@DESKTOP-FANFFGU MINGW64 ~/GitHub/poetry-master
$ python -m poetry shell
Spawning shell within C:\Users\matth\AppData\Local\pypoetry\Cache\virtualenvs\poetry-J1Xjz0Lw-py3.8

matth@DESKTOP-FANFFGU MINGW64 ~/GitHub/poetry-master
$ which python
/c/Users/matth/AppData/Local/pypoetry/Cache/virtualenvs/poetry-J1Xjz0Lw-py3.8/Scripts/python

matth@DESKTOP-FANFFGU MINGW64 ~/GitHub/poetry-master
$

As a postscript, another of my windows workstations, the PATH must be different because poetry just works. Poetry & shellinghams buggy currrent configuration might "work" on some peoples machines if the PATH is favoring the "correct" bash.

@matthewdeanmartin
Copy link
Author

One more datapoint, execute() is called all over the app, in particular, run and install commands, not just shell command. So just changing the one line in utils/shell.py is just the beginning of trying to get poetry to run on windows. I'll go file a bug on shellingham next.

@matthewdeanmartin
Copy link
Author

The shellingham fix appears to fix my issue in the narrow sense. Personally, I think poetry should pop a new shell using the subprocess module relevant function and args of shell=True, which is what Pipenv does. But since this now works for me, I'm going to close the ticket.

@abn abn removed the status/triage This issue needs to be triaged label Mar 3, 2022
Copy link

github-actions bot commented Mar 2, 2024

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug Something isn't working as expected
Projects
None yet
Development

No branches or pull requests

4 participants