Repair depmap --graph

- Simplify depmap text and graph handlers
- Make depmap --graph work again
- Add tests for depmap --graph
- Eliminate extraneous/duplicated code
- Convert to generators for immediate output/memory efficiency on larger
  projects

Testing Done:
https://travis-ci.org/pantsbuild/pants/builds/66331926

+ manual testing in birdcage, reviewing graphviz generated graphs, etc

Bugs closed: 1648

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

src/python/pants/backend/project_info/tasks/depmap.py

@@ -36,10 +36,6 @@ class SourceRootTypes(object):
     RESOURCE = 'RESOURCE'  # Resource belonging to Source Target
     TEST_RESOURCE = 'TEST_RESOURCE'  # Resource belonging to Test Target
 
-  @staticmethod
-  def _is_jvm(dep):
-    return dep.is_jvm or isinstance(dep, JvmApp)
-
   @staticmethod
   def _jar_id(jar):
     if jar.rev:
@@ -65,15 +61,19 @@ def register_options(cls, register):
                   'output (only external jars).')
     register('--minimal', default=False, action='store_true',
              help='For a textual dependency tree, only prints a dependency the 1st '
-                  'time it is encountered.  For graph output this does nothing.')
+                  'time it is encountered. This is a no-op for --graph.')
     register('--graph', default=False, action='store_true',
              help='Specifies the internal dependency graph should be output in the dot digraph '
                   'format.')
+    register('--tree', default=False, action='store_true',
+             help='For text output, show an ascii tree to help visually line up indentions.')
+    register('--show-types', default=False, action='store_true',
+             help='Show types of objects in depmap --graph.')
     register('--separator', default='-',
              help='Specifies the separator to use between the org/name/rev components of a '
                   'dependency\'s fully qualified name.')
     register('--path-to',
-             help='Show only items on the path to the given target.')
+             help='Show only items on the path to the given target. This is a no-op for --graph.')
 
   def __init__(self, *args, **kwargs):
     super(Depmap, self).__init__(*args, **kwargs)
@@ -86,132 +86,125 @@ def __init__(self, *args, **kwargs):
 
     self.is_minimal = self.get_options().minimal
     self.is_graph = self.get_options().graph
+    self.should_tree = self.get_options().tree
+    self.show_types = self.get_options().show_types
     self.path_to = self.get_options().path_to
     self.separator = self.get_options().separator
     self.target_aliases_map = None
 
   def console_output(self, targets):
     if len(self.context.target_roots) == 0:
       raise TaskError("One or more target addresses are required.")
+
     for target in self.context.target_roots:
-      if self.is_graph:
-        for line in self._output_digraph(target):
-          yield line
-      else:
-        for line in self._output_dependency_tree(target):
-          yield line
+      out = self._output_digraph(target) if self.is_graph else self._output_dependency_tree(target)
+      for line in out:
+        yield line
 
   def _dep_id(self, dependency):
-    """Returns a tuple of dependency_id , is_internal_dep."""
-
+    """Returns a tuple of dependency_id, is_internal_dep."""
     params = dict(sep=self.separator)
+
     if isinstance(dependency, JarDependency):
+      # TODO(kwilson): handle 'classifier' and 'type'.
       params.update(org=dependency.org, name=dependency.name, rev=dependency.rev)
+      is_internal_dep = False
     else:
       params.update(org='internal', name=dependency.id)
+      is_internal_dep = True
 
-    if params.get('rev'):
-      return "{org}{sep}{name}{sep}{rev}".format(**params), False
-    else:
-      return "{org}{sep}{name}".format(**params), True
+    return ('{org}{sep}{name}{sep}{rev}' if params.get('rev') else
+            '{org}{sep}{name}').format(**params), is_internal_dep
+
+  def _enumerate_visible_deps(self, dep, predicate):
+    dep_id, internal = self._dep_id(dep)
+
+    dependencies = sorted([x for x in getattr(dep, 'dependencies', [])]) + sorted(
+      [x for x in getattr(dep, 'jar_dependencies', [])] if not self.is_internal_only else [])
+
+    for inner_dep in dependencies:
+      dep_id, internal = self._dep_id(inner_dep)
+      if predicate(internal):
+        yield inner_dep
+
+  def output_candidate(self, internal):
+    return ((not self.is_internal_only and not self.is_external_only)
+            or (self.is_internal_only and internal)
+            or (self.is_external_only and not internal))
 
   def _output_dependency_tree(self, target):
-    def output_dep(dep, indent):
-      return "{}{}".format(indent * "  ", dep)
-
-    def check_path_to(jar_dep_id):
-      """
-      Check that jar_dep_id is the dep we are looking for with path_to
-      (or that path_to is not enabled)
-      """
-      return jar_dep_id == self.path_to or not self.path_to
-
-    def output_deps(dep, indent=0, outputted=set()):
-      dep_id, _ = self._dep_id(dep)
-      if dep_id in outputted:
-        return [output_dep("*{}".format(dep_id), indent)] if not self.is_minimal else []
+    """Plain-text depmap output handler."""
+
+    def make_line(dep, indent, is_dupe=False):
+      indent_join, indent_chars = ('--', '  |') if self.should_tree else ('', '  ')
+      dupe_char = '*' if is_dupe else ''
+      return ''.join((indent * indent_chars, indent_join, dupe_char, dep))
+
+    def output_deps(dep, indent, outputted, stack):
+      dep_id, internal = self._dep_id(dep)
+
+      if self.path_to:
+        # If we hit the search target from self.path_to, yield the stack items and bail.
+        if dep_id == self.path_to:
+          for dep_id, indent in stack + [(dep_id, indent)]:
+            yield make_line(dep_id, indent)
+          return
       else:
-        output = []
-        if not self.is_external_only:
-          indent += 1
-
-        jar_output = []
-        if not self.is_internal_only:
-          if self._is_jvm(dep):
-            for jar_dep in dep.jar_dependencies:
-              jar_dep_id, internal = self._dep_id(jar_dep)
-              if not internal:
-                if jar_dep_id not in outputted or (not self.is_minimal
-                                                   and not self.is_external_only):
-                  if check_path_to(jar_dep_id):
-                    jar_output.append(output_dep(jar_dep_id, indent))
-                  outputted.add(jar_dep_id)
-
-        dep_output = []
-        for internal_dep in dep.dependencies:
-          dep_output.extend(output_deps(internal_dep, indent, outputted))
-
-        if not check_path_to(dep_id) and not (jar_output or dep_output):
-          return []
-
-        if not self.is_external_only:
-          output.append(output_dep(dep_id, indent - 1))
+        if not (dep_id in outputted and self.is_minimal) and self.output_candidate(internal):
+          yield make_line(dep_id,
+                          0 if self.is_external_only else indent,
+                          is_dupe=dep_id in outputted)
           outputted.add(dep_id)
 
-        output.extend(dep_output)
-        output.extend(jar_output)
-        return output
-    return output_deps(target)
+      for sub_dep in self._enumerate_visible_deps(dep, self.output_candidate):
+        for item in output_deps(sub_dep, indent + 1, outputted, stack + [(dep_id, indent)]):
+          yield item
+
+    for item in output_deps(target, 0, set(), []):
+      yield item
 
   def _output_digraph(self, target):
+    """Graphviz format depmap output handler."""
     color_by_type = {}
 
-    def output_candidate(internal):
-      return ((self.is_internal_only and internal)
-               or (self.is_external_only and not internal)
-               or (not self.is_internal_only and not self.is_external_only))
+    def maybe_add_type(dep, dep_id):
+      """Add a class type to a dependency id if --show-types is passed."""
+      return dep_id if not self.show_types else '\\n'.join((dep_id, dep.__class__.__name__))
+
+    def make_node(dep, dep_id, internal):
+      line_fmt = '  "{id}" [style=filled, fillcolor={color}{internal}];'
+      int_shape = ', shape=ellipse' if not internal else ''
 
-    def output_dep(dep):
+      dep_class = dep.__class__.__name__
+      if dep_class not in color_by_type:
+        color_by_type[dep_class] = len(color_by_type.keys()) + 1
+
+      return line_fmt.format(id=dep_id, internal=int_shape, color=color_by_type[dep_class])
+
+    def make_edge(from_dep_id, to_dep_id, internal):
+      style = ' [style=dashed]' if not internal else ''
+      return '  "{}" -> "{}"{};'.format(from_dep_id, to_dep_id, style)
+
+    def output_deps(dep, parent, parent_id, outputted):
       dep_id, internal = self._dep_id(dep)
