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

If the working directory and the venv are on different disks, venv doesn't work in bash on Windows #103088

Closed
Zabolekar opened this issue Mar 28, 2023 · 25 comments
Labels
OS-windows stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@Zabolekar
Copy link
Contributor

Zabolekar commented Mar 28, 2023

Environment

  • OS: Windows 10
  • Architecture: x86_64
  • Shell: bash 4.4.23, part of the git 2.37.0 install
  • Python version: 3.11.1
  • Output of path/to/python -m pip list:
Package    Version
---------- -------
pip        23.0.1
setuptools 65.5.0

Issue

When I create a venv on D: and use it from C:, executables like pip or python can't be found:

$ pwd
/d
$ /path/to/python -m venv myenv
$ . myenv/Scripts/activate
$ pip --version
pip 22.3.1 from D:\myenv\Lib\site-packages\pip (python 3.11)
$ cd /c
$ pip --version
bash: \myenv/Scripts/pip: No such file or directory

Linked PRs

@Zabolekar Zabolekar added the type-bug An unexpected behavior, bug, or error label Mar 28, 2023
@eryksun
Copy link
Contributor

eryksun commented Mar 28, 2023

A POSIX shell delimits PATH with ":". Thus "D:\myenv" represents two separate pathnames "D" and "\myenv". If a path begins with "\", MSYS apparently resolves it against the drive path of the current working directory (e.g. "/c" or "/d").

You can work around this issue by editing "{env_dir}/Scripts/activate" to replace the value of VIRTUAL_ENV with the equivalent MSYS path (e.g. "D:\myenv" -> "/d/myenv").

Maybe the "activate" script could detect MSYS or Cygwin and automatically translate the value of VIRTUAL_ENV via export VIRTUAL_ENV=$(cygpath -u "$VIRTUAL_ENV").

@eryksun eryksun added OS-windows stdlib Python modules in the Lib dir labels Mar 28, 2023
@Zabolekar
Copy link
Contributor Author

Thanks for the answer. My current workaround was to move venvs and code to the same disk.

Now that you mention Cygwin I've tested the script with Cygwin's version of bash as well. The script fails because it contains \r. After applying dos2unix, it works but has the same issue as MSYS bash.

Let's say I want to fix the script for everyone and not just for me. What would be the process, and what bash variants other than MSYS and Cygwin should I be aware of?

@eryksun
Copy link
Contributor

eryksun commented Mar 29, 2023

Let's say I want to fix the script for everyone and not just for me. What would be the process, and what bash variants other than MSYS and Cygwin should I be aware of?

Cygwin and MSYS2 are the common POSIX environments on Windows, other than WSL. Create a pull request that modifies the template script "Lib/venv/scripts/common/activate".

VIRTUAL_ENV="__VENV_DIR__"
export VIRTUAL_ENV

Apparently MSYS and Cygwin can be detected on bash using the OSTYPE environment variable. The following change seems to do the job. I haven't checked whether it causes a problem for an environment that's created by the MSYS2 or Cygwin versions of Python, but I don't think it should cause a problem. cygpath -u should only modify a Windows path such as "D:\myenv", not a POSIX path such as "/d/myenv" or "/cygdrive/d/myenv".

if [ "$OSTYPE" = "cygwin" ] || [ "$OSTYPE" = "msys" ] ; then
    export VIRTUAL_ENV=$(cygpath -u "__VENV_DIR__")
else
    export VIRTUAL_ENV="__VENV_DIR__"
fi

The venv module replaces the __VENV_DIR__ template parameter with the path of the created virtual environment.

@Zabolekar
Copy link
Contributor Author

Thanks for your help. I didn't even know MSYS had cygpath, too. I've tested creating venvs in cmd and in MSYS2, then activating them in cmd, MSYS2, and cygwin, and looking at pip --version and python --version. Everything works as expected. Here is a pull request.

Two things I've noticed while doing this, not sure whether those are bugs or not:

  • the script has CR LF, so Cygwin only understands it after I run dos2unix on it
  • https://devguide.python.org/ advises to build on Windows with -d by default (set the configuration to Debug). So the resulting venvs don't contain python.exe, only python_d.exe, and calling pip directly doesn't work ("Fatal error in launcher"). If Python has been built without -d, everything works as expected.

