spack diff: more flexible tests, restore transitive diff with spec_clauses

In switching to hash facts for concrete specs, we lost the transitive facts
from dependencies. This was fine for solves, because they were implied by
the imposed constraints from every hash. However, for `spack diff`, we want
to see what the hashes mean, so we need another mode for `spec_clauses()` to
show that.

This adds a `expand_hashes` argument to `spec_clauses()` that allows us to
output *both* the hashes and their implications on dependencies. We use
this mode in `spack diff`.

3 files changed, 39 insertions, 12 deletions

diff --git a/lib/spack/spack/cmd/diff.py b/lib/spack/spack/cmd/diff.py
index 85b73b6545..8d7b99e834 100644
--- a/lib/spack/spack/cmd/diff.py
+++ b/lib/spack/spack/cmd/diff.py
@@ -68,8 +68,8 @@ def compare_specs(a, b, to_string=False, color=None):
     # Prepare a solver setup to parse differences
     setup = asp.SpackSolverSetup()
 
-    a_facts = set(t for t in setup.spec_clauses(a, body=True))
-    b_facts = set(t for t in setup.spec_clauses(b, body=True))
+    a_facts = set(t for t in setup.spec_clauses(a, body=True, expand_hashes=True))
+    b_facts = set(t for t in setup.spec_clauses(b, body=True, expand_hashes=True))
 
     # We want to present them to the user as simple key: values
     intersect = sorted(a_facts.intersection(b_facts))
diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 0c483d8dfc..1e604ed406 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -1081,7 +1081,7 @@ class SpackSolverSetup(object):
             raise RuntimeError(msg)
         return clauses
 
-    def _spec_clauses(self, spec, body=False, transitive=True):
+    def _spec_clauses(self, spec, body=False, transitive=True, expand_hashes=False):
         """Return a list of clauses for a spec mandates are true.
 
         Arguments:
@@ -1089,7 +1089,15 @@ class SpackSolverSetup(object):
             body (bool): if True, generate clauses to be used in rule bodies
                 (final values) instead of rule heads (setters).
             transitive (bool): if False, don't generate clauses from
-                 dependencies (default True)
+                dependencies (default True)
+            expand_hashes (bool): if True, descend into hashes of concrete specs
+                (default False)
+
+        Normally, if called with ``transitive=True``, ``spec_clauses()`` just generates
+        hashes for the dependency requirements of concrete specs. If ``expand_hashes``
+        is ``True``, we'll *also* output all the facts implied by transitive hashes,
+        which are redundant during a solve but useful outside of one (e.g.,
+        for spec ``diff``).
         """
         clauses = []
 
@@ -1200,7 +1208,7 @@ class SpackSolverSetup(object):
             for dep in spec.traverse(root=False):
                 if spec.concrete:
                     clauses.append(fn.hash(dep.name, dep.dag_hash()))
-                else:
+                if not spec.concrete or expand_hashes:
                     clauses.extend(
                         self._spec_clauses(dep, body, transitive=False)
                     )
diff --git a/lib/spack/spack/test/cmd/diff.py b/lib/spack/spack/test/cmd/diff.py
index 949e046b56..48435df2a6 100644
--- a/lib/spack/spack/test/cmd/diff.py
+++ b/lib/spack/spack/test/cmd/diff.py
@@ -61,12 +61,30 @@ def test_load_first(install_mockery, mock_fetch, mock_archive, mock_packages):
     output = diff_cmd('--json', 'mpileaks', 'mpileaks')
     result = sjson.load(output)
 
-    assert len(result['a_not_b']) == 0
-    assert len(result['b_not_a']) == 0
+    assert not result['a_not_b']
+    assert not result['b_not_a']
 
     assert 'mpileaks' in result['a_name']
     assert 'mpileaks' in result['b_name']
-    assert "intersect" in result and len(result['intersect']) > 50
+
+    # spot check attributes in the intersection to ensure they describe the spec
+    assert "intersect" in result
+    assert all(["node", dep] in result["intersect"] for dep in (
+        "mpileaks", "callpath", "dyninst", "libelf", "libdwarf", "mpich"
+    ))
+    assert all(
+        len([diff for diff in result["intersect"] if diff[0] == attr]) == 6
+        for attr in (
+            "version",
+            "node_target",
+            "node_platform",
+            "node_os",
+            "node_compiler",
+            "node_compiler_version",
+            "node",
+            "hash",
+        )
+    )
 
     # After we install another version, it should ask us to disambiguate
     install_cmd('mpileaks+debug')
@@ -87,7 +105,8 @@ def test_load_first(install_mockery, mock_fetch, mock_archive, mock_packages):
                       "mpileaks/{0}".format(no_debug_hash))
     result = sjson.load(output)
 
-    assert len(result['a_not_b']) == 1
-    assert len(result['b_not_a']) == 1
-    assert result['a_not_b'][0] == ['variant_value', 'mpileaks debug True']
-    assert result['b_not_a'][0] == ['variant_value', 'mpileaks debug False']
+    assert ['hash', 'mpileaks %s' % debug_hash] in result['a_not_b']
+    assert ['variant_value', 'mpileaks debug True'] in result['a_not_b']
+
+    assert ['hash', 'mpileaks %s' % no_debug_hash] in result['b_not_a']
+    assert ['variant_value', 'mpileaks debug False'] in result['b_not_a']