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

utils/envs: fix envs in MSYS2 always broken #3713

Merged
merged 1 commit into from
Mar 21, 2021

Conversation

danyeaw
Copy link
Contributor

@danyeaw danyeaw commented Feb 21, 2021

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

  • Added tests for changed code.
  • Updated documentation for changed code.

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.
@danyeaw
Copy link
Contributor Author

danyeaw commented Mar 20, 2021

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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Contributor Author

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

Comment on lines +884 to +887
if not self._is_windows or self._is_mingw:
bin_dir = "bin"
else:
bin_dir = "Scripts"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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?

Copy link
Contributor Author

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

Copy link
Member

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.

@abn abn merged commit 7360b09 into python-poetry:master Mar 21, 2021
@danyeaw
Copy link
Contributor Author

danyeaw commented Mar 21, 2021

Thanks @abn!

@danyeaw danyeaw deleted the fix-msys2-venv branch March 21, 2021 14:58
@danyeaw danyeaw mentioned this pull request Jul 4, 2021
8 tasks
s0600204 added a commit to s0600204/poetry that referenced this pull request Sep 2, 2021
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.
s0600204 added a commit to s0600204/poetry that referenced this pull request Sep 3, 2021
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.
@s0600204 s0600204 mentioned this pull request Sep 7, 2021
2 tasks
abn pushed a commit that referenced this pull request Oct 8, 2021
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
1nF0rmed pushed a commit to 1nF0rmed/poetry that referenced this pull request Nov 15, 2021
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
edvardm pushed a commit to edvardm/poetry that referenced this pull request Nov 24, 2021
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
s0600204 added a commit to s0600204/install.python-poetry.org that referenced this pull request Dec 26, 2021
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.
neersighted pushed a commit to python-poetry/install.python-poetry.org that referenced this pull request Jan 21, 2022
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.
Copy link

github-actions bot commented Mar 1, 2024

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Poetry always detects venvs created by mingw-w64-x86_64-python (MSYS2) as broken
2 participants