Fix grouping of requirements to handle editables better

piptools/repositories/base.py

@@ -44,3 +44,11 @@ def allow_all_wheels(self):
         """
         Monkey patches pip.Wheel to allow wheels from all platforms and Python versions.
         """
+
+    def copy_ireq_dependencies(self, source, dest):
+        """
+        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.
+        """

piptools/repositories/pypi.py

@@ -178,6 +178,10 @@ def resolve_reqs(self, download_dir, ireq, wheel_cache):
                 upgrade_strategy="to-satisfy-only",
             )
             results = resolver._resolve_one(reqset, ireq)
+            if not ireq.prepared:
+                # If still not prepared, e.g. a constraint, do enough to assign
+                # the ireq a name:
+                resolver._get_abstract_dist_for(ireq)
 
             if PIP_VERSION[:2] <= (20, 0):
                 reqset.cleanup_files()
@@ -235,6 +239,13 @@ def get_dependencies(self, ireq):
 
         return self._dependencies_cache[ireq]
 
+    def copy_ireq_dependencies(self, source, dest):
+        try:
+            self._dependencies_cache[dest] = self._dependencies_cache[source]
+        except KeyError:
+            # `source` may not be in cache yet.
+            pass
+
     def _get_project(self, ireq):
         """
         Return a dict of a project info from PyPI JSON API for a given

piptools/resolver.py

@@ -4,7 +4,7 @@
 import copy
 import os
 from functools import partial
-from itertools import chain, count
+from itertools import chain, count, groupby
 
 from pip._internal.req.constructors import install_req_from_line
 
@@ -14,7 +14,6 @@
     UNSAFE_PACKAGES,
     format_requirement,
     format_specifier,
-    full_groupby,
     is_pinned_requirement,
     is_url_requirement,
     key_from_ireq,
@@ -45,7 +44,7 @@ def __str__(self):
         return repr([self.key, self.specifier, self.extras])
 
 
-def combine_install_requirements(ireqs):
+def combine_install_requirements(repository, ireqs):
     """
     Return a single install requirement that reflects a combination of
     all the inputs.
@@ -56,12 +55,21 @@ def combine_install_requirements(ireqs):
     for ireq in ireqs:
         source_ireqs.extend(getattr(ireq, "_source_ireqs", [ireq]))
 
+    # Optimization. Don't bother with combination logic.
+    if len(source_ireqs) == 1:
+        return source_ireqs[0]
+
     # 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
-        if combined_ireq.req is not None and ireq.req is not None:
-            combined_ireq.req.specifier &= ireq.req.specifier
+        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)
         combined_ireq.constraint &= ireq.constraint
         # Return a sorted, de-duped tuple of extras
         combined_ireq.extras = tuple(
@@ -117,12 +125,7 @@ def __init__(
     @property
     def constraints(self):
         return set(
-            self._group_constraints(
-                chain(
-                    sorted(self.our_constraints, key=str),
-                    sorted(self.their_constraints, key=str),
-                )
-            )
+            self._group_constraints(chain(self.our_constraints, self.their_constraints))
         )
 
     def resolve_hashes(self, ireqs):
@@ -220,8 +223,22 @@ def _group_constraints(self, constraints):
             flask~=0.7
 
         """
-        for _, ireqs in full_groupby(constraints, key=key_from_ireq):
-            yield combine_install_requirements(ireqs)
+        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)
+
+        # 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,
+        ):
+            yield combine_install_requirements(self.repository, ireqs)
 
     def _resolve_one_round(self):
         """
@@ -256,7 +273,7 @@ def _resolve_one_round(self):
             for best_match in best_matches:
                 their_constraints.extend(self._iter_dependencies(best_match))
         # Grouping constraints to make clean diff between rounds
-        theirs = set(self._group_constraints(sorted(their_constraints, key=str)))
+        theirs = set(self._group_constraints(their_constraints))
 
         # NOTE: We need to compare RequirementSummary objects, since
         # InstallRequirement does not define equality

piptools/utils.py

@@ -3,7 +3,7 @@
 
 import sys
 from collections import OrderedDict
-from itertools import chain, groupby
+from itertools import chain
 
 import six
 from click.utils import LazyFile
@@ -144,11 +144,6 @@ def as_tuple(ireq):
     return name, version, extras
 
 
-def full_groupby(iterable, key=None):
-    """Like groupby(), but sorts the input on the group key first."""
-    return groupby(sorted(iterable, key=key), key=key)
-
-
 def flat_map(fn, collection):
     """Map a function over a collection and flatten the result by one-level"""
     return chain.from_iterable(map(fn, collection))

tests/test_cli_compile.py

@@ -219,6 +219,53 @@ def test_editable_package(pip_conf, runner):
     assert "small-fake-a==0.1" in out.stderr
 
 
+def test_editable_package_without_non_editable_duplicate(pip_conf, runner):
+    """
+    piptools keeps editable requirement,
+    without also adding a duplicate "non-editable" requirement variation
+    """
+    fake_package_dir = os.path.join(PACKAGES_PATH, "small_fake_a")
+    fake_package_dir = path_to_url(fake_package_dir)
+    with open("requirements.in", "w") as req_in:
+        # small_fake_with_unpinned_deps also requires small_fake_a
+        req_in.write(
+            "-e "
+            + fake_package_dir
+            + "\nsmall_fake_with_unpinned_deps"  # require editable fake package
+        )
+
+    out = runner.invoke(cli, ["-n"])
+
+    assert out.exit_code == 0
+    assert fake_package_dir in out.stderr
+    # Shouldn't include a non-editable small-fake-a==<version>.
+    assert "small-fake-a==" not in out.stderr
+
+
+def test_editable_package_constraint_without_non_editable_duplicate(pip_conf, runner):
+    """
+    piptools keeps editable constraint,
+    without also adding a duplicate "non-editable" requirement variation
+    """
+    fake_package_dir = os.path.join(PACKAGES_PATH, "small_fake_a")
+    fake_package_dir = path_to_url(fake_package_dir)
+    with open("constraints.txt", "w") as constraints:
+        constraints.write("-e " + fake_package_dir)  # require editable fake package
+
+    with open("requirements.in", "w") as req_in:
+        req_in.write(
+            "-c constraints.txt"  # require editable fake package
+            "\nsmall_fake_with_unpinned_deps"  # This one also requires small_fake_a
+        )
+
+    out = runner.invoke(cli, ["-n"])
+
+    assert out.exit_code == 0
+    assert fake_package_dir in out.stderr
+    # Shouldn't include a non-editable small-fake-a==<version>.
+    assert "small-fake-a==" not in out.stderr
+
+
 @pytest.mark.parametrize(("req_editable",), [(True,), (False,)])
 def test_editable_package_in_constraints(pip_conf, runner, req_editable):
     """

tests/test_data/packages/small_fake_a/setup.py

@@ -0,0 +1,3 @@
+from setuptools import setup
+
+setup(name="small_fake_a", version=0.1)

tests/test_resolver.py

@@ -271,17 +271,17 @@ def test_iter_dependencies_ignores_constraints(resolver, from_line):
         next(res._iter_dependencies(ireq))
 
 
-def test_combine_install_requirements(from_line):
+def test_combine_install_requirements(repository, from_line):
     celery30 = from_line("celery>3.0", comes_from="-r requirements.in")
     celery31 = from_line("celery==3.1.1", comes_from=from_line("fake-package"))
     celery32 = from_line("celery<3.2")
 
-    combined = combine_install_requirements([celery30, celery31])
+    combined = combine_install_requirements(repository, [celery30, celery31])
     assert combined.comes_from == celery31.comes_from  # shortest string
     assert set(combined._source_ireqs) == {celery30, celery31}
     assert str(combined.req.specifier) == "==3.1.1,>3.0"
 
-    combined_all = combine_install_requirements([celery32, combined])
+    combined_all = combine_install_requirements(repository, [celery32, combined])
     assert combined_all.comes_from is None
     assert set(combined_all._source_ireqs) == {celery30, celery31, celery32}
     assert str(combined_all.req.specifier) == "<3.2,==3.1.1,>3.0"