@zooba
Copy link
Member

zooba commented Apr 11, 2023

  • the script has CR LF, so Cygwin only understands it after I run dos2unix on it

It may depend on your Git settings, if you've cloned and built from source. Or maybe this file gets modified when venv copies it? @vsajip any thoughts?

  • resulting venvs don't contain python.exe, only python_d.exe, and calling pip directly doesn't work

Guessing this is a script launcher limitation, or maybe they aren't being generated properly in this scenario. In any case, it's external - I believe pip still gets their launchers from distlib?

@Zabolekar
Copy link
Contributor Author

It may depend on your Git settings, if you've cloned and built from source

It doesn't only affect those who build from source: Python 3.11.1 that I installed from python.org has CR LF in Lib/venv/scripts/common/activate, too.

It doesn't feel right to just clone everything with Unix line endings on Windows. I'm not sure whether all relevant programs can handle it correctly. On my computer, it seems to suffice if I add Lib/venv/scripts/common/activate text eol=lf to .gitattributes, then delete the file and do git checkout-index --force --all to recreate it. But I don't know how to fix it for the installer as well.

@zooba
Copy link
Member

zooba commented Apr 11, 2023

If that's added to .gitattributes then it should work fine for the installer. We don't do anything to that file.

I'd be more concerned about venv creation reading, modifying and writing the file with different line endings. If it doesn't do that, then modifying the attributes should be enough.

@vsajip
Copy link
Member

vsajip commented Apr 12, 2023

Or maybe this file gets modified when venv copies it?

The activate script in my repo on Linux has LF terminators, and the copying code reads and writes the files as binary (there are some variable substitutions, but none of those should introduce CRLF line endings).

@zooba
Copy link
Member

zooba commented Apr 12, 2023

I've merged both fixes - any thoughts on backports? I think 3.11 is going to last long enough to be worth it, and while it won't help anyone who doesn't recreate their venvs, we'll have plenty of new users in the future.

@zooba
Copy link
Member

zooba commented Apr 12, 2023

Well, buildbot failures mean we won't be backporting soon. Looks like:

  • some buildbots ignore .gitattributes (or maybe just need a clean checkout?)
  • we need to change the second parameter to assertFalse to use line.decode() - it doesn't like bytes

The second issue only appears because the test is failing, so worth fixing that one first.

@db3l
Copy link
Contributor

db3l commented Apr 12, 2023

I just checked, and the activate script has CRLF line endings on my buildbot, so not what the .gitattributes file in the same tree specifies. I certainly don't have anything on the buildbot to ignore .gitattributes at least not to my knowledge. Oh, and yes, it's that way in all the existing checkouts for other branches as well.

A quick test with a manual fresh checkout does end up with LF, so the buildbot tree is perhaps just git not adjusting an already checked out file. Once the parameter fix is addressed, I could manually flush the buildbot's 3.x tree to verify with a test run if it would help.

@Zabolekar
Copy link
Contributor Author

we need to change the second parameter to assertFalse to use line.decode() - it doesn't like bytes

I've checked again how it looks decoded vs. undecoded, found neither helpful and decided to remove the line itself from the error message and use the line number instead. See PR.

@Zabolekar
Copy link
Contributor Author

This might be why the tests fail: #103500 (comment)

@zooba
Copy link
Member

zooba commented Apr 13, 2023

This might be why the tests fail

It's not, the bit the docs are referring to aren't what matters here. We need the buildbots to do a clean checkout, that's all.

@zooba
Copy link
Member

zooba commented Apr 13, 2023

A quick test with a manual fresh checkout does end up with LF, so the buildbot tree is perhaps just git not adjusting an already checked out file. Once the parameter fix is addressed, I could manually flush the buildbot's 3.x tree to verify with a test run if it would help.

You can flush it anyway, the test fix looks good, and the test will pass once the checkout is clean. Thanks @db3l!

@db3l
Copy link
Contributor

db3l commented Apr 13, 2023

I flushed the 3.x tree on the AMD64 Windows10 3.x buildbot and re-executed the latest test run successfully including test_venv.

