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..c759b0ab0 100644 --- a/piptools/resolver.py +++ b/piptools/resolver.py @@ -72,18 +72,15 @@ 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})) + 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 @@ -208,6 +205,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 +257,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 diff --git a/tests/test_resolver.py b/tests/test_resolver.py index 03f6e9d9b..bf28ed6e4 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,47 @@ 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, 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) + """ + 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 + + _test_combine_install_requirements_extras(repository, with_extra, without_extra) + + +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 + + _test_combine_install_requirements_extras( + repository, local_package_with_extra, local_package_without_extra + ) + + def test_compile_failure_shows_provenance(resolver, from_line): """ Provenance of conflicting dependencies should be printed on failure.