From 17352765f06384fd135d63fff3f3fed031a628f8 Mon Sep 17 00:00:00 2001 From: Robert Collins Date: Wed, 25 Mar 2015 13:53:10 +1300 Subject: [PATCH] Issue #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 ++++ pip/req/req_install.py | 1 + pip/req/req_set.py | 86 ++++++++++++++---- tests/data/packages/README.txt | 12 +++ tests/data/packages/TopoRequires-0.0.1.tar.gz | Bin 0 -> 746 bytes tests/data/packages/TopoRequires/setup.py | 7 ++ .../TopoRequires/toporequires/__init__.py | 0 .../data/packages/TopoRequires2-0.0.1.tar.gz | Bin 0 -> 767 bytes .../packages/TopoRequires2/setup.cfg.pending | 4 + tests/data/packages/TopoRequires2/setup.py | 8 ++ .../TopoRequires2/toporequires2/__init__.py | 0 .../data/packages/TopoRequires3-0.0.1.tar.gz | Bin 0 -> 792 bytes .../packages/TopoRequires3/setup.cfg.pending | 4 + tests/data/packages/TopoRequires3/setup.py | 8 ++ .../TopoRequires3/toporequires3/__init__.py | 0 .../packages/TopoRequires4/setup.cfg.pending | 6 ++ tests/data/packages/TopoRequires4/setup.py | 8 ++ .../TopoRequires4/toporequires4/__init__.py | 0 tests/functional/test_install.py | 9 ++ 19 files changed, 153 insertions(+), 18 deletions(-) create mode 100644 tests/data/packages/TopoRequires-0.0.1.tar.gz create mode 100644 tests/data/packages/TopoRequires/setup.py create mode 100644 tests/data/packages/TopoRequires/toporequires/__init__.py create mode 100644 tests/data/packages/TopoRequires2-0.0.1.tar.gz create mode 100644 tests/data/packages/TopoRequires2/setup.cfg.pending create mode 100644 tests/data/packages/TopoRequires2/setup.py create mode 100644 tests/data/packages/TopoRequires2/toporequires2/__init__.py create mode 100644 tests/data/packages/TopoRequires3-0.0.1.tar.gz create mode 100644 tests/data/packages/TopoRequires3/setup.cfg.pending create mode 100644 tests/data/packages/TopoRequires3/setup.py create mode 100644 tests/data/packages/TopoRequires3/toporequires3/__init__.py create mode 100644 tests/data/packages/TopoRequires4/setup.cfg.pending create mode 100644 tests/data/packages/TopoRequires4/setup.py create mode 100644 tests/data/packages/TopoRequires4/toporequires4/__init__.py diff --git a/docs/reference/pip_install.rst b/docs/reference/pip_install.rst index 7eb37e6a7..319af1e0c 100644 --- a/docs/reference/pip_install.rst +++ b/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 ++++++++++++++++++++++++ diff --git a/pip/req/req_install.py b/pip/req/req_install.py index 393cb1526..22c12716a 100644 --- a/pip/req/req_install.py +++ b/pip/req/req_install.py @@ -318,6 +318,7 @@ class InstallRequirement(object): @property def setup_py(self): + assert self.source_dir, "No source dir for %s" % self try: import setuptools # noqa except ImportError: diff --git a/pip/req/req_set.py b/pip/req/req_set.py index 8aee642b8..2499ddf59 100644 --- a/pip/req/req_set.py +++ b/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 @@ class RequirementSet(object): 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,13 +187,27 @@ class RequirementSet(object): 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 @@ -198,15 +215,28 @@ class RequirementSet(object): 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 @@ class RequirementSet(object): 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 @@ class RequirementSet(object): ) ) + 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 diff --git a/tests/data/packages/README.txt b/tests/data/packages/README.txt index 70f1eb088..9e89e9114 100644 --- a/tests/data/packages/README.txt +++ b/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. diff --git a/tests/data/packages/TopoRequires-0.0.1.tar.gz b/tests/data/packages/TopoRequires-0.0.1.tar.gz new file mode 100644 index 0000000000000000000000000000000000000000..58182befe111fa283d9a100366664b9732dd732e GIT binary patch literal 746 zcmVv z#`B!NBI1QjDyf|dQIU93w(2S3jg0{}PV>)L*Jlu?THHivAWMwhR5Ag8sfArIGY~GrwF( z|9;=OhyH|GeM{+YQJZK6f&P!7zu&vCPRZh%!A)+iDcFWbX2<9xfU?CU7({4STbkDZ* z=eh=-vj6KLlZRn1N+;R&ivE9C{ilxQq5eN9{g3uf4qv`LGUYefcK)}mrvA1~`pEyA zpepwI&8i>lzU{pqz1i3ATog<081tN`W1a?=ejKG2;+9+U^TPZhvUJI`sv~~PXFM*y zqV*;f`TutR-y~#f{Xebw-&X#2o%#MB{$D?6#^C>Z`Tw`{+w%UOx()w3)P?^yLcNY6 z7r_5Z{ap1^&y@vm7ivE8yeD6fUFL=zE;GKanVX@#_MkiG5^x|MgMLT?FC)gQ# zogXmF|LXs0C%6XxTb|YMzvWt}{~O^0mu$==>wUa(`@oF=_H_i`5U>(Gm-{G1id>mE;76kkX8qJXEO_Sh>ff+EUg`L-%} z5HmT+isU*1@2<)JS4Tp@By8&)ch-N;Zv6l6QMDz2|2M#?O7Q+X!Jp}ph5q?8ipR## zC@)lA;Oa9eFT^ZWeZnl7CxJ2u1OB7_U!l^sQUB-u@7pQ_|D*n+{sRC20000000000 c000000000000000001=j3A5j>7ywWJ05wE~{Qv*} literal 0 HcmV?d00001 diff --git a/tests/data/packages/TopoRequires/setup.py b/tests/data/packages/TopoRequires/setup.py new file mode 100644 index 000000000..6cd29a7b6 --- /dev/null +++ b/tests/data/packages/TopoRequires/setup.py @@ -0,0 +1,7 @@ +from setuptools import setup + +setup( + name='TopoRequires', + version='0.0.1', + packages=['toporequires'], +) diff --git a/tests/data/packages/TopoRequires/toporequires/__init__.py b/tests/data/packages/TopoRequires/toporequires/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/data/packages/TopoRequires2-0.0.1.tar.gz b/tests/data/packages/TopoRequires2-0.0.1.tar.gz new file mode 100644 index 0000000000000000000000000000000000000000..1712f5ba1f0b26b5799426155ee704fd577504c9 GIT binary patch literal 767 zcmV0A5Ut#s8Ag!65#EGcX3&dGKH|+oy(-dW`Tf%Z8b}J^tf5*)hwnQu4AY_BR&rO;y z?yB|kW4~z*vOL=pUlvgzq&=V(wb-`NGF0RHe05|#r&o^^2D3T!JjPweH7Mhb#|^@b z4p>MP7K9i@b|MNjSvzji_39s~j{gV;*YscMT2a-u@1XxSwHZbK???ZGAW9<@1Xh0b z2mSXvcXR)_?>XGo{kI+F8ib<%chdh_%@XtZAWCQ1c8C5mkJ|A6p7p<45-jyqwd+6Q zjsCmL_uzlm{WlHp(71*F`#Xnw&tB}`TK|pyJHFlU-*J5k|GPj6wa9g}GuJeI_wB2} zoAJvXbMvHFH)JYukxoTAISb+_J(a&2o1YZcXPKqu4fddQV|08CVzCr)IjHS0`*8ig zu8%F{f7bAydmR3E0``3Pe?$Lk>uS6C-)rW7$6@fl3u@aV76bUd-v26_wg3Ac_nZ0O z_H6jy1>bs+3SA=%)(fTEz+Cokh~OyjIvZ$dy^+Km$R_<9l|_Z|EpWU*8R7A zr{TZtQ|$kp@Ls5J8me&c;oSBiVHP#VVIqd4HV@371IkHa?k|!gEY61HaQt?>^Kopx z&Jr=m!@0ODJzc0zSHw$P!XETxD4-NtEdO0d-z6&GPWbB$`w&38Q{(y7afo zD;GIo)GuexMKKR2r+O`rqoaQ97NdWBemzR{_a4VVbwQ82`nOlZ9>egv^nWs&cjP~9 z?*GiUDW3mz!jX=5!7APzn<|_KCyOYak`XCSbS@D3H7!r%GS#v!qm}Aq5qAEs{{ME+ xZ!7;Z&-JkXBmX1+0{{R300000000000000000000006*RegI!e&cXmt008=hi2VQn literal 0 HcmV?d00001 diff --git a/tests/data/packages/TopoRequires2/setup.cfg.pending b/tests/data/packages/TopoRequires2/setup.cfg.pending new file mode 100644 index 000000000..d42f01a53 --- /dev/null +++ b/tests/data/packages/TopoRequires2/setup.cfg.pending @@ -0,0 +1,4 @@ +[metadata] +name = TopoRequires2 +install-requires = + TopoRequires diff --git a/tests/data/packages/TopoRequires2/setup.py b/tests/data/packages/TopoRequires2/setup.py new file mode 100644 index 000000000..019f43cb2 --- /dev/null +++ b/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'], +) diff --git a/tests/data/packages/TopoRequires2/toporequires2/__init__.py b/tests/data/packages/TopoRequires2/toporequires2/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/data/packages/TopoRequires3-0.0.1.tar.gz b/tests/data/packages/TopoRequires3-0.0.1.tar.gz new file mode 100644 index 0000000000000000000000000000000000000000..165ad757f8fab7b1c6fc704e98561bfa93f8cf1f GIT binary patch literal 792 zcmV+z1Lyo7iwFoT8yi&u|72-%bT3qIaBos&adl~OWpgtvFfK4IF)nmrascg_TW^~% z7>2p#SD@TvlNxFR1}#!9rk&L-t$NrlstUP@!&o^DoM_dw|2{%WTTp3wptM`x=f>f) z2}(Y{{$A|;IEi<~&$*w9%p4G%=yX$Q8It2TY}rzWk;`p~LJdZ2n=*@83Zcw&h@vs2 z1Lm3JsirAud@NEqX&kq%zx*L(`;V}HMgB7(=ZT)2wwZt1wi@$qm~54Q=GZ1P^87oL zFhwKC|ARA2<4`LSF5@`JG(Sw@RIVbbS~M?IEhplk81*aDrN2@1EJT|5aWv`|v*#?B z@W~1HL^e9?%lw}7{GOv#zaQoQdl0x~M?UWIcQ1#%R2A?T`me-v;dui;n#G&<(*Ld2 ze}me@#`^!n@?T0-m%n7I>wjMFYx%bfVqpF6I{#R2vHoA9|Gn*l-8b*{ZY}?{`Zpb; zrhn6NEa<-rO0TarBziL{>&7O>ss1C2BlQL~ zsA){@H{C!iL{JQ>xX(@3^&?-pZi{a>&qn`O<*}XmPiRg5%x2JkC!pp-|2NcsWnFF8 z|J!x{Z<^Tu>4Zvo#9{#bH|xLjW_Q~ExmN$osr!G!wxRzn_}%j}Hx+^4ndl9*84ohC zq4m7nB%kf;=F#5d)y!g)_x?aI&sYEDt6=N;HypF3f5XA~Pd9uKl25thgRd*K4>hV2 zb<9IC)GG5pJs(g^5_NALhCDqTY6s)3@%Fc|`Y{g0AmN_42)&-m?{Qkc9*B_p!A115 zKM_%O>9)5bo1}gsi>H;roq)?(oQ4+>i1&f?U)?gf|1)lX{zD9E;r!=m^4|rAxxI6j_U=fP+;flTelXQWT5*y4 z0+D|v#YMJ=^7n*=zw`h_5cT(et^ap6`fd4tYFoAg{lovm{{sL3000000000000000 W000000000$6Mq3`RC28VPyhfvx1D7G literal 0 HcmV?d00001 diff --git a/tests/data/packages/TopoRequires3/setup.cfg.pending b/tests/data/packages/TopoRequires3/setup.cfg.pending new file mode 100644 index 000000000..cdd67122b --- /dev/null +++ b/tests/data/packages/TopoRequires3/setup.cfg.pending @@ -0,0 +1,4 @@ +[metadata] +name = TopoRequires3 +install-requires = + TopoRequires diff --git a/tests/data/packages/TopoRequires3/setup.py b/tests/data/packages/TopoRequires3/setup.py new file mode 100644 index 000000000..772ed618e --- /dev/null +++ b/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'], +) diff --git a/tests/data/packages/TopoRequires3/toporequires3/__init__.py b/tests/data/packages/TopoRequires3/toporequires3/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/data/packages/TopoRequires4/setup.cfg.pending b/tests/data/packages/TopoRequires4/setup.cfg.pending new file mode 100644 index 000000000..2fc2c551f --- /dev/null +++ b/tests/data/packages/TopoRequires4/setup.cfg.pending @@ -0,0 +1,6 @@ +[metadata] +name = TopoRequires4 +install-requires = + TopoRequires2 + TopoRequires + TopoRequires3 diff --git a/tests/data/packages/TopoRequires4/setup.py b/tests/data/packages/TopoRequires4/setup.py new file mode 100644 index 000000000..e276f55a2 --- /dev/null +++ b/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'], +) diff --git a/tests/data/packages/TopoRequires4/toporequires4/__init__.py b/tests/data/packages/TopoRequires4/toporequires4/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py index 426523589..f39051455 100644 --- a/tests/functional/test_install.py +++ b/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