gh-100809: Fix handling of drive-relative paths in pathlib.Path.absolute()#100812
gh-100809: Fix handling of drive-relative paths in pathlib.Path.absolute()#100812zooba merged 11 commits into
Conversation
….absolute() If it's available, use `nt._getfullpathname()` to retrieve an absolute path. This allows paths such as 'X:' to be made absolute even when `os.getcwd()` returns a path on another drive. It follows the behaviour of `os.path.abspath()`, except that no path normalisation is performed.
|
Are you planning to also fix this in 3.10 and 3.11? That requires fixing Maybe the fix for |
Sorry, something went wrong.
I fixed this in #95450 and we decided not to backport because it was seemed like quite an obscure bug. I'd like to do the same here as the bug seems equally obscure (there are no user-logged bugs on the tracker that I could find), but I don't mind backporting both if we collectively feel that it's important. |
Sorry, something went wrong.
|
With fresher eyes I realise my tests don't exercise per-drive working directories. One way to address this would be to add a |
Sorry, something went wrong.
Maybe skip fixing this in 3.10, but |
Sorry, something went wrong.
|
Maybe, but the bug is observable just from I don't think the presence of this bug in 3.11 makes Does that seem reasonable? |
Sorry, something went wrong.
The "subst.exe" app calls I wouldn't want to announce the new drive in the current session (i.e. try:
import ctypes
except ImportError:
def subst_drive(path):
raise NotImplementedError('ctypes is not available')
else:
import contextlib
import string
ERROR_FILE_NOT_FOUND = 2
DDD_REMOVE_DEFINITION = 2
DDD_EXACT_MATCH_ON_REMOVE = 4
DDD_NO_BROADCAST_SYSTEM = 8
kernel32 = ctypes.WinDLL('kernel32', use_last_error=True)
@contextlib.contextmanager
def subst_drive(path):
"""Temporarily yield a substitute drive for a given path."""
for c in reversed(string.ascii_uppercase):
drive = f'{c}:'
if (not kernel32.QueryDosDeviceW(drive, None, 0) and
ctypes.get_last_error() == ERROR_FILE_NOT_FOUND):
break
else:
raise FileExistsError('no available logical drive')
if not kernel32.DefineDosDeviceW(
DDD_NO_BROADCAST_SYSTEM, drive, path):
raise ctypes.WinError(ctypes.get_last_error())
try:
yield drive
finally:
if not kernel32.DefineDosDeviceW(
DDD_REMOVE_DEFINITION | DDD_EXACT_MATCH_ON_REMOVE,
drive, path):
raise ctypes.WinError(ctypes.get_last_error())Footnotes
|
Sorry, something went wrong.
Could you expand on why this is a bad idea please? Thank you! |
Sorry, something went wrong.
This is a drive that we're creating for our own temporary use. I don't think it's appropriate to have Explorer windows automatically updated to include it. I also wouldn't want unknown complications from what any other application might do when sent a Also, it's more efficient to directly call |
Sorry, something went wrong.
|
Gotcha, thanks. Is there a time-of-check to time-of-use issue with your code above? I wonder if we could skip the |
Sorry, something went wrong.
I don't know how to avoid the small race condition without resorting to the NT API. "subst.exe" doesn't handle this any better.
>>> flags = DDD_NO_BROADCAST_SYSTEM
>>> kernel32.DefineDosDeviceW(flags, 'Z:', 'C:\\Windows')
1
>>> kernel32.DefineDosDeviceW(flags, 'Z:', 'C:\\ProgramData')
1
>>> kernel32.DefineDosDeviceW(flags, 'Z:', 'C:\\Temp')
1
>>> os.path.samefile('Z:', 'C:\\Temp')
True
>>> flags = DDD_REMOVE_DEFINITION | DDD_EXACT_MATCH_ON_REMOVE
>>> kernel32.DefineDosDeviceW(flags, 'Z:', 'C:\\Temp')
1
>>> os.path.samefile('Z:', 'C:\\ProgramData')
True
>>> kernel32.DefineDosDeviceW(flags, 'Z:', 'C:\\ProgramData')
1
>>> os.path.samefile('Z:', 'C:\\Windows')
TrueI think this is bad behavior. The new drive definition affects all processes in the current logon session (i.e. the local device context). I wouldn't want to mask an existing drive definition. At least redefining a system drive requires administrator access. |
Sorry, something went wrong.
Co-authored-by: Eryk Sun <eryksun@gmail.com>
Co-authored-by: Eryk Sun <eryksun@gmail.com>
…697UT.rst Co-authored-by: Steve Dower <steve.dower@microsoft.com>
zooba
left a comment
There was a problem hiding this comment.
The macOS failure is known and unrelated, so this is good to merge if the rest of CI agrees.
Sorry, something went wrong.
|
Thanks @barneygale for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10. |
Sorry, something went wrong.
|
Thanks @barneygale for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11. |
Sorry, something went wrong.
|
Sorry, @barneygale and @zooba, I could not cleanly backport this to |
Sorry, something went wrong.
|
Sorry @barneygale and @zooba, I had trouble checking out the |
Sorry, something went wrong.
|
Thanks @barneygale for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11. |
Sorry, something went wrong.
|
Sorry, @barneygale and @zooba, I could not cleanly backport this to |
Sorry, something went wrong.
…ib.Path.absolute() (pythonGH-100812) Resolving the drive independently uses the OS API, which ensures it starts from the current directory on that drive.. (cherry picked from commit 072011b) Co-authored-by: Barney Gale <barney.gale@gmail.com>
Sorry, something went wrong.
|
Yeah, skip it. |
Sorry, something went wrong.
If it's available, use
nt._getfullpathname()to retrieve an absolute path. This allows paths such as 'X:' to be made absolute even whenos.getcwd()returns a path on another drive. It follows the behaviour ofos.path.abspath(), except that no path normalisation is performed.