Skip to content

Delay TempDirectory.delete resolution to cleanup #7608

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

Merged
merged 2 commits into from
Jan 22, 2020

Conversation

uranusjr
Copy link
Member

Quoting @chrahunt in #7571 (comment):

One issue with globally managing the temporary build directory is that InstallCommand and WheelCommand catch a PreviousBuildDirError and then explicitly set options.no_clean. This prevents us from taking a straightforward approach for configuring global tempdir handling if options.no_clean is set.

From my reading of the implementation, the problem is that the temp directory relies on the registry to decide whether it should clean up, but that decision is made on instantiation, so the PreviousBuildDirError handler cannot affect that decision. This patch would delay that decision until cleanup time instead, so the PreviousBuildDirError handler can be kept to tell the registry to stop the cleanup.

Feel free to close this if you think this is not the way to go.

Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach and I think it will let us solve the problem I mentioned in #7571 without making us depend on more refactoring to get rid of PreviousBuildDirError in order to move forward.

I would add a test for the new behavior in this PR.

We should also note that when this gets integrated with BaseCommand, we should add self.registry = self.enter_context(tempdir_registry()) before we initialize the global tmpdir manager here, otherwise the registry will get cleaned up before the temporary directories. This change will need more tests, so probably cleaner to do the change and tests in a separate PR.

@chrahunt chrahunt added the skip news Does not need a NEWS file entry (eg: trivial changes) label Jan 18, 2020
@pradyunsg pradyunsg added the S: awaiting response Waiting for a response/more information label Jan 18, 2020
@uranusjr
Copy link
Member Author

Added a test, hope it makes sense.

@chrahunt chrahunt merged commit f1cd4cb into pypa:master Jan 22, 2020
@chrahunt
Copy link
Member

This was a nice idea, thank you.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Feb 21, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 21, 2020
@uranusjr uranusjr deleted the global-cleanup branch March 3, 2020 08:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation S: awaiting response Waiting for a response/more information skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants