Deeper selection of internal targets during publishing

The current behavior of selecting directly declared Jarable targets broke ages ago during Patrick's target refactor. In order to appropriately walk through intermediate jar_library/target declarations, we need to recurse.

It's common practice internally to have a top-level alias (a 'target' declaration) to point deeper into the directory structure to an actual java_library or scala_library. This meant that publishing was 1) ignoring those deps during fingerprint calculation and 2) not including them via DependencyWriter in written poms.

Testing Done:
test publishes; will follow up to add unit tests here if things look sane.

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

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

@@ -282,7 +282,18 @@ def pushdb_coordinate(jar, entry):
 
 
 def target_internal_dependencies(target):
-  return filter(lambda tgt: isinstance(tgt, Jarable), target.dependencies)
+  """Returns internal Jarable dependencies that were "directly" declared.
+
+  Directly declared deps are those that are explicitly listed in the definition of a
+  target, rather than being depended on transitively. But in order to walk through
+  aggregator targets such as `target`, `dependencies`, or `jar_library`, this recursively
+  descends the dep graph and stops at Jarable instances."""
+  for dep in target.dependencies:
+    if isinstance(dep, Jarable):
+      yield dep
+    else:
+      for childdep in target_internal_dependencies(dep):
+        yield childdep
 
 
 class JarPublish(JarTask, ScmPublish):

tests/python/pants_test/base_test.py

@@ -191,27 +191,31 @@ def create_files(self, path, files):
     for f in files:
       self.create_file(os.path.join(path, f), contents=f)
 
-  def create_library(self, path, target_type, name, sources, **kwargs):
+  def create_library(self, path, target_type, name, sources=None, **kwargs):
     """Creates a library target of given type at the BUILD file at path with sources
 
      path: The relative path to the BUILD file from the build root.
      target_type: valid pants target type.
      name: Name of the library target.
      sources: List of source file at the path relative to path.
      **kwargs: Optional attributes that can be set for any library target.
-       Currently it includes support for resources and java_sources
+       Currently it includes support for resources, java_sources, provides
+       and dependencies.
     """
-    self.create_files(path, sources)
+    if sources:
+      self.create_files(path, sources)
     self.add_to_build_file(path, dedent('''
           %(target_type)s(name='%(name)s',
-            sources=%(sources)s,
+            %(sources)s
             %(resources)s
             %(java_sources)s
             %(provides)s
+            %(dependencies)s
           )
         ''' % dict(target_type=target_type,
                    name=name,
-                   sources=repr(sources or []),
+                   sources=('sources=%s,' % repr(sources)
+                              if sources else ''),
                    resources=('resources=["%s"],' % kwargs.get('resources')
                               if 'resources' in kwargs else ''),
                    java_sources=('java_sources=[%s],'
@@ -220,6 +224,8 @@ def create_library(self, path, target_type, name, sources, **kwargs):
                                  if 'java_sources' in kwargs else ''),
                    provides=('provides=%s,' % kwargs.get('provides')
                               if 'provides' in kwargs else ''),
+                   dependencies=('dependencies=%s,' % kwargs.get('dependencies')
+                              if 'dependencies' in kwargs else ''),
                    )))
     return self.target('%s:%s' % (path, name))
 

tests/python/pants_test/tasks/test_jar_publish.py

@@ -11,6 +11,7 @@
 from mock import Mock
 import pytest
 
+from pants.backend.core.targets.dependencies import Dependencies
 from pants.backend.jvm.artifact import Artifact
 from pants.backend.jvm.repository import Repository
 from pants.backend.jvm.targets.jar_library import JarLibrary
@@ -46,6 +47,7 @@ def alias_groups(self):
       targets={
         'jar_library': JarLibrary,
         'java_library': JavaLibrary,
+        'target': Dependencies,
       },
       objects={
         'artifact': Artifact,
@@ -54,20 +56,27 @@ def alias_groups(self):
       },
     )
 
