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 package permissions #9393

Closed
wants to merge 9 commits into from

Conversation

winsonluk
Copy link
Contributor

@winsonluk winsonluk commented Dec 29, 2020

This PR addresses issues that arise when packages in site-packages are not owned by the same user running pip.

Reproduce:

$ sudo pip install package        # installs into site-packages/package-1.0.dist-info with owner=root
$ pip install --upgrade package   # fails since $USER doesnt have permission to rename package-1.0.dist-info...
                                  #  ...but still generates site-packages/~ackage-1.0.dist-info with owner=$USER
$ pip freeze                      # WARNING when parsing site-packages/~ackage-1.0.dist-info

Changes:

  • Fail fast when upgrading packages: Currently, if a user tries to upgrade a package that they don't own, a temporary folder is created before pip fails, resulting in an orphaned temp folder within site-packages every time this command is run. This change adds a UninstallationError that is raised before the temporary folder is created, through a PermissionError exception at the utils::misc::renames level.
  • Add chown warning: Installing, upgrading, and uninstalling with pip fails if users don't have access to the package folder. This often happens most often when the package is owned by root from a previous sudo pip install. This change warns the user and offers potential solutions.
  • Fix pip freeze for temp folders: When orphaned temp directories are left in site-packages, pip freeze warns with a parsing error. This change makes pip ignore temp directories when running pip freeze.

Issues addressed:

Fixes #7269
Fixes #9235

@winsonluk
Copy link
Contributor Author

winsonluk commented Dec 29, 2020

Backstory: I've made the mistake of running sudo pip install too many times, leading to a few of these issues myself. I agree with #6409 - we should probably add a warning at the command line level when users run with sudo.

@uranusjr
Copy link
Member

uranusjr commented Dec 29, 2020

I don’t think this fixes #7450, but only eliminates the subsequent failure caused by the mess it creates. To properly fix the issue, pip needs to fail on installation instead.

@winsonluk
Copy link
Contributor Author

winsonluk commented Dec 29, 2020

I don’t think this fixes #7450, but only eliminates the subsequent failure caused by the mess it creates. To properly fix the issue, pip needs to fail on installation instead.

You're right, thanks for the flag. The symptom is the same but the cause is different. #7450 relates to temp directories generated from setup.py install; this PR fixes temp directories generated from a failed pip install --upgrade after a sudo pip install. I'll unlink the issues.

@winsonluk
Copy link
Contributor Author

FYI I've submitted a PR to CPython to fix the root cause (shutil.move() failing incorrectly): python/cpython#24001

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Feb 4, 2021
@winsonluk
Copy link
Contributor Author

Closing since this has been merged via #9669.

@winsonluk winsonluk closed this Mar 3, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs rebase or merge PR has conflicts with current master
Projects
None yet
3 participants