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

venv module produces spurious warning that location has moved #90329

Closed
layday mannequin opened this issue Dec 24, 2021 · 9 comments
Closed

venv module produces spurious warning that location has moved #90329

layday mannequin opened this issue Dec 24, 2021 · 9 comments
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes OS-windows stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@layday
Copy link
Mannequin

layday mannequin commented Dec 24, 2021

BPO 46171
Nosy @pfmoore, @vsajip, @tjguk, @zware, @eryksun, @zooba, @FFY00, @layday

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:

assignee = None
closed_at = None
created_at = <Date 2021-12-24.09:29:51.804>
labels = ['type-bug', '3.9', '3.10', '3.11', 'library', 'OS-windows']
title = 'venv module produces spurious warning that location has moved'
updated_at = <Date 2021-12-31.16:46:09.275>
user = 'https://github.com/layday'

bugs.python.org fields:

activity = <Date 2021-12-31.16:46:09.275>
actor = 'eryksun'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)', 'Windows']
creation = <Date 2021-12-24.09:29:51.804>
creator = 'layday'
dependencies = []
files = []
hgrepos = []
issue_num = 46171
keywords = []
message_count = 7.0
messages = ['409135', '409144', '409145', '409256', '409282', '409420', '409424']
nosy_count = 8.0
nosy_names = ['paul.moore', 'vinay.sajip', 'tim.golden', 'zach.ware', 'eryksun', 'steve.dower', 'FFY00', 'layday']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue46171'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

Linked PRs

@layday
Copy link
Mannequin Author

layday mannequin commented Dec 24, 2021

After 6811fda
the venv module produces spurious warnings for venv paths which contain
DOS-encoded parts e.g. "USER~1" in "C:\Users\USER~1".
tempfile.gettempdir() returns legacy paths like these for
user temp dirs.

MRE:

python -c "import tempfile
import venv

venv.create(tempfile.mkdtemp())"
Actual environment location may have moved due to redirects, links or junctions.
Requested location: "C:\Users\RUNNER~1\AppData\Local\Temp\tmpfoobar\Scripts\python.exe"
Actual location:    "C:\Users\runneradmin\AppData\Local\Temp\tmpfoobar\Scripts\python.exe"

@layday layday mannequin added 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes stdlib Python modules in the Lib dir OS-windows type-bug An unexpected behavior, bug, or error labels Dec 24, 2021
@eryksun
Copy link
Contributor

eryksun commented Dec 24, 2021

There's no point to making the user worry about short names, symlinks, or non-canonical mount points in the filesystem path of a virtual environment. It's still accessible where the user expects to find it. The problem for bpo-45337 is filesystem redirection in UWP apps, in particular for files and directories created under "%USERPROFILE%\AppData". The warning could be limited to just paths in that tree.

@eryksun
Copy link
Contributor

eryksun commented Dec 24, 2021

There's still the problem of the system using short names in %TEMP%, since by default that's under "%USERPROFILE%\AppData". Either an exception could be made for the temp directory, since it's never redirected (AFAIK), or support could be added for GetLongPathNameW() [1] as nt._getlongpathname().

---
[1] https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getlongpathnamew

@zooba
Copy link
Member

zooba commented Dec 28, 2021

