Fix absolute paths for Cygwin #1970

davidcoghlan · 2020-10-07T12:49:37Z

Proposed fix for: #1969

Absolute paths on Windows take the form "c:\somePath" -- these need to be mapped to the form "/cygdrive/c/somePath" on Cygwin. Otherwise, the virtualenv path gets garbled when activated on Cygwin, which can cause the wrong Python environment to be used.

Thanks for contributing, make sure you address all the checklists (for details on how see

development documentation)!

ran the linter to address style issues (tox -e fix_lint)
wrote descriptive pull request text
ensured there are test(s) validating the fix
added news fragment in docs/changelog folder
updated/extended the documentation

Absolute paths on Windows take the form "c:\somePath" -- these need to be mapped to the form "/cygdrive/c/somePath" on Cygwin. Otherwise, the virtualenv path gets garbled when activated on Cygwin, which can cause the wrong Python environment to be used.

codecov · 2020-10-07T12:53:15Z

Codecov Report

Merging #1970 into main will decrease coverage by 4.10%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1970      +/-   ##
==========================================
- Coverage   91.93%   87.83%   -4.11%     
==========================================
  Files         174      174              
  Lines        8496     8474      -22     
==========================================
- Hits         7811     7443     -368     
- Misses        685     1031     +346

Flag	Coverage Δ
#tests	`87.83% <ø> (-4.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/virtualenv/activation/via_template.py	`100.00% <ø> (ø)`
src/virtualenv/discovery/windows/pep514.py	`0.00% <0.00%> (-100.00%)`	⬇️
src/virtualenv/discovery/windows/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
src/virtualenv/util/subprocess/_win_subprocess.py	`0.00% <0.00%> (-95.09%)`	⬇️
src/virtualenv/util/path/_pathlib/via_os_path.py	`0.00% <0.00%> (-91.51%)`	⬇️
src/virtualenv/create/via_global_ref/store.py	`50.00% <0.00%> (-40.00%)`	⬇️
src/virtualenv/activation/batch/__init__.py	`66.66% <0.00%> (-33.34%)`	⬇️
.../create/via_global_ref/builtin/cpython/cpython3.py	`67.39% <0.00%> (-28.27%)`	⬇️
src/virtualenv/info.py	`77.50% <0.00%> (-22.50%)`	⬇️
...nv/create/via_global_ref/builtin/cpython/common.py	`86.84% <0.00%> (-13.16%)`	⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c9ca4d...a27fe8b. Read the comment docs.

gaborbernat

Didn't we already fixed this under #1940, did you check if this is still an issue with latest release?

davidcoghlan · 2020-10-07T14:30:54Z

Didn't we already fixed this under #1940, did you check if this is still an issue with latest release?

The problem still happens with the latest release:

C:\virtualenv>virtualenv --version
virtualenv 20.0.33 from c:\users\dn_co\.local\pipx\venvs\virtualenv\lib\site-packages\virtualenv\__init__.py

I saw the fix here: https://github.com/danyeaw/virtualenv/commit/927aa92d0d3cceb44c541029a0673e8c90863e92

However, this doesn't work on Cygwin, as sysconfig.get_platform() doesn't return "cygwin", unfortunately, so that codepath doesn't run:

