-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
utils/envs: fix envs in MSYS2 always broken #3713
Conversation
73e1eda
to
7ecb72f
Compare
Fixes python-poetry#2867, the virtual environment found seems to be broken messages when running in MSYS2. This change detects when in MSYS2 in order to use the bin directory for the virtualenv instead of Scripts which is normally used in Windows.
7ecb72f
to
f0bacb8
Compare
Hi all, could I please get a review on this PR? This has a huge impact on being able to deploy cross platform OSS apps for Windows. Thanks! |
@@ -879,9 +879,13 @@ class Env(object): | |||
|
|||
def __init__(self, path: Path, base: Optional[Path] = None) -> None: | |||
self._is_windows = sys.platform == "win32" | |||
self._is_mingw = sysconfig.get_platform() == "mingw" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self._is_mingw = sysconfig.get_platform() == "mingw" | |
self._is_mingw = sys.platform == "msys" |
This should work here, right? Would be good to keep the usage consistent. I am not too familiar with using mingw, would be good to get a confirmation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abn Thanks for the review, I really appreciate it!
No that won't work:
$ python
Python 3.8.8 (default, Feb 20 2021, 07:16:03) [GCC 10.2.0 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> print(sys.platform)
win32
if not self._is_windows or self._is_mingw: | ||
bin_dir = "bin" | ||
else: | ||
bin_dir = "Scripts" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not self._is_windows or self._is_mingw: | |
bin_dir = "bin" | |
else: | |
bin_dir = "Scripts" | |
bin = {"win32": "Scripts"}.get(sys.platform, "bin") |
Would this be slightly better maintainable considering we only care about exceptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work:
>>> bin = {"win32": "Scripts"}.get(sys.platform, "bin")
>>> print(bin)
Scripts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given msys seems to only patch get_platform()
, guess that is expected.
Thanks @abn! |
python, when installed via (msys) MingW on Windows, can be found under a path structure similar to that you might find on *nix systems. The install-poetry.py script was failing to take this into consideration, causing installation issues winth Windows + MingW. A similar fault in poetry itself was resolved in python-poetry#3713.
When running `python` within the various shells made available on Windows by MSys2, `sysconfig.get_platform()` returns: * MSys2 : `msys-3.2.0-x86-64` * MinGW x32 : `mingw_i686` * MinGW x64 : `mingw_x86_64` * MinGW UCRT64 : `mingw_x86_64_ucrt` * MinGW Clang64 : `mingw_x86_64_clang` In the case that `sysconfig.get_platform()` *does* return `mingw` (with no suffix), this change should still work as expected.
When running `python` within the various shells made available on Windows by MSys2, `sysconfig.get_platform()` returns: * MSys2 : `msys-3.2.0-x86-64` * MinGW x32 : `mingw_i686` * MinGW x64 : `mingw_x86_64` * MinGW UCRT64 : `mingw_x86_64_ucrt` * MinGW Clang64 : `mingw_x86_64_clang` In the case that `sysconfig.get_platform()` *does* return `mingw` (with no suffix), this change should still work as expected. Related-to: #3713
When running `python` within the various shells made available on Windows by MSys2, `sysconfig.get_platform()` returns: * MSys2 : `msys-3.2.0-x86-64` * MinGW x32 : `mingw_i686` * MinGW x64 : `mingw_x86_64` * MinGW UCRT64 : `mingw_x86_64_ucrt` * MinGW Clang64 : `mingw_x86_64_clang` In the case that `sysconfig.get_platform()` *does* return `mingw` (with no suffix), this change should still work as expected. Related-to: python-poetry#3713
When running `python` within the various shells made available on Windows by MSys2, `sysconfig.get_platform()` returns: * MSys2 : `msys-3.2.0-x86-64` * MinGW x32 : `mingw_i686` * MinGW x64 : `mingw_x86_64` * MinGW UCRT64 : `mingw_x86_64_ucrt` * MinGW Clang64 : `mingw_x86_64_clang` In the case that `sysconfig.get_platform()` *does* return `mingw` (with no suffix), this change should still work as expected. Related-to: python-poetry#3713
python[.exe], when installed via (msys) MinGW on Windows, can be found under a path structure similar to that you might find on *nix systems. The install-python.py script was not taking this into consideration, causing installation failure under Windows + MinGW. A similar fault in poetry itself was resolved with python-poetry/poetry#3713.
python[.exe], when installed via (msys) MinGW on Windows, can be found under a path structure similar to that you might find on *nix systems. The install-python.py script was not taking this into consideration, causing installation failure under Windows + MinGW. A similar fault in poetry itself was resolved with python-poetry/poetry#3713.
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
When running poetry in MSYS2 it gives a warning that the virtual environment found seems to be broken messages. This change detects when in MSYS2 in order to use the bin directory for the virtualenv instead of Scripts which is normally used in Windows.
Pull Request Check List
Resolves: #2867