gh-106045: Fix venv creation from a python executable symlink#115237
Uh oh!
There was an error while loading. Please reload this page.
Conversation
mayeutforce-pushed the venv-home-follow-symlink branch from 91e8d28 to ad36335Comparemayeutforce-pushed the venv-home-follow-symlink branch 2 times, most recently from ca424f1 to 340d9c2Compare
Lib/venv/__init__.py Outdated
| executable_=os.path.abspath(executable) | ||
| whileos.path.islink(executable_): | ||
| link=os.readlink(executable_) | ||
| ifos.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.
pelson left a comment
pelson left a comment 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) | ||
| whileos.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?
mayeut commented Jan 11, 2025
It appears that the linked PR does not resolve the issue. |
FFY00 commented Jan 11, 2025
That PR builds on top of GH-127972, so it only works with |
mayeutforce-pushed the venv-home-follow-symlink branch from 340d9c2 to f9ef949Compare
| - 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: |
4 participants
Uh oh!
There was an error while loading. Please reload this page.