Title: test_nt_helpers fails with case difference in drive letter
Type: behavior Stage: resolved
Components: Windows Versions: Python 3.8
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: tim.golden Nosy List: eryksun, paul.moore, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2018-07-23 07:41 by tim.golden, last changed 2018-07-25 19:12 by tim.golden. 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) * (Python committer) 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\", 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) * (Python committer) 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...

msg322189 - (view) Author: Eryk Sun (eryksun) * (Python triager) 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

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. 

msg322192 - (view) Author: Tim Golden (tim.golden) * (Python committer) 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) * (Python committer) 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.


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
>>> kernel32 = ctypes.WinDLL('kernel32', use_last_error=True)
>>> path = (ctypes.c_wchar * 4)()
>>> kernel32.GetVolumePathNameW(sys.executable, path, 4)
>>> path.value
msg322211 - (view) Author: Tim Golden (tim.golden) * (Python committer) 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) * (Python triager) 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) * (Python committer) 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)
msg322388 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2018-07-25 19:12
Test fixed to ignore case and volume differences between paths
Date User Action Args
2018-07-25 19:12:46tim.goldensetstatus: open -> closed
resolution: fixed
messages: + msg322388

stage: patch review -> resolved
2018-07-25 13:36:57tim.goldensetmessages: + msg322352
2018-07-24 19:03:32tim.goldensetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request7970
2018-07-23 13:48:37eryksunsetmessages: + msg322219
2018-07-23 12:50:32tim.goldensetmessages: + msg322211
2018-07-23 12:16:45tim.goldensetmessages: + msg322203
2018-07-23 11:00:07tim.goldensetmessages: + msg322192
2018-07-23 10:26:23eryksunsetnosy: + eryksun
messages: + msg322189
2018-07-23 10:09:28tim.goldensetmessages: + msg322187
2018-07-23 07:41:35tim.goldencreate