-      if internal:
-        fmt = '  "{id}" [style=filled, fillcolor="{color}"];'
-      else:
-        fmt = '  "{id}" [style=filled, fillcolor="{color}", shape=ellipse];'
-      if type(dep) not in color_by_type:
-        color_by_type[type(dep)] = len(color_by_type.keys()) + 1
-      return fmt.format(id=dep_id, color=color_by_type[type(dep)])
-
-    def output_deps(outputted, dep, parent=None):
-      output = []
-
-      if dep not in outputted:
-        outputted.add(dep)
-        output.append(output_dep(dep))
-        if parent:
-          output.append('  "{}" -> "{}";'.format(self._dep_id(parent)[0], self._dep_id(dep)[0]))
-
-        # TODO: This is broken. 'dependency' doesn't exist here, and we don't have
-        # internal_dependencies any more anyway.
-        if self._is_jvm(dependency):
-          for internal_dependency in dependency.internal_dependencies:
-            output += output_deps(outputted, internal_dependency, dependency)
-
-        for jar in (dependency.jar_dependencies if self._is_jvm(dependency) else [dependency]):
-          jar_id, internal = self._dep_id(jar)
-          if output_candidate(internal):
-            if jar not in outputted:
-              output += [output_dep(jar)]
-              outputted.add(jar)
-
-            target_id, _ = self._dep_id(target)
-            dep_id, _ = self._dep_id(dependency)
-            left_id = target_id if self.is_external_only else dep_id
-            if (left_id, jar_id) not in outputted:
-              styled = internal and not self.is_internal_only
-              output += ['  "{}" -> "{}"{};'.format(left_id, jar_id,
-                                                    ' [style="dashed"]' if styled else '')]
-              outputted.add((left_id, jar_id))
-      return output
-    header = ['digraph "{}" {{'.format(target.id)]
-    graph_attr = ['  node [shape=rectangle, colorscheme=set312;];', '  rankdir=LR;']
-    return header + graph_attr + output_deps(set(), target) + ['}']
+
+      if dep_id not in outputted:
+        yield make_node(dep, maybe_add_type(dep, dep_id), internal)
+        outputted.add(dep_id)
+
+        for sub_dep in self._enumerate_visible_deps(dep, self.output_candidate):
+          for item in output_deps(sub_dep, dep, dep_id, outputted):
+            yield item
+
+      if parent:
+        edge_id = (parent_id, dep_id)
+        if edge_id not in outputted:
+          yield make_edge(maybe_add_type(parent, parent_id), maybe_add_type(dep, dep_id), internal)
+          outputted.add(edge_id)
+
+    yield 'digraph "{}" {{'.format(target.id)
+    yield '  node [shape=rectangle, colorscheme=set312;];'
+    yield '  rankdir=LR;'
+    for line in output_deps(target, parent=None, parent_id=None, outputted=set()):
+      yield line
+    yield '}'

tests/python/pants_test/backend/project_info/tasks/BUILD

@@ -31,6 +31,7 @@ python_tests(
 python_tests(
   name = 'depmap',
   sources = ['test_depmap.py'],
+  coverage = ['pants.backend.project_info.tasks.depmap'],
   dependencies = [
     'src/python/pants/backend/core:plugin',
     'src/python/pants/backend/core/targets:common',

tests/python/pants_test/backend/project_info/tasks/test_depmap.py

@@ -138,6 +138,16 @@ def create_jvm_app(path, name, type, binary, deps=()):
       )
     """))
 
+    self.add_to_build_file('src/java/c', dedent('''
+      jar_library(
+        name='c_jar_lib',
+        jars=[
+          jar(org='org.pantsbuild.test', name='c_test', rev='1.0'),
+          jar(org='org.pantsbuild.test', name='d_test', rev=''),
+        ]
+      )
+    '''))
+
     # It makes no sense whatsoever to have a java_library that depends
     # on a Python library, but we want to ensure that depmap handles
     # cases like this anyway because there might be other cases which
@@ -293,3 +303,50 @@ def test_intermediate_dep(self):
       '    internal-src.java.b.b_lib',
       targets=[self.target('src/java/b:b_java')]
     )
+
+  def test_graph(self):
+    self.assert_console_output_ordered(
+      'digraph "common.h.h" {',
+      '  node [shape=rectangle, colorscheme=set312;];',
+      '  rankdir=LR;',
+      '  "internal-common.h.h" [style=filled, fillcolor=1];',
+      '  "internal-common.f.f" [style=filled, fillcolor=2];',
+      '  "internal-common.h.h" -> "internal-common.f.f";',
+      '}',
+      targets=[self.target('common/h')],
+      options={'graph': True}
+    )
+
+  def test_graph_show_types(self):
+    self.assert_console_output_ordered(
+      'digraph "common.h.h" {',
+      '  node [shape=rectangle, colorscheme=set312;];',
+      '  rankdir=LR;',
+      '  "internal-common.h.h\\nJvmApp" [style=filled, fillcolor=1];',
+      '  "internal-common.f.f\\nJvmBinary" [style=filled, fillcolor=2];',
+      '  "internal-common.h.h\\nJvmApp" -> "internal-common.f.f\\nJvmBinary";',
+      '}',
+      targets=[self.target('common/h')],
+      options={'graph': True, 'show_types': True}
+    )
+
+  def test_tree(self):
+    self.assert_console_output_ordered(
+      '--internal-overlaps.two',
+      '  |--internal-overlaps.one',
+      '  |  |--internal-common.h.h',
+      '  |  |  |--internal-common.f.f',
+      '  |  |--internal-common.i.i',
+      '  |  |  |--internal-common.g.g',
+      '  |  |  |  |--*internal-common.f.f',
+      targets=[self.target('overlaps:two')],
+      options={'tree': True}
+    )
+
+  def test_jar_library_external(self):
+    self.assert_console_output_ordered(
+      'org.pantsbuild.test-c_test-1.0',
+      'org.pantsbuild.test-d_test',
+      targets=[self.target('src/java/c:c_jar_lib')],
+      options={'external_only': True}
+    )