From f35f37ef72cf00ad84d72b81e778d42456e769f1 Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Fri, 24 Apr 2020 18:51:29 +0800 Subject: [PATCH 1/3] Add find_requirement() wrapper in legacy resolver --- src/pip/_internal/index/package_finder.py | 6 +- .../_internal/resolution/legacy/resolver.py | 12 +++- tests/unit/test_finder.py | 67 ++++++++++--------- 3 files changed, 47 insertions(+), 38 deletions(-) diff --git a/src/pip/_internal/index/package_finder.py b/src/pip/_internal/index/package_finder.py index e88ad9f5c69..98a6acaab5f 100644 --- a/src/pip/_internal/index/package_finder.py +++ b/src/pip/_internal/index/package_finder.py @@ -889,11 +889,11 @@ def find_best_candidate( return candidate_evaluator.compute_best_candidate(candidates) def find_requirement(self, req, upgrade): - # type: (InstallRequirement, bool) -> Optional[Link] + # type: (InstallRequirement, bool) -> Optional[InstallationCandidate] """Try to find a Link matching req Expects req, an InstallRequirement and upgrade, a boolean - Returns a Link if found, + Returns a InstallationCandidate if found, Raises DistributionNotFound or BestVersionAlreadyInstalled otherwise """ hashes = req.hashes(trust_internet=False) @@ -967,7 +967,7 @@ def _format_versions(cand_iter): best_candidate.version, _format_versions(best_candidate_result.iter_applicable()), ) - return best_candidate.link + return best_candidate def _find_name_version_sep(fragment, canonical_name): diff --git a/src/pip/_internal/resolution/legacy/resolver.py b/src/pip/_internal/resolution/legacy/resolver.py index cdb44d19dbe..3cf68bbd28a 100644 --- a/src/pip/_internal/resolution/legacy/resolver.py +++ b/src/pip/_internal/resolution/legacy/resolver.py @@ -46,6 +46,7 @@ from pip._internal.cache import WheelCache from pip._internal.distributions import AbstractDistribution from pip._internal.index.package_finder import PackageFinder + from pip._internal.models.link import Link from pip._internal.operations.prepare import RequirementPreparer from pip._internal.req.req_install import InstallRequirement from pip._internal.resolution.base import InstallRequirementProvider @@ -260,6 +261,14 @@ def _check_skip_installed(self, req_to_install): self._set_req_to_reinstall(req_to_install) return None + def _find_requirement_link(self, req): + # type: (InstallRequirement) -> Optional[Link] + upgrade = self._is_upgrade_allowed(req) + best_candidate = self.finder.find_requirement(req, upgrade) + if not best_candidate: + return None + return best_candidate.link + def _populate_link(self, req): # type: (InstallRequirement) -> None """Ensure that if a link can be found for this, that it is found. @@ -274,9 +283,8 @@ def _populate_link(self, req): mismatches. Furthermore, cached wheels at present have undeterministic contents due to file modification times. """ - upgrade = self._is_upgrade_allowed(req) if req.link is None: - req.link = self.finder.find_requirement(req, upgrade) + req.link = self._find_requirement_link(req) if self.wheel_cache is None or self.preparer.require_hashes: return diff --git a/tests/unit/test_finder.py b/tests/unit/test_finder.py index f8c143bc74f..853af723b5a 100644 --- a/tests/unit/test_finder.py +++ b/tests/unit/test_finder.py @@ -48,7 +48,7 @@ def test_no_mpkg(data): req = install_req_from_line("pkgwithmpkg") found = finder.find_requirement(req, False) - assert found.url.endswith("pkgwithmpkg-1.0.tar.gz"), found + assert found.link.url.endswith("pkgwithmpkg-1.0.tar.gz"), found def test_no_partial_name_match(data): @@ -57,7 +57,7 @@ def test_no_partial_name_match(data): req = install_req_from_line("gmpy") found = finder.find_requirement(req, False) - assert found.url.endswith("gmpy-1.15.tar.gz"), found + assert found.link.url.endswith("gmpy-1.15.tar.gz"), found def test_tilde(): @@ -79,23 +79,23 @@ def test_duplicates_sort_ok(data): req = install_req_from_line("duplicate") found = finder.find_requirement(req, False) - assert found.url.endswith("duplicate-1.0.tar.gz"), found + assert found.link.url.endswith("duplicate-1.0.tar.gz"), found def test_finder_detects_latest_find_links(data): """Test PackageFinder detects latest using find-links""" req = install_req_from_line('simple', None) finder = make_test_finder(find_links=[data.find_links]) - link = finder.find_requirement(req, False) - assert link.url.endswith("simple-3.0.tar.gz") + found = finder.find_requirement(req, False) + assert found.link.url.endswith("simple-3.0.tar.gz") def test_incorrect_case_file_index(data): """Test PackageFinder detects latest using wrong case""" req = install_req_from_line('dinner', None) finder = make_test_finder(index_urls=[data.find_links3]) - link = finder.find_requirement(req, False) - assert link.url.endswith("Dinner-2.0.tar.gz") + found = finder.find_requirement(req, False) + assert found.link.url.endswith("Dinner-2.0.tar.gz") @pytest.mark.network @@ -180,7 +180,7 @@ def test_find_wheel_supported(self, data, monkeypatch): finder = make_test_finder(find_links=[data.find_links]) found = finder.find_requirement(req, True) assert ( - found.url.endswith("simple.dist-0.1-py2.py3-none-any.whl") + found.link.url.endswith("simple.dist-0.1-py2.py3-none-any.whl") ), found def test_wheel_over_sdist_priority(self, data): @@ -191,7 +191,8 @@ def test_wheel_over_sdist_priority(self, data): req = install_req_from_line("priority") finder = make_test_finder(find_links=[data.find_links]) found = finder.find_requirement(req, True) - assert found.url.endswith("priority-1.0-py2.py3-none-any.whl"), found + assert found.link.url.endswith("priority-1.0-py2.py3-none-any.whl"), \ + found def test_existing_over_wheel_priority(self, data): """ @@ -292,8 +293,8 @@ def test_finder_priority_file_over_page(data): assert all(version.link.scheme == 'https' for version in all_versions[1:]), all_versions - link = finder.find_requirement(req, False) - assert link.url.startswith("file://") + found = finder.find_requirement(req, False) + assert found.link.url.startswith("file://") def test_finder_priority_nonegg_over_eggfragments(): @@ -306,9 +307,9 @@ def test_finder_priority_nonegg_over_eggfragments(): assert all_versions[0].link.url.endswith('tar.gz') assert all_versions[1].link.url.endswith('#egg=bar-1.0') - link = finder.find_requirement(req, False) + found = finder.find_requirement(req, False) - assert link.url.endswith('tar.gz') + assert found.link.url.endswith('tar.gz') links.reverse() @@ -316,9 +317,9 @@ def test_finder_priority_nonegg_over_eggfragments(): all_versions = finder.find_all_candidates(req.name) assert all_versions[0].link.url.endswith('tar.gz') assert all_versions[1].link.url.endswith('#egg=bar-1.0') - link = finder.find_requirement(req, False) + found = finder.find_requirement(req, False) - assert link.url.endswith('tar.gz') + assert found.link.url.endswith('tar.gz') def test_finder_only_installs_stable_releases(data): @@ -330,21 +331,21 @@ def test_finder_only_installs_stable_releases(data): # using a local index (that has pre & dev releases) finder = make_test_finder(index_urls=[data.index_url("pre")]) - link = finder.find_requirement(req, False) - assert link.url.endswith("bar-1.0.tar.gz"), link.url + found = finder.find_requirement(req, False) + assert found.link.url.endswith("bar-1.0.tar.gz"), found.link.url # using find-links links = ["https://foo/bar-1.0.tar.gz", "https://foo/bar-2.0b1.tar.gz"] finder = make_no_network_finder(links) - link = finder.find_requirement(req, False) - assert link.url == "https://foo/bar-1.0.tar.gz" + found = finder.find_requirement(req, False) + assert found.link.url == "https://foo/bar-1.0.tar.gz" links.reverse() finder = make_no_network_finder(links) - link = finder.find_requirement(req, False) - assert link.url == "https://foo/bar-1.0.tar.gz" + found = finder.find_requirement(req, False) + assert found.link.url == "https://foo/bar-1.0.tar.gz" def test_finder_only_installs_data_require(data): @@ -383,21 +384,21 @@ def test_finder_installs_pre_releases(data): index_urls=[data.index_url("pre")], allow_all_prereleases=True, ) - link = finder.find_requirement(req, False) - assert link.url.endswith("bar-2.0b1.tar.gz"), link.url + found = finder.find_requirement(req, False) + assert found.link.url.endswith("bar-2.0b1.tar.gz"), found.link.url # using find-links links = ["https://foo/bar-1.0.tar.gz", "https://foo/bar-2.0b1.tar.gz"] finder = make_no_network_finder(links, allow_all_prereleases=True) - link = finder.find_requirement(req, False) - assert link.url == "https://foo/bar-2.0b1.tar.gz" + found = finder.find_requirement(req, False) + assert found.link.url == "https://foo/bar-2.0b1.tar.gz" links.reverse() finder = make_no_network_finder(links, allow_all_prereleases=True) - link = finder.find_requirement(req, False) - assert link.url == "https://foo/bar-2.0b1.tar.gz" + found = finder.find_requirement(req, False) + assert found.link.url == "https://foo/bar-2.0b1.tar.gz" def test_finder_installs_dev_releases(data): @@ -412,8 +413,8 @@ def test_finder_installs_dev_releases(data): index_urls=[data.index_url("dev")], allow_all_prereleases=True, ) - link = finder.find_requirement(req, False) - assert link.url.endswith("bar-2.0.dev1.tar.gz"), link.url + found = finder.find_requirement(req, False) + assert found.link.url.endswith("bar-2.0.dev1.tar.gz"), found.link.url def test_finder_installs_pre_releases_with_version_spec(): @@ -424,14 +425,14 @@ def test_finder_installs_pre_releases_with_version_spec(): links = ["https://foo/bar-1.0.tar.gz", "https://foo/bar-2.0b1.tar.gz"] finder = make_no_network_finder(links) - link = finder.find_requirement(req, False) - assert link.url == "https://foo/bar-2.0b1.tar.gz" + found = finder.find_requirement(req, False) + assert found.link.url == "https://foo/bar-2.0b1.tar.gz" links.reverse() finder = make_no_network_finder(links) - link = finder.find_requirement(req, False) - assert link.url == "https://foo/bar-2.0b1.tar.gz" + found = finder.find_requirement(req, False) + assert found.link.url == "https://foo/bar-2.0b1.tar.gz" class TestLinkEvaluator(object): From 806067f09f6a6e0fd605728e0914a986dd4f936f Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Fri, 24 Apr 2020 18:53:56 +0800 Subject: [PATCH 2/3] Move yanked link warning into the legacy resolver --- src/pip/_internal/index/package_finder.py | 16 ---------------- src/pip/_internal/resolution/legacy/resolver.py | 17 ++++++++++++++++- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/src/pip/_internal/index/package_finder.py b/src/pip/_internal/index/package_finder.py index 98a6acaab5f..441992b92b3 100644 --- a/src/pip/_internal/index/package_finder.py +++ b/src/pip/_internal/index/package_finder.py @@ -556,23 +556,7 @@ def sort_best_candidate( """ if not candidates: return None - best_candidate = max(candidates, key=self._sort_key) - - # Log a warning per PEP 592 if necessary before returning. - link = best_candidate.link - if link.is_yanked: - reason = link.yanked_reason or '' - msg = ( - # Mark this as a unicode string to prevent - # "UnicodeEncodeError: 'ascii' codec can't encode character" - # in Python 2 when the reason contains non-ascii characters. - u'The candidate selected for download or install is a ' - 'yanked version: {candidate}\n' - 'Reason for being yanked: {reason}' - ).format(candidate=best_candidate, reason=reason) - logger.warning(msg) - return best_candidate def compute_best_candidate( diff --git a/src/pip/_internal/resolution/legacy/resolver.py b/src/pip/_internal/resolution/legacy/resolver.py index 3cf68bbd28a..2854e21033d 100644 --- a/src/pip/_internal/resolution/legacy/resolver.py +++ b/src/pip/_internal/resolution/legacy/resolver.py @@ -267,7 +267,22 @@ def _find_requirement_link(self, req): best_candidate = self.finder.find_requirement(req, upgrade) if not best_candidate: return None - return best_candidate.link + + # Log a warning per PEP 592 if necessary before returning. + link = best_candidate.link + if link.is_yanked: + reason = link.yanked_reason or '' + msg = ( + # Mark this as a unicode string to prevent + # "UnicodeEncodeError: 'ascii' codec can't encode character" + # in Python 2 when the reason contains non-ascii characters. + u'The candidate selected for download or install is a ' + 'yanked version: {candidate}\n' + 'Reason for being yanked: {reason}' + ).format(candidate=best_candidate, reason=reason) + logger.warning(msg) + + return link def _populate_link(self, req): # type: (InstallRequirement) -> None From 367e6617bd1b613f708cb28081f8f331d33be57f Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Sat, 25 Apr 2020 03:14:09 +0800 Subject: [PATCH 3/3] Move yanked message unit tests to use resolver --- tests/lib/index.py | 14 +++ tests/unit/test_index.py | 71 +----------- tests/unit/test_resolution_legacy_resolver.py | 107 ++++++++++++++++++ 3 files changed, 122 insertions(+), 70 deletions(-) create mode 100644 tests/lib/index.py diff --git a/tests/lib/index.py b/tests/lib/index.py new file mode 100644 index 00000000000..0f507a0e7ff --- /dev/null +++ b/tests/lib/index.py @@ -0,0 +1,14 @@ +from pip._internal.models.candidate import InstallationCandidate +from pip._internal.models.link import Link + + +def make_mock_candidate(version, yanked_reason=None, hex_digest=None): + url = 'https://example.com/pkg-{}.tar.gz'.format(version) + if hex_digest is not None: + assert len(hex_digest) == 64 + url += '#sha256={}'.format(hex_digest) + + link = Link(url, yanked_reason=yanked_reason) + candidate = InstallationCandidate('mypackage', version, link) + + return candidate diff --git a/tests/unit/test_index.py b/tests/unit/test_index.py index 24dff7b38b0..2ae847ae0b7 100644 --- a/tests/unit/test_index.py +++ b/tests/unit/test_index.py @@ -15,7 +15,6 @@ _find_name_version_sep, filter_unallowed_hashes, ) -from pip._internal.models.candidate import InstallationCandidate from pip._internal.models.link import Link from pip._internal.models.search_scope import SearchScope from pip._internal.models.selection_prefs import SelectionPreferences @@ -24,18 +23,7 @@ from pip._internal.utils.compatibility_tags import get_supported from pip._internal.utils.hashes import Hashes from tests.lib import CURRENT_PY_VERSION_INFO - - -def make_mock_candidate(version, yanked_reason=None, hex_digest=None): - url = 'https://example.com/pkg-{}.tar.gz'.format(version) - if hex_digest is not None: - assert len(hex_digest) == 64 - url += '#sha256={}'.format(hex_digest) - - link = Link(url, yanked_reason=yanked_reason) - candidate = InstallationCandidate('mypackage', version, link) - - return candidate +from tests.lib.index import make_mock_candidate @pytest.mark.parametrize('requires_python, expected', [ @@ -470,63 +458,6 @@ def test_sort_best_candidate__no_candidates(self): actual = evaluator.sort_best_candidate([]) assert actual is None - def test_sort_best_candidate__all_yanked(self, caplog): - """ - Test all candidates yanked. - """ - candidates = [ - make_mock_candidate('1.0', yanked_reason='bad metadata #1'), - # Put the best candidate in the middle, to test sorting. - make_mock_candidate('3.0', yanked_reason='bad metadata #3'), - make_mock_candidate('2.0', yanked_reason='bad metadata #2'), - ] - expected_best = candidates[1] - evaluator = CandidateEvaluator.create('my-project') - actual = evaluator.sort_best_candidate(candidates) - assert actual is expected_best - assert str(actual.version) == '3.0' - - # Check the log messages. - assert len(caplog.records) == 1 - record = caplog.records[0] - assert record.levelname == 'WARNING' - assert record.message == ( - 'The candidate selected for download or install is a yanked ' - "version: 'mypackage' candidate " - '(version 3.0 at https://example.com/pkg-3.0.tar.gz)\n' - 'Reason for being yanked: bad metadata #3' - ) - - @pytest.mark.parametrize('yanked_reason, expected_reason', [ - # Test no reason given. - ('', ''), - # Test a unicode string with a non-ascii character. - (u'curly quote: \u2018', u'curly quote: \u2018'), - ]) - def test_sort_best_candidate__yanked_reason( - self, caplog, yanked_reason, expected_reason, - ): - """ - Test the log message with various reason strings. - """ - candidates = [ - make_mock_candidate('1.0', yanked_reason=yanked_reason), - ] - evaluator = CandidateEvaluator.create('my-project') - actual = evaluator.sort_best_candidate(candidates) - assert str(actual.version) == '1.0' - - assert len(caplog.records) == 1 - record = caplog.records[0] - assert record.levelname == 'WARNING' - expected_message = ( - 'The candidate selected for download or install is a yanked ' - "version: 'mypackage' candidate " - '(version 1.0 at https://example.com/pkg-1.0.tar.gz)\n' - 'Reason for being yanked: ' - ) + expected_reason - assert record.message == expected_message - def test_sort_best_candidate__best_yanked_but_not_all( self, caplog, ): diff --git a/tests/unit/test_resolution_legacy_resolver.py b/tests/unit/test_resolution_legacy_resolver.py index 64968358d86..561313c002b 100644 --- a/tests/unit/test_resolution_legacy_resolver.py +++ b/tests/unit/test_resolution_legacy_resolver.py @@ -1,5 +1,6 @@ import logging +import mock import pytest from pip._vendor import pkg_resources @@ -7,10 +8,14 @@ NoneMetadataError, UnsupportedPythonVersion, ) +from pip._internal.req.constructors import install_req_from_line from pip._internal.resolution.legacy.resolver import ( + Resolver, _check_dist_requires_python, ) from pip._internal.utils.packaging import get_requires_python +from tests.lib import make_test_finder +from tests.lib.index import make_mock_candidate # We need to inherit from DistInfoDistribution for the `isinstance()` @@ -169,3 +174,105 @@ def test_empty_metadata_error(self, caplog, metadata_name): "None {} metadata found for distribution: " "".format(metadata_name) ) + + +class TestYankedWarning(object): + """ + Test _populate_link() emits warning if one or more candidates are yanked. + """ + def _make_test_resolver(self, monkeypatch, mock_candidates): + def _find_candidates(project_name): + return mock_candidates + + finder = make_test_finder() + monkeypatch.setattr(finder, "find_all_candidates", _find_candidates) + + return Resolver( + finder=finder, + preparer=mock.Mock(), # Not used. + make_install_req=install_req_from_line, + wheel_cache=None, + use_user_site=False, + force_reinstall=False, + ignore_dependencies=False, + ignore_installed=False, + ignore_requires_python=False, + upgrade_strategy="to-satisfy-only", + ) + + def test_sort_best_candidate__has_non_yanked(self, caplog, monkeypatch): + """ + Test unyanked candidate preferred over yanked. + """ + candidates = [ + make_mock_candidate('1.0'), + make_mock_candidate('2.0', yanked_reason='bad metadata #2'), + ] + ireq = install_req_from_line("pkg") + + resolver = self._make_test_resolver(monkeypatch, candidates) + resolver._populate_link(ireq) + + assert ireq.link == candidates[0].link + assert len(caplog.records) == 0 + + def test_sort_best_candidate__all_yanked(self, caplog, monkeypatch): + """ + Test all candidates yanked. + """ + candidates = [ + make_mock_candidate('1.0', yanked_reason='bad metadata #1'), + # Put the best candidate in the middle, to test sorting. + make_mock_candidate('3.0', yanked_reason='bad metadata #3'), + make_mock_candidate('2.0', yanked_reason='bad metadata #2'), + ] + ireq = install_req_from_line("pkg") + + resolver = self._make_test_resolver(monkeypatch, candidates) + resolver._populate_link(ireq) + + assert ireq.link == candidates[1].link + + # Check the log messages. + assert len(caplog.records) == 1 + record = caplog.records[0] + assert record.levelname == 'WARNING' + assert record.message == ( + 'The candidate selected for download or install is a yanked ' + "version: 'mypackage' candidate " + '(version 3.0 at https://example.com/pkg-3.0.tar.gz)\n' + 'Reason for being yanked: bad metadata #3' + ) + + @pytest.mark.parametrize('yanked_reason, expected_reason', [ + # Test no reason given. + ('', ''), + # Test a unicode string with a non-ascii character. + (u'curly quote: \u2018', u'curly quote: \u2018'), + ]) + def test_sort_best_candidate__yanked_reason( + self, caplog, monkeypatch, yanked_reason, expected_reason, + ): + """ + Test the log message with various reason strings. + """ + candidates = [ + make_mock_candidate('1.0', yanked_reason=yanked_reason), + ] + ireq = install_req_from_line("pkg") + + resolver = self._make_test_resolver(monkeypatch, candidates) + resolver._populate_link(ireq) + + assert ireq.link == candidates[0].link + + assert len(caplog.records) == 1 + record = caplog.records[0] + assert record.levelname == 'WARNING' + expected_message = ( + 'The candidate selected for download or install is a yanked ' + "version: 'mypackage' candidate " + '(version 1.0 at https://example.com/pkg-1.0.tar.gz)\n' + 'Reason for being yanked: ' + ) + expected_reason + assert record.message == expected_message