Isolate, reuse PackageFinder best candidate logic (#5971)

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

@@ -0,0 +1 @@
+Make pip's self version check avoid recommending upgrades to prereleases if the currently-installed version is stable.

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,67 @@ 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
+
+ def iter_all(self):
+ # type: () -> Iterable[InstallationCandidate]
+ """Iterate through all candidates.
+ """
+ return iter(self._candidates)
+
+ 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
+ # 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)
+
+ def get_best(self):
+ # type: () -> Optional[InstallationCandidate]
+ """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):
  """This finds packages.
 
@@ -628,6 +690,25 @@ 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, # type: str
+ specifier=specifiers.SpecifierSet(), # type: specifiers.BaseSpecifier
+ ):
+ """Find matches for the given project and specifier.
+
+ If given, `specifier` should implement `filter` to allow version
+ filtering (e.g. ``packaging.specifiers.SpecifierSet``).
+
+ Returns a `FoundCandidates` instance.
+ """
+ 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,52 +717,28 @@ 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.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
+ # 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 all_candidates},
- key=parse_version,
- )
- )
+ _format_versions(candidates.iter_all()),
  )
 
  raise DistributionNotFound(
@@ -716,15 +773,14 @@ def find_requirement(self, req, upgrade):
  'Installed version (%s) is most up-to-date (past versions: '
  '%s)',
  installed_version,
- ', '.join(sorted(compatible_versions, key=parse_version)) or
- "none",
+ _format_versions(candidates.iter_applicable()),
  )
  raise BestVersionAlreadyInstalled
 
  logger.debug(
  'Using version %s (newest of versions: %s)',
  best_candidate.version,
- ', '.join(sorted(compatible_versions, key=parse_version))
+ _format_versions(candidates.iter_applicable()),
  )
  return best_candidate.location
 

src/pip/_internal/utils/outdated.py

@@ -129,12 +129,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").get_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)

tests/unit/test_unit_outdated.py

@@ -12,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'
@@ -28,8 +36,8 @@ 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):
+ return MockFoundCandidates(self.INSTALLATION_CANDIDATES[0])
 
 
 class MockDistribution(object):