Skip to content
New issue

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

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

Already on GitHub? # to your account

Resolve symlinks in locker.lock path to real physical paths #5850

Merged
merged 1 commit into from
Aug 3, 2022

Conversation

spoorn
Copy link
Contributor

@spoorn spoorn commented Jun 15, 2022

Fixes #5849

Pull Request Check List

Resolves: #5849

  • [ x ] Added tests for changed code.
  • Updated documentation for changed code.

@spoorn spoorn force-pushed the master branch 4 times, most recently from 9066eca to d0c15a2 Compare June 15, 2022 09:49
@spoorn
Copy link
Contributor Author

spoorn commented Jun 15, 2022

Having some trouble getting the new test to pass on Windows.

Windows Python < 3.8 does not resolve symlinks at all using os.path.realpath(): https://docs.python.org/3/library/os.path.html#os.path.realpath

The suggestion is instead to use os.readlink(), but that does not give the correct path for Python >= 3.8 as it resolves to the substitute path which contains a prefix \\\\?\\, instead of the display path, due to https://bugs.python.org/issue42957

Few options we can take:

  1. Use pathlib.Path().resolve(): 8.0.2 doesn't validate paths correctly pallets/click#2088 (comment). Supposedly this has a little more overhead, but poetry uses pathlib everywhere already and the consistently amongst versions is more maintainable
  2. Use os.readlink() and remove prefix "\\?\"
  3. Check for python version. If < 3.8, use os.readlink(), else use os.path.realpath()
  4. Disable the test for Python < 3.8, so only Windows 3.7 will not resolve symlinks

Will try out option 1

Edit: Option 1 passes the checks

@spoorn
Copy link
Contributor Author

spoorn commented Jul 27, 2022

Could I get a review for this?

Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

Just some minor nitpicks in the test.

@radoering radoering merged commit 62701fe into python-poetry:master Aug 3, 2022
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

poetry.lock at ~/.config/pypoetry incorrect when installing local plugin via wheel due to symbolic links
2 participants