Issue pypa#2478 - topological install order.

This is needed for setup-requires, since without it its possible
to cause installation to fail in sort-circuit scenarios such as
the added functional test case demonstrates.

pip/req/req_install.py

@@ -325,6 +325,7 @@ def url_name(self):
 
     @property
     def setup_py(self):
+        assert self.source_dir, "No source dir for %s" % self
         try:
             import setuptools  # noqa
         except ImportError:

pip/req/req_set.py

@@ -1,5 +1,6 @@
 from __future__ import absolute_import
 
+from collections import defaultdict
 import functools
 import itertools
 import logging
@@ -273,6 +274,8 @@ def __init__(self, build_dir, src_dir, download_dir, upgrade=False,
         if wheel_download_dir:
             wheel_download_dir = normalize_path(wheel_download_dir)
         self.wheel_download_dir = wheel_download_dir
+        # Maps from install_req -> dependencies_of_install_req
+        self._dependencies = defaultdict(list)
 
     def __str__(self):
         reqs = [req for req in self.requirements.values()
@@ -287,29 +290,55 @@ def __repr__(self):
         return ('<%s object; %d requirement(s): %s>'
                 % (self.__class__.__name__, len(reqs), reqs_str))
 
-    def add_requirement(self, install_req):
-        if not install_req.match_markers():
+    def add_requirement(self, install_req, parent_req_name=None):
+        """Add install_req as a requirement to install.
+
+        :param parent_req_name: The name of the requirement that needed this
+            added. The name is used because when multiple unnamed requirements
+            resolve to the same name, we could otherwise end up with dependency
+            links that point outside the Requirements set. parent_req must
+            already be added.
+        :return: Additional requirements to scan. That is either [] if
+            the requirement is not applicable, or [install_req] if the
+            requirement is applicable and has just been added.
+        """
+        name = install_req.name
+        if ((not name or not self.has_requirement(name)) and not
+                install_req.match_markers()):
+            # Only log if we haven't already got install_req from somewhere.
             logger.debug("Ignore %s: markers %r don't match",
                          install_req.name, install_req.markers)
-            return
+            return []
 
-        name = install_req.name
         install_req.as_egg = self.as_egg
         install_req.use_user_site = self.use_user_site
         install_req.target_dir = self.target_dir
         install_req.pycompile = self.pycompile
         if not name:
             # url or path requirement w/o an egg fragment
             self.unnamed_requirements.append(install_req)
+            return [install_req]
         else:
-            if self.has_requirement(name):
+            if parent_req_name is None and self.has_requirement(name):
                 raise InstallationError(
                     'Double requirement given: %s (already in %s, name=%r)'
                     % (install_req, self.get_requirement(name), name))
-            self.requirements[name] = install_req
-            # FIXME: what about other normalizations?  E.g., _ vs. -?
-            if name.lower() != name:
-                self.requirement_aliases[name.lower()] = name
+            if not self.has_requirement(name):
+                # Add requirement
+                self.requirements[name] = install_req
+                # FIXME: what about other normalizations?  E.g., _ vs. -?
+                if name.lower() != name:
+                    self.requirement_aliases[name.lower()] = name
+                result = [install_req]
+            else:
+                # Canonicalise to the already-added object
+                install_req = self.get_requirement(name)
+                # No need to scan, this is a duplicate requirement.
+                result = []
+            if parent_req_name:
+                parent_req = self.get_requirement(parent_req_name)
+                self._dependencies[parent_req].append(install_req)
+            return result
 
     def has_requirement(self, project_name):
         for name in project_name, project_name.lower():
@@ -610,23 +639,20 @@ def _prepare_file(self, finder, req_to_install):
             more_reqs = []
 
             def add_req(subreq):
-                if self.has_requirement(subreq.project_name):
-                    # FIXME: check for conflict
-                    return
-                subreq = InstallRequirement(
+                sub_install_req = InstallRequirement(
                     str(subreq),
                     req_to_install,
                     isolated=self.isolated,
                 )
-                more_reqs.append(subreq)
-                self.add_requirement(subreq)
+                more_reqs.extend(self.add_requirement(
+                    sub_install_req, req_to_install.name))
 
             # We add req_to_install before its dependencies, so that when
             # to_install is calculated, which reverses the order,
             # req_to_install is installed after its dependencies.
             if not self.has_requirement(req_to_install.name):
                 # 'unnamed' requirements will get added here
-                self.add_requirement(req_to_install)
+                self.add_requirement(req_to_install, None)
 
             if not self.ignore_dependencies:
                 if getattr(dist, 'setup_requires', None):
@@ -688,13 +714,32 @@ def _pip_has_created_build_dir(self):
             )
         )
 
+    def _to_install(self):
+        """Create the installation order.
+
+        We install the user specified things in the order given, except when
+        dependencies require putting other things first.
+        """
+        order = []
+        ordered_reqs = set()
+
+        def schedule(req):
+            if req.satisfied_by or req in ordered_reqs:
+                return
+            ordered_reqs.add(req)
+            for dep in self._dependencies[req]:
+                schedule(dep)
+            order.append(req)
+        for install_req in self.requirements.values():
+            schedule(install_req)
+        return order
+
     def install(self, install_options, global_options=(), *args, **kwargs):
         """
         Install everything in this set (after having downloaded and unpacked
         the packages)
         """
-        to_install = [r for r in self.requirements.values()[::-1]
-                      if not r.satisfied_by]
+        to_install = self._to_install()
 
         # DISTRIBUTE TO SETUPTOOLS UPGRADE HACK (1 of 3 parts)
         # move the distribute-0.7.X wrapper to the end because it does not

tests/data/packages/README.txt

@@ -103,6 +103,17 @@ requires SetupRequires2[a,b], as using extras for local paths is currently
 broken (issue 1236). Ideally SetupRequires3 would have the extras itself
 and no requires-dist (to test declarative extras as sole requirements).
 
+TopoRequires[123][-0.0.1.tar.gz]
+--------------------------------
+
+These are used for testing topological handling of requirements: we have
+TopoRequires, which is setup-required by TopoRequires2 and TopoRequires3
+and finally TopoRequires4 which install-requires both TopoRequires2 and 3
+and also setup-Requires TopoRequires.
+This creates a diamond where no matter which way we walk without topological
+awareness we'll end up attempting to install TopoRequires after one of
+TopoRequires2, TopoRequires3 or TopoRequires4. (prefix iteration works as its
+topological, suffix iteration likewise, infix breaks).
 
 simple[2]-[123].0.tar.gz
 ------------------------

tests/data/packages/TopoRequires/setup.py

@@ -0,0 +1,7 @@
+from setuptools import setup
+
+setup(
+    name='TopoRequires',
+    version='0.0.1',
+    packages=['toporequires'],
+)

tests/data/packages/TopoRequires2/setup.cfg

@@ -0,0 +1,4 @@
+[metadata]
+name = TopoRequires2
+setup-requires =
+  TopoRequires

tests/data/packages/TopoRequires2/setup.py

@@ -0,0 +1,8 @@
+from setuptools import setup
+import toporequires
+
+setup(
+    name='TopoRequires2',
+    version='0.0.1',
+    packages=['toporequires2'],
+)

tests/data/packages/TopoRequires3/setup.cfg

@@ -0,0 +1,4 @@
+[metadata]
+name = TopoRequires3
+setup-requires =
+  TopoRequires

tests/data/packages/TopoRequires3/setup.py

@@ -0,0 +1,8 @@
+from setuptools import setup
+import toporequires
+
+setup(
+    name='TopoRequires3',
+    version='0.0.1',
+    packages=['toporequires3'],
+)

tests/data/packages/TopoRequires4/setup.cfg

@@ -0,0 +1,7 @@
+[metadata]
+name = TopoRequires4
+install-requires =
+  TopoRequires2
+  TopoRequires3
+setup-requires =
+  TopoRequires

tests/data/packages/TopoRequires4/setup.py

@@ -0,0 +1,8 @@
+from setuptools import setup
+import toporequires
+
+setup(
+    name='TopoRequires4',
+    version='0.0.1',
+    packages=['toporequires4'],
+)

tests/functional/test_install.py

@@ -764,3 +764,12 @@ def test_install_declarative_extras(script, data):
     assert 'Running setup.py install for simple2\n' in str(res), str(res)
     assert 'Running setup.py install for SetupRequires3\n' in str(res), \
         str(res)
+
+
+def test_install_topological_sort(script, data):
+    to_install = data.packages.join('TopoRequires4')
+    args = ['install'] + [to_install, '-f', data.packages]
+    res = str(script.pip(*args, expect_error=False))
+    order1 = 'TopoRequires, TopoRequires2, TopoRequires3, TopoRequires4'
+    order2 = 'TopoRequires, TopoRequires3, TopoRequires2, TopoRequires4'
+    assert order1 in res or order2 in res, res