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 Windows folder writable detection #8025

Merged
merged 1 commit into from
Apr 14, 2020
Merged

Fix Windows folder writable detection #8025

merged 1 commit into from
Apr 14, 2020

Conversation

catPill
Copy link

@catPill catPill commented Apr 12, 2020

Fixes #8013

Copy link
Contributor

@gutsytechster gutsytechster left a comment

Choose a reason for hiding this comment

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

You will need to add a test for this change and also please add a news file entry specifying this change.

@@ -161,7 +161,7 @@ def _test_writable_dir_win(path):
except OSError as e:
if e.errno == errno.EEXIST:
continue
if e.errno == errno.EPERM:
if e.errno == errno.EPERM or e.errno == errno.EACCES:
# This could be because there's a directory with the same name.
# But it's highly unlikely there's a directory called that,
# so we'll assume it's because the parent dir is not writable.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can update the comment as well indicating the other reason, this would return False.

@uranusjr
Copy link
Member

I’m OK with the fix (EACCES is indeed an error that would happen), but as @gutsytechster said, you might need to either modify the comment, or put EACCES in its separate except block and explain it. I think this error happens if the parent directory is not readable, but a test would be the best way to make sure. I know this could be difficult to test, but please at least try.

Also, you will need to add a news fragment describing the change. Please take a look at the development guide to learn how.

@catPill
Copy link
Author

catPill commented Apr 12, 2020

Maybe a subprocess is need to run os.open as non-privilege user in Windows tests, is that acceptable?

The original code was copied from Flit, in which PermissionError is used and works well. exception PermissionError correspondents to EACCES and EPERM in Python 3.

@uranusjr
Copy link
Member

pip already run subprocesses in tests. Would it be possible to extend script.pip() to run the subprocess as a different user?

If not, I think it’d be enough to simply add a comment to the copied functions pointing to Flit’s corresponding code, and describe briefly how they are ported to Python 2.

@pradyunsg pradyunsg added this to the 20.1 milestone Apr 12, 2020
@pfmoore pfmoore merged commit 6a244de into pypa:master Apr 14, 2020
@pfmoore
Copy link
Member

pfmoore commented Apr 14, 2020

Thanks for the contribution, @catPill 🙂

@catPill catPill deleted the fix-windows-writable-test branch April 15, 2020 02:13
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 20, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Installing packages with non-privileged users on Windows, failed with PermissionError
5 participants