From 78b294e7461c0df200b894a54362681c7e994815 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Gia=20Phong?= Date: Fri, 4 Sep 2020 16:20:13 +0700 Subject: [PATCH 1/3] Remove download_dir exist check Both pip download and wheel call endure_dir on the directory. --- src/pip/_internal/commands/download.py | 1 - src/pip/_internal/operations/prepare.py | 18 ++---------------- 2 files changed, 2 insertions(+), 17 deletions(-) diff --git a/src/pip/_internal/commands/download.py b/src/pip/_internal/commands/download.py index 0861d9e67..31eebd962 100644 --- a/src/pip/_internal/commands/download.py +++ b/src/pip/_internal/commands/download.py @@ -87,7 +87,6 @@ class DownloadCommand(RequirementCommand): cmdoptions.check_dist_restriction(options) options.download_dir = normalize_path(options.download_dir) - ensure_dir(options.download_dir) session = self.get_default_session(options) diff --git a/src/pip/_internal/operations/prepare.py b/src/pip/_internal/operations/prepare.py index 3060bafa1..c48bcca63 100644 --- a/src/pip/_internal/operations/prepare.py +++ b/src/pip/_internal/operations/prepare.py @@ -353,20 +353,6 @@ class RequirementPreparer(object): # Previous "header" printed for a link-based InstallRequirement self._previous_requirement_header = ("", "") - @property - def _download_should_save(self): - # type: () -> bool - if not self.download_dir: - return False - - if os.path.exists(self.download_dir): - return True - - logger.critical('Could not find download directory') - raise InstallationError( - "Could not find or access download directory '{}'" - .format(self.download_dir)) - def _log_preparing_link(self, req): # type: (InstallRequirement) -> None """Provide context for the requirement being prepared.""" @@ -568,9 +554,9 @@ class RequirementPreparer(object): download_path = display_path(download_location) logger.info('Saved %s', download_path) - if self._download_should_save: + if link.is_vcs: # Make a .zip of the source_dir we already created. - if link.is_vcs: + if self.download_dir is not None: req.archive(self.download_dir) return dist From 6887b0795b3567ac2a85578e5509dd8cfcc2b284 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Gia=20Phong?= Date: Sat, 12 Sep 2020 20:57:12 +0700 Subject: [PATCH 2/3] Merge usage of download_dir and wheel_download_dir In every cases, at least one of them is None. By doing this, it is also possible to simplify wrapper codes around download_dir. --- src/pip/_internal/cli/req_command.py | 2 -- src/pip/_internal/commands/wheel.py | 2 +- src/pip/_internal/operations/prepare.py | 46 +++++++------------------ src/pip/_internal/req/req_install.py | 4 ++- tests/unit/test_req.py | 1 - 5 files changed, 17 insertions(+), 38 deletions(-) diff --git a/src/pip/_internal/cli/req_command.py b/src/pip/_internal/cli/req_command.py index 0757e34f6..03cc52f69 100644 --- a/src/pip/_internal/cli/req_command.py +++ b/src/pip/_internal/cli/req_command.py @@ -203,7 +203,6 @@ class RequirementCommand(IndexGroupCommand): finder, # type: PackageFinder use_user_site, # type: bool download_dir=None, # type: str - wheel_download_dir=None, # type: str ): # type: (...) -> RequirementPreparer """ @@ -229,7 +228,6 @@ class RequirementCommand(IndexGroupCommand): build_dir=temp_build_dir_path, src_dir=options.src_dir, download_dir=download_dir, - wheel_download_dir=wheel_download_dir, build_isolation=options.build_isolation, req_tracker=req_tracker, session=session, diff --git a/src/pip/_internal/commands/wheel.py b/src/pip/_internal/commands/wheel.py index 0f718566b..38a9b197f 100644 --- a/src/pip/_internal/commands/wheel.py +++ b/src/pip/_internal/commands/wheel.py @@ -137,7 +137,7 @@ class WheelCommand(RequirementCommand): req_tracker=req_tracker, session=session, finder=finder, - wheel_download_dir=options.wheel_dir, + download_dir=options.wheel_dir, use_user_site=False, ) diff --git a/src/pip/_internal/operations/prepare.py b/src/pip/_internal/operations/prepare.py index c48bcca63..8767115b8 100644 --- a/src/pip/_internal/operations/prepare.py +++ b/src/pip/_internal/operations/prepare.py @@ -300,7 +300,6 @@ class RequirementPreparer(object): build_dir, # type: str download_dir, # type: Optional[str] src_dir, # type: str - wheel_download_dir, # type: Optional[str] build_isolation, # type: bool req_tracker, # type: RequirementTracker session, # type: PipSession @@ -325,16 +324,6 @@ class RequirementPreparer(object): # not saved, and are deleted immediately after unpacking. self.download_dir = download_dir - # Where still-packed .whl files should be written to. If None, they are - # written to the download_dir parameter. Separate to download_dir to - # permit only keeping wheel archives for pip wheel. - self.wheel_download_dir = wheel_download_dir - - # NOTE - # download_dir and wheel_download_dir overlap semantically and may - # be combined if we're willing to have non-wheel archives present in - # the wheelhouse output by 'pip wheel'. - # Is build isolation allowed? self.build_isolation = build_isolation @@ -371,15 +360,8 @@ class RequirementPreparer(object): with indent_log(): logger.info("Using cached %s", req.link.filename) - def _get_download_dir(self, link): - # type: (Link) -> Optional[str] - if link.is_wheel and self.wheel_download_dir: - # Download wheels to a dedicated dir when doing `pip wheel`. - return self.wheel_download_dir - return self.download_dir - - def _ensure_link_req_src_dir(self, req, download_dir, parallel_builds): - # type: (InstallRequirement, Optional[str], bool) -> None + def _ensure_link_req_src_dir(self, req, parallel_builds): + # type: (InstallRequirement, bool) -> None """Ensure source_dir of a linked InstallRequirement.""" # Since source_dir is only set for editable requirements. if req.link.is_wheel: @@ -480,10 +462,9 @@ class RequirementPreparer(object): # Check if the relevant file is already available # in the download directory file_path = None - download_dir = self._get_download_dir(req.link) - if download_dir is not None and link.is_wheel: + if self.download_dir is not None and link.is_wheel: hashes = self._get_linked_req_hashes(req) - file_path = _check_download_dir(req.link, download_dir, hashes) + file_path = _check_download_dir(req.link, self.download_dir, hashes) if file_path is not None: # The file is already available, so mark it as downloaded @@ -514,15 +495,14 @@ class RequirementPreparer(object): # type: (InstallRequirement, bool) -> Distribution assert req.link link = req.link - download_dir = self._get_download_dir(link) - self._ensure_link_req_src_dir(req, download_dir, parallel_builds) + self._ensure_link_req_src_dir(req, parallel_builds) hashes = self._get_linked_req_hashes(req) if link.url not in self._downloaded: try: local_file = unpack_url( link, req.source_dir, self._download, - download_dir, hashes, + self.download_dir, hashes, ) except NetworkConnectionError as exc: raise InstallationError( @@ -544,11 +524,13 @@ class RequirementPreparer(object): req, self.req_tracker, self.finder, self.build_isolation, ) - if download_dir: + if self.download_dir is not None: if link.is_existing_dir(): logger.info('Link is a directory, ignoring download_dir') elif local_file: - download_location = os.path.join(download_dir, link.filename) + download_location = os.path.join( + self.download_dir, link.filename + ) if not os.path.exists(download_location): shutil.copy(local_file.path, download_location) download_path = display_path(download_location) @@ -556,8 +538,7 @@ class RequirementPreparer(object): if link.is_vcs: # Make a .zip of the source_dir we already created. - if self.download_dir is not None: - req.archive(self.download_dir) + req.archive(self.download_dir) return dist def prepare_editable_requirement( @@ -579,14 +560,13 @@ class RequirementPreparer(object): 'hash.'.format(req) ) req.ensure_has_source_dir(self.src_dir) - req.update_editable(not self._download_should_save) + req.update_editable(self.download_dir is None) dist = _get_prepared_distribution( req, self.req_tracker, self.finder, self.build_isolation, ) - if self._download_should_save: - req.archive(self.download_dir) + req.archive(self.download_dir) req.check_if_exists(self.use_user_site) return dist diff --git a/src/pip/_internal/req/req_install.py b/src/pip/_internal/req/req_install.py index 42999a59f..8ce299503 100644 --- a/src/pip/_internal/req/req_install.py +++ b/src/pip/_internal/req/req_install.py @@ -700,12 +700,14 @@ class InstallRequirement(object): return self.name + '/' + name def archive(self, build_dir): - # type: (str) -> None + # type: (Optional[str]) -> None """Saves archive to provided build_dir. Used for saving downloaded VCS requirements as part of `pip download`. """ assert self.source_dir + if build_dir is None: + return create_archive = True archive_name = '{}-{}.zip'.format(self.name, self.metadata["version"]) diff --git a/tests/unit/test_req.py b/tests/unit/test_req.py index 730a26a88..083d2c2c6 100644 --- a/tests/unit/test_req.py +++ b/tests/unit/test_req.py @@ -82,7 +82,6 @@ class TestRequirementSet(object): build_dir=os.path.join(self.tempdir, 'build'), src_dir=os.path.join(self.tempdir, 'src'), download_dir=None, - wheel_download_dir=None, build_isolation=True, req_tracker=tracker, session=session, From b28e2c4928cc62d90b738a4613886fb1e2ad6a81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Gia=20Phong?= Date: Sat, 12 Sep 2020 21:08:49 +0700 Subject: [PATCH 3/3] New resolver: Avoid polluting dest dir Previously, during dependency resolution for `pip download -d ` or `pip wheel -w `, distributions downloaded are always saved to , even for those are only used in backtracking and are not part of the returned requirement set. --- news/8827.bugfix.rst | 2 ++ src/pip/_internal/commands/download.py | 1 + src/pip/_internal/commands/wheel.py | 11 +++++--- src/pip/_internal/operations/prepare.py | 36 ++++++++++++++++--------- 4 files changed, 33 insertions(+), 17 deletions(-) create mode 100644 news/8827.bugfix.rst diff --git a/news/8827.bugfix.rst b/news/8827.bugfix.rst new file mode 100644 index 000000000..608cd3d5c --- /dev/null +++ b/news/8827.bugfix.rst @@ -0,0 +1,2 @@ +Avoid polluting the destination directory by resolution artifacts +when the new resolver is used for ``pip download`` or ``pip wheel``. diff --git a/src/pip/_internal/commands/download.py b/src/pip/_internal/commands/download.py index 31eebd962..2f151e049 100644 --- a/src/pip/_internal/commands/download.py +++ b/src/pip/_internal/commands/download.py @@ -137,6 +137,7 @@ class DownloadCommand(RequirementCommand): for req in requirement_set.requirements.values(): if not req.editable and req.satisfied_by is None: assert req.name is not None + preparer.save_linked_requirement(req) downloaded.append(req.name) if downloaded: write_output('Successfully downloaded %s', ' '.join(downloaded)) diff --git a/src/pip/_internal/commands/wheel.py b/src/pip/_internal/commands/wheel.py index 38a9b197f..8f5783c35 100644 --- a/src/pip/_internal/commands/wheel.py +++ b/src/pip/_internal/commands/wheel.py @@ -21,6 +21,7 @@ if MYPY_CHECK_RUNNING: from optparse import Values from typing import List + from pip._internal.req.req_install import InstallRequirement logger = logging.getLogger(__name__) @@ -156,10 +157,12 @@ class WheelCommand(RequirementCommand): reqs, check_supported_wheels=True ) - reqs_to_build = [ - r for r in requirement_set.requirements.values() - if should_build_for_wheel_command(r) - ] + reqs_to_build = [] # type: List[InstallRequirement] + for req in requirement_set.requirements.values(): + if req.is_wheel: + preparer.save_linked_requirement(req) + elif should_build_for_wheel_command(req): + reqs_to_build.append(req) # build wheels build_successes, build_failures = build( diff --git a/src/pip/_internal/operations/prepare.py b/src/pip/_internal/operations/prepare.py index 8767115b8..de017504a 100644 --- a/src/pip/_internal/operations/prepare.py +++ b/src/pip/_internal/operations/prepare.py @@ -523,23 +523,33 @@ class RequirementPreparer(object): dist = _get_prepared_distribution( req, self.req_tracker, self.finder, self.build_isolation, ) + return dist - if self.download_dir is not None: - if link.is_existing_dir(): - logger.info('Link is a directory, ignoring download_dir') - elif local_file: - download_location = os.path.join( - self.download_dir, link.filename - ) - if not os.path.exists(download_location): - shutil.copy(local_file.path, download_location) - download_path = display_path(download_location) - logger.info('Saved %s', download_path) - + def save_linked_requirement(self, req): + # type: (InstallRequirement) -> None + assert self.download_dir is not None + assert req.link is not None + link = req.link if link.is_vcs: # Make a .zip of the source_dir we already created. req.archive(self.download_dir) - return dist + return + + if link.is_existing_dir(): + logger.debug( + 'Not copying link to destination directory ' + 'since it is a directory: %s', link, + ) + return + if req.local_file_path is None: + # No distribution was downloaded for this requirement. + return + + download_location = os.path.join(self.download_dir, link.filename) + if not os.path.exists(download_location): + shutil.copy(req.local_file_path, download_location) + download_path = display_path(download_location) + logger.info('Saved %s', download_path) def prepare_editable_requirement( self,