Globally manage and track some temp build directories #7677
Conversation
This file contains 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
chrahunt
added
type: refactor
chrahunt
changed the title
|
Ping. |
pradyunsg
approved these changes
Feb 4, 2020
|
Thanks, that was fast. :) |
|
I was reviewing this when you commented; so that helped. :) |
uranusjr
reviewed
Feb 4, 2020
uranusjr
reviewed
Feb 4, 2020
xavfernandez
approved these changes
Feb 4, 2020
We want to rely on --no-clean being a valid option for RequirementCommand types, so move it to one place close to the code that will depend on it.
This mirrors the current logic within the individual requirement-related commands (install, wheel) for setting options.no_clean, which is used to determine whether we need to delete directories. Next, we'll add the actual directories to track and remove them from being managed by other objects.
Now we can refactor this to be globally managed, and it will have the same behavior as it does currently (if there is any PreviousBuildDirError it will not be cleaned up).
InstallRequirement.remove_temporary_source was already being called at the end of processing (as part of RequirementSet.cleanup()), so this doesn't change behavior - cleanup still happens right after the command finishes.
Similar to the InstallRequirement temp build dir, now we'll be able to refactor this to be globally managed.
chrahunt
force-pushed
the
refactor/remove-req-set-cleanup
branch
from
cb64543
to
e800cb1
Compare
chrahunt
added
skip news
|
Thank you for the reviews and nice feedback. |
This was referenced Feb 5, 2020
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.
There are several directories cleaned up in
InstallRequirement.cleanup(). These directories need to consider the behavior of the--no-cleanoption:--no-cleanis provided, they shouldn't be deletedPreviousBuildDirErroris raised, they shouldn't be deletedand the timing of deletion i.e. deleted after all operations are done.
In this PR we tackle this for the build environment temporary directory and
InstallRequirementtemp build directory, building on top of the tempdir registry introduced in prior PRs to provide the correct behavior for--no-clean, and then mark these twoglobally_managedso they are cleaned up after our commands are finished.Progresses #7571 and #7638.