From 112c4335e98243f7cd82d79d8fe2a82d823ece7a Mon Sep 17 00:00:00 2001 From: Christoph Ludwig Date: Thu, 7 Sep 2023 14:52:53 +0200 Subject: [PATCH 01/10] Ignore extras when dropping existing constraints --- .pre-commit-config.yaml | 1 + piptools/resolver.py | 4 +- piptools/utils.py | 11 ++++++ tests/test_resolver.py | 85 ++++++++++++++++++++++++++++++++++++++++- tests/test_utils.py | 27 +++++++++++++ 5 files changed, 125 insertions(+), 3 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 6c5d1a303..fd51dcb83 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -34,6 +34,7 @@ repos: - pip==20.3.4 - build==1.0.0 - pyproject_hooks==1.0.0 + - pytest>=7.2.0 - repo: https://github.com/PyCQA/bandit rev: 1.7.5 hooks: diff --git a/piptools/resolver.py b/piptools/resolver.py index b6d2e1378..e6768f49d 100644 --- a/piptools/resolver.py +++ b/piptools/resolver.py @@ -39,7 +39,7 @@ is_pinned_requirement, is_url_requirement, key_from_ireq, - key_from_req, + key_no_extra_from_req, omit_list_value, strip_extras, ) @@ -648,7 +648,7 @@ def _do_resolve( # Collect all incompatible install requirement names cause_ireq_names = { - key_from_req(cause.requirement) for cause in cause_exc.causes + key_no_extra_from_req(cause.requirement) for cause in cause_exc.causes } # Looks like resolution is impossible, try to fix diff --git a/piptools/utils.py b/piptools/utils.py index 70f1ee17f..596543cc1 100644 --- a/piptools/utils.py +++ b/piptools/utils.py @@ -23,6 +23,7 @@ from click.utils import LazyFile from pip._internal.req import InstallRequirement from pip._internal.req.constructors import install_req_from_line, parse_req_from_line +from pip._internal.resolution.resolvelib.base import Requirement as PipRequirement from pip._internal.utils.misc import redact_auth_from_url from pip._internal.vcs import is_url from pip._vendor.packaging.markers import Marker @@ -77,6 +78,16 @@ def key_from_req(req: InstallRequirement | Requirement) -> str: return str(canonicalize_name(req.name)) +def key_no_extra_from_req( + req: InstallRequirement | Requirement | PipRequirement, +) -> str: + """Get an all-lowercase version of the requirement's name without any extras.""" + name = req.name + extra_start_index = name.find("[") + package_name = name if extra_start_index == -1 else name[:extra_start_index] + return str(canonicalize_name(package_name)) + + def comment(text: str) -> str: return click.style(text, fg="green") diff --git a/tests/test_resolver.py b/tests/test_resolver.py index 9c0627fb4..e79232d39 100644 --- a/tests/test_resolver.py +++ b/tests/test_resolver.py @@ -1,11 +1,23 @@ from __future__ import annotations +from itertools import chain +from typing import Callable, Sequence +from unittest.mock import Mock, NonCallableMock + import pytest from pip._internal.exceptions import DistributionNotFound +from pip._internal.req.req_install import InstallRequirement +from pip._internal.resolution.resolvelib.requirements import SpecifierRequirement +from pip._internal.resolution.resolvelib.resolver import Resolver from pip._internal.utils.urls import path_to_url +from pip._vendor.resolvelib.resolvers import ResolutionImpossible from piptools.exceptions import NoCandidateFound -from piptools.resolver import RequirementSummary, combine_install_requirements +from piptools.resolver import ( + BacktrackingResolver, + RequirementSummary, + combine_install_requirements, +) @pytest.mark.parametrize( @@ -575,3 +587,74 @@ def resolve(self, *args, **kwargs): resolver=FakePipResolver(), compatible_existing_constraints={}, ) + + +@pytest.mark.parametrize( + ("constraints", "conflicting_existing", "compatible_existing"), + ( + ( + [ + "poetry==1.6.1", + ], + ["cachecontrol[filecache]==0.12.14"], + ["doesnotexist==1.0.0"], + ), + ( + [ + "poetry==1.6.1", + ], + ["keyring==22.1.0", "pkginfo==1.7.2"], + ["platformdirs==3.0.0", "doesnotexist==1.0.0"], + ), + ), +) +def test_backtracking_resolver_drops_existing_conflicting_constraints( + backtracking_resolver: Callable[..., BacktrackingResolver], + from_line: Callable[..., InstallRequirement], + constraints: Sequence[str], + conflicting_existing: Sequence[str], + compatible_existing: Sequence[str], +) -> None: + def wrap_resolution_impossible(*args, **kwargs): + """ + Raise a ``DistributionNotFound`` exception that has a ``ResolutionImpossible`` + exception as its cause. + """ + try: + causes = [ + NonCallableMock( + requirement=SpecifierRequirement(existing_constraints[ireq.name]) + ) + for ireq in conflicting_ireqs + ] + raise ResolutionImpossible(causes) + except ResolutionImpossible as e: + raise DistributionNotFound("resolution impossible") from e + + constraint_ireqs = [from_line(req, constraint=False) for req in constraints] + conflicting_ireqs = [ + from_line(req, constraint=True) for req in conflicting_existing + ] + compatible_ireqs = [from_line(req, constraint=True) for req in compatible_existing] + existing_constraints = { + ireq.name: ireq for ireq in chain(compatible_ireqs, conflicting_ireqs) + } + + bt_resolver = backtracking_resolver( + constraint_ireqs, existing_constraints=existing_constraints + ) + resolver = NonCallableMock( + spec=Resolver, + resolve=Mock(side_effect=wrap_resolution_impossible), + ) + + # resolver has been rigged to raise a DistributionNotFound exception with + # a cause that refers to the entries of conflicting_ireqs. + # We expect _do_resolve() to handle this exception by dropping + # the these entries from existing_constraints and returning False. + # It should _not_ drop any compatible constraint or re-raise + # the DistributionNotFound exception. + result = bt_resolver._do_resolve(resolver, existing_constraints) + + assert result is False + assert set(existing_constraints.keys()) == {ireq.name for ireq in compatible_ireqs} diff --git a/tests/test_utils.py b/tests/test_utils.py index 7d31af92f..abde8c3d3 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -11,6 +11,8 @@ import pip import pytest from click import BadOptionUsage, Context, FileError +from pip._internal.resolution.resolvelib.base import Requirement as PipRequirement +from pip._internal.resolution.resolvelib.requirements import SpecifierRequirement from pip._vendor.packaging.version import Version from piptools.scripts.compile import cli as compile_cli @@ -29,6 +31,7 @@ is_pinned_requirement, is_url_requirement, key_from_ireq, + key_no_extra_from_req, lookup_table, lookup_table_from_tuples, override_defaults_from_config_file, @@ -285,6 +288,30 @@ def test_key_from_ireq_normalization(from_line): assert len(keys) == 1 +@pytest.fixture(params=["InstallRequirement", "SpecifierRequirement"]) +def req_factory(from_line, request): + def specified_requirement(line: str) -> PipRequirement: + return SpecifierRequirement(from_line(line)) + + if request.param == "SpecifierRequirement": + return specified_requirement + return from_line + + +@pytest.mark.parametrize( + ("line", "expected"), + ( + ("build", "build"), + ("cachecontrol[filecache]", "cachecontrol"), + ("some-package[a,b]", "some-package"), + ), +) +def test_key_no_extra_from_req(req_factory, line, expected): + result = key_no_extra_from_req(req_factory(line)) + + assert result == expected + + @pytest.mark.parametrize( ("line", "expected"), ( From 8439d8b875023f667a4d85c17671f4fd674621ad Mon Sep 17 00:00:00 2001 From: Christoph Ludwig Date: Thu, 7 Sep 2023 21:58:32 +0200 Subject: [PATCH 02/10] Improve readability of extra stripping and fix typo Co-authored-by: chrysle --- piptools/utils.py | 3 +-- tests/test_resolver.py | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/piptools/utils.py b/piptools/utils.py index 596543cc1..c2e7e5e3e 100644 --- a/piptools/utils.py +++ b/piptools/utils.py @@ -83,8 +83,7 @@ def key_no_extra_from_req( ) -> str: """Get an all-lowercase version of the requirement's name without any extras.""" name = req.name - extra_start_index = name.find("[") - package_name = name if extra_start_index == -1 else name[:extra_start_index] + package_name = name.split("[", 1)[0] return str(canonicalize_name(package_name)) diff --git a/tests/test_resolver.py b/tests/test_resolver.py index e79232d39..cc71c94a0 100644 --- a/tests/test_resolver.py +++ b/tests/test_resolver.py @@ -651,7 +651,7 @@ def wrap_resolution_impossible(*args, **kwargs): # resolver has been rigged to raise a DistributionNotFound exception with # a cause that refers to the entries of conflicting_ireqs. # We expect _do_resolve() to handle this exception by dropping - # the these entries from existing_constraints and returning False. + # these entries from existing_constraints and returning False. # It should _not_ drop any compatible constraint or re-raise # the DistributionNotFound exception. result = bt_resolver._do_resolve(resolver, existing_constraints) From 2d0bb544834e4d23100822a7c832af0e33a0f9ad Mon Sep 17 00:00:00 2001 From: Christoph Ludwig Date: Thu, 7 Sep 2023 22:17:06 +0200 Subject: [PATCH 03/10] Avoid unnecessary try...except --- tests/test_resolver.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/tests/test_resolver.py b/tests/test_resolver.py index cc71c94a0..ce7350979 100644 --- a/tests/test_resolver.py +++ b/tests/test_resolver.py @@ -620,16 +620,15 @@ def wrap_resolution_impossible(*args, **kwargs): Raise a ``DistributionNotFound`` exception that has a ``ResolutionImpossible`` exception as its cause. """ - try: - causes = [ - NonCallableMock( - requirement=SpecifierRequirement(existing_constraints[ireq.name]) - ) - for ireq in conflicting_ireqs - ] - raise ResolutionImpossible(causes) - except ResolutionImpossible as e: - raise DistributionNotFound("resolution impossible") from e + causes = [ + NonCallableMock( + requirement=SpecifierRequirement(existing_constraints[ireq.name]) + ) + for ireq in conflicting_ireqs + ] + raise DistributionNotFound("resolution impossible") from ResolutionImpossible( + causes + ) constraint_ireqs = [from_line(req, constraint=False) for req in constraints] conflicting_ireqs = [ From d7cd1b4ff81f0cc0e19d6371f468a274881ed835 Mon Sep 17 00:00:00 2001 From: Christoph Ludwig Date: Fri, 8 Sep 2023 22:16:04 +0200 Subject: [PATCH 04/10] Merge key_no_extra_from_req into key_from_req --- piptools/resolver.py | 5 ++-- piptools/utils.py | 16 ++++++------- tests/test_utils.py | 54 ++++++++++++++++++++++++++++++++------------ 3 files changed, 49 insertions(+), 26 deletions(-) diff --git a/piptools/resolver.py b/piptools/resolver.py index e6768f49d..d974a131d 100644 --- a/piptools/resolver.py +++ b/piptools/resolver.py @@ -39,7 +39,7 @@ is_pinned_requirement, is_url_requirement, key_from_ireq, - key_no_extra_from_req, + key_from_req, omit_list_value, strip_extras, ) @@ -648,7 +648,8 @@ def _do_resolve( # Collect all incompatible install requirement names cause_ireq_names = { - key_no_extra_from_req(cause.requirement) for cause in cause_exc.causes + key_from_req(cause.requirement, remove_extras=True) + for cause in cause_exc.causes } # Looks like resolution is impossible, try to fix diff --git a/piptools/utils.py b/piptools/utils.py index c2e7e5e3e..96fd2943c 100644 --- a/piptools/utils.py +++ b/piptools/utils.py @@ -73,18 +73,16 @@ def key_from_ireq(ireq: InstallRequirement) -> str: return key_from_req(ireq.req) -def key_from_req(req: InstallRequirement | Requirement) -> str: - """Get an all-lowercase version of the requirement's name.""" - return str(canonicalize_name(req.name)) - - -def key_no_extra_from_req( +def key_from_req( req: InstallRequirement | Requirement | PipRequirement, + *, + remove_extras: bool = False, ) -> str: - """Get an all-lowercase version of the requirement's name without any extras.""" + """Get an all-lowercase version of the requirement's name.""" name = req.name - package_name = name.split("[", 1)[0] - return str(canonicalize_name(package_name)) + if remove_extras: + name = strip_extras(name) + return str(canonicalize_name(name)) def comment(text: str) -> str: diff --git a/tests/test_utils.py b/tests/test_utils.py index abde8c3d3..195816858 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -7,11 +7,12 @@ import sys from pathlib import Path from textwrap import dedent +from typing import Callable import pip import pytest from click import BadOptionUsage, Context, FileError -from pip._internal.resolution.resolvelib.base import Requirement as PipRequirement +from pip._internal.req import InstallRequirement from pip._internal.resolution.resolvelib.requirements import SpecifierRequirement from pip._vendor.packaging.version import Version @@ -31,7 +32,7 @@ is_pinned_requirement, is_url_requirement, key_from_ireq, - key_no_extra_from_req, + key_from_req, lookup_table, lookup_table_from_tuples, override_defaults_from_config_file, @@ -288,26 +289,49 @@ def test_key_from_ireq_normalization(from_line): assert len(keys) == 1 -@pytest.fixture(params=["InstallRequirement", "SpecifierRequirement"]) -def req_factory(from_line, request): - def specified_requirement(line: str) -> PipRequirement: - return SpecifierRequirement(from_line(line)) - - if request.param == "SpecifierRequirement": - return specified_requirement - return from_line - - +@pytest.mark.parametrize("remove_extras", (True, False)) @pytest.mark.parametrize( ("line", "expected"), ( ("build", "build"), ("cachecontrol[filecache]", "cachecontrol"), - ("some-package[a,b]", "some-package"), + ("some-package[a-b,c_d]", "some-package"), + ("other_package[a.b]", "other-package"), + ), +) +def test_key_from_req_on_install_requirement( + from_line: Callable[[str], InstallRequirement], + line: str, + expected: str, + remove_extras: bool, +) -> None: + ireq = from_line(line) + result = key_from_req(ireq, remove_extras=remove_extras) + + assert result == expected + + +@pytest.mark.parametrize( + ("line", "expected", "remove_extras"), + ( + ("build", "build", False), + ("build", "build", True), + ("cachecontrol[filecache]", "cachecontrol[filecache]", False), + ("cachecontrol[filecache]", "cachecontrol", True), + ("some-package[a-b,c_d]", "some-package[a-b,c-d]", False), + ("some-package[a-b,c_d]", "some-package", True), + ("other_package[a.b]", "other-package[a-b]", False), + ("other_package[a.b]", "other-package", True), ), ) -def test_key_no_extra_from_req(req_factory, line, expected): - result = key_no_extra_from_req(req_factory(line)) +def test_key_from_req_on_specifier_requirement( + from_line: Callable[[str], InstallRequirement], + line: str, + expected: str, + remove_extras: bool, +) -> None: + req = SpecifierRequirement(from_line(line)) + result = key_from_req(req, remove_extras=remove_extras) assert result == expected From 58f531b2791b86462f1a237fa45b3b6d0f35aefa Mon Sep 17 00:00:00 2001 From: Christoph Ludwig Date: Fri, 8 Sep 2023 22:29:08 +0200 Subject: [PATCH 05/10] Update docstring of key_from_req --- piptools/utils.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/piptools/utils.py b/piptools/utils.py index 96fd2943c..a2b226bcd 100644 --- a/piptools/utils.py +++ b/piptools/utils.py @@ -78,7 +78,17 @@ def key_from_req( *, remove_extras: bool = False, ) -> str: - """Get an all-lowercase version of the requirement's name.""" + """ + Get an all-lowercase version of the requirement's name. + + :param req: the requirement the key is computed for + :param remove_extras: if this parameter evaluates as ``True``, + then any extras specification that is + part of the requirement's name is stripped + off the result. + :return: the canonical name of the requirement, optionally + with any extras specification removed + """ name = req.name if remove_extras: name = strip_extras(name) From 5326e9318728c569a4b2ac219df77520c4a299bf Mon Sep 17 00:00:00 2001 From: Christoph Ludwig Date: Mon, 11 Sep 2023 20:36:29 +0200 Subject: [PATCH 06/10] Use strip_extras in place of invocation --- piptools/resolver.py | 2 +- piptools/utils.py | 27 +++++++++++---------------- tests/test_utils.py | 21 +++++++-------------- 3 files changed, 19 insertions(+), 31 deletions(-) diff --git a/piptools/resolver.py b/piptools/resolver.py index d974a131d..aaa8cd606 100644 --- a/piptools/resolver.py +++ b/piptools/resolver.py @@ -648,7 +648,7 @@ def _do_resolve( # Collect all incompatible install requirement names cause_ireq_names = { - key_from_req(cause.requirement, remove_extras=True) + strip_extras(key_from_req(cause.requirement)) for cause in cause_exc.causes } diff --git a/piptools/utils.py b/piptools/utils.py index a2b226bcd..eb8e7f249 100644 --- a/piptools/utils.py +++ b/piptools/utils.py @@ -73,26 +73,21 @@ def key_from_ireq(ireq: InstallRequirement) -> str: return key_from_req(ireq.req) -def key_from_req( - req: InstallRequirement | Requirement | PipRequirement, - *, - remove_extras: bool = False, -) -> str: +def key_from_req(req: InstallRequirement | Requirement | PipRequirement) -> str: """ Get an all-lowercase version of the requirement's name. + **Note:** If the argument is an instance of + ``pip._internal.resolution.resolvelib.base.Requirement`` (like + ``pip._internal.resolution.resolvelib.requirements.SpecifierRequirement``), + then the name might include an extras specification. + Apply :py:func:`strip_extras` to the result of this function if you need + the package name only. + :param req: the requirement the key is computed for - :param remove_extras: if this parameter evaluates as ``True``, - then any extras specification that is - part of the requirement's name is stripped - off the result. - :return: the canonical name of the requirement, optionally - with any extras specification removed - """ - name = req.name - if remove_extras: - name = strip_extras(name) - return str(canonicalize_name(name)) + :return: the canonical name of the requirement + """ + return str(canonicalize_name(req.name)) def comment(text: str) -> str: diff --git a/tests/test_utils.py b/tests/test_utils.py index 195816858..edf012aab 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -289,7 +289,6 @@ def test_key_from_ireq_normalization(from_line): assert len(keys) == 1 -@pytest.mark.parametrize("remove_extras", (True, False)) @pytest.mark.parametrize( ("line", "expected"), ( @@ -303,35 +302,29 @@ def test_key_from_req_on_install_requirement( from_line: Callable[[str], InstallRequirement], line: str, expected: str, - remove_extras: bool, ) -> None: ireq = from_line(line) - result = key_from_req(ireq, remove_extras=remove_extras) + result = key_from_req(ireq) assert result == expected @pytest.mark.parametrize( - ("line", "expected", "remove_extras"), + ("line", "expected"), ( - ("build", "build", False), - ("build", "build", True), - ("cachecontrol[filecache]", "cachecontrol[filecache]", False), - ("cachecontrol[filecache]", "cachecontrol", True), - ("some-package[a-b,c_d]", "some-package[a-b,c-d]", False), - ("some-package[a-b,c_d]", "some-package", True), - ("other_package[a.b]", "other-package[a-b]", False), - ("other_package[a.b]", "other-package", True), + ("build", "build"), + ("cachecontrol[filecache]", "cachecontrol[filecache]"), + ("some-package[a-b,c_d]", "some-package[a-b,c-d]"), + ("other_package[a.b]", "other-package[a-b]"), ), ) def test_key_from_req_on_specifier_requirement( from_line: Callable[[str], InstallRequirement], line: str, expected: str, - remove_extras: bool, ) -> None: req = SpecifierRequirement(from_line(line)) - result = key_from_req(req, remove_extras=remove_extras) + result = key_from_req(req) assert result == expected From f50bb90cadd01fa925bfb2ef63a6f235530c19aa Mon Sep 17 00:00:00 2001 From: Christoph Ludwig Date: Mon, 18 Sep 2023 21:53:04 +0200 Subject: [PATCH 07/10] Replace mocked unit with integration test --- tests/conftest.py | 21 ++++-- tests/test_cli_compile.py | 136 ++++++++++++++++++++++++++++++++++++++ tests/test_resolver.py | 84 +---------------------- tests/utils.py | 46 +++++++++++++ 4 files changed, 198 insertions(+), 89 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 0f68570ee..abac23fc9 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -44,7 +44,12 @@ ) from .constants import MINIMAL_WHEELS_PATH, TEST_DATA_PATH -from .utils import looks_like_ci +from .utils import ( + MakePackageProtocol, + MakeSDistProtocol, + RunSetupFileProtocol, + looks_like_ci, +) @dataclass @@ -287,7 +292,7 @@ def pip_with_index_conf(make_pip_conf): @pytest.fixture(scope="session") -def make_package(tmp_path_factory): +def make_package(tmp_path_factory: pytest.TempPathFactory) -> MakePackageProtocol: """ Make a package from a given name, version and list of required packages. """ @@ -333,12 +338,14 @@ def _make_package(name, version="0.1", install_requires=None, extras_require=Non @pytest.fixture(scope="session") -def run_setup_file(): +def run_setup_file() -> RunSetupFileProtocol: """ Run a setup.py file from a given package dir. """ - def _run_setup_file(package_dir_path, *args): + def _run_setup_file( + package_dir_path: Path, *args: str + ) -> subprocess.CompletedProcess[bytes]: setup_file = package_dir_path / "setup.py" return subprocess.run( [sys.executable, str(setup_file), *args], @@ -365,12 +372,14 @@ def _make_wheel(package_dir, dist_dir, *args): @pytest.fixture -def make_sdist(run_setup_file): +def make_sdist(run_setup_file: RunSetupFileProtocol) -> MakeSDistProtocol: """ Make a source distribution from a given package dir. """ - def _make_sdist(package_dir, dist_dir, *args): + def _make_sdist( + package_dir: Path, dist_dir: str | Path, *args: str + ) -> subprocess.CompletedProcess[bytes]: return run_setup_file(package_dir, "sdist", "--dist-dir", str(dist_dir), *args) return _make_sdist diff --git a/tests/test_cli_compile.py b/tests/test_cli_compile.py index bd2bb077a..b5630eb8f 100644 --- a/tests/test_cli_compile.py +++ b/tests/test_cli_compile.py @@ -5,10 +5,12 @@ import shutil import subprocess import sys +from pathlib import Path from textwrap import dedent from unittest import mock import pytest +from click.testing import CliRunner from pip._internal.utils.hashes import FAVORITE_HASH from pip._internal.utils.urls import path_to_url @@ -16,6 +18,7 @@ from piptools.utils import COMPILE_EXCLUDE_OPTIONS from .constants import MINIMAL_WHEELS_PATH, PACKAGES_PATH +from .utils import MakePackageArgs, MakePackageProtocol, MakeSDistProtocol legacy_resolver_only = pytest.mark.parametrize( "current_resolver", @@ -2775,6 +2778,139 @@ def test_cli_compile_strip_extras(runner, make_package, make_sdist, tmpdir): assert "[more]" not in out.stderr +@pytest.mark.parametrize( + ("package_specs", "constraints", "existing_reqs", "expected_reqs"), + ( + ( + [ + { + "name": "test_package_1", + "version": "1.1", + "install_requires": ["test_package_2 ~= 1.1"], + }, + { + "name": "test_package_2", + "version": "1.1", + "extras_require": {"more": "test_package_3"}, + }, + ], + """ + test_package_1 == 1.1 + """, + """ + test_package_1 == 1.0 + test_package_2 == 1.0 + """, + """ + test-package-1==1.1 + test-package-2==1.1 + """, + ), + ( + [ + { + "name": "test_package_1", + "version": "1.1", + "install_requires": ["test_package_2[more] ~= 1.1"], + }, + { + "name": "test_package_2", + "version": "1.1", + "extras_require": {"more": "test_package_3"}, + }, + { + "name": "test_package_3", + "version": "0.1", + }, + ], + """ + test_package_1 == 1.1 + """, + """ + test_package_1 == 1.0 + test_package_2 == 1.0 + test_package_3 == 0.1 + """, + """ + test-package-1==1.1 + test-package-2==1.1 + test-package-3==0.1 + """, + ), + ( + [ + { + "name": "test_package_1", + "version": "1.1", + "install_requires": ["test_package_2[more] ~= 1.1"], + }, + { + "name": "test_package_2", + "version": "1.1", + "extras_require": {"more": "test_package_3"}, + }, + { + "name": "test_package_3", + "version": "0.1", + }, + ], + """ + test_package_1 == 1.1 + """, + """ + test_package_1 == 1.0 + test_package_2[more] == 1.0 + test_package_3 == 0.1 + """, + """ + test-package-1==1.1 + test-package-2==1.1 + test-package-3==0.1 + """, + ), + ), + ids=("no-extra", "extra-stripped-from-existing", "with-extra-in-existing"), +) +def test_resolver_drops_existing_conflicting_constraint( + runner: CliRunner, + make_package: MakePackageProtocol, + make_sdist: MakeSDistProtocol, + tmpdir: Path, + package_specs: list[MakePackageArgs], + constraints: str, + existing_reqs: str, + expected_reqs: str, +) -> None: + """ + Test that the resolver will find a solution even if some of the existing + (indirect) requirements are incompatible with the new constraints. + + This must succeed even if the conflicting requirement includes some extra, + no matter whether the extra is mentioned in the existing requirements + or not (cf. `issue #1977 `_). + """ + expected_requirements = {line.strip() for line in expected_reqs.splitlines()} + dists_dir = tmpdir / "dists" + + packages = [make_package(**spec) for spec in package_specs] + for pkg in packages: + make_sdist(pkg, dists_dir) + + with open("requirements.txt", "w") as existing_reqs_out: + existing_reqs_out.write(dedent(existing_reqs)) + + with open("requirements.in", "w") as constraints_out: + constraints_out.write(dedent(constraints)) + + out = runner.invoke(cli, ["--strip-extras", "--find-links", str(dists_dir)]) + + assert out.exit_code == 0, out + + with open("requirements.txt") as req_txt: + req_txt_content = req_txt.read() + assert expected_requirements.issubset(req_txt_content.splitlines()) + + def test_resolution_failure(runner): """Test resolution impossible for unknown package.""" with open("requirements.in", "w") as reqs_out: diff --git a/tests/test_resolver.py b/tests/test_resolver.py index ce7350979..9c0627fb4 100644 --- a/tests/test_resolver.py +++ b/tests/test_resolver.py @@ -1,23 +1,11 @@ from __future__ import annotations -from itertools import chain -from typing import Callable, Sequence -from unittest.mock import Mock, NonCallableMock - import pytest from pip._internal.exceptions import DistributionNotFound -from pip._internal.req.req_install import InstallRequirement -from pip._internal.resolution.resolvelib.requirements import SpecifierRequirement -from pip._internal.resolution.resolvelib.resolver import Resolver from pip._internal.utils.urls import path_to_url -from pip._vendor.resolvelib.resolvers import ResolutionImpossible from piptools.exceptions import NoCandidateFound -from piptools.resolver import ( - BacktrackingResolver, - RequirementSummary, - combine_install_requirements, -) +from piptools.resolver import RequirementSummary, combine_install_requirements @pytest.mark.parametrize( @@ -587,73 +575,3 @@ def resolve(self, *args, **kwargs): resolver=FakePipResolver(), compatible_existing_constraints={}, ) - - -@pytest.mark.parametrize( - ("constraints", "conflicting_existing", "compatible_existing"), - ( - ( - [ - "poetry==1.6.1", - ], - ["cachecontrol[filecache]==0.12.14"], - ["doesnotexist==1.0.0"], - ), - ( - [ - "poetry==1.6.1", - ], - ["keyring==22.1.0", "pkginfo==1.7.2"], - ["platformdirs==3.0.0", "doesnotexist==1.0.0"], - ), - ), -) -def test_backtracking_resolver_drops_existing_conflicting_constraints( - backtracking_resolver: Callable[..., BacktrackingResolver], - from_line: Callable[..., InstallRequirement], - constraints: Sequence[str], - conflicting_existing: Sequence[str], - compatible_existing: Sequence[str], -) -> None: - def wrap_resolution_impossible(*args, **kwargs): - """ - Raise a ``DistributionNotFound`` exception that has a ``ResolutionImpossible`` - exception as its cause. - """ - causes = [ - NonCallableMock( - requirement=SpecifierRequirement(existing_constraints[ireq.name]) - ) - for ireq in conflicting_ireqs - ] - raise DistributionNotFound("resolution impossible") from ResolutionImpossible( - causes - ) - - constraint_ireqs = [from_line(req, constraint=False) for req in constraints] - conflicting_ireqs = [ - from_line(req, constraint=True) for req in conflicting_existing - ] - compatible_ireqs = [from_line(req, constraint=True) for req in compatible_existing] - existing_constraints = { - ireq.name: ireq for ireq in chain(compatible_ireqs, conflicting_ireqs) - } - - bt_resolver = backtracking_resolver( - constraint_ireqs, existing_constraints=existing_constraints - ) - resolver = NonCallableMock( - spec=Resolver, - resolve=Mock(side_effect=wrap_resolution_impossible), - ) - - # resolver has been rigged to raise a DistributionNotFound exception with - # a cause that refers to the entries of conflicting_ireqs. - # We expect _do_resolve() to handle this exception by dropping - # these entries from existing_constraints and returning False. - # It should _not_ drop any compatible constraint or re-raise - # the DistributionNotFound exception. - result = bt_resolver._do_resolve(resolver, existing_constraints) - - assert result is False - assert set(existing_constraints.keys()) == {ireq.name for ireq in compatible_ireqs} diff --git a/tests/utils.py b/tests/utils.py index 06ebe8ea1..2d00b54a2 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -1,6 +1,10 @@ from __future__ import annotations import os +from collections.abc import Sequence +from pathlib import Path +from subprocess import CompletedProcess +from typing import Protocol, TypedDict # NOTE: keep in sync with "passenv" in tox.ini CI_VARIABLES = {"CI", "GITHUB_ACTIONS"} @@ -8,3 +12,45 @@ def looks_like_ci(): return bool(set(os.environ.keys()) & CI_VARIABLES) + + +class MakePackageProtocol(Protocol): + """ + The protocol implemented by the ``make_package`` fixture. + """ + + def __call__( + self, + name: str, + version: str = "0.1", + install_requires: Sequence[str] | None = None, + extras_require: dict[str, str | list[str]] | None = None, + ) -> Path: + ... + + +class MakePackageArgs(TypedDict, total=False): + name: str + version: str + install_requires: Sequence[str] | None + extras_require: dict[str, str | list[str]] | None + + +class RunSetupFileProtocol(Protocol): + """ + The protocol implemented by the ``run_setup_file`` fixture. + """ + + def __call__(self, package_dir_path: Path, *args: str) -> CompletedProcess[bytes]: + ... + + +class MakeSDistProtocol(Protocol): + """ + The protocol implemented by the ``make_sdist`` fixture. + """ + + def __call__( + self, package_dir: Path, dist_dir: str | Path, *args: str + ) -> CompletedProcess[bytes]: + ... From 30d34f557c93c3112373ceb8a88a7f994b62d958 Mon Sep 17 00:00:00 2001 From: Christoph Ludwig Date: Thu, 21 Sep 2023 07:32:52 +0200 Subject: [PATCH 08/10] allow partially typed defs in tests --- pyproject.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/pyproject.toml b/pyproject.toml index 004bfddde..159b769a2 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -95,6 +95,7 @@ exclude = "^tests/test_data/" [[tool.mypy.overrides]] module = ["tests.*"] disallow_untyped_defs = false +disallow_incomplete_defs = false [tool.pytest.ini_options] addopts = [ From ee789c8bc410a5bcdc69807350c974272e9e34ac Mon Sep 17 00:00:00 2001 From: Christoph Ludwig Date: Thu, 21 Sep 2023 07:43:25 +0200 Subject: [PATCH 09/10] drop test annotations --- tests/conftest.py | 21 +++++------------- tests/test_cli_compile.py | 19 +++++++--------- tests/utils.py | 46 --------------------------------------- 3 files changed, 14 insertions(+), 72 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index abac23fc9..0f68570ee 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -44,12 +44,7 @@ ) from .constants import MINIMAL_WHEELS_PATH, TEST_DATA_PATH -from .utils import ( - MakePackageProtocol, - MakeSDistProtocol, - RunSetupFileProtocol, - looks_like_ci, -) +from .utils import looks_like_ci @dataclass @@ -292,7 +287,7 @@ def pip_with_index_conf(make_pip_conf): @pytest.fixture(scope="session") -def make_package(tmp_path_factory: pytest.TempPathFactory) -> MakePackageProtocol: +def make_package(tmp_path_factory): """ Make a package from a given name, version and list of required packages. """ @@ -338,14 +333,12 @@ def _make_package(name, version="0.1", install_requires=None, extras_require=Non @pytest.fixture(scope="session") -def run_setup_file() -> RunSetupFileProtocol: +def run_setup_file(): """ Run a setup.py file from a given package dir. """ - def _run_setup_file( - package_dir_path: Path, *args: str - ) -> subprocess.CompletedProcess[bytes]: + def _run_setup_file(package_dir_path, *args): setup_file = package_dir_path / "setup.py" return subprocess.run( [sys.executable, str(setup_file), *args], @@ -372,14 +365,12 @@ def _make_wheel(package_dir, dist_dir, *args): @pytest.fixture -def make_sdist(run_setup_file: RunSetupFileProtocol) -> MakeSDistProtocol: +def make_sdist(run_setup_file): """ Make a source distribution from a given package dir. """ - def _make_sdist( - package_dir: Path, dist_dir: str | Path, *args: str - ) -> subprocess.CompletedProcess[bytes]: + def _make_sdist(package_dir, dist_dir, *args): return run_setup_file(package_dir, "sdist", "--dist-dir", str(dist_dir), *args) return _make_sdist diff --git a/tests/test_cli_compile.py b/tests/test_cli_compile.py index b5630eb8f..5ac1f6f2e 100644 --- a/tests/test_cli_compile.py +++ b/tests/test_cli_compile.py @@ -5,12 +5,10 @@ import shutil import subprocess import sys -from pathlib import Path from textwrap import dedent from unittest import mock import pytest -from click.testing import CliRunner from pip._internal.utils.hashes import FAVORITE_HASH from pip._internal.utils.urls import path_to_url @@ -18,7 +16,6 @@ from piptools.utils import COMPILE_EXCLUDE_OPTIONS from .constants import MINIMAL_WHEELS_PATH, PACKAGES_PATH -from .utils import MakePackageArgs, MakePackageProtocol, MakeSDistProtocol legacy_resolver_only = pytest.mark.parametrize( "current_resolver", @@ -2872,14 +2869,14 @@ def test_cli_compile_strip_extras(runner, make_package, make_sdist, tmpdir): ids=("no-extra", "extra-stripped-from-existing", "with-extra-in-existing"), ) def test_resolver_drops_existing_conflicting_constraint( - runner: CliRunner, - make_package: MakePackageProtocol, - make_sdist: MakeSDistProtocol, - tmpdir: Path, - package_specs: list[MakePackageArgs], - constraints: str, - existing_reqs: str, - expected_reqs: str, + runner, + make_package, + make_sdist, + tmpdir, + package_specs, + constraints, + existing_reqs, + expected_reqs, ) -> None: """ Test that the resolver will find a solution even if some of the existing diff --git a/tests/utils.py b/tests/utils.py index 2d00b54a2..06ebe8ea1 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -1,10 +1,6 @@ from __future__ import annotations import os -from collections.abc import Sequence -from pathlib import Path -from subprocess import CompletedProcess -from typing import Protocol, TypedDict # NOTE: keep in sync with "passenv" in tox.ini CI_VARIABLES = {"CI", "GITHUB_ACTIONS"} @@ -12,45 +8,3 @@ def looks_like_ci(): return bool(set(os.environ.keys()) & CI_VARIABLES) - - -class MakePackageProtocol(Protocol): - """ - The protocol implemented by the ``make_package`` fixture. - """ - - def __call__( - self, - name: str, - version: str = "0.1", - install_requires: Sequence[str] | None = None, - extras_require: dict[str, str | list[str]] | None = None, - ) -> Path: - ... - - -class MakePackageArgs(TypedDict, total=False): - name: str - version: str - install_requires: Sequence[str] | None - extras_require: dict[str, str | list[str]] | None - - -class RunSetupFileProtocol(Protocol): - """ - The protocol implemented by the ``run_setup_file`` fixture. - """ - - def __call__(self, package_dir_path: Path, *args: str) -> CompletedProcess[bytes]: - ... - - -class MakeSDistProtocol(Protocol): - """ - The protocol implemented by the ``make_sdist`` fixture. - """ - - def __call__( - self, package_dir: Path, dist_dir: str | Path, *args: str - ) -> CompletedProcess[bytes]: - ... From 4dc5a576cfaf22935e7584236b52251e8ba34fc8 Mon Sep 17 00:00:00 2001 From: Christoph Ludwig Date: Wed, 27 Sep 2023 15:20:48 +0200 Subject: [PATCH 10/10] Pin pytest in pre-commit mypy configuration As discussed with chrysle, pinning of the dependencies in pyproject.toml will be addressed in the context of #1927. Co-authored-by: chrysle --- .pre-commit-config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index fd51dcb83..ef1c34f7b 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -34,7 +34,7 @@ repos: - pip==20.3.4 - build==1.0.0 - pyproject_hooks==1.0.0 - - pytest>=7.2.0 + - pytest==7.4.2 - repo: https://github.com/PyCQA/bandit rev: 1.7.5 hooks: