graph: refactor static graphs

- `spack graph --static` (and `spack.graph.dot_graph`) now do the "right
  thing" and print the possible dependency graph of provided packages.

- `spack graph --static` no longer concretizes specs, as it only relies
  on class level metadata

- Previously the behavior was not consistent -- `spack graph --static`
  would graph possible dependencies of concrete specs, but would only
  include some of them.  The new code properly pursues all possible
  dependencies, and allows traversing by different dependency types.

4 files changed, 73 insertions, 100 deletions

diff --git a/lib/spack/spack/cmd/graph.py b/lib/spack/spack/cmd/graph.py
index b7741c9ab2..c07b009286 100644
--- a/lib/spack/spack/cmd/graph.py
+++ b/lib/spack/spack/cmd/graph.py
@@ -9,9 +9,9 @@ import argparse
 import llnl.util.tty as tty
 
 import spack.cmd
+import spack.cmd.common.arguments as arguments
 import spack.config
 import spack.store
-from spack.dependency import all_deptypes, canonical_deptype
 from spack.graph import graph_dot, graph_ascii
 
 description = "generate graphs of package dependency relationships"
@@ -31,21 +31,14 @@ def setup_parser(subparser):
         help="generate graph in dot format and print to stdout")
 
     subparser.add_argument(
-        '-n', '--normalize', action='store_true',
-        help="skip concretization; only print normalized spec")
-
-    subparser.add_argument(
         '-s', '--static', action='store_true',
-        help="use static information from packages, not dynamic spec info")
+        help="graph static (possible) deps, don't concretize (implies --dot)")
 
     subparser.add_argument(
         '-i', '--installed', action='store_true',
         help="graph all installed specs in dot format (implies --dot)")
 
-    subparser.add_argument(
-        '-t', '--deptype', action='store',
-        help="comma-separated list of deptypes to traverse. default=%s"
-        % ','.join(all_deptypes))
+    arguments.add_common_arguments(subparser, ['deptype'])
 
     subparser.add_argument(
         'specs', nargs=argparse.REMAINDER,
@@ -53,7 +46,6 @@ def setup_parser(subparser):
 
 
 def graph(parser, args):
-    concretize = not args.normalize
     if args.installed:
         if args.specs:
             tty.die("Can't specify specs with --installed")
@@ -61,26 +53,21 @@ def graph(parser, args):
         specs = spack.store.db.query()
 
     else:
-        specs = spack.cmd.parse_specs(
-            args.specs, normalize=True, concretize=concretize)
+        specs = spack.cmd.parse_specs(args.specs, concretize=not args.static)
 
     if not specs:
         setup_parser.parser.print_help()
         return 1
 
-    deptype = all_deptypes
-    if args.deptype:
-        deptype = tuple(args.deptype.split(','))
-        if deptype == ('all',):
-            deptype = 'all'
-        deptype = canonical_deptype(deptype)
+    if args.static:
+        args.dot = True
 
-    if args.dot:  # Dot graph only if asked for.
-        graph_dot(specs, static=args.static, deptype=deptype)
+    if args.dot:
+        graph_dot(specs, static=args.static, deptype=args.deptype)
 
     elif specs:  # ascii is default: user doesn't need to provide it explicitly
         debug = spack.config.get('config:debug')
-        graph_ascii(specs[0], debug=debug, deptype=deptype)
+        graph_ascii(specs[0], debug=debug, deptype=args.deptype)
         for spec in specs[1:]:
             print()  # extra line bt/w independent graphs
             graph_ascii(spec, debug=debug)
diff --git a/lib/spack/spack/graph.py b/lib/spack/spack/graph.py
index 8a91ff2fd0..9a1571d75c 100644
--- a/lib/spack/spack/graph.py
+++ b/lib/spack/spack/graph.py
@@ -43,13 +43,10 @@ can take a number of specs as input.
 
 """
 import sys
-
 from heapq import heapify, heappop, heappush
-from six import iteritems
 
 from llnl.util.tty.color import ColorStream
 
-from spack.spec import Spec
 from spack.dependency import all_deptypes, canonical_deptype
 
 
@@ -499,76 +496,67 @@ def graph_dot(specs, deptype='all', static=False, out=None):
         spack graph --dot qt | dot -Tpdf > spack-graph.pdf
 
     """
+    if not specs:
+        raise ValueError("Must provide specs to graph_dot")
+
     if out is None:
         out = sys.stdout
     deptype = canonical_deptype(deptype)
 
+    def static_graph(spec, deptype):
+        pkg = spec.package
+        possible = pkg.possible_dependencies(
+            expand_virtuals=True, deptype=deptype)
+
+        nodes = set()  # elements are (node name, node label)
+        edges = set()  # elements are (src key, dest key)
+        for name, deps in possible.items():
+            nodes.add((name, name))
+            edges.update((name, d) for d in deps)
+        return nodes, edges
+
+    def dynamic_graph(spec, deptypes):
+        nodes = set()  # elements are (node key, node label)
+        edges = set()  # elements are (src key, dest key)
+        for s in spec.traverse(deptype=deptype):
+            nodes.add((s.dag_hash(), s.name))
+            for d in s.dependencies(deptype=deptype):
+                edge = (s.dag_hash(), d.dag_hash())
+                edges.add(edge)
+        return nodes, edges
+
+    nodes = set()
+    edges = set()
+    for spec in specs:
+        if static:
+            n, e = static_graph(spec, deptype)
+        else:
+            n, e = dynamic_graph(spec, deptype)
+        nodes.update(n)
+        edges.update(e)
+
     out.write('digraph G {\n')
     out.write('  labelloc = "b"\n')
     out.write('  rankdir = "TB"\n')
-    out.write('  ranksep = "5"\n')
-    out.write('node[\n')
+    out.write('  ranksep = "1"\n')
+    out.write('  edge[\n')
+    out.write('     penwidth=4')
+    out.write('  ]\n')
+    out.write('  node[\n')
     out.write('     fontname=Monaco,\n')
-    out.write('     penwidth=2,\n')
-    out.write('     fontsize=12,\n')
-    out.write('     margin=.1,\n')
+    out.write('     penwidth=4,\n')
+    out.write('     fontsize=24,\n')
+    out.write('     margin=.2,\n')
     out.write('     shape=box,\n')
     out.write('     fillcolor=lightblue,\n')
-    out.write('     style="rounded,filled"]\n')
+    out.write('     style="rounded,filled"')
+    out.write('  ]\n')
 
     out.write('\n')
-
-    def q(string):
-        return '"%s"' % string
-
-    if not specs:
-        raise ValueError("Must provide specs ot graph_dot")
-
-    # Static graph includes anything a package COULD depend on.
-    if static:
-        names = set.union(*[
-            s.package.possible_dependencies(expand_virtuals=False)
-            for s in specs])
-        specs = [Spec(name) for name in names]
-
-    labeled = set()
-
-    def label(key, label):
-        if key not in labeled:
-            out.write('  "%s" [label="%s"]\n' % (key, label))
-            labeled.add(key)
-
-    deps = set()
-    for spec in specs:
-        if static:
-            out.write('  "%s" [label="%s"]\n' % (spec.name, spec.name))
-
-            # Skip virtual specs (we'll find out about them from concrete ones.
-            if spec.virtual:
-                continue
-
-            # Add edges for each depends_on in the package.
-            for dep_name, dep in iteritems(spec.package.dependencies):
-                deps.add((spec.name, dep_name))
-
-            # If the package provides something, add an edge for that.
-            for provider in set(s.name for s in spec.package.provided):
-                deps.add((provider, spec.name))
-
-        else:
-            def key_label(s):
-                return s.dag_hash(), "%s/%s" % (s.name, s.dag_hash(7))
-
-            for s in spec.traverse(deptype=deptype):
-                skey, slabel = key_label(s)
-                out.write('  "%s" [label="%s"]\n' % (skey, slabel))
-
-                for d in s.dependencies(deptype=deptype):
-                    dkey, _ = key_label(d)
-                    deps.add((skey, dkey))
+    for key, label in nodes:
+        out.write('  "%s" [label="%s"]\n' % (key, label))
 
     out.write('\n')
-
-    for pair in deps:
-        out.write('  "%s" -> "%s"\n' % pair)
+    for src, dest in edges:
+        out.write('  "%s" -> "%s"\n' % (src, dest))
     out.write('}\n')
diff --git a/lib/spack/spack/test/cmd/graph.py b/lib/spack/spack/test/cmd/graph.py
index c53cca9bd5..0ddbc4f149 100644
--- a/lib/spack/spack/test/cmd/graph.py
+++ b/lib/spack/spack/test/cmd/graph.py
@@ -27,13 +27,6 @@ def test_graph_dot():
 
 @pytest.mark.db
 @pytest.mark.usefixtures('mock_packages', 'database')
-def test_graph_normalize():
-    """Tests spack graph --normalize"""
-    graph('--normalize', 'dt-diamond')
-
-
-@pytest.mark.db
-@pytest.mark.usefixtures('mock_packages', 'database')
 def test_graph_static():
     """Tests spack graph --static"""
     graph('--static', 'dt-diamond')
diff --git a/lib/spack/spack/test/graph.py b/lib/spack/spack/test/graph.py
index a29779e3ca..57e7d54933 100644
--- a/lib/spack/spack/test/graph.py
+++ b/lib/spack/spack/test/graph.py
@@ -5,6 +5,7 @@
 
 from six import StringIO
 
+import spack.repo
 from spack.spec import Spec
 from spack.graph import AsciiGraph, topological_sort, graph_dot
 
@@ -47,34 +48,38 @@ def test_static_graph_mpileaks(mock_packages):
     assert '  "libelf" [label="libelf"]\n'     in dot
     assert '  "libdwarf" [label="libdwarf"]\n' in dot
 
+    mpi_providers = spack.repo.path.providers_for('mpi')
+    for spec in mpi_providers:
+        assert ('"mpileaks" -> "%s"' % spec.name) in dot
+        assert ('"callpath" -> "%s"' % spec.name) in dot
+
     assert '  "dyninst" -> "libdwarf"\n'  in dot
     assert '  "callpath" -> "dyninst"\n'  in dot
-    assert '  "mpileaks" -> "mpi"\n'      in dot
     assert '  "libdwarf" -> "libelf"\n'   in dot
-    assert '  "callpath" -> "mpi"\n'      in dot
     assert '  "mpileaks" -> "callpath"\n' in dot
     assert '  "dyninst" -> "libelf"\n'    in dot
 
 
-def test_dynamic_dot_graph_mpileaks(mock_packages):
+def test_dynamic_dot_graph_mpileaks(mock_packages, config):
     """Test dynamically graphing the mpileaks package."""
-    s = Spec('mpileaks').normalized()
+    s = Spec('mpileaks').concretized()
 
     stream = StringIO()
     graph_dot([s], static=False, out=stream)
 
     dot = stream.getvalue()
+    print(dot)
 
-    mpileaks_hash, mpileaks_lbl = s.dag_hash(), s.format('{name}{/hash:7}')
-    mpi_hash, mpi_lbl = s['mpi'].dag_hash(), s['mpi'].format('{name}{/hash:7}')
+    mpileaks_hash, mpileaks_lbl = s.dag_hash(), s.format('{name}')
+    mpi_hash, mpi_lbl = s['mpi'].dag_hash(), s['mpi'].format('{name}')
     callpath_hash, callpath_lbl = (
-        s['callpath'].dag_hash(), s['callpath'].format('{name}{/hash:7}'))
+        s['callpath'].dag_hash(), s['callpath'].format('{name}'))
     dyninst_hash, dyninst_lbl = (
-        s['dyninst'].dag_hash(), s['dyninst'].format('{name}{/hash:7}'))
+        s['dyninst'].dag_hash(), s['dyninst'].format('{name}'))
     libdwarf_hash, libdwarf_lbl = (
-        s['libdwarf'].dag_hash(), s['libdwarf'].format('{name}{/hash:7}'))
+        s['libdwarf'].dag_hash(), s['libdwarf'].format('{name}'))
     libelf_hash, libelf_lbl = (
-        s['libelf'].dag_hash(), s['libelf'].format('{name}{/hash:7}'))
+        s['libelf'].dag_hash(), s['libelf'].format('{name}'))
 
     assert '  "%s" [label="%s"]\n' % (mpileaks_hash, mpileaks_lbl) in dot
     assert '  "%s" [label="%s"]\n' % (callpath_hash, callpath_lbl) in dot