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.

docs/reference/pip_install.rst

@@ -18,6 +18,24 @@ Description
 
 .. _`Requirements File Format`:
 
+Installation Order
+++++++++++++++++++
+
+Pip installs dependencies before the things that depend on them. In the event
+of a dependency cycle, the first encountered member of the cycle is installed
+last.
+
+For instance, if quux depends on foo which depends on bar which depends on baz,
+which depends on foo::
+
+ pip install quux
+ ...
+ Installing collected packages baz, bar, foo, quux
+
+ pip install bar
+ ...
+ Installing collected packages foo, baz, bar
+
 Requirements File Format
 ++++++++++++++++++++++++
 

pip/req/req_install.py

@@ -318,6 +318,7 @@ def 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
@@ -170,6 +171,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()
@@ -184,29 +187,56 @@ 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. Note that None implies that this is a user
+ supplied requirement, vs an inferred one.
+ :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():
@@ -507,22 +537,19 @@ 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 we
  # can refer to it when adding 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 (req_to_install.extras):
@@ -575,13 +602,36 @@ def _pip_has_created_build_dir(self):
  )
  )
 
+ def _to_install(self):
+ """Create the installation order.
+
+ The installation order is topological - requirements are installed
+ before the requiring thing. We break cycles at an arbitrary point,
+ and make no other guarantees.
+ """
+ # The current implementation, which we may change at any point
+ # installs 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

@@ -71,6 +71,18 @@ priority-*
 ----------
 used for testing wheel priority over sdists
 
+TopoRequires[123][-0.0.1.tar.gz]
+--------------------------------
+
+These are used for testing topological handling of requirements: we have
+TopoRequires, which is install-required by TopoRequires2 and TopoRequires3
+and finally TopoRequires4 which install-requires both TopoRequires2 and 3
+and also install-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
 ------------------------
 contains "simple[2]" package; good for basic testing and version logic.

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.pending

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

tests/data/packages/TopoRequires2/setup.py

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

tests/data/packages/TopoRequires3/setup.cfg.pending

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

tests/data/packages/TopoRequires3/setup.py

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

tests/data/packages/TopoRequires4/setup.cfg.pending

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

tests/data/packages/TopoRequires4/setup.py

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

tests/functional/test_install.py

@@ -732,3 +732,12 @@ def test_install_upgrade_editable_depending_on_other_editable(script):
  script.pip('install', '--upgrade', '--editable', pkgb_path)
  result = script.pip('list')
  assert "pkgb" in result.stdout
+
+
+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