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

Effectively disable the wheel cache if not writable #7489

Merged
merged 2 commits into from
Dec 28, 2019

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented Dec 15, 2019

Move the test for cache writeability from the install command to the WheelCache object.

The test was ineffective in the install command because it was done after creating the wheel cache object. The test was also missing in the wheel and freeze commands.

The test is in WheelCache and not Cache to avoid a double warning via Cache and SimpleWheelCache.

I also considered putting the check in the option parser, but at that point the logger is not fully initialized and warning was not colored.

Fixes #7488

@pradyunsg
Copy link
Member

pradyunsg commented Dec 15, 2019

Windows CI isn't happy - possibly since the test/code isn't working there as expected.

@sbidoul
Copy link
Member Author

sbidoul commented Dec 15, 2019

I also considered putting the check in the option parser, but at that point the logger is not fully initialized and warning was not colored.

OTOH, doing it in main() after the logger is setup looks like a better place. Disabling the cache early (as in the option value) avoids code duplication, and avoids pip warning more than once about the cache not being writable.

@sbidoul
Copy link
Member Author

sbidoul commented Dec 15, 2019

Windows CI isn't happy - possibly since the test/code isn't working there as expected.

Yes, apparently os.chmod is not doing what I expected on Windows. I'll need to figure out a way to create a non writable directory on Windows.

@sbidoul
Copy link
Member Author

sbidoul commented Dec 16, 2019

I squashed the history a little bit.

@chrahunt
Copy link
Member

chrahunt commented Dec 16, 2019

Regarding the test on Windows, we have a test helper make_unreadable_file that should work or a similar version created for "read only".

@sbidoul sbidoul force-pushed the readonly-cache-sbi branch 2 times, most recently from ad825f7 to edcc521 Compare December 18, 2019 10:50
@sbidoul
Copy link
Member Author

sbidoul commented Dec 18, 2019

According to this comment

The no_perms test are useless on Windows since SafeFileCache uses
pip._internal.utils.filesystem.check_path_owner which is based on
os.geteuid which is absent on Windows.

it looks like check_path_owner does not work on Windows so it would mean this whole mechanism to check for cache ownership has no effect on windows?

@sbidoul
Copy link
Member Author

sbidoul commented Dec 27, 2019

I think this one is good to go, because the cache permission check was never implemented on Windows, so it's ok to have the new test disabled on Windows.

Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

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

Just one comment.

In a followup it may be good to separate and rename check_path_owner to actually only check whether the path is owned by the current user, and then we could use test_writable_dir to check whether it is writable like if not owned_by_current_user(...) or not is_writable_dir(...).

src/pip/_internal/cli/base_command.py Outdated Show resolved Hide resolved
This avoid code duplication (for the wheel and http
cache) and repeated warnings.
@pradyunsg pradyunsg requested a review from chrahunt December 28, 2019 12:44
@pradyunsg
Copy link
Member

Merging since there's multiple approving reviews. :)

@pradyunsg pradyunsg merged commit 7420629 into pypa:master Dec 28, 2019
@sbidoul sbidoul deleted the readonly-cache-sbi branch December 28, 2019 19:39
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jan 27, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 27, 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.

pip wheel/install with non-writable cache fails despite warning the cache has been disabled
4 participants