From 0b4ec28a3900b58b258a7c2c760ad467c29f7929 Mon Sep 17 00:00:00 2001 From: Chris Hunt Date: Sat, 25 Jan 2020 15:56:32 -0500 Subject: [PATCH 1/8] Centralize addition of no_clean argument 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. --- src/pip/_internal/cli/req_command.py | 9 ++++++++- src/pip/_internal/commands/download.py | 1 - src/pip/_internal/commands/install.py | 1 - src/pip/_internal/commands/wheel.py | 1 - 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/pip/_internal/cli/req_command.py b/src/pip/_internal/cli/req_command.py index 9383b3b8dca..3383e11af03 100644 --- a/src/pip/_internal/cli/req_command.py +++ b/src/pip/_internal/cli/req_command.py @@ -9,6 +9,7 @@ import os from functools import partial +from pip._internal.cli import cmdoptions from pip._internal.cli.base_command import Command from pip._internal.cli.command_context import CommandContextMixIn from pip._internal.exceptions import CommandError @@ -32,7 +33,7 @@ if MYPY_CHECK_RUNNING: from optparse import Values - from typing import List, Optional, Tuple + from typing import Any, List, Optional, Tuple from pip._internal.cache import WheelCache from pip._internal.models.target_python import TargetPython from pip._internal.req.req_set import RequirementSet @@ -151,6 +152,12 @@ def handle_pip_version_check(self, options): class RequirementCommand(IndexGroupCommand): + def __init__(self, *args, **kw): + # type: (Any, Any) -> None + super(RequirementCommand, self).__init__(*args, **kw) + + self.cmd_opts.add_option(cmdoptions.no_clean()) + @staticmethod def make_requirement_preparer( temp_build_dir, # type: TempDirectory diff --git a/src/pip/_internal/commands/download.py b/src/pip/_internal/commands/download.py index 24da3eb2a26..4a19091ae04 100644 --- a/src/pip/_internal/commands/download.py +++ b/src/pip/_internal/commands/download.py @@ -52,7 +52,6 @@ def __init__(self, *args, **kw): cmd_opts.add_option(cmdoptions.prefer_binary()) cmd_opts.add_option(cmdoptions.src()) cmd_opts.add_option(cmdoptions.pre()) - cmd_opts.add_option(cmdoptions.no_clean()) cmd_opts.add_option(cmdoptions.require_hashes()) cmd_opts.add_option(cmdoptions.progress_bar()) cmd_opts.add_option(cmdoptions.no_build_isolation()) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 02a187c8aa2..2b548642274 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -227,7 +227,6 @@ def __init__(self, *args, **kw): cmd_opts.add_option(cmdoptions.no_binary()) cmd_opts.add_option(cmdoptions.only_binary()) cmd_opts.add_option(cmdoptions.prefer_binary()) - cmd_opts.add_option(cmdoptions.no_clean()) cmd_opts.add_option(cmdoptions.require_hashes()) cmd_opts.add_option(cmdoptions.progress_bar()) diff --git a/src/pip/_internal/commands/wheel.py b/src/pip/_internal/commands/wheel.py index eb44bcee459..016be1f8bd8 100644 --- a/src/pip/_internal/commands/wheel.py +++ b/src/pip/_internal/commands/wheel.py @@ -101,7 +101,6 @@ def __init__(self, *args, **kw): "pip only finds stable versions."), ) - cmd_opts.add_option(cmdoptions.no_clean()) cmd_opts.add_option(cmdoptions.require_hashes()) index_opts = cmdoptions.make_option_group( From 7068e58b6ff1aefba437dd4e3e98e733061882b9 Mon Sep 17 00:00:00 2001 From: Chris Hunt Date: Sat, 25 Jan 2020 16:23:46 -0500 Subject: [PATCH 2/8] Configure tempdir registry 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. --- src/pip/_internal/cli/req_command.py | 40 ++++++++++++++++++++++++-- src/pip/_internal/commands/download.py | 3 +- src/pip/_internal/commands/install.py | 3 +- src/pip/_internal/commands/wheel.py | 3 +- 4 files changed, 44 insertions(+), 5 deletions(-) diff --git a/src/pip/_internal/cli/req_command.py b/src/pip/_internal/cli/req_command.py index 3383e11af03..cac49a0303d 100644 --- a/src/pip/_internal/cli/req_command.py +++ b/src/pip/_internal/cli/req_command.py @@ -12,7 +12,7 @@ from pip._internal.cli import cmdoptions from pip._internal.cli.base_command import Command from pip._internal.cli.command_context import CommandContextMixIn -from pip._internal.exceptions import CommandError +from pip._internal.exceptions import CommandError, PreviousBuildDirError from pip._internal.index.package_finder import PackageFinder from pip._internal.legacy_resolve import Resolver from pip._internal.models.selection_prefs import SelectionPreferences @@ -34,11 +34,16 @@ if MYPY_CHECK_RUNNING: from optparse import Values from typing import Any, List, Optional, Tuple + from pip._internal.cache import WheelCache from pip._internal.models.target_python import TargetPython from pip._internal.req.req_set import RequirementSet from pip._internal.req.req_tracker import RequirementTracker - from pip._internal.utils.temp_dir import TempDirectory + from pip._internal.utils.temp_dir import ( + TempDirectory, + TempDirectoryTypeRegistry, + ) + logger = logging.getLogger(__name__) @@ -150,6 +155,37 @@ def handle_pip_version_check(self, options): pip_self_version_check(session, options) +KEEPABLE_TEMPDIR_TYPES = [] # type: List[str] + + +def with_cleanup(func): + # type: (Any) -> Any + """Decorator for common logic related to managing temporary + directories. + """ + def configure_tempdir_registry(registry): + # type: (TempDirectoryTypeRegistry) -> None + for t in KEEPABLE_TEMPDIR_TYPES: + registry.set_delete(t, False) + + def wrapper(self, options, args): + # type: (RequirementCommand, Values, List[Any]) -> Optional[int] + assert self.tempdir_registry is not None + if options.no_clean: + configure_tempdir_registry(self.tempdir_registry) + + try: + return func(self, options, args) + except PreviousBuildDirError: + # This kind of conflict can occur when the user passes an explicit + # build directory with a pre-existing folder. In that case we do + # not want to accidentally remove it. + configure_tempdir_registry(self.tempdir_registry) + raise + + return wrapper + + class RequirementCommand(IndexGroupCommand): def __init__(self, *args, **kw): diff --git a/src/pip/_internal/commands/download.py b/src/pip/_internal/commands/download.py index 4a19091ae04..d1c59990a83 100644 --- a/src/pip/_internal/commands/download.py +++ b/src/pip/_internal/commands/download.py @@ -8,7 +8,7 @@ from pip._internal.cli import cmdoptions from pip._internal.cli.cmdoptions import make_target_python -from pip._internal.cli.req_command import RequirementCommand +from pip._internal.cli.req_command import RequirementCommand, with_cleanup from pip._internal.req import RequirementSet from pip._internal.req.req_tracker import get_requirement_tracker from pip._internal.utils.misc import ensure_dir, normalize_path, write_output @@ -76,6 +76,7 @@ def __init__(self, *args, **kw): self.parser.insert_option_group(0, index_opts) self.parser.insert_option_group(0, cmd_opts) + @with_cleanup def run(self, options, args): options.ignore_installed = True # editable doesn't really make sense for `pip download`, but the bowels diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 2b548642274..cf629c81c0b 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -21,7 +21,7 @@ from pip._internal.cache import WheelCache from pip._internal.cli import cmdoptions from pip._internal.cli.cmdoptions import make_target_python -from pip._internal.cli.req_command import RequirementCommand +from pip._internal.cli.req_command import RequirementCommand, with_cleanup from pip._internal.cli.status_codes import ERROR, SUCCESS from pip._internal.exceptions import ( CommandError, @@ -238,6 +238,7 @@ def __init__(self, *args, **kw): self.parser.insert_option_group(0, index_opts) self.parser.insert_option_group(0, cmd_opts) + @with_cleanup def run(self, options, args): # type: (Values, List[Any]) -> int cmdoptions.check_install_build_global(options) diff --git a/src/pip/_internal/commands/wheel.py b/src/pip/_internal/commands/wheel.py index 016be1f8bd8..d9308b152a4 100644 --- a/src/pip/_internal/commands/wheel.py +++ b/src/pip/_internal/commands/wheel.py @@ -11,7 +11,7 @@ from pip._internal.cache import WheelCache from pip._internal.cli import cmdoptions -from pip._internal.cli.req_command import RequirementCommand +from pip._internal.cli.req_command import RequirementCommand, with_cleanup from pip._internal.exceptions import CommandError, PreviousBuildDirError from pip._internal.req import RequirementSet from pip._internal.req.req_tracker import get_requirement_tracker @@ -111,6 +111,7 @@ def __init__(self, *args, **kw): self.parser.insert_option_group(0, index_opts) self.parser.insert_option_group(0, cmd_opts) + @with_cleanup def run(self, options, args): # type: (Values, List[Any]) -> None cmdoptions.check_install_build_global(options) From 2f4bfc3efc3093d234078a9778c20a0b217ac136 Mon Sep 17 00:00:00 2001 From: Chris Hunt Date: Sat, 25 Jan 2020 16:37:29 -0500 Subject: [PATCH 3/8] Add InstallRequirement._temp_build_dir to tempdir registry 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). --- src/pip/_internal/cli/req_command.py | 3 ++- src/pip/_internal/req/req_install.py | 4 ++-- src/pip/_internal/utils/temp_dir.py | 9 ++++++++- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/pip/_internal/cli/req_command.py b/src/pip/_internal/cli/req_command.py index cac49a0303d..8386aa85d5a 100644 --- a/src/pip/_internal/cli/req_command.py +++ b/src/pip/_internal/cli/req_command.py @@ -29,6 +29,7 @@ make_link_collector, pip_self_version_check, ) +from pip._internal.utils.temp_dir import tempdir_kinds from pip._internal.utils.typing import MYPY_CHECK_RUNNING if MYPY_CHECK_RUNNING: @@ -155,7 +156,7 @@ def handle_pip_version_check(self, options): pip_self_version_check(session, options) -KEEPABLE_TEMPDIR_TYPES = [] # type: List[str] +KEEPABLE_TEMPDIR_TYPES = [tempdir_kinds.REQ_BUILD] def with_cleanup(func): diff --git a/src/pip/_internal/req/req_install.py b/src/pip/_internal/req/req_install.py index e560cf449bf..0c5301e30f2 100644 --- a/src/pip/_internal/req/req_install.py +++ b/src/pip/_internal/req/req_install.py @@ -49,7 +49,7 @@ rmtree, ) from pip._internal.utils.packaging import get_metadata -from pip._internal.utils.temp_dir import TempDirectory +from pip._internal.utils.temp_dir import TempDirectory, tempdir_kinds from pip._internal.utils.typing import MYPY_CHECK_RUNNING from pip._internal.utils.virtualenv import running_under_virtualenv from pip._internal.vcs import vcs @@ -358,7 +358,7 @@ def ensure_build_location(self, build_dir): # Some systems have /tmp as a symlink which confuses custom # builds (such as numpy). Thus, we ensure that the real path # is returned. - self._temp_build_dir = TempDirectory(kind="req-build") + self._temp_build_dir = TempDirectory(kind=tempdir_kinds.REQ_BUILD) return self._temp_build_dir.path if self.editable: diff --git a/src/pip/_internal/utils/temp_dir.py b/src/pip/_internal/utils/temp_dir.py index ae730e4917e..c2dd4bfde18 100644 --- a/src/pip/_internal/utils/temp_dir.py +++ b/src/pip/_internal/utils/temp_dir.py @@ -9,7 +9,7 @@ from pip._vendor.contextlib2 import ExitStack -from pip._internal.utils.misc import rmtree +from pip._internal.utils.misc import enum, rmtree from pip._internal.utils.typing import MYPY_CHECK_RUNNING if MYPY_CHECK_RUNNING: @@ -21,6 +21,13 @@ logger = logging.getLogger(__name__) +# Kinds of temporary directories. Only needed for ones that are +# globally-managed. +tempdir_kinds = enum( + REQ_BUILD="req-build" +) + + _tempdir_manager = None # type: Optional[ExitStack] From c35cb7819bf2c02624b75df68955bd013d8452d5 Mon Sep 17 00:00:00 2001 From: Chris Hunt Date: Thu, 30 Jan 2020 21:58:04 -0500 Subject: [PATCH 4/8] Globally-manage InstallRequirement._temp_build_dir 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. --- src/pip/_internal/req/req_install.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/pip/_internal/req/req_install.py b/src/pip/_internal/req/req_install.py index 0c5301e30f2..e49adb63742 100644 --- a/src/pip/_internal/req/req_install.py +++ b/src/pip/_internal/req/req_install.py @@ -358,7 +358,9 @@ def ensure_build_location(self, build_dir): # Some systems have /tmp as a symlink which confuses custom # builds (such as numpy). Thus, we ensure that the real path # is returned. - self._temp_build_dir = TempDirectory(kind=tempdir_kinds.REQ_BUILD) + self._temp_build_dir = TempDirectory( + kind=tempdir_kinds.REQ_BUILD, globally_managed=True + ) return self._temp_build_dir.path if self.editable: @@ -418,9 +420,7 @@ def remove_temporary_source(self): logger.debug('Removing source in %s', self.source_dir) rmtree(self.source_dir) self.source_dir = None - if self._temp_build_dir: - self._temp_build_dir.cleanup() - self._temp_build_dir = None + self._temp_build_dir = None self.build_env.cleanup() def check_if_exists(self, use_user_site): From bdde27bfd847f1ad4fdc95326239011ac286a8d6 Mon Sep 17 00:00:00 2001 From: Chris Hunt Date: Thu, 30 Jan 2020 22:04:37 -0500 Subject: [PATCH 5/8] Add BuildEnvironment._temp_dir to tempdir registry Similar to the InstallRequirement temp build dir, now we'll be able to refactor this to be globally managed. --- src/pip/_internal/build_env.py | 4 ++-- src/pip/_internal/cli/req_command.py | 2 +- src/pip/_internal/utils/temp_dir.py | 3 ++- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/pip/_internal/build_env.py b/src/pip/_internal/build_env.py index f55f0e6b8d9..9e914115ff0 100644 --- a/src/pip/_internal/build_env.py +++ b/src/pip/_internal/build_env.py @@ -17,7 +17,7 @@ from pip import __file__ as pip_location from pip._internal.utils.subprocess import call_subprocess -from pip._internal.utils.temp_dir import TempDirectory +from pip._internal.utils.temp_dir import TempDirectory, tempdir_kinds from pip._internal.utils.typing import MYPY_CHECK_RUNNING from pip._internal.utils.ui import open_spinner @@ -54,7 +54,7 @@ class BuildEnvironment(object): def __init__(self): # type: () -> None - self._temp_dir = TempDirectory(kind="build-env") + self._temp_dir = TempDirectory(kind=tempdir_kinds.BUILD_ENV) self._prefixes = OrderedDict(( (name, _Prefix(os.path.join(self._temp_dir.path, name))) diff --git a/src/pip/_internal/cli/req_command.py b/src/pip/_internal/cli/req_command.py index 8386aa85d5a..58fe49dc772 100644 --- a/src/pip/_internal/cli/req_command.py +++ b/src/pip/_internal/cli/req_command.py @@ -156,7 +156,7 @@ def handle_pip_version_check(self, options): pip_self_version_check(session, options) -KEEPABLE_TEMPDIR_TYPES = [tempdir_kinds.REQ_BUILD] +KEEPABLE_TEMPDIR_TYPES = [tempdir_kinds.BUILD_ENV, tempdir_kinds.REQ_BUILD] def with_cleanup(func): diff --git a/src/pip/_internal/utils/temp_dir.py b/src/pip/_internal/utils/temp_dir.py index c2dd4bfde18..428173d8c96 100644 --- a/src/pip/_internal/utils/temp_dir.py +++ b/src/pip/_internal/utils/temp_dir.py @@ -24,7 +24,8 @@ # Kinds of temporary directories. Only needed for ones that are # globally-managed. tempdir_kinds = enum( - REQ_BUILD="req-build" + BUILD_ENV="build-env", + REQ_BUILD="req-build", ) From 39d1c51fdbd23c835fe64a7e89d6362f7b992815 Mon Sep 17 00:00:00 2001 From: Chris Hunt Date: Thu, 30 Jan 2020 22:08:57 -0500 Subject: [PATCH 6/8] Globally-manage BuildEnvironment._temp_dir --- src/pip/_internal/build_env.py | 4 +++- src/pip/_internal/req/req_install.py | 1 - tests/unit/test_build_env.py | 17 ++++++++++------- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/pip/_internal/build_env.py b/src/pip/_internal/build_env.py index 9e914115ff0..1c217065bfe 100644 --- a/src/pip/_internal/build_env.py +++ b/src/pip/_internal/build_env.py @@ -54,7 +54,9 @@ class BuildEnvironment(object): def __init__(self): # type: () -> None - self._temp_dir = TempDirectory(kind=tempdir_kinds.BUILD_ENV) + self._temp_dir = TempDirectory( + kind=tempdir_kinds.BUILD_ENV, globally_managed=True + ) self._prefixes = OrderedDict(( (name, _Prefix(os.path.join(self._temp_dir.path, name))) diff --git a/src/pip/_internal/req/req_install.py b/src/pip/_internal/req/req_install.py index e49adb63742..6088bacc1af 100644 --- a/src/pip/_internal/req/req_install.py +++ b/src/pip/_internal/req/req_install.py @@ -421,7 +421,6 @@ def remove_temporary_source(self): rmtree(self.source_dir) self.source_dir = None self._temp_build_dir = None - self.build_env.cleanup() def check_if_exists(self, use_user_site): # type: (bool) -> None diff --git a/tests/unit/test_build_env.py b/tests/unit/test_build_env.py index ff3b2e90cef..b4469046bdd 100644 --- a/tests/unit/test_build_env.py +++ b/tests/unit/test_build_env.py @@ -29,6 +29,7 @@ def run_with_build_env(script, setup_script_contents, SelectionPreferences ) from pip._internal.network.session import PipSession + from pip._internal.utils.temp_dir import global_tempdir_manager link_collector = LinkCollector( session=PipSession(), @@ -41,19 +42,21 @@ def run_with_build_env(script, setup_script_contents, link_collector=link_collector, selection_prefs=selection_prefs, ) - build_env = BuildEnvironment() - try: + with global_tempdir_manager(): + build_env = BuildEnvironment() ''' % str(script.scratch_path)) + indent(dedent(setup_script_contents), ' ') + - dedent( - ''' + indent( + dedent( + ''' if len(sys.argv) > 1: with build_env: subprocess.check_call((sys.executable, sys.argv[1])) - finally: - build_env.cleanup() - ''') + ''' + ), + ' ' + ) ) args = ['python', build_env_script] if test_script_contents is not None: From d99462a067ba5d9a91328d52fea93de8c864e518 Mon Sep 17 00:00:00 2001 From: Chris Hunt Date: Thu, 30 Jan 2020 22:09:53 -0500 Subject: [PATCH 7/8] Remove unused BuildEnvironment.cleanup --- src/pip/_internal/build_env.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/pip/_internal/build_env.py b/src/pip/_internal/build_env.py index 1c217065bfe..6134e0a368e 100644 --- a/src/pip/_internal/build_env.py +++ b/src/pip/_internal/build_env.py @@ -135,10 +135,6 @@ def __exit__(self, exc_type, exc_val, exc_tb): else: os.environ[varname] = old_value - def cleanup(self): - # type: () -> None - self._temp_dir.cleanup() - def check_requirements(self, reqs): # type: (Iterable[str]) -> Tuple[Set[Tuple[str, str]], Set[str]] """Return 2 sets: From e800cb16046e4992f50569a9fa1e78017c55f2ea Mon Sep 17 00:00:00 2001 From: Chris Hunt Date: Thu, 30 Jan 2020 22:10:47 -0500 Subject: [PATCH 8/8] Make BuildEnvironment._temp_dir a local variable --- src/pip/_internal/build_env.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/pip/_internal/build_env.py b/src/pip/_internal/build_env.py index 6134e0a368e..0cbcfdf2811 100644 --- a/src/pip/_internal/build_env.py +++ b/src/pip/_internal/build_env.py @@ -54,12 +54,12 @@ class BuildEnvironment(object): def __init__(self): # type: () -> None - self._temp_dir = TempDirectory( + temp_dir = TempDirectory( kind=tempdir_kinds.BUILD_ENV, globally_managed=True ) self._prefixes = OrderedDict(( - (name, _Prefix(os.path.join(self._temp_dir.path, name))) + (name, _Prefix(os.path.join(temp_dir.path, name))) for name in ('normal', 'overlay') )) @@ -78,7 +78,7 @@ def __init__(self): get_python_lib(plat_specific=True), ) } - self._site_dir = os.path.join(self._temp_dir.path, 'site') + self._site_dir = os.path.join(temp_dir.path, 'site') if not os.path.exists(self._site_dir): os.mkdir(self._site_dir) with open(os.path.join(self._site_dir, 'sitecustomize.py'), 'w') as fp: