Skip to content
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

Fix symlink resolution on Windows w/short filenames #365

Merged
merged 2 commits into from
Apr 4, 2024

Conversation

cottsay
Copy link
Member

@cottsay cottsay commented Apr 1, 2024

Two issues here:

  1. os.path.realpath() doesn't resolve symlinks at all prior to Python 3.8. Best we can do is skip the test there.
  2. It doesn't appear that os.path.realpath() is idempotent on Windows when path resolution requires both symlink resolution and 8.3/short filename resolution. Calling it twice seems to give the result we want.

Two issues here:
1. os.path.realpath() doesn't resolve symlinks at all prior to Python
   3.8. Best we can do is skip the test there.
2. It doesn't appear that os.path.realpath() is idempotent on Windows
   when path resolution requires both symlink resolution and 8.3/short
   filename resolution. Calling it twice seems to give the result we
   want.
@cottsay cottsay added the bug label Apr 1, 2024
@cottsay cottsay requested a review from nuclearsandwich April 1, 2024 20:52
@cottsay cottsay self-assigned this Apr 1, 2024
@cottsay cottsay mentioned this pull request Apr 1, 2024
Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

That's a really weird behavior. Can you add a comment in the code with a link to a reference why it's being duplicated so that in the future a reviewer doesn't accidentally cause a regression trying to clean it up.

@cottsay
Copy link
Member Author

cottsay commented Apr 4, 2024

That's a really weird behavior.

It was pretty hard to track down since I wasn't hitting the problem locally. It appears that the GitHub Actions runners use 8.3 filenames in TMP (probably in an effort to make the directory path shorter), which is the only reason I became aware of the misbehavior.

Can you add a comment

Done in 7cc7ad6.

@cottsay cottsay merged commit 4e147f6 into master Apr 4, 2024
15 checks passed
@cottsay cottsay deleted the cottsay/symlink-res-windows branch April 4, 2024 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants