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

Ignore errors in temporary directory cleanup #11394

Merged
merged 5 commits into from
Jul 27, 2023

Conversation

ales-erjavec
Copy link
Contributor

@ales-erjavec ales-erjavec commented Aug 19, 2022

Pip frequently fails on windows with a PermissionError in temporary directory cleanup after it has already successfully installed upgraded/uninstalled a package.

E.g if you have import matplotlib in one python session and run

> pip install matplotlib==3.4.3

in another, you get

...
Installing collected packages: matplotlib
  Attempting uninstall: matplotlib
    Found existing installation: matplotlib 3.4.3
    Uninstalling matplotlib-3.4.3:
      Successfully uninstalled matplotlib-3.4.3
ERROR: Could not install packages due to an OSError: [WinError 5] Access is denied: 'C:\\Users\\ales\\.envs\\Lib\\site-packages\\~4tplotlib\\ft2font.cp39-win_amd64.pyd'
Check the permissions.

(note the '\\~4tplotlib\\' name) indicating this is failure to remove an temp backup; a new package is already fully installed at this point)

With this applied

Installing collected packages: matplotlib
  Attempting uninstall: matplotlib
    Found existing installation: matplotlib 3.4.2
    Uninstalling matplotlib-3.4.2:
      Successfully uninstalled matplotlib-3.4.2
  WARNING: Failed to remove a temporary file 'C:\Users\ales\.envs\Lib\site-packages\~-tplotlib\ft2font.cp39-win_amd64.pyd' due to PermissionError: [WinError 5] Access is denied: 'C:\\Users\\ales\\.envs\\Lib\\site-packages\\~-tplotlib\\ft2font.cp39-win_amd64.pyd'.
  You can safely remove it manually.
  WARNING: Failed to remove a temporary file 'C:\Users\ales\.envs\Lib\site-packages\~-tplotlib\_c_internal_utils.cp39-win_amd64.pyd' due to PermissionError: [WinError 5] Access is denied: 'C:\\Users\\ales\\.envs\\Lib\\site-packages\\~-tplotlib\\_c_internal_utils.cp39-win_amd64.pyd'.
  You can safely remove it manually.
  WARNING: Failed to remove a temporary file 'C:\Users\ales\.envs\Lib\site-packages\~-tplotlib\_path.cp39-win_amd64.pyd' due to PermissionError: [WinError 5] Access is denied: 'C:\\Users\\ales\\.envs\\Lib\\site-packages\\~-tplotlib\\_path.cp39-win_amd64.pyd'.
  You can safely remove it manually.
  WARNING: Failed to remove a temporary file 'C:\Users\ales\.envs\Lib\site-packages\~-tplotlib' due to OSError: [WinError 145] The directory is not empty: 'C:\\Users\\ales\\.envs\\Lib\\site-packages\\~-tplotlib'.
  You can safely remove it manually.
Successfully installed matplotlib-3.4.3

This also fixes #6327 (similar problem on nfs)

@ales-erjavec ales-erjavec force-pushed the temp-cleanup-ignore-errors branch 3 times, most recently from 2a6b557 to 1e903c2 Compare August 22, 2022 10:40
@uranusjr
Copy link
Member

I wonder how viable it’d be to aggregate these removal failures and show just one warning message before the process exits instead. This can be quite spammy if a package contains a lot of files cannot be removed.

@horacehoff
Copy link

Is this even viable ? As @uranusjr said, it would be a mess when installing packages containing many files, and would spam the console. Perhaps leave this as an option ?

@ales-erjavec
Copy link
Contributor Author

I have changed it so only a single warning is displayed for a temporary directory.

...
Installing collected packages: matplotlib
  Attempting uninstall: matplotlib
    Found existing installation: matplotlib 3.4.3
    Uninstalling matplotlib-3.4.3:
      Successfully uninstalled matplotlib-3.4.3
  WARNING: Failed to remove contents of a temporary directory 'C:\Users\ales\.envs\Lib\site-packages\~6tplotlib'.
  You can safely remove it manually.
Successfully installed matplotlib-3.4.2

This is still done when TempDirectory.cleanup is called.

TempDirectory does have means of being globally_managed (to cleanup at exit?), but the 'uninstall backups' do not use this and I do not feel comfortable changing this.

@tmacecode
Copy link

Hi. Is there anything blocking this PR being merged? This will fix a problem we are hitting regularly. Many thanks

ales-erjavec and others added 5 commits July 17, 2023 15:15
pip should not exit with an error when it fails to cleanup temporary
files after it has already successfully installed packages.
Preserve existing mode flags, handle case where we even lack
permission to change the mode.
Log individual errors at debug logging level.
Co-authored-by: Tzu-ping Chung <[email protected]>
l0b0 added a commit to linz/emergency-management-tools that referenced this pull request Jul 19, 2023
@uranusjr uranusjr merged commit d064174 into pypa:main Jul 27, 2023
22 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pip 19.x --upgrade breaks on nfs
4 participants