Ensure pip-tools unions dependencies of multiple declarations of a package with different extras

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:

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)

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

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)
 

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"""

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"),
     (

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

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.