Skip to content

gh-129333: fix import error over nfs on Windows #129616

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ChrisDenton
Copy link

@ChrisDenton ChrisDenton commented Feb 3, 2025

On Windows, getpath_realpath uses GetFinalPathNameByHandleW to resolve paths. This returns paths starting with a \\?\ prefix, which is usually not what we want.

This change attempts to remove the prefix for \\?\UNC\ paths. A similar thing was already done for drive paths such as \\?\C:\.

On Windows, realpath uses `GetFinalPathNameByHandleW` to resolve paths. This returns paths starting with a `\\?\` prefix, which is usually not what we want.

This change attempts to remove the prefix for \\?\UNC\ paths. A similar thing was already done for drive paths such as \\?\C:\.
@ChrisDenton ChrisDenton requested a review from FFY00 as a code owner February 3, 2025 14:17
@ghost
Copy link

ghost commented Feb 3, 2025

All commit authors signed the Contributor License Agreement.
CLA signed

@ChrisDenton ChrisDenton changed the title gh-12933: fix import error over nfs on Windows gh-129333: fix import error over nfs on Windows Feb 3, 2025
@ChrisDenton
Copy link
Author

The test failures in free-threading look unrelated to my eye but maybe I'm missing some context. E.g. I'm not sure how this change could cause cause a concurrency issue:

     self._send_bytes(m[offset:offset + size])
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\a\cpython\cpython\Lib\multiprocessing\connection.py", line 293, in _send_bytes
    raise ValueError("concurrent send_bytes() calls "
                     "are not supported")
ValueError: concurrent send_bytes() calls are not supported

@FFY00
Copy link
Member

FFY00 commented Feb 4, 2025

@eryksun, would you mind having a look at this one? Thanks!

@FFY00 FFY00 requested a review from eryksun February 4, 2025 04:32
@FFY00
Copy link
Member

FFY00 commented Feb 4, 2025

The test failures in free-threading look unrelated to my eye but maybe I'm missing some context.

I triggered a re-run, let's see if it reproduces.

@FFY00
Copy link
Member

FFY00 commented Feb 4, 2025

@nineteendo could also use your input, if you have time to look at this. Cheers!

@nineteendo
Copy link
Contributor

Well, I can't test this on my machine because python crashes with these warnings after the build (his is not a new issue):

C:\Users\wanne\cpython\PCbuild\python.vcxproj(134,5): warning MSB3073: The command "setlocal
C:\Users\wanne\cpython\PCbuild\python.vcxproj(134,5): warning MSB3073: set PYTHONPATH=C:\Users\wanne\cpython\Lib
C:\Users\wanne\cpython\PCbuild\python.vcxproj(134,5): warning MSB3073: "C:\Users\wanne\cpython\PCbuild\amd64\python.exe" "C:\Users\wanne\cpython\PC\validate_ucrtbase.py" ucrtbase" exited with code -1073741819 
.

You still need to add tests though.

@ChrisDenton
Copy link
Author

You still need to add tests though.

I'm not sure how to add tests. realpath requires that the path really exist on a network path. We can't fake it because the point of realpath is to resolve to the real path.

@FFY00
Copy link
Member

FFY00 commented Feb 5, 2025

Is there no other way to get GetFinalPathNameByHandleW to return UNC paths?

@FFY00
Copy link
Member

FFY00 commented Feb 5, 2025

Well, I can't test this on my machine because python crashes with these warnings after the build (his is not a new issue):

Is it fixed in main? If so we can sync the PR, so that the fix is included.

@nineteendo
Copy link
Contributor

nineteendo commented Feb 5, 2025

No, this issue is still open: #123414

UPDATE: fixed by creating a clean clone, but I can't install Python libraries it seems

@ChrisDenton
Copy link
Author

Is there no other way to get GetFinalPathNameByHandleW to return UNC paths?

Hmm... maybe I could try using a \\localhost\c$ path. I'll experiment.

@ChrisDenton
Copy link
Author

ChrisDenton commented Feb 6, 2025

That seems to work but the next problem I'm encountering is that there doesn't appear to be any existing unit tests for the C implementation of realpath (or any of the other functions in getpath.c) and I don't know how to add them. The test_getpath.py shims them for its testing.

I could maybe test using sysconfig::get_paths() but that means we'd need to run the test python using a \\?\UNC\localhost\ style path and not all drives support that (iirc dev drives don't). So it might be a bit flaky.

@ChrisDenton
Copy link
Author

This can be tested locally without needing to import anything by starting python with a \\?\UNC path. E.g. if the build directory is in C:\Users\Chris\Python\PCBuild\amd64\python.exe then run:

\\?\UNC\localhost\c$\Users\Chris\Python\PCBuild\amd64\python.exe -c "import sysconfig; print(sysconfig.get_paths()['include'])"

This should print the simplified path: \\localhost\c$\Users\Chris\Python\Include. Whereas before this change it will have the long \\?\UNC path.

However, this relies on the drive supporting \\locahost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants