Multiple dependency_managements with multiple ivy resolves.

This is the second pass of implementing dependency management in
pants.

The first pass was reviewed here:

    https://rbcommons.com/s/twitter/r/3336/

The design document is available here:

    https://docs.google.com/document/d/1AM_0e1Az_NHtR150Zsuyaa6u7InzBQGLq8MrUT57Od8/edit#

This change allows managed_jar_dependencies targets to be chained
together; the set of pinned artifact is the union of all jar
coordinates specified by the transitive closure of the target
specified in a jar_library's managed_dependencies field.

When there are conflicts between dependencies in the transitive
closure, the default behavior is to raise an error. Optionally,
if using --no-jar-dependency-management-require-disjoint-sets,
the most direct version (the first version encountered in a BFS)
will be used, and a warning will be logged.

The change to make ivy do multiple resolves is fairly simple; see
ivy_task_mixin.py's resolve() method.

Testing Done:
Added integration tests for running multiple resolves, and for the new managed_jar_libraries factory.

Added unit tests for transitive artifact set calculation.

CI went green here: https://travis-ci.org/pantsbuild/pants/builds/104711840
CI went green here: https://travis-ci.org/pantsbuild/pants/builds/104947421
CI went green here: https://travis-ci.org/pantsbuild/pants/builds/104985770

Bugs closed: 2830

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

src/python/pants/backend/jvm/ivy_utils.py

@@ -354,7 +354,8 @@ def _parse_xml_report(cls, conf, path):
     return ret
 
   @classmethod
-  def generate_ivy(cls, targets, jars, excludes, ivyxml, confs, resolve_hash_name=None):
+  def generate_ivy(cls, targets, jars, excludes, ivyxml, confs, resolve_hash_name=None,
+                   pinned_artifacts=None):
     if resolve_hash_name:
       org = IvyUtils.INTERNAL_ORG_NAME
       name = resolve_hash_name