-  def _prepare_for_publishing(self):
-    a = self.create_library('a', 'java_library', 'a', ['A.java'],
-                            provides="""artifact(org='com.example', name='nail', repo=internal)""")
+  def _prepare_for_publishing(self, with_alias=False):
+    targets = {}
+    targets['a'] = self.create_library('a', 'java_library', 'a', ['A.java'],
+                                       provides="""artifact(org='com.example', name='nail', repo=internal)""")
 
-    b = self.create_library('b', 'java_library', 'b', ['B.java'],
-                            provides="""artifact(org='com.example', name='shoe', repo=internal)""",
-                            dependencies=[a])
+    targets['b'] = self.create_library('b', 'java_library', 'b', ['B.java'],
+                                   provides="""artifact(org='com.example', name='shoe', repo=internal)""",
+                                   dependencies=['a'])
 
-    c = self.create_library('c', 'java_library', 'c', ['C.java'],
-                            provides="""artifact(org='com.example', name='horse', repo=internal)""",
-                            dependencies=[b])
+    if with_alias:
+      # add an alias target between c and b
+      targets['z'] = self.create_library('z', 'target', 'z', dependencies=['b'])
+      c_deps = ['z']
+    else:
+      c_deps = ['b']
+
+    targets['c'] = self.create_library('c', 'java_library', 'c', ['C.java'],
+                                       provides="""artifact(org='com.example', name='horse', repo=internal)""",
+                                       dependencies=c_deps)
 
-    targets = [a, b, c]
-    return targets
+    return targets.values()
 
   def _get_config(self):
     return """
@@ -142,29 +151,31 @@ def test_publish_local_dryrun(self):
                         "Expected publish not to be called")
 
   def test_publish_local(self):
-    targets = self._prepare_for_publishing()
-
-    with temporary_dir() as publish_dir:
-      task = self.prepare_task(args=['--test-local=%s' % publish_dir,
-                                     '--no-test-dryrun'],
-                               build_graph=self.build_graph,
-                               build_file_parser=self.build_file_parser,
-                               targets=targets)
-      self._prepare_mocks(task)
-      task.execute()
-
-      #Nothing is written to the pushdb during a local publish
-      #(maybe some directories are created, but git will ignore them)
-      files = []
-      for _, _, filenames in safe_walk(self.push_db_basedir):
-        files.extend(filenames)
-      self.assertEquals(0, len(files),
-                        "Nothing should be written to the pushdb during a local publish")
-
-      self.assertEquals(len(targets), task.confirm_push.call_count,
-                        "Expected one call to confirm_push per artifact")
-      self.assertEquals(len(targets), task.publish.call_count,
-                        "Expected one call to publish per artifact")
+    for with_alias in [True, False]:
+      targets = self._prepare_for_publishing(with_alias=with_alias)
+
+      with temporary_dir() as publish_dir:
+        task = self.prepare_task(args=['--test-local=%s' % publish_dir,
+                                      '--no-test-dryrun'],
+                                build_graph=self.build_graph,
+                                build_file_parser=self.build_file_parser,
+                                targets=targets)
+        self._prepare_mocks(task)
+        task.execute()
+
+        #Nothing is written to the pushdb during a local publish
+        #(maybe some directories are created, but git will ignore them)
+        files = []
+        for _, _, filenames in safe_walk(self.push_db_basedir):
+          files.extend(filenames)
+        self.assertEquals(0, len(files),
+                          "Nothing should be written to the pushdb during a local publish")
+
+        publishable_count = len(targets) - (1 if with_alias else 0)
+        self.assertEquals(publishable_count, task.confirm_push.call_count,
+                          "Expected one call to confirm_push per artifact")
+        self.assertEquals(publishable_count, task.publish.call_count,
+                          "Expected one call to publish per artifact")
 
   def test_publish_remote(self):
     targets = self._prepare_for_publishing()
@@ -224,7 +235,6 @@ def test_publish_retry_eventually_fails(self):
     with self.assertRaises(Scm.RemoteException):
       task.execute()
 
-
   def test_publish_local_only(self):
     with pytest.raises(TaskError) as exc:
       self.prepare_task()