Issue34195
Created on 2018-07-23 07:41 by tim.golden, last changed 2022-04-11 14:59 by admin. This issue is now closed.
| Pull Requests | |||
|---|---|---|---|
| URL | Status | Linked | Edit |
| PR 8448 | merged | tim.golden, 2018-07-24 19:03 | |
| Messages (9) | |||
|---|---|---|---|
| msg322185 - (view) | Author: Tim Golden (tim.golden) * ![]() |
Date: 2018-07-23 07:41 | |
From a fresh build on Win10 with VS2017: python -munittest -v test.test_ntpath.TestNtpath.test_nt_helpers gives the following error: ====================================================================== FAIL: test_nt_helpers (test.test_ntpath.TestNtpath) ---------------------------------------------------------------------- Traceback (most recent call last): File "c:\work-in-progress\python\cpython\lib\test\test_ntpath.py", line 432, in test_nt_helpers self.assertEqual(drive, nt._getvolumepathname(sys.executable)) AssertionError: 'c:\\' != 'C:\\' - c:\ ? ^ + C:\ ? ^ Ad hoc, it appears that: `sys.executable` gives a lower-case path while `nt._getvolumepathname` gives an upper-case drive letter. While the test could be trivially fixed, it seems worth investigating a little further to see what's happening inside `nt._getvolumepathname` |
|||
| msg322187 - (view) | Author: Tim Golden (tim.golden) * ![]() |
Date: 2018-07-23 10:09 | |
import nt, sys; assert sys.executable.startswith(nt._getvolumepathname(sys.executable)) This code fails only when run from the python.bat as created by pcbuild\build.bat. The obvious difference is that the batch file sets PYTHONHOME which, presumably, is used to form sys.executable (haven't checked the startup code yet). The docs for GetVolumePathName [*] don't specify that the drive letter of the path returned will be upper-case, but it doesn't seem unlikely. So... it looks as though the test is unduly sensitive to case-differences in the face of something like PYTHONHOME which affects the way in which sys.executable is formed. Phew! I'll put a test patch together later... [*] https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-getvolumepathnamew |
|||
| msg322189 - (view) | Author: Eryk Sun (eryksun) * ![]() |
Date: 2018-07-23 10:26 | |
os__getvolumepathname_impl in Modules/posixmodule.c doesn't directly modify the case. So that leaves WinAPI GetVolumePathNameW as the culprit, but, according to the following test, apparently not in Windows 10.0.17134: kernel32 = ctypes.WinDLL('kernel32', use_last_error=True) path = (ctypes.c_wchar * 4)() kernel32.GetVolumePathNameW(r'c:\windows', path, 4) >>> path.value 'c:\\' On a related note, nowadays we need to be careful to only use a case-insensitive comparison for drive letters and other device names. On account of WSL, recent versions of NTFS support flagging directories as case sensitive [1][2]. For example, a volume may be mounted using a junction in a case-sensitive directory. [1]: https://blogs.msdn.microsoft.com/commandline/2018/02/28/per-directory-case-sensitivity-and-wsl [2]: https://blogs.msdn.microsoft.com/commandline/2018/06/14/improved-per-directory-case-sensitivity-support-in-wsl |
|||
| msg322192 - (view) | Author: Tim Golden (tim.golden) * ![]() |
Date: 2018-07-23 11:00 | |
Thanks, @eryksun. Whatever the reason, it's consistently failing in the way I describe. A case-insensitive test is obviously good for that and for the other reasons you give, so I'll patch the test anyway. |
|||
| msg322203 - (view) | Author: Tim Golden (tim.golden) * ![]() |
Date: 2018-07-23 12:16 | |
@eryksun almost idly I ran your ctypes code in the built interpreter. As written, it produces a lower-case c:\\ as yours did.
But...
Running Debug|Win32 interpreter...
Python 3.8.0a0 (heads/master:7a3056f, Jul 23 2018, 08:23:33) [MSC v.1912 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import ctypes, sys
>>>
>>> sys.executable
'c:\\work-in-progress\\python\\cpython\\PCbuild\\win32\\python_d.exe'
>>> kernel32 = ctypes.WinDLL('kernel32', use_last_error=True)
>>> path = (ctypes.c_wchar * 4)()
>>> kernel32.GetVolumePathNameW(sys.executable, path, 4)
1
>>> path.value
'C:\\'
>>>
|
|||
| msg322211 - (view) | Author: Tim Golden (tim.golden) * ![]() |
Date: 2018-07-23 12:50 | |
I think I've got down to the determining factor. For info: PYTHONHOME has nothing to do with it: the same thing happens if I cd into PCBuild\win32 and run python_d.exe directly For historical reasons the directory in which I'm building (c:\work-in-progress) is actually a junction to c:\users\<me>\work-in-progress. There is some commentary in the API docs about traversing junctions, so presumably that's special-cased in some way which results in an uppercase drive letter. If I rebuild in, eg, c:\temp which is a normal directory, I don't see the uppercase conversion. So, while I still need to fix the underlying test to be case-insensitive, it looks like the Mystery of the Uppercase Drive Letter is at least mostly solved. |
|||
| msg322219 - (view) | Author: Eryk Sun (eryksun) * ![]() |
Date: 2018-07-23 13:48 | |
> the directory in which I'm building (c:\work-in-progress) is actually
> a junction to c:\users\<me>\work-in-progress
That makes sense. GetVolumePathName traverses backwards from the final component. If it reaches a reparse point (other than a junction that targets a volume name of the form "\??\Volume{GUID}\"), it has to start over from the reparsed final path obtained from GetFinalPathNameByHandle. Subsequently it's traversing back from "C:\Users\<me>\work-in-progress" down to upper-case "C:\".
|
|||
| msg322352 - (view) | Author: Tim Golden (tim.golden) * ![]() |
Date: 2018-07-25 13:36 | |
New changeset ff64add8d4be2e37c552ba702f629b0b6639cd33 by Tim Golden in branch 'master': bpo-34195: Fix case-sensitive comparison in test_nt_helpers (GH-8448) https://github.com/python/cpython/commit/ff64add8d4be2e37c552ba702f629b0b6639cd33 |
|||
| msg322388 - (view) | Author: Tim Golden (tim.golden) * ![]() |
Date: 2018-07-25 19:12 | |
Test fixed to ignore case and volume differences between paths |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022-04-11 14:59:03 | admin | set | github: 78376 |
| 2018-07-25 19:12:46 | tim.golden | set | status: open -> closed resolution: fixed messages: + msg322388 stage: patch review -> resolved |
| 2018-07-25 13:36:57 | tim.golden | set | messages: + msg322352 |
| 2018-07-24 19:03:32 | tim.golden | set | keywords:
+ patch stage: needs patch -> patch review pull_requests: + pull_request7970 |
| 2018-07-23 13:48:37 | eryksun | set | messages: + msg322219 |
| 2018-07-23 12:50:32 | tim.golden | set | messages: + msg322211 |
| 2018-07-23 12:16:45 | tim.golden | set | messages: + msg322203 |
| 2018-07-23 11:00:07 | tim.golden | set | messages: + msg322192 |
| 2018-07-23 10:26:23 | eryksun | set | nosy:
+ eryksun messages: + msg322189 |
| 2018-07-23 10:09:28 | tim.golden | set | messages: + msg322187 |
| 2018-07-23 07:41:35 | tim.golden | create | |

