Globally manage InstallRequirement.source_dir #7696
Merged
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
|
Nice! I think we can also drop the |
pradyunsg
reviewed
Feb 5, 2020
|
|
chrahunt
force-pushed
the
refactor/auto-cleanup-source-dir
branch
from
7c5fa63
to
3adcdab
Compare
pradyunsg
approved these changes
Feb 5, 2020
Indeed! I missed this change, because the UI collapsed that. I'm legit glad that we finally got rid of LGTM (I've desk-reviewed this one🤷♂) |
We want to use this value to determine whether a globally-managed source_dir should delegate choosing deletion to the global tempdir manager, so it needs to be above our call to InstallRequirement.ensure_has_source_dir.
Since we explicitly disable deletion this is a no-op, but we'll parameterize the deletion soon.
Previously we were writing a delete marker file which is checked in InstallRequirement.remove_temporary_source which is only invoked if the user did not pass --no-clean (and a PreviousBuildDirError was not raised). Since our TempDirectory machinery now respects these conditions we can just wrap our source directory in that instead of using this ad-hoc mechanism for tracking our delete preference. This will let us clean up a lot of dead code that only existed for this use case.
Since nothing in our code writes the delete marker file, this block will never execute.
Nothing checks for this file, so no need to write it.
Since all directories are now globally-managed, we don't need to be concerned with resetting the member values. This will also let us remove several responsibilities from RequirementSet, which will make integrating the new resolver easier.
chrahunt
force-pushed
the
refactor/auto-cleanup-source-dir
branch
from
3adcdab
to
85ab574
Compare
|
Updated to fix trivial merge conflict. |
|
Thanks for reviewing! |
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.
Originally we would write a marker file to determine whether we needed to automatically delete
the InstallRequirement source directory
Now, we manage this explicitly using the machinery that we set up in #7677.
Progresses #7571 and #7638.