There are plenty of other ways to get a venv through a potentially unexpected path (turns out I've been doing one for years), which is why I went with the general warning rather than limiting it to specific behaviours that are subject to change outside of our control.

I'm not opposed to strengthening this check to ignore DOS encoded names, but I think it should remain for anything where the realised executable path isn't the same as what you'd assume from the requested path. Without being very aware of how your directories are all configured, it could be near impossible to diagnose, so the warning in the command-line tool is appropriate.

@eryksun
Copy link
Contributor

eryksun commented Dec 29, 2021

There are plenty of other ways to get a venv through a potentially
unexpected path (turns out I've been doing one for years)

Examples would be appreciated because I'm drawing a blank here. A junction or directory symlink in the parent path shouldn't be a problem. Otherwise I'd prefer that the solution for bpo-45337 was limited to an app container and paths in "%USERPROFILE%\AppData" (excluding "%TEMP%"). An app container can be detected via the package family name, if any, as returned by GetCurrentPackageFamilyName().

@zooba
Copy link
Member

zooba commented Dec 31, 2021

My VHDX mounted in a directory is affected by this, and actually broke a couple of tools until I also mounted it as a drive. (Using a VHDX helps bypass a lot of filesystem related perf impacts on Windows, so it's worth the occasional broken tool.)

I suspect (but haven't tested) that some file sharing handlers or attached devices could also hit it - thinking here of IoT devices that provide a filesystem-like interface.

Either way, I don't think the warning is spurious, and while it's not particularly helpful for people who pass in 8.3 names that are then expanded (it shouldn't ever happen the other way), that's easily fixed on the user's side. (And tempfile, which clearly needs fixing.)

I'm also *strongly against* warnings/errors that only occur based on a decision you made ages ago (or that someone else made for you), such as how you installed Python. Right now we have a warning that can be explained in exactly the same way on every platform and every circumstance, but Eryk's proposal/preference would suddenly make the case for the warning way too complex. Tutorials and teachers don't get to ignore it just because it only happens to some of their readers/students, and so we only make things worse for them by making it harder to understand. Better to have a simple, consistent message in this case.

@eryksun
Copy link
Contributor

eryksun commented Dec 31, 2021

My VHDX mounted in a directory is affected by this

I created a VHDX and mounted it in a directory. It's just a regular volume mount point with a junction (IO_REPARSE_TAG_MOUNT_POINT). That won't cause any problems, so I guess your setup must be different in some way.

I suspect (but haven't tested) that some file sharing handlers or
attached devices could also hit it - thinking here of IoT devices
that provide a filesystem-like interface.

My takeaway is that because filesystem filter drivers on some systems may arbitrarily redirect some paths in some cases, and redirect differently or not at all in some other context, then we need to always log a warning showing the real path and always embed the real path in pip.exe. The only clear example we have is bpo-45337, but we're erring on the side of caution instead of targeting the warning at the one known case. The generic approach happens to also include any common use of directory symbolic links or junctions, but it's not practical to handle them differently. You're open to special casing short DOS names, however, e.g. by comparing the real path to the long path name from GetLongPathNameW().

Note that if the user continues to use the path with a reparse point or redirection to access the virtual environment (if possible; it's not for bpo-45337), then running python -m pip will install scripts or executable script wrappers that refer to the accessed path, not the real path. pip uses distlib for entrypoint scripts. Apparently distlib doesn't resolve the real path of sys.executable when creating the script shebang.

For POSIX, including macOS, I don't know the range of possibilities for dependent filesystem redirection. Bind mounts (i.e. mounting an arbitrary path on a directory) can be an issue since they're temporary unless configured in /etc/fstab. However, we can't do anything to warn about them because POSIX realpath() doesn't resolve them.

warning that can be explained in exactly the same way on every platform

I guess you mean every edition of Windows.

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
EliahKagan added a commit to EliahKagan/GitPython that referenced this issue Jan 10, 2024
The main case where this happens is when tempfile.gettempdir() has
a component in it that uses an 8.3-encoded path, e.g.,
C:\Users\Administrator\... -> C:\Users\ADMINI~1\...

This is a workaround for python/cpython#90329. I call realpath only
once, when the venv is created, and not on any paths inside the
venv, to make it less likely this masks the problems the warning is
meant for. (For example, if Scripts, or python.exe in it, produced
this even with the venv created as it is now, then that may indicte
an actual problem.)

Note that copying python.exe from Scripts to one level up in the
venv, and changing its name to bash.exe to use it to simulate the
bash.exe impostor, as is done in test_hook_uses_shell_not_from_cwd,
should not (and does not) produce this warning. If that ever starts
to do so, then that should be examined as a sign of brittleness.
zooba added a commit to zooba/cpython that referenced this issue Apr 12, 2024
zooba added a commit to zooba/cpython that referenced this issue Apr 12, 2024
@zooba
Copy link
Member

zooba commented Apr 12, 2024

Pull request is up that will convert paths to the long version and compare again before warning. Would like a review before merging though.

@zooba
Copy link
Member

zooba commented Apr 15, 2024

Thanks Vinay for reviewing!

@zooba zooba closed this as completed Apr 15, 2024
diegorusso pushed a commit to diegorusso/cpython that referenced this issue Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes 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

2 participants