Do not cleanup archive download tempdir immediately #7479
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The previous logic forced us to handle populating the download directory in this function right next to the download and hash checking. By extending the lifetime of the directory we can more easily separate the code. This also allows for additional optimizations later: by using metadata from wheels directly instead of unpacking them, we can avoid extracting wheels unnecessarily. Unpacked files can be easily 3x larger than the archives themselves, so this should reduce disk utilization and general IO significantly.
chrahunt
added
type: refactor
pradyunsg
approved these changes
Dec 14, 2019
|
Me too! I think we'll probably want to make sure we're not over-using it periodically, but it definitely helps unblock a lot of refactoring. |
danizen
pushed a commit
to danizen/pip
that referenced
this pull request
Jan 2, 2020
- TempDirectory() tries to delete the directory as normal - if cleanup fails on Windows due to EACCES, warn about virus scanner - This is a more specific error than previous error, but does not change the behavior when a user is attempting to pip uninstall in a system directory - This changes the messaging, but risks leaving the directory behind. - Leaving the directory behind, however, is what is already happening - The fix for pypa#7479 gives the Virus scanner more time to finish its work, but there is still a possibility for a race condition that leaves the impression that there is an error in pip itself.
danizen
pushed a commit
to danizen/pip
that referenced
this pull request
Jan 2, 2020
- TempDirectory() tries to delete the directory as normal - if cleanup fails on Windows due to EACCES, warn about virus scanner - This is a more specific error than previous error, but does not change the behavior when a user is attempting to pip uninstall in a system directory - This changes the messaging, but risks leaving the directory behind. - Leaving the directory behind, however, is what is already happening - The fix for pypa#7479 gives the Virus scanner more time to finish its work, but there is still a possibility for a race condition that leaves the impression that there is an error in pip itself.
lock
bot
added
the
auto-locked
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The previous logic forced us to handle populating the download directory in this function right next to the actual HTTP download and the hash checking. By extending the lifetime of the temporary download directory we can more easily separate the code.
This also allows for additional optimizations later: by using metadata
from wheels directly instead of unpacking them, we can avoid extracting
wheels unnecessarily. Unpacked files can be easily 3x larger than the
archives themselves, so this will let us reduce disk utilization and general
IO significantly, especially when we introduce the dependency resolver.
Progresses #7049.