Merge pull request #7696 from chrahunt/refactor/auto-cleanup-source-dir

Globally manage InstallRequirement.source_dir

Author: chrahunt

src/pip/_internal/commands/download.py

@@ -140,8 +140,4 @@ def run(self, options, args):
             if downloaded:
                 write_output('Successfully downloaded %s', downloaded)
 
-            # Clean up
-            if not options.no_clean:
-                requirement_set.cleanup_files()
-
         return requirement_set

src/pip/_internal/commands/install.py

@@ -443,10 +443,6 @@ def run(self, options, args):
             except PreviousBuildDirError:
                 options.no_clean = True
                 raise
-            finally:
-                # Clean up
-                if not options.no_clean:
-                    requirement_set.cleanup_files()
 
         if options.target_dir:
             self._handle_target_dir(

src/pip/_internal/commands/wheel.py

@@ -191,6 +191,3 @@ def run(self, options, args):
             except PreviousBuildDirError:
                 options.no_clean = True
                 raise
-            finally:
-                if not options.no_clean:
-                    requirement_set.cleanup_files()

src/pip/_internal/legacy_resolve.py

@@ -327,9 +327,6 @@ def _resolve_one(
 
         req_to_install.prepared = True
 
-        # register tmp src for cleanup in case something goes wrong
-        requirement_set.reqs_to_cleanup.append(req_to_install)
-
         abstract_dist = self._get_abstract_dist_for(req_to_install)
 
         # Parse and return dependencies

src/pip/_internal/operations/prepare.py

@@ -28,7 +28,6 @@
 from pip._internal.utils.filesystem import copy2_fixed
 from pip._internal.utils.hashes import MissingHashes
 from pip._internal.utils.logging import indent_log
-from pip._internal.utils.marker_files import write_delete_marker_file
 from pip._internal.utils.misc import (
     ask_path_exists,
     backup_dir,
@@ -416,14 +415,33 @@ def prepare_linked_requirement(
         else:
             logger.info('Collecting %s', req.req or req)
 
+        download_dir = self.download_dir
+        if link.is_wheel and self.wheel_download_dir:
+            # when doing 'pip wheel` we download wheels to a
+            # dedicated dir.
+            download_dir = self.wheel_download_dir
+
+        if link.is_wheel:
+            if download_dir:
+                # When downloading, we only unpack wheels to get
+                # metadata.
+                autodelete_unpacked = True
+            else:
+                # When installing a wheel, we use the unpacked
+                # wheel.
+                autodelete_unpacked = False
+        else:
+            # We always delete unpacked sdists after pip runs.
+            autodelete_unpacked = True
+
         with indent_log():
             # @@ if filesystem packages are not marked
             # editable in a req, a non deterministic error
             # occurs when the script attempts to unpack the
             # build directory
             # Since source_dir is only set for editable requirements.
             assert req.source_dir is None
-            req.ensure_has_source_dir(self.build_dir)
+            req.ensure_has_source_dir(self.build_dir, autodelete_unpacked)
             # If a checkout exists, it's unwise to keep going.  version
             # inconsistencies are logged later, but do not fail the
             # installation.
@@ -471,12 +489,6 @@ def prepare_linked_requirement(
                 # showing the user what the hash should be.
                 hashes = MissingHashes()
 
-            download_dir = self.download_dir
-            if link.is_wheel and self.wheel_download_dir:
-                # when doing 'pip wheel` we download wheels to a
-                # dedicated dir.
-                download_dir = self.wheel_download_dir
-
             try:
                 local_file = unpack_url(
                     link, req.source_dir, self.downloader, download_dir,
@@ -498,21 +510,6 @@ def prepare_linked_requirement(
             if local_file:
                 req.local_file_path = local_file.path
 
-            if link.is_wheel:
-                if download_dir:
-                    # When downloading, we only unpack wheels to get
-                    # metadata.
-                    autodelete_unpacked = True
-                else:
-                    # When installing a wheel, we use the unpacked
-                    # wheel.
-                    autodelete_unpacked = False
-            else:
-                # We always delete unpacked sdists after pip runs.
-                autodelete_unpacked = True
-            if autodelete_unpacked:
-                write_delete_marker_file(req.source_dir)
-
             abstract_dist = _get_prepared_distribution(
                 req, self.req_tracker, self.finder, self.build_isolation,
             )

src/pip/_internal/req/req_install.py

@@ -33,10 +33,6 @@
 from pip._internal.utils.deprecation import deprecated
 from pip._internal.utils.hashes import Hashes
 from pip._internal.utils.logging import indent_log
-from pip._internal.utils.marker_files import (
-    PIP_DELETE_MARKER_FILENAME,
-    has_delete_marker_file,
-)
 from pip._internal.utils.misc import (
     ask_path_exists,
     backup_dir,
@@ -46,7 +42,6 @@
     get_installed_version,
     hide_url,
     redact_auth_from_url,
-    rmtree,
 )
 from pip._internal.utils.packaging import get_metadata
 from pip._internal.utils.temp_dir import TempDirectory, tempdir_kinds
@@ -348,8 +343,8 @@ def from_path(self):
                 s += '->' + comes_from
         return s
 
-    def ensure_build_location(self, build_dir):
-        # type: (str) -> str
+    def ensure_build_location(self, build_dir, autodelete):
+        # type: (str, bool) -> str
         assert build_dir is not None
         if self._temp_build_dir is not None:
             assert self._temp_build_dir.path
@@ -372,7 +367,16 @@ def ensure_build_location(self, build_dir):
         if not os.path.exists(build_dir):
             logger.debug('Creating directory %s', build_dir)
             os.makedirs(build_dir)
-        return os.path.join(build_dir, name)
+        actual_build_dir = os.path.join(build_dir, name)
+        # `None` indicates that we respect the globally-configured deletion
+        # settings, which is what we actually want when auto-deleting.
+        delete_arg = None if autodelete else False
+        return TempDirectory(
+            path=actual_build_dir,
+            delete=delete_arg,
+            kind=tempdir_kinds.REQ_BUILD,
+            globally_managed=True,
+        ).path
 
     def _set_requirement(self):
         # type: () -> None
@@ -412,16 +416,6 @@ def warn_on_mismatching_name(self):
         )
         self.req = Requirement(metadata_name)
 
-    def remove_temporary_source(self):
-        # type: () -> None
-        """Remove the source files from this requirement, if they are marked
-        for deletion"""
-        if self.source_dir and has_delete_marker_file(self.source_dir):
-            logger.debug('Removing source in %s', self.source_dir)
-            rmtree(self.source_dir)
-        self.source_dir = None
-        self._temp_build_dir = None
-
     def check_if_exists(self, use_user_site):
         # type: (bool) -> None
         """Find an installed distribution that satisfies or conflicts
@@ -599,8 +593,8 @@ def assert_source_matches_version(self):
             )
 
     # For both source distributions and editables
-    def ensure_has_source_dir(self, parent_dir):
-        # type: (str) -> None
+    def ensure_has_source_dir(self, parent_dir, autodelete=False):
+        # type: (str, bool) -> None
         """Ensure that a source_dir is set.
 
         This will create a temporary build dir if the name of the requirement
@@ -611,7 +605,9 @@ def ensure_has_source_dir(self, parent_dir):
         :return: self.source_dir
         """
         if self.source_dir is None:
-            self.source_dir = self.ensure_build_location(parent_dir)
+            self.source_dir = self.ensure_build_location(
+                parent_dir, autodelete
+            )
 
     # For editable installations
     def update_editable(self, obtain=True):
@@ -755,8 +751,6 @@ def archive(self, build_dir):
                     zipdir.external_attr = 0x1ED << 16  # 0o755
                     zip_output.writestr(zipdir, '')
                 for filename in filenames:
-                    if filename == PIP_DELETE_MARKER_FILENAME:
-                        continue
                     file_arcname = self._get_archive_name(
                         filename, parentdir=dirpath, rootdir=dir,
                     )

src/pip/_internal/req/req_set.py

@@ -11,7 +11,6 @@
 from pip._internal import pep425tags
 from pip._internal.exceptions import InstallationError
 from pip._internal.models.wheel import Wheel
-from pip._internal.utils.logging import indent_log
 from pip._internal.utils.typing import MYPY_CHECK_RUNNING
 
 if MYPY_CHECK_RUNNING:
@@ -34,7 +33,6 @@ def __init__(self, check_supported_wheels=True):
 
         self.unnamed_requirements = []  # type: List[InstallRequirement]
         self.successfully_downloaded = []  # type: List[InstallRequirement]
-        self.reqs_to_cleanup = []  # type: List[InstallRequirement]
 
     def __str__(self):
         # type: () -> str
@@ -162,7 +160,6 @@ def add_requirement(
             )
         )
         if does_not_satisfy_constraint:
-            self.reqs_to_cleanup.append(install_req)
             raise InstallationError(
                 "Could not satisfy constraints for '{}': "
                 "installation from path or url cannot be "
@@ -199,11 +196,3 @@ def get_requirement(self, name):
             return self.requirements[project_name]
 
         raise KeyError("No project with the name %r" % name)
-
-    def cleanup_files(self):
-        # type: () -> None
-        """Clean up files, remove builds."""
-        logger.debug('Cleaning up...')
-        with indent_log():
-            for req in self.reqs_to_cleanup:
-                req.remove_temporary_source()

src/pip/_internal/utils/marker_files.py

@@ -4,7 +4,6 @@
 import pytest
 
 from pip._internal.cli.status_codes import PREVIOUS_BUILD_DIR_ERROR
-from pip._internal.utils.marker_files import write_delete_marker_file
 from tests.lib import need_mercurial, windows_workaround_7667
 from tests.lib.local_repos import local_checkout
 
@@ -126,7 +125,6 @@ def test_cleanup_prevented_upon_build_dir_exception(script, data):
     build = script.venv_path / 'build'
     build_simple = build / 'simple'
     os.makedirs(build_simple)
-    write_delete_marker_file(build_simple)
     build_simple.joinpath("setup.py").write_text("#")
     result = script.pip(
         'install', '-f', data.find_links, '--no-index', 'simple',

tests/functional/test_install_cleanup.py

@@ -6,7 +6,6 @@
 import pytest
 
 from pip._internal.cli.status_codes import ERROR, PREVIOUS_BUILD_DIR_ERROR
-from pip._internal.utils.marker_files import write_delete_marker_file
 from tests.lib import pyversion
 
 
@@ -225,7 +224,6 @@ def test_pip_wheel_fail_cause_of_previous_build_dir(script, data):
     # Given that I have a previous build dir of the `simple` package
     build = script.venv_path / 'build' / 'simple'
     os.makedirs(build)
-    write_delete_marker_file(script.venv_path / 'build' / 'simple')
     build.joinpath('setup.py').write_text('#')
 
     # When I call pip trying to install things again

tests/functional/test_wheel.py

@@ -20,7 +20,9 @@ def test_tmp_build_directory(self):
         # Make sure we're handling it correctly with real path.
         requirement = InstallRequirement(None, None)
         tmp_dir = tempfile.mkdtemp('-build', 'pip-')
-        tmp_build_dir = requirement.ensure_build_location(tmp_dir)
+        tmp_build_dir = requirement.ensure_build_location(
+            tmp_dir, autodelete=False
+        )
         assert (
             os.path.dirname(tmp_build_dir) ==
             os.path.realpath(os.path.dirname(tmp_dir))