From d720fe605b7a0c88be8a9afba5029b1408c8295b Mon Sep 17 00:00:00 2001 From: Richard Frank Date: Fri, 17 Sep 2021 20:42:48 -0400 Subject: [PATCH 1/4] Fix a regression where multiple declarations of a dependency with different extras resulted in a non-deterministic choice between the resolved dependencies of the different declared sets of extras. --- piptools/repositories/base.py | 11 ------ piptools/repositories/local.py | 5 --- piptools/repositories/pypi.py | 9 ----- piptools/resolver.py | 67 +++++++++++++++++++++++++++------- tests/conftest.py | 4 -- tests/test_cli_compile.py | 32 ++++++++++++++++ tests/test_repository_local.py | 25 ------------- 7 files changed, 86 insertions(+), 67 deletions(-) diff --git a/piptools/repositories/base.py b/piptools/repositories/base.py index 734ac6f57..3ffec38d6 100644 --- a/piptools/repositories/base.py +++ b/piptools/repositories/base.py @@ -47,17 +47,6 @@ def allow_all_wheels(self) -> Iterator[None]: Monkey patches pip.Wheel to allow wheels from all platforms and Python versions. """ - @abstractmethod - def copy_ireq_dependencies( - self, source: InstallRequirement, dest: InstallRequirement - ) -> None: - """ - Notifies the repository that `dest` is a copy of `source`, and so it - has the same dependencies. Otherwise, once we prepare an ireq to assign - it its name, we would lose track of those dependencies on combining - that ireq with others. - """ - @property @abstractmethod def options(self) -> optparse.Values: diff --git a/piptools/repositories/local.py b/piptools/repositories/local.py index 40edca7ef..754b6076a 100644 --- a/piptools/repositories/local.py +++ b/piptools/repositories/local.py @@ -95,8 +95,3 @@ def get_hashes(self, ireq: InstallRequirement) -> Set[str]: def allow_all_wheels(self) -> Iterator[None]: with self.repository.allow_all_wheels(): yield - - def copy_ireq_dependencies( - self, source: InstallRequirement, dest: InstallRequirement - ) -> None: - self.repository.copy_ireq_dependencies(source, dest) diff --git a/piptools/repositories/pypi.py b/piptools/repositories/pypi.py index a6f147a0a..1c59201df 100644 --- a/piptools/repositories/pypi.py +++ b/piptools/repositories/pypi.py @@ -239,15 +239,6 @@ def get_dependencies(self, ireq: InstallRequirement) -> Set[InstallRequirement]: return self._dependencies_cache[ireq] - def copy_ireq_dependencies( - self, source: InstallRequirement, dest: InstallRequirement - ) -> None: - try: - self._dependencies_cache[dest] = self._dependencies_cache[source] - except KeyError: - # `source` may not be in cache yet. - pass - def _get_project(self, ireq: InstallRequirement) -> Any: """ Return a dict of a project info from PyPI JSON API for a given diff --git a/piptools/resolver.py b/piptools/resolver.py index 14971b0bf..2cc1cb704 100644 --- a/piptools/resolver.py +++ b/piptools/resolver.py @@ -72,15 +72,11 @@ def combine_install_requirements( # deepcopy the accumulator so as to not modify the inputs combined_ireq = copy.deepcopy(source_ireqs[0]) - repository.copy_ireq_dependencies(source_ireqs[0], combined_ireq) for ireq in source_ireqs[1:]: # NOTE we may be losing some info on dropped reqs here - combined_ireq.req.specifier &= ireq.req.specifier - if combined_ireq.constraint: - # We don't find dependencies for constraint ireqs, so copy them - # from non-constraints: - repository.copy_ireq_dependencies(ireq, combined_ireq) + if combined_ireq.req is not None and ireq.req is not None: + combined_ireq.req.specifier &= ireq.req.specifier combined_ireq.constraint &= ireq.constraint # Return a sorted, de-duped tuple of extras combined_ireq.extras = tuple(sorted({*combined_ireq.extras, *ireq.extras})) @@ -208,6 +204,39 @@ def resolve(self, max_rounds: int = 10) -> Set[InstallRequirement]: return results + def _get_ireq_with_name( + self, + ireq: InstallRequirement, + proxy_cache: Dict[InstallRequirement, InstallRequirement], + ) -> InstallRequirement: + """ + Return the given ireq, if it has a name, or a proxy for the given ireq + which has been prepared and therefore has a name. + + Preparing the ireq is side-effect-ful and can only be done once for an + instance, so we use a proxy instead. combine_install_requirements may + use the given ireq as a template for its aggregate result, mutating it + further by combining extras, etc. In that situation, we don't want that + aggregate ireq to be prepared prior to mutation, since its dependencies + will be frozen with those of only a subset of extras. + + i.e. We both want its name early (via preparation), but we also need to + prepare it after any mutation for combination purposes. So we use a + proxy here for the early preparation. + """ + if ireq.name is not None: + return ireq + + if ireq in proxy_cache: + return proxy_cache[ireq] + + # get_dependencies has the side-effect of assigning name to ireq + # (so we can group by the name in _group_constraints below). + name_proxy = copy.deepcopy(ireq) + self.repository.get_dependencies(name_proxy) + proxy_cache[ireq] = name_proxy + return name_proxy + def _group_constraints( self, constraints: Iterable[InstallRequirement] ) -> Iterator[InstallRequirement]: @@ -227,19 +256,31 @@ def _group_constraints( """ constraints = list(constraints) - for ireq in constraints: - if ireq.name is None: - # get_dependencies has side-effect of assigning name to ireq - # (so we can group by the name below). - self.repository.get_dependencies(ireq) + + cache: Dict[InstallRequirement, InstallRequirement] = {} + + def key_from_ireq_with_name(ireq: InstallRequirement) -> str: + """ + See _get_ireq_with_name for context. + + We use a cache per call here because it should only be necessary + the first time an ireq is passed here (later on in the round, it + will be prepared and dependencies for it calculated), but we can + save time by reusing the proxy between the sort and groupby calls + below. + """ + return key_from_ireq(self._get_ireq_with_name(ireq, cache)) # Sort first by name, i.e. the groupby key. Then within each group, # sort editables first. # This way, we don't bother with combining editables, since the first # ireq will be editable, if one exists. for _, ireqs in groupby( - sorted(constraints, key=(lambda x: (key_from_ireq(x), not x.editable))), - key=key_from_ireq, + sorted( + constraints, + key=(lambda x: (key_from_ireq_with_name(x), not x.editable)), + ), + key=key_from_ireq_with_name, ): yield combine_install_requirements(self.repository, ireqs) diff --git a/tests/conftest.py b/tests/conftest.py index be063d121..225d48f8a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -89,10 +89,6 @@ def allow_all_wheels(self): # No need to do an actual pip.Wheel mock here. yield - def copy_ireq_dependencies(self, source, dest): - # No state to update. - pass - @property def options(self) -> optparse.Values: """Not used""" diff --git a/tests/test_cli_compile.py b/tests/test_cli_compile.py index bb15877b4..a56aefafc 100644 --- a/tests/test_cli_compile.py +++ b/tests/test_cli_compile.py @@ -1727,6 +1727,38 @@ def test_duplicate_reqs_combined( assert "test-package-1==0.1" in out.stderr +def test_combine_extras(pip_conf, runner, make_package): + """ + Ensure that multiple declarations of a dependency that specify different + extras produces a requirement for that package with the union of the extras + """ + package_with_extras = make_package( + "package_with_extras", + extras_require={ + "extra1": ["small-fake-a==0.1"], + "extra2": ["small-fake-b==0.1"], + }, + ) + + with open("requirements.in", "w") as req_in: + req_in.writelines( + [ + "-r ./requirements-second.in\n", + f"{package_with_extras}[extra1]", + ] + ) + + with open("requirements-second.in", "w") as req_sec_in: + req_sec_in.write(f"{package_with_extras}[extra2]") + + out = runner.invoke(cli, ["-n"]) + + assert out.exit_code == 0 + assert "package-with-extras" in out.stderr + assert "small-fake-a==" in out.stderr + assert "small-fake-b==" in out.stderr + + @pytest.mark.parametrize( ("pkg2_install_requires", "req_in_content", "out_expected_content"), ( diff --git a/tests/test_repository_local.py b/tests/test_repository_local.py index 9381770a2..9d18c4e33 100644 --- a/tests/test_repository_local.py +++ b/tests/test_repository_local.py @@ -1,10 +1,7 @@ -import copy - import pytest from piptools.repositories.local import LocalRequirementsRepository from piptools.utils import key_from_ireq -from tests.conftest import FakeRepository EXPECTED = {"sha256:5e6071ee6e4c59e0d0408d366fe9b66781d2cf01be9a6e19a2433bb3c5336330"} @@ -57,25 +54,3 @@ def test_toggle_reuse_hashes_local_repository( captured = capsys.readouterr() assert captured.out == "" assert captured.err == "" - - -class FakeRepositoryChecksForCopy(FakeRepository): - def __init__(self): - super().__init__() - self.copied = [] - - def copy_ireq_dependencies(self, source, dest): - self.copied.append(source) - - -def test_local_repository_copy_ireq_dependencies(from_line): - # Ensure that local repository forwards any messages to update its state - # of ireq dependencies. - checker = FakeRepositoryChecksForCopy() - local_repository = LocalRequirementsRepository({}, checker) - - src = from_line("small-fake-a==0.1") - dest = copy.deepcopy(src) - local_repository.copy_ireq_dependencies(src, dest) - - assert src in checker.copied From a69540d926ec792394abb421652ad69abcd3ecaa Mon Sep 17 00:00:00 2001 From: Richard Frank Date: Fri, 17 Sep 2021 20:43:45 -0400 Subject: [PATCH 2/4] InstallRequirement.extras is expected to be a set anyway Pointed out in https://github.com/jazzband/pip-tools/issues/1451#issuecomment-919264865 --- piptools/resolver.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/piptools/resolver.py b/piptools/resolver.py index 2cc1cb704..4a475e7d1 100644 --- a/piptools/resolver.py +++ b/piptools/resolver.py @@ -78,8 +78,7 @@ def combine_install_requirements( if combined_ireq.req is not None and ireq.req is not None: combined_ireq.req.specifier &= ireq.req.specifier combined_ireq.constraint &= ireq.constraint - # Return a sorted, de-duped tuple of extras - combined_ireq.extras = tuple(sorted({*combined_ireq.extras, *ireq.extras})) + combined_ireq.extras = {*combined_ireq.extras, *ireq.extras} # InstallRequirements objects are assumed to come from only one source, and # so they support only a single comes_from entry. This function breaks this From 1942484c41f5ecc5de36008c2cb807324a9874c0 Mon Sep 17 00:00:00 2001 From: Richard Frank Date: Fri, 24 Sep 2021 20:03:05 -0400 Subject: [PATCH 3/4] Ensure extras are propagated when ireq has a req Fixes https://github.com/jazzband/pip-tools/issues/852 --- piptools/resolver.py | 2 ++ tests/test_resolver.py | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/piptools/resolver.py b/piptools/resolver.py index 4a475e7d1..c759b0ab0 100644 --- a/piptools/resolver.py +++ b/piptools/resolver.py @@ -79,6 +79,8 @@ def combine_install_requirements( combined_ireq.req.specifier &= ireq.req.specifier combined_ireq.constraint &= ireq.constraint combined_ireq.extras = {*combined_ireq.extras, *ireq.extras} + if combined_ireq.req is not None: + combined_ireq.req.extras = set(combined_ireq.extras) # InstallRequirements objects are assumed to come from only one source, and # so they support only a single comes_from entry. This function breaks this diff --git a/tests/test_resolver.py b/tests/test_resolver.py index 03f6e9d9b..7978fb1f7 100644 --- a/tests/test_resolver.py +++ b/tests/test_resolver.py @@ -1,4 +1,5 @@ import pytest +from pip._internal.utils.urls import path_to_url from piptools.exceptions import NoCandidateFound from piptools.resolver import RequirementSummary, combine_install_requirements @@ -299,6 +300,43 @@ def test_combine_install_requirements(repository, from_line): assert str(combined_all.req.specifier) == "<3.2,==3.1.1,>3.0" +def test_combine_install_requirements_extras(repository, from_line, make_package): + """ + Extras should be unioned in combined install requirements + (whether or not InstallRequirement.req is None, and testing either order of the inputs) + """ + with_extra = from_line("edx-opaque-keys[django]==1.0.1") + assert with_extra.req is not None + without_extra = from_line("edx-opaque-keys") + assert without_extra.req is not None + + combined = combine_install_requirements(repository, [without_extra, with_extra]) + assert str(combined) == str(with_extra) + assert combined.extras == with_extra.extras + + combined = combine_install_requirements(repository, [with_extra, without_extra]) + assert str(combined) == str(with_extra) + assert combined.extras == with_extra.extras + + test_package = make_package("test-package", extras_require={"extra": []}) + local_package_with_extra = from_line(f"{test_package}[extra]") + assert local_package_with_extra.req is None + local_package_without_extra = from_line(path_to_url(test_package)) + assert local_package_without_extra.req is None + + combined = combine_install_requirements( + repository, [local_package_without_extra, local_package_with_extra] + ) + assert str(combined) == str(local_package_with_extra) + assert combined.extras == local_package_with_extra.extras + + combined = combine_install_requirements( + repository, [local_package_with_extra, local_package_without_extra] + ) + assert str(combined) == str(local_package_with_extra) + assert combined.extras == local_package_with_extra.extras + + def test_compile_failure_shows_provenance(resolver, from_line): """ Provenance of conflicting dependencies should be printed on failure. From ddb7c3ac06df9dbf0b041623185773a7e935b875 Mon Sep 17 00:00:00 2001 From: Richard Frank Date: Sun, 3 Oct 2021 21:13:48 -0400 Subject: [PATCH 4/4] Move test blocks into their own test cases --- tests/test_resolver.py | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/tests/test_resolver.py b/tests/test_resolver.py index 7978fb1f7..bf28ed6e4 100644 --- a/tests/test_resolver.py +++ b/tests/test_resolver.py @@ -300,7 +300,17 @@ def test_combine_install_requirements(repository, from_line): assert str(combined_all.req.specifier) == "<3.2,==3.1.1,>3.0" -def test_combine_install_requirements_extras(repository, from_line, make_package): +def _test_combine_install_requirements_extras(repository, with_extra, without_extra): + combined = combine_install_requirements(repository, [without_extra, with_extra]) + assert str(combined) == str(with_extra) + assert combined.extras == with_extra.extras + + combined = combine_install_requirements(repository, [with_extra, without_extra]) + assert str(combined) == str(with_extra) + assert combined.extras == with_extra.extras + + +def test_combine_install_requirements_extras_req(repository, from_line, make_package): """ Extras should be unioned in combined install requirements (whether or not InstallRequirement.req is None, and testing either order of the inputs) @@ -310,31 +320,25 @@ def test_combine_install_requirements_extras(repository, from_line, make_package without_extra = from_line("edx-opaque-keys") assert without_extra.req is not None - combined = combine_install_requirements(repository, [without_extra, with_extra]) - assert str(combined) == str(with_extra) - assert combined.extras == with_extra.extras + _test_combine_install_requirements_extras(repository, with_extra, without_extra) - combined = combine_install_requirements(repository, [with_extra, without_extra]) - assert str(combined) == str(with_extra) - assert combined.extras == with_extra.extras +def test_combine_install_requirements_extras_no_req( + repository, from_line, make_package +): + """ + Extras should be unioned in combined install requirements + (whether or not InstallRequirement.req is None, and testing either order of the inputs) + """ test_package = make_package("test-package", extras_require={"extra": []}) local_package_with_extra = from_line(f"{test_package}[extra]") assert local_package_with_extra.req is None local_package_without_extra = from_line(path_to_url(test_package)) assert local_package_without_extra.req is None - combined = combine_install_requirements( - repository, [local_package_without_extra, local_package_with_extra] - ) - assert str(combined) == str(local_package_with_extra) - assert combined.extras == local_package_with_extra.extras - - combined = combine_install_requirements( - repository, [local_package_with_extra, local_package_without_extra] + _test_combine_install_requirements_extras( + repository, local_package_with_extra, local_package_without_extra ) - assert str(combined) == str(local_package_with_extra) - assert combined.extras == local_package_with_extra.extras def test_compile_failure_shows_provenance(resolver, from_line):