Fix resolve to track packages cached mid-resolve.

Before the fix the `_ResolvableSet` was blind to any packages built and
cached in earlier loops of a complex resolve.  This change makes the
resolvable set aware by tracking built packages and replacing unbuilt
packages with them in the resolvable set.

Failing tests were added that now pass with the fixes.

Testing Done:
CI went green here:
  https://travis-ci.org/pantsbuild/pex/builds/69049830

Bugs closed: 120, 121

Reviewed at https://rbcommons.com/s/twitter/r/2341/

Author: jsirois

CHANGES.rst

@@ -13,6 +13,9 @@ CHANGES
 * Bug fix: PEXBuilder did not respect the target interpreter when compiling source to bytecode.
   Fixes `#127 <https://github.com/pantsbuild/pex/issues/127>`_.
 
+* Bug fix: Fix complex resolutions when using a cache.
+  Fixes: `#120 <https://github.com/pantsbuild/pex/issues/120>`_.
+
 -----
 1.0.0
 -----

pex/link.py

@@ -76,6 +76,9 @@ def __init__(self, url):
       purl = urlparse.urlparse(self._normalize(url))
     self._url = purl
 
+  def __ne__(self, other):
+    return not self.__eq__(other)
+
   def __eq__(self, link):
     return self.__class__ == link.__class__ and self._url == link._url
 

pex/resolver.py

@@ -18,6 +18,7 @@
 from .resolvable import ResolvableRequirement, resolvables_from_iterable
 from .resolver_options import ResolverOptionsBuilder
 from .tracer import TRACER
+from .util import DistributionHelper
 
 
 class Untranslateable(Exception):
@@ -55,9 +56,9 @@ def merge(self, other):
 
 
 class _ResolvableSet(object):
-  def __init__(self):
+  def __init__(self, tuples=None):
     # A list of _ResolvedPackages
-    self.__tuples = []
+    self.__tuples = tuples or []
 
   def _collapse(self):
     # Collapse all resolvables by name along with the intersection of all compatible packages.
@@ -112,6 +113,18 @@ def extras(self, name):
     return set.union(
         *[set(tup.resolvable.extras()) for tup in self.__tuples if tup.resolvable.name == name])
 
+  def replace_built(self, built_packages):
+    """Return a copy of this resolvable set but with built packages.
+
+    :param dict built_packages: A mapping from a resolved package to its locally built package.
+    :returns: A new resolvable set with built package replacements made.
+    """
+    def map_packages(resolved_packages):
+      packages = OrderedSet(built_packages.get(p, p) for p in resolved_packages.packages)
+      return _ResolvedPackages(resolved_packages.resolvable, packages, resolved_packages.parent)
+
+    return _ResolvableSet([map_packages(rp) for rp in self.__tuples])
+
 
 class Resolver(object):
   """Interface for resolving resolvable entities into python packages."""
@@ -165,6 +178,7 @@ def resolve(self, resolvables, resolvable_set=None):
         resolvable_set.merge(resolvable, packages, parent)
         processed_resolvables.add(resolvable)
 
+      built_packages = {}
       for resolvable, packages, parent in resolvable_set.packages():
         assert len(packages) > 0, 'ResolvableSet.packages(%s) should not be empty' % resolvable
         package = next(iter(packages))
@@ -174,13 +188,19 @@ def resolve(self, resolvables, resolvable_set=None):
             raise self.Error('Ambiguous resolvable: %s' % resolvable)
           continue
         if package not in distributions:
-          distributions[package] = self.build(package, resolvable.options)
+          dist = self.build(package, resolvable.options)
+          built_package = Package.from_href(dist.location)
+          built_packages[package] = built_package
+          distributions[built_package] = dist
+          package = built_package
+
         distribution = distributions[package]
         processed_packages[resolvable.name] = package
         new_parent = '%s->%s' % (parent, resolvable) if parent else str(resolvable)
         resolvables.extend(
             (ResolvableRequirement(req, resolvable.options), new_parent) for req in
             distribution.requires(extras=resolvable_set.extras(resolvable.name)))
+      resolvable_set = resolvable_set.replace_built(built_packages)
 
     return list(distributions.values())
 
@@ -233,7 +253,8 @@ def build(self, package, options):
       shutil.copyfile(dist.location, target + '~')
       os.rename(target + '~', target)
     os.utime(target, None)
-    return dist
+
+    return DistributionHelper.distribution_from_path(target)
 
 
 def resolve(

pex/testing.py

@@ -77,6 +77,7 @@ def write_zipfile(directory, dest, reverse=False):
               'scripts/shell_script',
           ],
           package_data={'my_package': ['package_data/*.dat']},