I flushed everything, but I'm pretty sure just removing the single activate file would work too, so we just need to prune that from all the active trees on the Windows buildbots if backporting.

carljm added a commit to carljm/cpython that referenced this issue Apr 13, 2023
* main:
  pythongh-103479: [Enum] require __new__ to be considered a data type (pythonGH-103495)
  pythongh-103365: [Enum] STRICT boundary corrections (pythonGH-103494)
  pythonGH-103488: Use return-offset, not yield-offset. (pythonGH-103502)
  pythongh-103088: Fix test_venv error message to avoid bytes/str warning (pythonGH-103500)
  pythonGH-103082: Turn on branch events for FOR_ITER instructions. (python#103507)
  pythongh-102978: Fix mock.patch function signatures for class and staticmethod decorators (python#103228)
  pythongh-103462: Ensure SelectorSocketTransport.writelines registers a writer when data is still pending (python#103463)
  pythongh-95299: Rework test_cppext.py to not invoke setup.py directly (python#103316)
@zooba
Copy link
Member

zooba commented Apr 13, 2023

Hopefully this (#103524) approach to cleaning up all the buildbots will work. Would be nice to have something a bit narrower/faster, but this will do.

@db3l
Copy link
Contributor

db3l commented Apr 13, 2023

Isn't that just using the PR branch directory on the buildbots and not any of the actual branch trees?

Sort of a kludge, but perhaps actually touching the activate file in some innocuous way to ensure it will have to be updated simultaneous with or subsequent to the .gitattributes change?

Or, any thought to just not going down this road, and expecting Cygwin users to use the igncr shell option? It's been a while since my Cygwin days but having to deal with CRLF scripts wasn't that uncommon. Separately, as it is, this change only fixes use under Cygwin with sh/bash (e.g., the csh script is still CRLF).

@zooba
Copy link
Member

zooba commented Apr 13, 2023

Ah yeah, it will only touch the PR branches.

Touching the file and merging it is probably the best option then. We've already merged the .gitattributes change, and I think it's fine to leave the line endings alone. Windows is basically okay with LF endings virtually everywhere these days. If there's another file that ought to have LF endings preserved, then we should update that one at the same time (i.e. before the RM starts looking at buildbots for the next release).

@db3l
Copy link
Contributor

db3l commented Apr 13, 2023

I just happened to notice the activate.csh and activate.fish versions in the posix folder, which are also CRLF on Windows. It seemed plausible that changes for activate would hold for those too. Then again, the venn diagram overlap of Cygwin users using those shells with Python testing may be null...

@zooba
Copy link
Member

zooba commented Apr 13, 2023

I don't think those even get copied for a venv on Windows, even in Cygwin. We moved activate to common for Cygwin (specifically Git Bash) users. But it wouldn't be unreasonable to mark the whole posix folder there as explicitly LF, just in case.

@Zabolekar
Copy link
Contributor Author

Cygwin (specifically Git Bash)

But git bash is not Cygwin.

@GalaxySnail
Copy link
Contributor

Cygwin (specifically Git Bash)

But git bash is not Cygwin.

Git-bash is based on msys2 [1], which is based on Cygwin [2].

[1] https://github.com/git-for-windows/git/wiki#about
[2] https://github.com/msys2/msys2-runtime

@zooba
Copy link
Member

zooba commented Apr 17, 2023

It's where the source code lives, ultimately. So most shell bugs/limitations that affect Cygwin also end up affecting msys2 and Git Bash, which makes them far more significant than "just" Cygwin.

I'll update the attributes and touch the files.

zooba added a commit to zooba/cpython that referenced this issue Apr 17, 2023
Also touches the files in no meaningful way to ensure they get updated when pulling
zooba added a commit that referenced this issue Apr 17, 2023
…03591)

Also touches the affected files in meaningless ways to ensure they get updated when pulling
aisk pushed a commit to aisk/cpython that referenced this issue Apr 18, 2023
@zooba
Copy link
Member

zooba commented Apr 18, 2023

I'm going to consider this closed, unless someone really wants it backported to 3.11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OS-windows stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

6 participants