python / cpython

The Python programming language
https://www.python.org
Other
63.04k stars 30.19k forks source link

venv on Windows with symlinks is broken if invoked with -I #86179

Open 1cd0ebb8-58db-4ad7-b442-53d6c1cdbdef opened 4 years ago

1cd0ebb8-58db-4ad7-b442-53d6c1cdbdef commented 4 years ago
BPO 42013
Nosy @pfmoore, @tjguk, @zware, @eryksun, @zooba, @gaborbernat

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['3.10', '3.9', 'OS-windows'] title = 'venv on Windows with symlinks is broken if invoked with -I' updated_at = user = 'https://github.com/gaborbernat' ``` bugs.python.org fields: ```python activity = actor = 'eryksun' assignee = 'none' closed = False closed_date = None closer = None components = ['Windows'] creation = creator = 'gaborjbernat' dependencies = [] files = [] hgrepos = [] issue_num = 42013 keywords = [] message_count = 12.0 messages = ['378485', '378487', '378488', '378490', '378491', '378492', '378493', '378494', '378499', '378507', '378512', '378532'] nosy_count = 6.0 nosy_names = ['paul.moore', 'tim.golden', 'zach.ware', 'eryksun', 'steve.dower', 'gaborjbernat'] pr_nums = [] priority = 'normal' resolution = None stage = None status = 'open' superseder = None type = None url = 'https://bugs.python.org/issue42013' versions = ['Python 3.9', 'Python 3.10'] ```

Linked PRs

1cd0ebb8-58db-4ad7-b442-53d6c1cdbdef commented 4 years ago

Here's a small reproducible, run it on a Windows OS that has symlinks enabled:

import shutil
import venv
import subprocess

shutil.rmtree("venv", ignore_errors=True)
venv.EnvBuilder(with_pip=False, symlinks=True).create("venv")

# works subprocess.check_call(["venv\\Scripts\\python.exe", "-c", "import sys; print(sys.executable)"])

# fails with No module named 'encodings' subprocess.check_call(["venv\\Scripts\\python.exe", "-Ic", "import sys; print(sys.executable)"])

eryksun commented 4 years ago

I can't reproduce the issue with the normal 3.9.0 distribution from python.org. For example:

    >>> venv.EnvBuilder(with_pip=False, symlinks=True).create("venv")
    >>> subprocess.check_call(["venv\\Scripts\\python.exe", "-Ic", "import sys; print(sys.executable)"])
    C:\Temp\env\venv\Scripts\python.exe
    0

Which Python version(s) and distribution(s) did you test? Do you have the PYTHONHOME and/or PYTHONPATH environment variables set?

1cd0ebb8-58db-4ad7-b442-53d6c1cdbdef commented 4 years ago

❯ py -c 'import sys; print(sys.version)' 3.9.0 (tags/v3.9.0:9cf6752, Oct 5 2020, 15:34:40) [MSC v.1927 64 bit (AMD64)]

FFY00 commented 4 years ago

This error most likely happens because sys.path isn't being set properly and the standard library isn't being included.

We need more information about the specific environment this is triggering.

Which CPython build are you using? The one from Github Actions, right? We also need the environment variables.

FFY00 commented 4 years ago

Well, actually the environment variables /should/ not matter as -I implies -E.

1cd0ebb8-58db-4ad7-b442-53d6c1cdbdef commented 4 years ago

Ok, the missing link is that the python you run into needs to be also a symlink venv:

❯ py -m venv env --without-pip --clear --symlinks ❯ .\env\Scripts\python.exe -c "import venv, subprocess; venv.EnvBuilder(with_pip=False, symlinks=True).create('venv'); subprocess.check_call(['venv\\Scripts\\python.exe', '-Ic', 'import sys; print(sys.executable)'])"

Fatal Python error: init_fs_encoding: failed to get the Python codec of the filesystem encoding Python runtime state: core initialized ModuleNotFoundError: No module named 'encodings'

1cd0ebb8-58db-4ad7-b442-53d6c1cdbdef commented 4 years ago

We need more information about the specific environment this is triggering.

Both Gitub Actions Windows CPython3.9 or installer as downloaded from (on Windows 10) https://www.python.org/downloads/release/python-390/.

PYTHONHOME should not be set all. PYTHONPATH needs to be set carefully. It should never include standard-library path

The issue manifests independent of those environment variables, netiher of them are set.

eryksun commented 4 years ago

Well, actually the environment variables /should/ not matter as -I implies -E.

The operative word there is "should". I was grasping for anything that might explain why I couldn't reproduce the issue.

Ok, the missing link is that the python you run into needs to be also a symlink venv

Thank you, that reproduces the problem for me.

eryksun commented 4 years ago

This issue is partly due to bpo-8901, which changed the behavior of the -E and -I command-line options to make them ignore the default PythonPath value in the registry key "Software\Python\PythonCore\X.Y\PythonPath". The change itself is not wrong. It's just exposing an underlying problem.

The home path in pyvenv.cfg is from sys._base_executable. In a launcher-based environment that's created from the base installation, sys._base_executable is the real base executable. OTOH, in a symlink-based virtual environment, sys._base_executable is the same as sys.executable. Consequently, if a virtual environment is created from a symlink-based virtual environment, the home path in pyvenv.cfg refers to the creating environment instead of the base installation. In this case, with the current implementation, the standard library can only be found by falling back on the default PythonPath in the registry.

zooba commented 4 years ago

Thanks for figuring that out, Eryk.

Probably we should just update venv to do a realpath(sys._base_executable) to handle the venv-from-symlinked-venv scenario.

Though I'd also be quite happy to just disallow that entirely (as we used to?). If you enable system site packages thinking you're chaining venvs together, you may be surprised, so it might just be better to error out here. (On the *other* hand, if you're programmatically invoking venv, maybe you know what you're doing and we should allow it?)

pfmoore commented 4 years ago

I'm inclined to think that creating a venv from within another venv should be allowed. Tools like pipx can result in *other* tools being run from within a virtual environment, and I don't think it's good to disallow that usage - as an example, I have virtualenv, tox and nox installed via pipx, and all of them create virtual environments.

So I'm +1 on fixing this by calling realpath.

I don't think we need to do anything special for --system-site-packages. The docs say "Give the virtual environment access to the system site-packages dir". If people are trying to chain venvs using it, they are misreading that comment - and the best they could hope for is to request that it's made clearer.

eryksun commented 4 years ago

So I'm +1 on fixing this by calling realpath.

In POSIX, calculate_path() in Modules/getpath.c calls calculate_argv0_path() before it calls calculate_read_pyenv(), and calculate_argv0_path() in turn calls resolve_symlinks(&calculate->argv0_path). Thus "pyvenv.cfg" isn't found and isn't used to find the standard library in POSIX when a virtual environment uses symlinks. Later on, "pyvenv.cfg" gets read by the site module, if importing the latter isn't skipped via -S. The site module sets sys._home to the home value, but sys._home only appears to be used by sysconfig when running from the build directory.

In Windows, calculate_path() in PC/getpathp.c could get the final (real) path for argv0_path before calling calculate_pyvenv_file(). Then, just like in POSIX, the environment's "pyvenv.cfg" file won't be found and won't be used to find the standard library when a virtual environment uses symlinks.

vsalvino commented 1 year ago

I had previously always used --symlinks on Windows when creating a venv, which worked fine, with Python installed from python.org (which by default installs as real files in C:\Program Files or AppData). However, it seems that the Python installed from the Microsoft Store falls victim to this issue, as it is managed as a series of hardlinks in AppData.

So this issue will become increasingly more common as folks start to install Python from the Microsoft Store vs Python.org

zooba commented 1 year ago

The fact that symlinks to appexec links don't work has been reported upstream. It's best for Windows to fix that rather than us to try and work around it (not that I can think of any workarounds other than blocking --symlinks on Windows).

vsalvino commented 1 year ago

Good to know. To be fair the Python docs explicitly warn about --symlinks on Windows (I have a penchant for breaking things). This also explains some other broken behavior I've experienced with the new build package which also creates venvs from venvs by default. It seems like the behavior of tools creating their own venvs (poetry, pipenv, pipx, etc.) is becoming more common.

zooba commented 1 year ago

Creating venvs from venvs ought to be fine (assuming you're not trying to chain them together, which has never been supported, though it's possible) - I did the changes necessary to venv back when we first added Store support to make it work regardless of your scenario.

If python -m venv ... or using venv.VenvBulider inside a venv isn't working properly, that's worth a new bug. But if they're doing something unique, that's on them.

zooba commented 10 months ago

Taking another look at this one, it seems like the problem currently is that sys._base_executable is still the symlink in a symlink venv - the realpath call in getpath.py is not having an effect.

Fixing that ought to resolve this issue, and others (such as one that I worked around in my PR 112778 that I'd rather not).

If anyone happens to look at this and spots why the code in getpath.py that's explicitly trying to resolve the symlink for a symlinked venv executable isn't working, please say so! I figured that much out but have to leave it for a bit...

zooba commented 10 months ago

Minor regression in the change - as I posted on the PR:

The failure is because the realpath implementation will convert drive names to uppercase, which causes case-sensitive path comparisons in the tests to fail. I'm pretty sure this was reported somewhere else for os.path.realpath as well, so I guess we need to do one more comparison before returning in both cases and not return case-only changes.

zooba commented 10 months ago

I'm hesitant to backport the fix, since people always get upset when we change initialisation paths at all.

If anyone has a strong case for backporting, let me know. Otherwise I'll close this and #113121 in a few days and just call it done.

vsalvino commented 10 months ago

I think as long as the fix is applied in 3.12 or 3.13, that's fine, it doesn't need to be backported to older versions.

zooba commented 10 months ago

3.13 is where the fix will be. Going to 3.12 would be a backport.