+          install_requires=%(install_requires)r,
       )
   '''),
   'scripts/hello_world': '#!/usr/bin/env python\nprint("hello world!")\n',
@@ -89,21 +90,23 @@ def write_zipfile(directory, dest, reverse=False):
 
 
 @contextlib.contextmanager
-def make_installer(name='my_project', installer_impl=EggInstaller, zip_safe=True):
-  interp = {'project_name': name, 'zip_safe': zip_safe}
+def make_installer(name='my_project', installer_impl=EggInstaller, zip_safe=True,
+                   install_reqs=None):
+  interp = {'project_name': name, 'zip_safe': zip_safe, 'install_requires': install_reqs or []}
   with temporary_content(PROJECT_CONTENT, interp=interp) as td:
     yield installer_impl(td)
 
 
 @contextlib.contextmanager
-def make_source_dir(name='my_project'):
-  interp = {'project_name': name, 'zip_safe': True}
+def make_source_dir(name='my_project', install_reqs=None):
+  interp = {'project_name': name, 'zip_safe': True, 'install_requires': install_reqs or []}
   with temporary_content(PROJECT_CONTENT, interp=interp) as td:
     yield td
 
 
-def make_sdist(name='my_project', zip_safe=True):
-  with make_installer(name=name, installer_impl=Packager, zip_safe=zip_safe) as packager:
+def make_sdist(name='my_project', zip_safe=True, install_reqs=None):
+  with make_installer(name=name, installer_impl=Packager, zip_safe=zip_safe,
+                      install_reqs=install_reqs) as packager:
     return packager.sdist()
 
 

pex/version.py

@@ -1,7 +1,7 @@
 # Copyright 2015 Pants project contributors (see CONTRIBUTORS.md).
 # Licensed under the Apache License, Version 2.0 (see LICENSE).
 
-__version__ = '1.0.0'
+__version__ = '1.0.1.dev1'
 
 SETUPTOOLS_REQUIREMENT = 'setuptools>=2.2,<16'
 WHEEL_REQUIREMENT = 'wheel>=0.24.0,<0.25.0'

tests/test_link.py

@@ -52,3 +52,8 @@ def test_link_schemes():
   assert link.scheme == 'file'
   assert link.local
   assert link.path == os.path.realpath('/foo/bar')
+
+
+def test_link_equality():
+  assert Link('http://www.google.com') == Link('http://www.google.com')
+  assert Link('http://www.google.com') != Link('http://www.twitter.com')

tests/test_resolver.py

@@ -34,6 +34,20 @@ def test_simple_local_resolve():
     assert len(dists) == 1
 
 
+def test_diamond_local_resolve_cached():
+  # This exercises the issue described here: https://github.com/pantsbuild/pex/issues/120
+  project1_sdist = make_sdist(name='project1', install_reqs=['project2<1.0.0'])
+  project2_sdist = make_sdist(name='project2')
+
+  with temporary_dir() as dd:
+    for sdist in (project1_sdist, project2_sdist):
+      safe_copy(sdist, os.path.join(dd, os.path.basename(sdist)))
+    fetchers = [Fetcher([dd])]
+    with temporary_dir() as cd:
+      dists = resolve(['project1', 'project2'], fetchers=fetchers, cache=cd, cache_ttl=1000)
+      assert len(dists) == 2
+
+
 def test_resolvable_set():
   builder = ResolverOptionsBuilder()
   rs = _ResolvableSet()
@@ -56,4 +70,24 @@ def test_resolvable_set():
     rs.merge(rq, [binary_pkg])
 
 
+def test_resolvable_set_built():
+  builder = ResolverOptionsBuilder()
+  rs = _ResolvableSet()
+  rq = ResolvableRequirement.from_string('foo', builder)
+  source_pkg = SourcePackage.from_href('foo-2.3.4.tar.gz')
+  binary_pkg = EggPackage.from_href('foo-2.3.4-py3.4.egg')
+
+  rs.merge(rq, [source_pkg])
+  assert rs.get('foo') == set([source_pkg])
+  assert rs.packages() == [(rq, set([source_pkg]), None)]
+
+  with pytest.raises(Unsatisfiable):
+    rs.merge(rq, [binary_pkg])
+
+  updated_rs = rs.replace_built({source_pkg: binary_pkg})
+  updated_rs.merge(rq, [binary_pkg])
+  assert updated_rs.get('foo') == set([binary_pkg])
+  assert updated_rs.packages() == [(rq, set([binary_pkg]), None)]
+
+
 # TODO(wickman) Write more than simple resolver test.