@@ -369,7 +370,7 @@ def generate_ivy(cls, targets, jars, excludes, ivyxml, confs, resolve_hash_name=
       jars.append(jar)
 
     manager = JarDependencyManagement.global_instance()
-    artifact_set = PinnedJarArtifactSet(manager.for_targets(targets))
+    artifact_set = PinnedJarArtifactSet(pinned_artifacts) # Copy, because we're modifying it.
     for jars in jars_by_key.values():
       for i, dep in enumerate(jars):
         direct_coord = M2Coordinate.create(dep)

src/python/pants/backend/jvm/subsystems/jar_dependency_management.py

@@ -173,33 +173,6 @@ def targets_by_artifact_set(self, targets):
       sets_to_targets[self.for_target(target)].append(target)
     return dict(sets_to_targets)
 
-  def for_targets(self, targets):
-    """Attempts to find a single PinnedJarArtifactSet that works for all input targets.
-
-    This just partitions the targets using the targets_by_artifact_set partitioning logic, and
-    returns the key that works for all targets (ie the only key in the resulting map).
-
-    Returns None if the resulting map has no entries, and raises an error if it has more than one.
-
-    :param list targets: the input targets.
-    :return: the single PinnedJarArtifactSet that works for everything, or None.
-    :rtype: PinnedJarArtifactSet
-    :raises: JarDependencyManagement.IncompatibleManagedJarDependencies
-    """
-    # TODO(gmalmquist): Remove this method once we properly run multiple ivy resolves for different
-    # sets of pinned artifacts.
-    sets_to_targets = self.targets_by_artifact_set(targets)
-    sets = [artifact_set for artifact_set in sets_to_targets
-            if any(isinstance(target, JarLibrary) for target in sets_to_targets[artifact_set])]
-    if not sets:
-      return None
-    if len(sets) == 1:
-      return next(iter(sets))
-    raise self.IncompatibleManagedJarDependencies(
-      'Targets use multiple incompatible managed_jar_dependencies: {}'
-      .format(', '.join(map(str, sets)))
-    )
-
   def for_target(self, target):
     """Computes and returns the PinnedJarArtifactSet that should be used to manage the given target.
 
@@ -332,6 +305,10 @@ def __init__(self, target, coord, rev1, rev2):
         )
       )
 
+  class IllegalVersionOverride(TaskError):
+    """An artifact version in a managed_jar_dependencies() target differs from that of a dependency.
+    """
+
   class MissingVersion(TargetDefinitionException):
     """A jar used to construct a managed_jar_dependencies artifact set is missing a version."""
 
@@ -355,15 +332,6 @@ def execute(self):
       if library.managed_dependencies:
         targets.add(library.managed_dependencies)
     self._compute_artifact_sets(targets)
-    self._validate_managed_jar_dependencies_targets(targets)
-
-  def _validate_managed_jar_dependencies_targets(self, targets):
-    for target in targets:
-      if target.dependencies:
-        # TODO(gmalmquist): Remove this when we handle merging of transitive
-        # managed_jar_dependencies.
-        self.context.warn('{}: Chaining managed_jar_dependencies objects is not yet supported.'
-                          .format(target.address.spec))
 
   def _resolve_default_target(self):
     manager = JarDependencyManagement.global_instance()
@@ -377,9 +345,10 @@ def _resolve_default_target(self):
         'Unable to resolve default managed_jar_dependencies target: {}'.format(spec))
     target = targets[0]
     if not isinstance(target, ManagedJarDependencies):
-      raise self.InvalidDefaultTarget(
-        'Default managed_jar_dependencies target is an invalid target type: "{}" is a {}.'
-        .format(spec, type(target).__name__))
+      if not any(isinstance(t, ManagedJarDependencies) for t in target.closure()):
+        raise self.InvalidDefaultTarget(
+          'Neither the default target nor any of its transitive dependencies is a '
+          'managed_jar_dependencies() target! "{}" is a {}.'.format(spec, type(target).__name__))
     manager._artifact_set_map[target.id] = self._compute_artifact_set(target)
     manager._default_target = target
 
@@ -407,16 +376,56 @@ def _jar_iterator(self, managed_jar_dependencies):
                                         'Artifacts must be jar() objects or the addresses of '
                                         'jar_library objects.')
 
-  def _compute_artifact_set(self, managed_jar_dependencies):
-    # TODO(gmalmquist): Extend this to be the union of any pinned artifacts from transitive
-    # dependencies of this managed_jar_dependencies object. If there are conflicts between the versions
-    # specified by parents and children, the children should win.
+  def _compute_artifact_set(self, management_target):
+    """Computes the set of pinned artifacts specified by this target, and any of its dependencies.
+
+    An error is raised if a conflict exists between a pinned version between a
+    ManagedJarDependencies target and any of its dependencies, or if two versions of a jar exist in
+    the same ManagedJarDependencies target.
+
+    :param Target management_target: a target object which is (or at least depends on) a
+      ManagedJarDependencies target.
+    :return: the computed transitive artifact set (approximately the union of all pinned artifacts
+      in the transitive closure of the input target).
+    :rtype: PinnedJarArtifactSet
+    """
     artifact_set = PinnedJarArtifactSet()
-    for jar in self._jar_iterator(managed_jar_dependencies):
-      if jar.rev is None:
-        raise self.MissingVersion(managed_jar_dependencies, jar)
-      if jar in artifact_set and artifact_set[jar].rev != jar.rev:
-        raise self.DuplicateCoordinateError(managed_jar_dependencies, jar, artifact_set[jar].rev,
-                                            jar.rev)
-      artifact_set.put(jar)
+
+    # Keeps track of where pinned artifacts came from for logging purposes.
+    specs_by_coordinate = {}
+
+    def handle_managed_jar_dependencies(target):
+      subset = PinnedJarArtifactSet()
+      for jar in self._jar_iterator(target):
+        if jar.rev is None:
+          raise self.MissingVersion(target, jar)
+        if jar in subset and subset[jar].rev != jar.rev:
+          raise self.DuplicateCoordinateError(target, jar, artifact_set[jar].rev, jar.rev)
+        subset.put(jar)
+      return subset
+
+    def handle_conflict(artifact, target):
+      previous_coord = artifact_set[artifact]
+      previous_spec = specs_by_coordinate[previous_coord]
+      message = ('Artifact {previous_coord} (from {previous_target}) overridden by {new_coord} '
+                 '(in {new_target}).'.format(previous_coord=previous_coord,
+                                             previous_target=previous_spec,
+                                             new_coord=artifact,
+                                             new_target=target.address.spec))
+      raise self.IllegalVersionOverride(message)
+
+    def handle_target(target):
+      if not isinstance(target, ManagedJarDependencies):
+        return
+      for artifact in handle_managed_jar_dependencies(target):
+        if artifact.rev != artifact_set[artifact].rev:
+          handle_conflict(artifact, target)
+        specs_by_coordinate[M2Coordinate.create(artifact)] = target.address.spec
+        artifact_set.put(artifact)
+
+    self.context.build_graph.walk_transitive_dependency_graph(
+      addresses=[management_target.address],
+      work=handle_target,
+      postorder=True, # Process dependencies first.
+    )
     return artifact_set

src/python/pants/backend/jvm/tasks/ivy_resolve.py

@@ -85,13 +85,14 @@ def execute(self):
     targets = self.context.targets()
     compile_classpath = self.context.products.get_data('compile_classpath',
         init_func=ClasspathProducts.init_func(self.get_options().pants_workdir))
-    resolve_hash_name = self.resolve(executor=executor,
-                                     targets=targets,
-                                     classpath_products=compile_classpath,
-                                     confs=self.get_options().confs,
-                                     extra_args=self._args)
+    resolve_hash_names = self.resolve(executor=executor,
+                                      targets=targets,
+                                      classpath_products=compile_classpath,
+                                      confs=self.get_options().confs,
+                                      extra_args=self._args)
     if self._report:
-      self._generate_ivy_report(resolve_hash_name)
+      for resolve_hash_name in resolve_hash_names:
+        self._generate_ivy_report(resolve_hash_name)
 
   def check_artifact_cache_for(self, invalidation_check):
     # Ivy resolution is an output dependent on the entire target set, and is not divisible

src/python/pants/backend/jvm/tasks/ivy_task_mixin.py

@@ -111,9 +111,21 @@ def resolve(self, executor, targets, classpath_products, confs=None, extra_args=
     :returns: The id of the reports associated with this resolve.
     :rtype: string
     """
-
+    targets_by_sets = JarDependencyManagement.global_instance().targets_by_artifact_set(targets)
+    resolve_hash_names = []
+    for artifact_set, target_subset in targets_by_sets.items():
+      resolve_hash_names.append(self._resolve_subset(executor,
+                                                     target_subset,
+                                                     classpath_products,
+                                                     confs=confs,
+                                                     extra_args=extra_args,
+                                                     invalidate_dependents=invalidate_dependents,
+                                                     pinned_artifacts=artifact_set))
+    return resolve_hash_names
+
+  def _resolve_subset(self, executor, targets, classpath_products, confs=None, extra_args=None,
+              invalidate_dependents=False, pinned_artifacts=None):
     classpath_products.add_excludes_for_targets(targets)
-
     confs = confs or ('default',)
 
     # After running ivy, we parse the resulting report, and record the dependencies for
@@ -125,6 +137,7 @@ def resolve(self, executor, targets, classpath_products, confs=None, extra_args=
       confs=confs,
       custom_args=extra_args,
       invalidate_dependents=invalidate_dependents,
+      pinned_artifacts=pinned_artifacts,
     )
 
     if not resolve_hash_name:
@@ -182,7 +195,8 @@ def ivy_resolve(self,
                   workunit_name=None,
                   confs=None,
                   custom_args=None,
-                  invalidate_dependents=False):
+                  invalidate_dependents=False,
+                  pinned_artifacts=None):
     """Resolves external dependencies for the given targets.
 
     If there are no targets suitable for jvm transitive dependency resolution, an empty result is
@@ -212,8 +226,6 @@ def ivy_resolve(self,
 
     fingerprint_strategy = IvyResolveFingerprintStrategy(confs)
 
-    # TODO(gmalmquist): Targets should be partitioned and resolved in separate groups depending on
-    # what sets of pinned artifacts they depend on.
     with self.invalidated(targets,
                           invalidate_dependents=invalidate_dependents,
                           silent=silent,
@@ -261,7 +273,8 @@ def ivy_resolve(self,
             workunit_name=workunit_name,
             confs=confs,
             use_soft_excludes=self.get_options().soft_excludes,
-            resolve_hash_name=resolve_hash_name)
+            resolve_hash_name=resolve_hash_name,
+            pinned_artifacts=pinned_artifacts)
 
         if not os.path.exists(raw_target_classpath_file_tmp):
           raise self.Error('Ivy failed to create classpath file at {}'
@@ -296,7 +309,8 @@ def _exec_ivy(self,
                ivy=None,
                workunit_name='ivy',
                use_soft_excludes=False,
-               resolve_hash_name=None):
+               resolve_hash_name=None,
+               pinned_artifacts=None):
     ivy_jvm_options = self.get_options().jvm_options[:]
     # Disable cache in File.getCanonicalPath(), makes Ivy work with -symlink option properly on ng.
     ivy_jvm_options.append('-Dsun.io.useCanonCaches=false')
@@ -317,7 +331,8 @@ def _exec_ivy(self,
     try:
       jars, excludes = IvyUtils.calculate_classpath(targets, gather_excludes=not use_soft_excludes)
       with IvyUtils.ivy_lock:
-        IvyUtils.generate_ivy(targets, jars, excludes, ivyxml, confs_to_resolve, resolve_hash_name)
+        IvyUtils.generate_ivy(targets, jars, excludes, ivyxml, confs_to_resolve, resolve_hash_name,
+                              pinned_artifacts)
         runner = ivy.runner(jvm_options=ivy_jvm_options, args=ivy_args, executor=executor)
         try:
           result = execute_runner(runner, workunit_factory=self.context.new_workunit,

testprojects/src/java/org/pantsbuild/testproject/depman/TEST_BUILD

@@ -23,6 +23,13 @@ jar_library(name='library-managed',
   managed_dependencies=':manager',
 )
 
+jar_library(name='library-managed2',
+  jars=[
+    jar(org='jersey', name='jersey'),
+  ],
+  managed_dependencies=':manager2',
+)
+
 jar_library(name='library-managed-forceful',
   jars=[
     jar(org='jersey', name='jersey', rev='0.5-ea', force=True),
@@ -68,6 +75,15 @@ jvm_binary(name='managed',
   ],
 )
 
+jvm_binary(name='managed2',
+  main='org.pantsbuild.testproject.depman.PrintClasspath',
+  source='PrintClasspath.java',
+  platform='java7',
+  dependencies=[
+    ':library-managed2',
+  ],
+)
+
 jvm_binary(name='managed-auto',
   main='org.pantsbuild.testproject.depman.PrintClasspath',
   source='PrintClasspath.java',

testprojects/tests/java/org/pantsbuild/testproject/depman/BUILD

@@ -0,0 +1,54 @@
+# Tests for dependency management integration.
+
+managed_jar_dependencies(name='new-manager',
+  artifacts=[
+    jar('jersey', 'jersey', '0.7-ea'),
+  ],
+)
+
+managed_jar_dependencies(name='old-manager',
+  artifacts=[
+    jar('jersey', 'jersey', '0.4-ea'),
+  ],
+)
+
+jar_library(name='common-lib',
+  jars=[
+    jar('javax.annotation', 'jsr250-api', '1.0'),
+    jar('javax.persistence', 'persistence-api', '1.0.2'),
+    jar('javax.servlet', 'servlet-api', '2.5'),
+    jar('com.sun.xml.txw2', 'txw2', '20110809'),
+  ],
+)
+
+jar_library(name='old-jersey',
+  jars=[
+    jar('jersey', 'jersey'),
+  ],
+  managed_dependencies=':old-manager',
+)
+
+jar_library(name='new-jersey',
+  jars=[
+    jar('jersey', 'jersey'),
+  ],
+  managed_dependencies=':new-manager',
+)
+
+junit_tests(name='old-tests',
+  sources=['OldTest.java'],
+  dependencies=[
+    '3rdparty:junit',
+    ':common-lib',
+    ':old-jersey',
+  ],
+)
+
+junit_tests(name='new-tests',
+  sources=['NewTest.java'],
+  dependencies=[
+    '3rdparty:junit',
+    ':common-lib',
+    ':new-jersey',
+  ],
+)

testprojects/tests/java/org/pantsbuild/testproject/depman/NewTest.java

@@ -0,0 +1,21 @@
+package org.pantsbuild.testproject.depman.NewTest;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+
+/** Tests that should have jersey-0.7-ea on the classpath. */
+public class NewTest {
+
+  @Test
+  public void testNewIsPresent() {
+    assertEquals("WadlFactory", com.sun.ws.rest.impl.wadl.WadlFactory.class.getSimpleName());
+  }
+
+  @Test(expected=ClassNotFoundException.class)
+  public void testOldNotPresent() throws ClassNotFoundException {
+    String notInNew = "com.sun.ws.rest.tools.webapp.writer.WebApp";
+    getClass().getClassLoader().loadClass(notInNew);
+  }
+
+}

testprojects/tests/java/org/pantsbuild/testproject/depman/OldTest.java

@@ -0,0 +1,21 @@
+package org.pantsbuild.testproject.depman.OldTest;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+
+/** Tests that should have jersey-0.4-ea on the classpath. */
+public class OldTest {
+
+  @Test
+  public void testOldIsPresent() {
+    assertEquals("WebApp", com.sun.ws.rest.tools.webapp.writer.WebApp.class.getSimpleName());
+  }
+
+  @Test(expected=ClassNotFoundException.class)
+  public void testNewNotPresent() throws ClassNotFoundException {
+    String notInOld = "com.sun.ws.rest.impl.wadl.WadlFactory";
+    getClass().getClassLoader().loadClass(notInOld);
+  }
+
+}