gh-106045: Fix venv creation from a python executable symlink
#115237
Conversation
mayeut
commented
Feb 10, 2024
•
mayeut
commented
- Issue: venv base path does not resolve symlinks using realpath() #106045
mayeut
force-pushed
the
venv-home-follow-symlink
branch
from
91e8d28 to
ad36335
Compare
mayeut
force-pushed
the
venv-home-follow-symlink
branch
2 times, most recently
from
ca424f1 to
340d9c2
Compare
Lib/venv/__init__.py
Outdated
| executable_ = os.path.abspath(executable) | ||
| while os.path.islink(executable_): | ||
| link = os.readlink(executable_) | ||
| if os.path.isabs(link): | ||
| executable_ = link | ||
| else: | ||
| executable_ = os.path.join(os.path.dirname(executable_), link) |
There was a problem hiding this comment.
Can you move this to a helper function? This way the ensure_directories and symlink resolving logic is clearly separated,
Lib/test/test_venv.py
Outdated
| subprocess.check_call(cmd) | ||
| data = self.get_text_file_contents('pyvenv.cfg') | ||
| self.assertIn('home = %s' % tree_symlink, data) | ||
| self.assertIn('executable = %s' % |
There was a problem hiding this comment.
nitpick: Since this is so path-operation heavy, it would be nice to use pathlib here instead.
There was a problem hiding this comment.
My view is that this is doing the right thing, but in the wrong place. Instead I think we should not resolve the executable path at all in venv, and instead we should resolve the prefix based on the executable symlink at Python startup (in getpath.py).
Based on what I understand, the issue that this PR is fixing was just fixed by #127974.
Lib/venv/__init__.py
Outdated
| # we don't want to overwrite the executable used in context | ||
| executable_ = os.path.abspath(executable) | ||
| while os.path.islink(executable_): | ||
| link = os.readlink(executable_) |
There was a problem hiding this comment.
Technically we need to watch out for circular links. I think having this in a separate function (as suggested) will make that less painful when implementing.
FWIW, I don't know why there isn't something in the standard library for recursively resolving a symlink (without recursively resolving all child paths as realpath does). Perhaps one for Python ideas?
It appears that the linked PR does not resolve the issue. |
|
That PR builds on top of GH-127972, so it only works with |
mayeut
force-pushed
the
venv-home-follow-symlink
branch
from
340d9c2 to
f9ef949
Compare
| - we stop if a candidate does not resolve to the same file | ||
| (this can happen with normpath) | ||
| """ | ||
| result = os.path.abspath(path) |
There was a problem hiding this comment.
os.path.abspath is done first as was used before. It does normalize the path which may end-up with an invalid path in case of symlinks (e.g. "symlink_dir/../segment").
|
The following commit authors need to sign the Contributor License Agreement: |
