From 49ed41b028596d2314c219d174e533f8142ad6e4 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Wed, 3 Nov 2021 14:02:06 -0700 Subject: [PATCH] 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`. --- lib/spack/spack/cmd/diff.py | 4 ++-- lib/spack/spack/solver/asp.py | 14 +++++++++++--- lib/spack/spack/test/cmd/diff.py | 33 +++++++++++++++++++++++++------- 3 files changed, 39 insertions(+), 12 deletions(-) diff --git a/lib/spack/spack/cmd/diff.py b/lib/spack/spack/cmd/diff.py index 85b73b6545c..8d7b99e8347 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 0c483d8dfc6..1e604ed406f 100644 --- a/lib/spack/spack/solver/asp.py +++ b/lib/spack/spack/solver/asp.py @@ -1081,7 +1081,7 @@ def spec_clauses(self, *args, **kwargs): 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 @@ def _spec_clauses(self, spec, body=False, transitive=True): 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 Body(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 949e046b563..48435df2a62 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']