From ea1d5ac484cc635abf1f4dd3d5cb0ae6e1ae8f4e Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Wed, 31 Oct 2018 05:56:40 +0900 Subject: [PATCH 1/5] Isolate, reuse PackageFinder best candidate logic Split out how PackageFinder finds the best candidate, and reuse it in the self version check, to avoid the latter duplicating (and incorrectly implementing) the same logic. --- news/5175.bugfix | 1 + src/pip/_internal/index.py | 123 ++++++++++++++++++++-------- src/pip/_internal/utils/outdated.py | 9 +- tests/unit/test_unit_outdated.py | 7 +- 4 files changed, 99 insertions(+), 41 deletions(-) create mode 100644 news/5175.bugfix diff --git a/news/5175.bugfix b/news/5175.bugfix new file mode 100644 index 00000000000..1ab90f4f35d --- /dev/null +++ b/news/5175.bugfix @@ -0,0 +1 @@ +Make pip's self version check avoid recommending upgrades to prereleases if the currently-installed version is stable. diff --git a/src/pip/_internal/index.py b/src/pip/_internal/index.py index f251c4b58b6..173d90c3a8f 100644 --- a/src/pip/_internal/index.py +++ b/src/pip/_internal/index.py @@ -55,7 +55,8 @@ BuildTag = Tuple[Any, ...] # either empty tuple or Tuple[int, str] CandidateSortingKey = Tuple[int, _BaseVersion, BuildTag, Optional[int]] -__all__ = ['FormatControl', 'PackageFinder'] + +__all__ = ['FormatControl', 'FoundCandidates', 'PackageFinder'] SECURE_ORIGINS = [ @@ -254,6 +255,63 @@ def _get_html_page(link, session=None): return None +class FoundCandidates(object): + """A collection of candidates, returned by `PackageFinder.find_candidates`. + + Arguments: + + * `candidates`: A sequence of all available candidates found. + * `specifier`: Specifier to filter applicable versions. + * `prereleases`: Whether prereleases should be accounted. Pass None to + infer from the specifier. + * `sort_key`: A callable used as the key function when choosing the best + candidate. + """ + def __init__( + self, + candidates, # type: List[InstallationCandidate] + specifier, # type: specifiers.BaseSpecifier + prereleases, # type: Optional[bool] + sort_key, # type: Callable[[InstallationCandidate], Any] + ): + # type: (...) -> None + self._candidates = candidates + self._specifier = specifier + self._prereleases = prereleases + self._sort_key = sort_key + + @property + def all(self): + # type: () -> Iterable[InstallationCandidate] + return iter(self._candidates) + + @property + def applicable(self): + # type: () -> Iterable[InstallationCandidate] + # Filter out anything which doesn't match our specifier. + versions = set(self._specifier.filter( + # We turn the version object into a str here because otherwise + # when we're debundled but setuptools isn't, Python will see + # packaging.version.Version and + # pkg_resources._vendor.packaging.version.Version as different + # types. This way we'll use a str as a common data interchange + # format. If we stop using the pkg_resources provided specifier + # and start using our own, we can drop the cast to str(). + [str(c.version) for c in self._candidates], + prereleases=self._prereleases, + )) + # Again, converting to str to deal with debundling. + return (c for c in self._candidates if str(c.version) in versions) + + @property + def best(self): + # type: () -> Optional[InstallationCandidate] + try: + return max(self.applicable, key=self._sort_key) + except ValueError: # Raised by max() if self.applicable is empty. + return None + + class PackageFinder(object): """This finds packages. @@ -628,6 +686,27 @@ def find_all_candidates(self, project_name): # This is an intentional priority ordering return file_versions + find_links_versions + page_versions + def find_candidates(self, project_name, specifier): + """Find matches for the given project and specifier. + + `specifier` should implement `filter` to allow version filtering (e.g. + ``packaging.specifiers.SpecifierSet``). + + Returns a FoundCandidates instance that contains the following + properties: + + * `all`: All candidates matching `project_name` found on the index. + * `applicable`: Candidates matching `project_name` and `specifier`. + * `best`: The best candidate available. This may be None if no + applicable candidates are found. + """ + return FoundCandidates( + self.find_all_candidates(project_name), + specifier=specifier, + prereleases=(self.allow_all_prereleases or None), + sort_key=self._candidate_sort_key, + ) + def find_requirement(self, req, upgrade): # type: (InstallRequirement, bool) -> Optional[Link] """Try to find a Link matching req @@ -636,35 +715,8 @@ def find_requirement(self, req, upgrade): Returns a Link if found, Raises DistributionNotFound or BestVersionAlreadyInstalled otherwise """ - all_candidates = self.find_all_candidates(req.name) - - # Filter out anything which doesn't match our specifier - compatible_versions = set( - req.specifier.filter( - # We turn the version object into a str here because otherwise - # when we're debundled but setuptools isn't, Python will see - # packaging.version.Version and - # pkg_resources._vendor.packaging.version.Version as different - # types. This way we'll use a str as a common data interchange - # format. If we stop using the pkg_resources provided specifier - # and start using our own, we can drop the cast to str(). - [str(c.version) for c in all_candidates], - prereleases=( - self.allow_all_prereleases - if self.allow_all_prereleases else None - ), - ) - ) - applicable_candidates = [ - # Again, converting to str to deal with debundling. - c for c in all_candidates if str(c.version) in compatible_versions - ] - - if applicable_candidates: - best_candidate = max(applicable_candidates, - key=self._candidate_sort_key) - else: - best_candidate = None + candidates = self.find_candidates(req.name, req.specifier) + best_candidate = candidates.best if req.satisfied_by is not None: installed_version = parse_version(req.satisfied_by.version) @@ -678,7 +730,7 @@ def find_requirement(self, req, upgrade): req, ', '.join( sorted( - {str(c.version) for c in all_candidates}, + {str(c.version) for c in candidates.all}, key=parse_version, ) ) @@ -710,21 +762,24 @@ def find_requirement(self, req, upgrade): ) return None + compatible_version_display = ", ".join( + str(v) for v in sorted({c.version for c in candidates.applicable}) + ) or "none" + if best_installed: # We have an existing version, and its the best version logger.debug( 'Installed version (%s) is most up-to-date (past versions: ' '%s)', installed_version, - ', '.join(sorted(compatible_versions, key=parse_version)) or - "none", + compatible_version_display, ) raise BestVersionAlreadyInstalled logger.debug( 'Using version %s (newest of versions: %s)', best_candidate.version, - ', '.join(sorted(compatible_versions, key=parse_version)) + compatible_version_display, ) return best_candidate.location diff --git a/src/pip/_internal/utils/outdated.py b/src/pip/_internal/utils/outdated.py index f7692761186..7d7d50cfdef 100644 --- a/src/pip/_internal/utils/outdated.py +++ b/src/pip/_internal/utils/outdated.py @@ -8,6 +8,7 @@ from pip._vendor import lockfile, pkg_resources from pip._vendor.packaging import version as packaging_version +from pip._vendor.packaging.specifiers import SpecifierSet from pip._internal.index import PackageFinder from pip._internal.utils.compat import WINDOWS @@ -129,12 +130,10 @@ def pip_version_check(session, options): trusted_hosts=options.trusted_hosts, session=session, ) - all_candidates = finder.find_all_candidates("pip") - if not all_candidates: + candidate = finder.find_candidates("pip", SpecifierSet()).best + if candidate is None: return - pypi_version = str( - max(all_candidates, key=lambda c: c.version).version - ) + pypi_version = str(candidate.version) # save that we've performed a check state.save(pypi_version, current_time) diff --git a/tests/unit/test_unit_outdated.py b/tests/unit/test_unit_outdated.py index 31b5a7fa384..0696b71c7a4 100644 --- a/tests/unit/test_unit_outdated.py +++ b/tests/unit/test_unit_outdated.py @@ -1,3 +1,4 @@ +import collections import datetime import os import sys @@ -28,8 +29,10 @@ class MockPackageFinder(object): def __init__(self, *args, **kwargs): pass - def find_all_candidates(self, project_name): - return self.INSTALLATION_CANDIDATES + def find_candidates(self, project_name, specifier): + return collections.namedtuple("FoundCandidates", "best")( + best=self.INSTALLATION_CANDIDATES[0], + ) class MockDistribution(object): From 1e3b4dbfe2fa214c20664c9454af9e872dddafb9 Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Mon, 8 Apr 2019 15:32:42 +0800 Subject: [PATCH 2/5] Turn FoundCandidates properties into functions Also fix the missing parse_version call, and add comment for it. --- src/pip/_internal/index.py | 56 ++++++++++++++--------------- src/pip/_internal/utils/outdated.py | 2 +- 2 files changed, 28 insertions(+), 30 deletions(-) diff --git a/src/pip/_internal/index.py b/src/pip/_internal/index.py index 173d90c3a8f..baa061534bf 100644 --- a/src/pip/_internal/index.py +++ b/src/pip/_internal/index.py @@ -280,14 +280,16 @@ def __init__( self._prereleases = prereleases self._sort_key = sort_key - @property - def all(self): + def iter_all(self): # type: () -> Iterable[InstallationCandidate] + """Iterate through all candidates. + """ return iter(self._candidates) - @property - def applicable(self): + def iter_applicable(self): # type: () -> Iterable[InstallationCandidate] + """Iterate through candidates matching the given specifier. + """ # Filter out anything which doesn't match our specifier. versions = set(self._specifier.filter( # We turn the version object into a str here because otherwise @@ -303,13 +305,15 @@ def applicable(self): # Again, converting to str to deal with debundling. return (c for c in self._candidates if str(c.version) in versions) - @property - def best(self): + def get_best(self): # type: () -> Optional[InstallationCandidate] - try: - return max(self.applicable, key=self._sort_key) - except ValueError: # Raised by max() if self.applicable is empty. + """Return the best candidate available, or None if no applicable + candidates are found. + """ + candidates = list(self.iter_applicable()) + if not candidates: return None + return max(candidates, key=self._sort_key) class PackageFinder(object): @@ -692,13 +696,7 @@ def find_candidates(self, project_name, specifier): `specifier` should implement `filter` to allow version filtering (e.g. ``packaging.specifiers.SpecifierSet``). - Returns a FoundCandidates instance that contains the following - properties: - - * `all`: All candidates matching `project_name` found on the index. - * `applicable`: Candidates matching `project_name` and `specifier`. - * `best`: The best candidate available. This may be None if no - applicable candidates are found. + Returns a `FoundCandidates` instance. """ return FoundCandidates( self.find_all_candidates(project_name), @@ -716,24 +714,28 @@ def find_requirement(self, req, upgrade): Raises DistributionNotFound or BestVersionAlreadyInstalled otherwise """ candidates = self.find_candidates(req.name, req.specifier) - best_candidate = candidates.best + best_candidate = candidates.get_best() if req.satisfied_by is not None: installed_version = parse_version(req.satisfied_by.version) else: installed_version = None + def _format_versions(cand_iter): + # This repeated parse_version and str() conversion is needed to + # handle different vendoring sources from pip and pkg_resources. + # If we stop using the pkg_resources provided specifier. + return ", ".join(sorted( + {str(c.version) for c in cand_iter}, + key=parse_version, + )) or "none" + if installed_version is None and best_candidate is None: logger.critical( 'Could not find a version that satisfies the requirement %s ' '(from versions: %s)', req, - ', '.join( - sorted( - {str(c.version) for c in candidates.all}, - key=parse_version, - ) - ) + _format_versions(candidates.iter_all()), ) raise DistributionNotFound( @@ -762,24 +764,20 @@ def find_requirement(self, req, upgrade): ) return None - compatible_version_display = ", ".join( - str(v) for v in sorted({c.version for c in candidates.applicable}) - ) or "none" - if best_installed: # We have an existing version, and its the best version logger.debug( 'Installed version (%s) is most up-to-date (past versions: ' '%s)', installed_version, - compatible_version_display, + _format_versions(candidates.iter_applicable()), ) raise BestVersionAlreadyInstalled logger.debug( 'Using version %s (newest of versions: %s)', best_candidate.version, - compatible_version_display, + _format_versions(candidates.iter_applicable()), ) return best_candidate.location diff --git a/src/pip/_internal/utils/outdated.py b/src/pip/_internal/utils/outdated.py index 7d7d50cfdef..071596b7905 100644 --- a/src/pip/_internal/utils/outdated.py +++ b/src/pip/_internal/utils/outdated.py @@ -130,7 +130,7 @@ def pip_version_check(session, options): trusted_hosts=options.trusted_hosts, session=session, ) - candidate = finder.find_candidates("pip", SpecifierSet()).best + candidate = finder.find_candidates("pip", SpecifierSet()).get_best() if candidate is None: return pypi_version = str(candidate.version) From adc498a662d9382d69aa751436f84cc731bb8b36 Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Mon, 8 Apr 2019 15:44:11 +0800 Subject: [PATCH 3/5] Make specifier argument optional --- src/pip/_internal/index.py | 10 +++++++--- src/pip/_internal/utils/outdated.py | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/pip/_internal/index.py b/src/pip/_internal/index.py index baa061534bf..eeb8a55d428 100644 --- a/src/pip/_internal/index.py +++ b/src/pip/_internal/index.py @@ -690,11 +690,15 @@ def find_all_candidates(self, project_name): # This is an intentional priority ordering return file_versions + find_links_versions + page_versions - def find_candidates(self, project_name, specifier): + def find_candidates( + self, + project_name, # type: str + specifier=specifiers.SpecifierSet(), # type: specifiers.BaseSpecifier + ): """Find matches for the given project and specifier. - `specifier` should implement `filter` to allow version filtering (e.g. - ``packaging.specifiers.SpecifierSet``). + If given, `specifier` should implement `filter` to allow version + filtering (e.g. ``packaging.specifiers.SpecifierSet``). Returns a `FoundCandidates` instance. """ diff --git a/src/pip/_internal/utils/outdated.py b/src/pip/_internal/utils/outdated.py index 071596b7905..e5b418591a7 100644 --- a/src/pip/_internal/utils/outdated.py +++ b/src/pip/_internal/utils/outdated.py @@ -130,7 +130,7 @@ def pip_version_check(session, options): trusted_hosts=options.trusted_hosts, session=session, ) - candidate = finder.find_candidates("pip", SpecifierSet()).get_best() + candidate = finder.find_candidates("pip").get_best() if candidate is None: return pypi_version = str(candidate.version) From 4dfda6b23bfba7ab6d22f4287a715256e93b25b4 Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Mon, 8 Apr 2019 15:50:13 +0800 Subject: [PATCH 4/5] Keep linters happy --- src/pip/_internal/index.py | 3 +-- src/pip/_internal/utils/outdated.py | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/pip/_internal/index.py b/src/pip/_internal/index.py index eeb8a55d428..7f20c660c19 100644 --- a/src/pip/_internal/index.py +++ b/src/pip/_internal/index.py @@ -720,10 +720,9 @@ def find_requirement(self, req, upgrade): candidates = self.find_candidates(req.name, req.specifier) best_candidate = candidates.get_best() + installed_version = None # type: Optional[_BaseVersion] if req.satisfied_by is not None: installed_version = parse_version(req.satisfied_by.version) - else: - installed_version = None def _format_versions(cand_iter): # This repeated parse_version and str() conversion is needed to diff --git a/src/pip/_internal/utils/outdated.py b/src/pip/_internal/utils/outdated.py index e5b418591a7..3b58cd5e2ff 100644 --- a/src/pip/_internal/utils/outdated.py +++ b/src/pip/_internal/utils/outdated.py @@ -8,7 +8,6 @@ from pip._vendor import lockfile, pkg_resources from pip._vendor.packaging import version as packaging_version -from pip._vendor.packaging.specifiers import SpecifierSet from pip._internal.index import PackageFinder from pip._internal.utils.compat import WINDOWS From 2882042811bf4c3ed969f4d44149a07195cbca9e Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Mon, 8 Apr 2019 15:52:16 +0800 Subject: [PATCH 5/5] Fix mock --- tests/unit/test_unit_outdated.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/unit/test_unit_outdated.py b/tests/unit/test_unit_outdated.py index 0696b71c7a4..9d46ac41c84 100644 --- a/tests/unit/test_unit_outdated.py +++ b/tests/unit/test_unit_outdated.py @@ -1,4 +1,3 @@ -import collections import datetime import os import sys @@ -13,6 +12,14 @@ from pip._internal.utils import outdated +class MockFoundCandidates(object): + def __init__(self, best): + self._best = best + + def get_best(self): + return self._best + + class MockPackageFinder(object): BASE_URL = 'https://pypi.org/simple/pip-{0}.tar.gz' @@ -29,10 +36,8 @@ class MockPackageFinder(object): def __init__(self, *args, **kwargs): pass - def find_candidates(self, project_name, specifier): - return collections.namedtuple("FoundCandidates", "best")( - best=self.INSTALLATION_CANDIDATES[0], - ) + def find_candidates(self, project_name): + return MockFoundCandidates(self.INSTALLATION_CANDIDATES[0]) class MockDistribution(object):