Skip to content

util/path: don't strip leading characters of UNC paths in sanitize_win_longpath - #53252

Open
kayaal34 wants to merge 1 commit into
spack:developfrom
kayaal34:fix-sanitize-win-longpath
Open

kayaal34 wants to merge 1 commit into
spack:developfrom
kayaal34:fix-sanitize-win-longpath

Conversation

@kayaal34

@kayaal34 kayaal34 commented Oct 23, 2026 •

Copy link
Copy Markdown

sanitize_win_longpath used path.lstrip("\\\\?\\"). str.lstrip takes a set of characters, not a prefix, so it removes every leading \ and ?:

input before after
\\?\C:\spack\opt C:\spack\opt C:\spack\opt
\\server\share\spack (UNC, no prefix) server\share\spack \\server\share\spack
\\?\UNC\server\share\spack UNC\server\share\spack \\server\share\spack
?dir\file dir\file ?dir\file

It is used by _windows_readlink, so on Windows a symlink pointing to a network share came back as a relative-looking server\share\... (or UNC\server\... when os.readlink returns the extended form). The function now checks for the prefix, and maps \\?\UNC\ back to \\, the same way CPython's ntpath does.

Added a parametrized test_sanitize_win_longpath in lib/spack/spack/test/util/path.py (pure string checks, so it runs on any platform); three of the cases fail without the change. ruff check and ruff format --check pass on both files.

Note: I found this with a static check (ruff PLE1310) and used Claude (an AI assistant) to help write the fix and test; I reviewed and ran them.

@johnwparent
johnwparent self-requested a review October 23, 2026 16:07

@johnwparent johnwparent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Looks pretty good and catches a bone-headed bug I added.
Just a couple of small things

Comment thread main.obj Outdated
Comment thread lib/spack/spack/util/path.py Outdated
Comment thread lib/spack/spack/test/util/path.py
…n_longpath

str.lstrip removes any leading backslashes and question marks, not the
'\?\' prefix, so a UNC path such as \server\share lost its leading
backslashes. Compare the prefix instead, and map the '\?\UNC\' form back
to a regular UNC path.
@kayaal34
kayaal34 force-pushed the fix-sanitize-win-longpath branch from d43d528 to 9259982 Compare October 26, 2026 10:44
@kayaal34

Copy link
Copy Markdown
Author

Thanks for the review, @johnwparent. Updated (force-pushed, still one commit):

  • main.obj is gone from the commit and the history (it was a build artifact from a local test run, sorry about that).
  • Took your suggestion: the prefix is stripped only when what follows is UNC\ (case-insensitive, mapped back to \\) or a drive according to ntpath.splitdrive; otherwise the path is returned unchanged, so \\?\Volume{GUID}\... keeps its prefix.
  • Added cases for lower-case unc, a drive-relative path (\\?\C:spack\opt), a Volume GUID path, a prefixed path with no drive, and plain relative / drive-relative paths without the prefix. The test is now only_windows.

All 12 cases pass; against the previous revision of this PR three of them fail (lower-case unc, Volume GUID, prefix with no drive).

@johnwparent johnwparent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now, thanks!

@johnwparent

Copy link
Copy Markdown
Contributor

Thanks for the interest in Spack on Windows @kayaal34! Please don't hesitate to reach out on our Spack on Windows slack channel if you have any other questions.

This branch has not been deployed

No deployments
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.

2 participants