dn_co@DESKTOP-1TDKNNU /cygdrive/d
$ python
Python 3.9.0 (tags/v3.9.0:9cf6752, Oct  5 2020, 15:34:40) [MSC v.1927 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import sysconfig
>>> sysconfig.get_platform()
'win-amd64'

Even if we made that codepath run, it still wouldn't work on Cygwin, as Cygwin needs the path to begin with /cygdrive/, which the previous fix doesn't include. It could be that the previous fix works fine on mingw and msys, I'm not too familiar with those environments.

gaborbernat · 2020-10-07T14:35:16Z

Invoking @danyeaw for clarification. Either way the solution needs to be in the python code, not the activator script though, to make it work for all shells.

davidcoghlan · 2020-10-07T16:23:06Z

Either way the solution needs to be in the python code, not the activator script though, to make it work for all shells.

I do see what you mean. However there are a couple of issues with that:

The manipulation from c:\somePath to /cygdrive/c/somePath only makes sense for some of the shells. E.g., this wouldn't work for Powershell or cmd.exe (batch), as these only understand Windows-style paths. So doing the manipulation for all the shells is arguably incorrect; it would break certain scenarios that would otherwise work (e.g., running Powershell inside Cygwin -- admittedly, I doubt this is a mainstream scenario, but still...)

Cygwin can be detected by checking for $OSTYPE, but this is a shell variable, so it would need to be exported before running virtualenv if we wanted to read it in the Python code. Not impossible, but people would need to know to do that before running virtualenv, and it would be very easy to forget. It's not something that's needed for v16.

I can't speak for all Cygwin users, but bash is the default shell, so getting back to parity with v16 for Cygwin+bash does seem like an improvement.

danyeaw · 2020-10-07T19:35:51Z

cygpath is 1100 lines of cpp, I am sure it covers numerous edge cases and is going to be extremely difficult to design something in Python that covers all use cases, without reimplementing cygpath in Python.
https://github.com/mirror/newlib-cygwin/blob/c1f7c4d1b6d7424d7081fd382ac83a631b7671ff/winsup/utils/cygpath.cc

I have struggled to get even a working cygwin development environment working. Does someone want to work with me to cover more of the use cases if we don't want to call cygpath directly?

pfmoore · 2020-10-07T20:12:42Z

I don't see any reason not to call cygpath, you'd just have to do it via subprocess. Why do you think you'd need to reimplement it?

danyeaw · 2020-10-07T20:16:05Z

I don't see any reason not to call cygpath, you'd just have to do it via subprocess. Why do you think you'd need to reimplement it?

That was my initial PR, but @gaborbernat requested that I try to reimplement what was needed in Python. I attempted (although not very well apparently) to use a regex to convert the paths. It works for MSYS2, but it sounds like there are a bunch of edge cases of calling cygwin using other shells.

pfmoore · 2020-10-07T20:38:30Z

@danyeaw Ah, OK. My apologies for not checking the history.

Edit: Actually @gaborbernat's original comment was "if it's just a simple regex". If cygpath is 1100 lines of C++, that doesn't sound anything like a "simple regex" to me...

gaborbernat

Yeah if this is getting complicated let's switch to the subprocess call. Though I'm not sure if those whole 1.1k lines of C++ are needed for this change 🤔 But I'm happy to proceede with the subprocess call.

danyeaw · 2020-10-07T20:47:53Z

Ok, sounds good. I can submit an updated PR. Could someone volunteer to test with cygwin?

gaborbernat · 2020-10-07T20:56:36Z

I don't use cygwin, but hopefully @davidcoghlan can do it 🤔 Thanks!

pfmoore · 2020-10-07T21:03:56Z

I don't use cygwin either, sorry.

davidcoghlan · 2020-10-08T12:14:58Z

Hi @gaborbernat, @danyeaw, @pfmoore -- thanks for the comments! I just wanted to clarify a couple of things.

In v16, the detection of whether you are in Cygwin or not happens at virtualenv activation time. In v20, it happens at virtualenv creation time. This is a behaviour change -- are you sure you prefer the new behaviour?

This does have some undesirable consequences I think. Namely, if you create the virtualenv from inside Cygwin, you cannot activate it from outside Cygwin, since we've hardcoded Cygwin-style paths into the activation script.

E.g., I create the virtualenv in Cygwin, I activate it using Cygwin bash; everything is fine. I later come along inside WSL and run the pre-existing activation bash script -- it won't work, since WSL doesn't understand Cygwin paths.

Similarly, if we create the virtualenv from Cygwin, some of the activation scripts that are generated are inherently broken and will never work. Powershell and cmd.exe in particular don't understand Cygwin-style paths. So we've generated a set of activation scripts, but only a subset of these are valid.

I'm not sure how much of a problem it will be in practice. Maybe people are okay with the fact that virtual environments created from inside Cygwin can't be used outside of it. Still, the v16 behaviour seems preferable to me.

The other problem then is -- how do we detect that we're running inside Cygwin? Right now v20 checks the return value of sysconfig.get_platform() and looks for the string "cygwin". Unfortunately, this doesn't work as far as I can tell. I see "win-amd64" as the result:

dn_co@DESKTOP-1TDKNNU /cygdrive/c
$ python
Python 3.9.0 (tags/v3.9.0:9cf6752, Oct  5 2020, 15:34:40) [MSC v.1927 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import sysconfig
>>> sysconfig.get_platform()
'win-amd64'

In v16, the activation script checks the value of the shell variable $OSTYPE, which does seem to be reliable:

dn_co@DESKTOP-1TDKNNU /cygdrive/c
$ echo $OSTYPE
cygwin

However, if we want to read this variable inside the Python code, the user would need to export it before invoking virtualenv, since it's only a shell variable. This seems like it would be a significant regression -- it would be very easy to omit this step and end up with a broken virtualenv. Coming up with some other cross-platform way of detecting Cygwin may be tricky. E.g., you could invoke uname, but this doesn't exist on regular Windows.

Overall, the v16 behaviour seems preferable to me when it comes to Cygwin (hence this pull request :)) I hope that explains my reasoning anyway, even if you disagree :)

danyeaw · 2020-10-08T12:41:34Z

I was going with @asottile nice summary of what is returned on each platform:
https://github.com/asottile/scratch/wiki/platforms#cygwin-cpython-3x-64-bit

It wasn't my expectations that if I mix environments that I would expect them to work properly. WSL, cygwin, msys2, cmd.exe, and PowerShell are all very different environments.

gaborbernat · 2020-10-08T12:49:43Z

It wasn't my expectations that if I mix environments that I would expect them to work properly. WSL, cygwin, msys2, cmd.exe, and PowerShell are all very different environments.

Yeah, I think the only contract we can oblige to is that the activation script will work in the same environment you're creating it. Anything else might be stretching it.

davidcoghlan · 2020-10-09T10:25:45Z

Hello, just to tie this one up -- I discovered the reason why sysconfig.get_platform() returns the wrong value here. We are using the main Windows Python build, rather than the Cygwin-specific build. If I install one of the Cygwin Python packages, it does return the expected result:

dn_co@DESKTOP-1TDKNNU /cygdrive/c
$ python3.8
Python 3.8.3 (default, May 23 2020, 15:50:53)
[GCC 9.3.0] on cygwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import sysconfig
>>> sysconfig.get_platform()
'cygwin-3.1.7-x86_64'

Sadly this doesn't really help me, as our environment is such that changing to the Cygwin-specific Python build isn't really possible.

It's a shame that this used to work on v16 and doesn't any more, but I understand if your position on this is that you only want to support Cygwin users who are running the Cygwin-specific Python build.

Closing the PR, thanks all.

gaborbernat · 2020-10-09T10:34:16Z

Does cygwin sets any evironment variable that would allow us to detect at creation time the user is running in cygwin?

davidcoghlan · 2020-10-09T10:41:04Z

Hi @gaborbernat , there is a shell variable, OSTYPE, which is set to "cygwin". If you're happy to add an additional bit of code to read this, we could update our workflow to export this variable -- it's a bit non-obvious if anybody else has the same problem as us (but then perhaps nobody else does, as we're the only ones who have raised this with you :))

gaborbernat

I see. In light of this maybe the solution proposed here is the best. If you reopen and replace this one with the existing one, I'm tempted to accept it 👍

davidcoghlan · 2020-10-09T11:43:40Z

Hi @gaborbernat, great! To be clear, the proposal is to go with the OSTYPE environment variable?

We would also need @danyeaw 's PR here to make this work, since the regex solution doesn't return the correct paths right now. But the combination of the two changes would solve the problem for us.

gaborbernat · 2020-10-09T11:52:05Z

I'm happy to go ahead with the current contents of this PR, but we should also remove what we already have in place. That should suffice, not?

davidcoghlan · 2020-10-09T12:00:23Z

For Cygwin, yes this would certainly solve it.

I can't comment on mingw or msys though, I don't have any experience with those, and @danyeaw 's original fix seems to have some support in place for those. I could just take 'cygwin' off the list there, and leave that code in place for msys/mingw, which could be safer.

davidcoghlan · 2020-10-09T12:05:24Z

src/virtualenv/activation/via_template.py

@@ -34,7 +34,7 @@ def generate(self, creator):

    def replacements(self, creator, dest_folder):
        current_platform = sysconfig.get_platform()
-        platforms = ["mingw", "cygwin", "msys"]
+        platforms = ["mingw", "msys"]


I can remove this section of code entirely if you prefer, just wondered if this would be safer.

I mean someone needs to check if mings, msys works same/different, and if is there a cygpath there. There're also tests you need to ammend/remove.

I download MinGW/MSYS -- as I understand it, MSYS is an environment built on top of the MinGW tools (http://www.mingw.org/wiki/MSYS). The string that gets returned from sysconfig.get_platform() depends on which flavour you install, but they are all different versions of the same thing.

Running it locally, it seems that MSYS wants paths in the form /c/somePath, and it does not understand Cygwin-style paths:

dn_co@DESKTOP-1TDKNNU /c/Users/dn_co $ cd /cygdrive/c/ sh: cd: /cygdrive/c/: No such file or directory

So I think the current implementation is good for MSYS/MinGW.

It would suffer though from the same problem as cygwin does. Does msys has it's own variant of cygpath that we should be using to offer same guarantees we're offering for cygwin?

https://stackoverflow.com/a/12063651 has some detailed answer to this

👍 I can confirm that MSYS has cygpath, so we can approach it in the same way.

gaborbernat

You're also missing a changelog entry.

gaborbernat · 2020-10-09T12:33:59Z

src/virtualenv/activation/via_template.py

@@ -34,7 +34,7 @@ def generate(self, creator):

    def replacements(self, creator, dest_folder):
        current_platform = sysconfig.get_platform()
-        platforms = ["mingw", "cygwin", "msys"]
+        platforms = ["mingw", "msys"]


I mean someone needs to check if mings, msys works same/different, and if is there a cygpath there. There're also tests you need to ammend/remove.

gaborbernat · 2020-10-09T12:34:35Z

src/virtualenv/activation/bash/activate.sh

@@ -46,7 +46,7 @@ deactivate () {
 # unset irrelevant variables
 deactivate nondestructive

-VIRTUAL_ENV='__VIRTUAL_ENV__'
+VIRTUAL_ENV="$(if [ "$OSTYPE" = "cygwin" ]; then cygpath -u '__VIRTUAL_ENV__'; else echo '__VIRTUAL_ENV__'; fi;)"


I'd rather have this line un-changed and have a follow-up line that normalizes the env-var (if needed). Let's not cramp things in a single line.

gaborbernat · 2020-10-09T13:40:41Z

src/virtualenv/activation/bash/activate.sh

@@ -46,7 +46,12 @@ deactivate () {
 # unset irrelevant variables
 deactivate nondestructive

-VIRTUAL_ENV="$(if [ "$OSTYPE" = "cygwin" ]; then cygpath -u '__VIRTUAL_ENV__'; else echo '__VIRTUAL_ENV__'; fi;)"
+VIRTUAL_ENV='__VIRTUAL_ENV__'
+


No need for this empty line 👍

gaborbernat · 2020-10-09T13:41:00Z

src/virtualenv/activation/bash/activate.sh

+VIRTUAL_ENV='__VIRTUAL_ENV__'
+
+if [ "$OSTYPE" = "cygwin" ]; then
+    VIRTUAL_ENV=$(cygpath -u "$VIRTUAL_ENV")


we should validate that cygpath is an existing command, and ignore if it fails 🤔

I've tested the updated code on cygwin & msys, and it works for both.

I've added the check to make sure cygpath exists before we invoke it. Do you mean that we should also ignore the error if invoking cygpath fails? I think we probably want to fail the operation in that case, don't we, since the virtualenv isn't going to work?

gaborbernat

Fix absolute paths for Cygwin

63050db

Absolute paths on Windows take the form "c:\somePath" -- these need to be mapped to the form "/cygdrive/c/somePath" on Cygwin. Otherwise, the virtualenv path gets garbled when activated on Cygwin, which can cause the wrong Python environment to be used.

gaborbernat requested changes Oct 7, 2020

View reviewed changes

davidcoghlan closed this Oct 9, 2020

gaborbernat mentioned this pull request Oct 9, 2020

Fix activation paths in cygwin using cygpath #1973

Closed

5 tasks

gaborbernat reviewed Oct 9, 2020

View reviewed changes

davidcoghlan reopened this Oct 9, 2020

Remove Cygwin from the list, rely on the shell script for that

0779f40

davidcoghlan commented Oct 9, 2020

View reviewed changes

gaborbernat requested changes Oct 9, 2020

View reviewed changes

davidcoghlan added 4 commits October 9, 2020 13:55

Improved formatting

b6cd6cc

Update tests to handle mingw + msys explicitly

30f4aeb

Add changelog entry

05e8a92

lint errors

75a8a04

gaborbernat reviewed Oct 9, 2020

View reviewed changes

davidcoghlan added 2 commits October 9, 2020 16:26

Add msys support to the activation script

a34b438

Fix script error & linting

a27fe8b

gaborbernat approved these changes Oct 12, 2020

View reviewed changes

gaborbernat merged commit 2b0bbba into pypa:main Oct 12, 2020

gaborbernat mentioned this pull request Oct 17, 2020

virtualenv path not set correctly on Cygwin #1969

Closed

MrMino mentioned this pull request Sep 18, 2021

Use pathlib parent relationships to compare virtualenv directories ipython/ipython#13140

Merged

Fix absolute paths for Cygwin #1970

Fix absolute paths for Cygwin #1970

Conversation

davidcoghlan commented Oct 7, 2020 • edited Loading

Thanks for contributing, make sure you address all the checklists (for details on how see

codecov bot commented Oct 7, 2020 • edited Loading

Codecov Report

gaborbernat left a comment

Choose a reason for hiding this comment

davidcoghlan commented Oct 7, 2020 • edited Loading

gaborbernat commented Oct 7, 2020

davidcoghlan commented Oct 7, 2020

danyeaw commented Oct 7, 2020 • edited Loading

pfmoore commented Oct 7, 2020

danyeaw commented Oct 7, 2020

pfmoore commented Oct 7, 2020 • edited Loading

gaborbernat left a comment

Choose a reason for hiding this comment

danyeaw commented Oct 7, 2020

gaborbernat commented Oct 7, 2020

pfmoore commented Oct 7, 2020

davidcoghlan commented Oct 8, 2020

danyeaw commented Oct 8, 2020

gaborbernat commented Oct 8, 2020

davidcoghlan commented Oct 9, 2020

gaborbernat commented Oct 9, 2020

davidcoghlan commented Oct 9, 2020

gaborbernat left a comment

Choose a reason for hiding this comment

davidcoghlan commented Oct 9, 2020

gaborbernat commented Oct 9, 2020

davidcoghlan commented Oct 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborbernat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborbernat left a comment

Choose a reason for hiding this comment

davidcoghlan commented Oct 7, 2020 •

edited

Loading

codecov bot commented Oct 7, 2020 •

edited

Loading

davidcoghlan commented Oct 7, 2020 •

edited

Loading

danyeaw commented Oct 7, 2020 •

edited

Loading

pfmoore commented Oct 7, 2020 •

edited

Loading