concretizer: enable hash reuse with full hash

With the original DAG hash, we did not store build dependencies in the database, but
with the full DAG hash, we do. Previously, we'd never tell the concretizer about build
dependencies of things used by hash, because we never had them. Now, we have to avoid
telling the concretizer about them, or they'll unnecessarily constrain build
dependencies for new concretizations.

- [x] Make database track all dependencies included in the `dag_hash`
- [x] Modify spec_clauses so that build dependency information is optional
      and off by default.
- [x] `spack diff` asks `spec_clauses` for build dependencies for completeness
- [x] Modify `concretize.lp` so that reuse optimization doesn't affect fresh
      installations.
- [x] Modify concretizer setup so that it does *not* prioritize installed versions
      over package versions. We don't need this with reuse, so they're low priority.
- [x] Fix `test_installed_deps` for full hash and new concretizer (does not work
      for old concretizer with full hash -- leave this for later if we need it)
- [x] Move `test_installed_deps` mock packages to `builtin.mock` for easier debugging
      with `spack -m`.
- [x] Fix `test_reuse_installed_packages_when_package_def_changes` for full hash

lib/spack/spack/cmd/diff.py

@@ -68,8 +68,14 @@ 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, expand_hashes=True))
+    # get facts for specs, making sure to include build dependencies of concrete
-    b_facts = set(t for t in setup.spec_clauses(b, body=True, expand_hashes=True))
+    # specs and to descend into dependency hashes so we include all facts.
+    a_facts = set(t for t in setup.spec_clauses(
+        a, body=True, expand_hashes=True, concrete_build_deps=True,
+    ))
+    b_facts = set(t for t in setup.spec_clauses(
+        b, body=True, expand_hashes=True, concrete_build_deps=True,
+    ))
 
     # We want to present them to the user as simple key: values
     intersect = sorted(a_facts.intersection(b_facts))

lib/spack/spack/database.py

@@ -91,7 +91,8 @@
 _pkg_lock_timeout = None
 
 # Types of dependencies tracked by the database
-_tracked_deps = ('link', 'run')
+# We store by DAG hash, so we track the dependencies that the DAG hash includes.
+_tracked_deps = ht.dag_hash.deptype
 
 # Default list of fields written for each install record
 default_install_record_fields = [
@@ -1601,10 +1602,11 @@ def unused_specs(self):
         needed, visited = set(), set()
         with self.read_transaction():
             for key, rec in self._data.items():
-                if rec.explicit:
+                if not rec.explicit:
-                    # recycle `visited` across calls to avoid
+                    continue
-                    # redundantly traversing
+
-                    for spec in rec.spec.traverse(visited=visited):
+                # recycle `visited` across calls to avoid redundantly traversing
+                for spec in rec.spec.traverse(visited=visited, deptype=("link", "run")):
                     needed.add(spec.dag_hash())
 
             unused = [rec.spec for key, rec in self._data.items()

lib/spack/spack/solver/asp.py

@@ -77,22 +77,25 @@ def getter(node):
 ast_type = ast_getter("ast_type", "type")
 ast_sym = ast_getter("symbol", "term")
 
+#: Order of precedence for version origins. Topmost types are preferred.
+version_origin_fields = [
+    'spec',
+    'external',
+    'packages_yaml',
+    'package_py',
+    'installed',
+]
+
+#: Look up version precedence strings by enum id
+version_origin_str = {
+    i: name for i, name in enumerate(version_origin_fields)
+}
 
 #: Enumeration like object to mark version provenance
 version_provenance = collections.namedtuple(  # type: ignore
     'VersionProvenance',
-    ['external', 'packages_yaml', 'package_py', 'spec', 'installed']
+    version_origin_fields,
-)(installed=0, spec=1, external=2, packages_yaml=3, package_py=4)
+)(**{name: i for i, name in enumerate(version_origin_fields)})
-
-#: String representation of version origins, to emit legible
-# facts for the ASP solver
-version_origin_str = {
-    0: 'installed',
-    1: 'spec',
-    2: 'external',
-    3: 'packages_yaml',
-    4: 'package_py'
-}
 
 #: Named tuple to contain information on declared versions
 DeclaredVersion = collections.namedtuple(
@@ -698,15 +701,12 @@ def __init__(self, reuse=False, tests=False):
     def pkg_version_rules(self, pkg):
         """Output declared versions of a package.
 
-        This uses self.possible_versions so that we include any versions
+        This uses self.declared_versions so that we include any versions
         that arise from a spec.
         """
         def key_fn(version):
-            # Origins are sorted by order of importance:
+            # Origins are sorted by precedence defined in `version_origin_str`,
-            # 1. Spec from command line
+            # then by order added.
-            # 2. Externals
-            # 3. Package preferences
-            # 4. Directives in package.py
             return version.origin, version.idx
 
         pkg = packagize(pkg)
@@ -1149,7 +1149,14 @@ def spec_clauses(self, *args, **kwargs):
             raise RuntimeError(msg)
         return clauses
 
-    def _spec_clauses(self, spec, body=False, transitive=True, expand_hashes=False):
+    def _spec_clauses(
+            self,
+            spec,
+            body=False,
+            transitive=True,
+            expand_hashes=False,
+            concrete_build_deps=False,
+    ):
         """Return a list of clauses for a spec mandates are true.
 
         Arguments:
@@ -1160,6 +1167,8 @@ def _spec_clauses(self, spec, body=False, transitive=True, expand_hashes=False):
                 dependencies (default True)
             expand_hashes (bool): if True, descend into hashes of concrete specs
                 (default False)
+            concrete_build_deps (bool): if False, do not include pure build deps
+                of concrete specs (as they have no effect on runtime constraints)
 
         Normally, if called with ``transitive=True``, ``spec_clauses()`` just generates
         hashes for the dependency requirements of concrete specs. If ``expand_hashes``
@@ -1267,18 +1276,34 @@ class Body(object):
 
         # add all clauses from dependencies
         if transitive:
-            if spec.concrete:
+            # TODO: Eventually distinguish 2 deps on the same pkg (build and link)
-                # TODO: We need to distinguish 2 specs from the same package later
+            for dspec in spec.edges_to_dependencies():
-                for edge in spec.edges_to_dependencies():
+                dep = dspec.spec
-                    for dtype in edge.deptypes:
-                        clauses.append(fn.depends_on(spec.name, edge.spec.name, dtype))
 
-            for dep in spec.traverse(root=False):
                 if spec.concrete:
+                    # We know dependencies are real for concrete specs. For abstract
+                    # specs they just mean the dep is somehow in the DAG.
+                    for dtype in dspec.deptypes:
+                        # skip build dependencies of already-installed specs
+                        if concrete_build_deps or dtype != "build":
+                            clauses.append(fn.depends_on(spec.name, dep.name, dtype))
+
+                    # imposing hash constraints for all but pure build deps of
+                    # already-installed concrete specs.
+                    if concrete_build_deps or dspec.deptypes != ("build",):
                         clauses.append(fn.hash(dep.name, dep.dag_hash()))
+
+                # if the spec is abstract, descend into dependencies.
+                # if it's concrete, then the hashes above take care of dependency
+                # constraints, but expand the hashes if asked for.
                 if not spec.concrete or expand_hashes:
                     clauses.extend(
-                        self._spec_clauses(dep, body, transitive=False)
+                        self._spec_clauses(
+                            dep,
+                            body=body,
+                            expand_hashes=expand_hashes,
+                            concrete_build_deps=concrete_build_deps,
+                        )
                     )
 
         return clauses
@@ -1812,12 +1837,14 @@ def setup(self, driver, specs):
                 fn.virtual_root(spec.name) if spec.virtual
                 else fn.root(spec.name)
             )
+
             for clause in self.spec_clauses(spec):
                 self.gen.fact(clause)
                 if clause.name == 'variant_set':
-                    self.gen.fact(fn.variant_default_value_from_cli(
+                    self.gen.fact(
-                        *clause.args
+                        fn.variant_default_value_from_cli(*clause.args)
-                    ))
+                    )
+
         self.gen.h1("Variant Values defined in specs")
         self.define_variant_values()
 
@@ -1840,6 +1867,7 @@ class SpecBuilder(object):
     ignored_attributes = ["opt_criterion"]
 
     def __init__(self, specs):
+        self._specs = {}
         self._result = None
         self._command_line_specs = specs
         self._flag_sources = collections.defaultdict(lambda: set())

lib/spack/spack/solver/concretize.lp

@@ -885,8 +885,27 @@ build(Package) :- not hash(Package, _), node(Package).
 %   200+        Shifted priorities for build nodes; correspond to priorities 0 - 99.
 %   100 - 199   Unshifted priorities. Currently only includes minimizing #builds.
 %   0   -  99   Priorities for non-built nodes.
-build_priority(Package, 200) :- build(Package), node(Package).
+build_priority(Package, 200) :- build(Package), node(Package), optimize_for_reuse().
-build_priority(Package, 0)   :- not build(Package), node(Package).
+build_priority(Package, 0)   :- not build(Package), node(Package), optimize_for_reuse().
+
+% don't adjust build priorities if reuse is not enabled
+build_priority(Package, 0)   :- node(Package), not optimize_for_reuse().
+
+% don't assign versions from installed packages unless reuse is enabled
+% NOTE: that "installed" means the declared version was only included because
+% that package happens to be installed, NOT because it was asked for on the
+% command line.  If the user specifies a hash, the origin will be "spec".
+%
+% TODO: There's a slight inconsistency with this: if the user concretizes
+% and installs `foo ^bar`, for some build dependency `bar`, and then later
+% does a `spack install --fresh foo ^bar/abcde` (i.e.,the hash of `bar`, it
+% currently *won't* force versions for `bar`'s build dependencies -- `--fresh`
+% will instead build the latest bar. When we actually include transitive
+% build deps in the solve, consider using them as a preference to resolve this.
+:- version(Package, Version),
+   version_weight(Package, Weight),
+   version_declared(Package, Version, Weight, "installed"),
+   not optimize_for_reuse().
 
 #defined installed_hash/2.
 

lib/spack/spack/spec.py

@@ -1599,7 +1599,15 @@ def traverse(self, **kwargs):
     def traverse_edges(self, visited=None, d=0, deptype='all',
                        dep_spec=None, **kwargs):
         """Generic traversal of the DAG represented by this spec.
-           This will yield each node in the spec.  Options:
+
+           This yields ``DependencySpec`` objects as they are traversed.
+
+           When traversing top-down, an imaginary incoming edge to the root
+           is yielded first as ``DependencySpec(None, root, ())``. When
+           traversing bottom-up, imaginary edges to leaves are yielded first
+           as ``DependencySpec(left, None, ())`` objects.
+
+           Options:
 
            order    [=pre|post]
                Order to traverse spec nodes. Defaults to preorder traversal.

lib/spack/spack/test/concretize.py

@@ -1292,8 +1292,10 @@ def test_reuse_installed_packages_when_package_def_changes(
         assert ht.build_hash(root) == ht.build_hash(new_root_with_reuse)
         assert ht.build_hash(root) != ht.build_hash(new_root_without_reuse)
 
-        # package hashes are different, so dag hashes will be different
+        # DAG hash should be the same with reuse since only the dependency changed
-        assert root.dag_hash() != new_root_with_reuse.dag_hash()
+        assert root.dag_hash() == new_root_with_reuse.dag_hash()
+
+        # Structure and package hash will be different without reuse
         assert root.dag_hash() != new_root_without_reuse.dag_hash()
 
     @pytest.mark.regression('20784')

lib/spack/spack/test/spec_dag.py

@@ -84,54 +84,58 @@ def test_test_deptype():
 
 
 @pytest.mark.usefixtures('config')
-def test_installed_deps(monkeypatch):
+def test_installed_deps(monkeypatch, mock_packages):
-    """Preinstall a package P with a constrained build dependency D, then
+    """Ensure that concrete specs and their build deps don't constrain solves.
-    concretize a dependent package which also depends on P and D, specifying
+
-    that the installed instance of P should be used. In this case, D should
+    Preinstall a package ``c`` that has a constrained build dependency on ``d``, then
-    not be constrained by P since P is already built.
+    install ``a`` and ensure that neither:
+
+      * ``c``'s package constraints, nor
+      * the concrete ``c``'s build dependencies
+
+    constrain ``a``'s dependency on ``d``.
+
     """
-    # FIXME: this requires to concretize build deps separately if we are
+    if spack.config.get('config:concretizer') == 'original':
-    # FIXME: using the clingo based concretizer
+        pytest.xfail('fails with the original concretizer and full hashes')
-    if spack.config.get('config:concretizer') == 'clingo':
-        pytest.xfail('requires separate concretization of build dependencies')
 
-    default = ('build', 'link')
+    # see installed-deps-[abcde] test packages.
-    build_only = ('build',)
+    #     a
+    #    / \
+    #   b   c   b --> d build/link
+    #   |\ /|   b --> e build/link
+    #   |/ \|   c --> d build
+    #   d   e   c --> e build/link
+    #
+    a, b, c, d, e = ["installed-deps-%s" % s for s in "abcde"]
 
-    mock_repo = MockPackageMultiRepo()
+    # install C, which will force d's version to be 2
-    e = mock_repo.add_package('e', [], [])
+    # BUT d is only a build dependency of C, so it won't constrain
-    d = mock_repo.add_package('d', [], [])
+    # link/run dependents of C when C is depended on as an existing
-    c_conditions = {
+    # (concrete) installation.
-        d.name: {
+    c_spec = Spec(c)
-            'c': 'd@2'
-        },
-        e.name: {
-            'c': 'e@2'
-        }
-    }
-    c = mock_repo.add_package('c', [d, e], [build_only, default],
-                              conditions=c_conditions)
-    b = mock_repo.add_package('b', [d, e], [default, default])
-    mock_repo.add_package('a', [b, c], [default, default])
-
-    with spack.repo.use_repositories(mock_repo):
-        c_spec = Spec('c')
     c_spec.concretize()
-        assert c_spec['d'].version == spack.version.Version('2')
+    assert c_spec[d].version == spack.version.Version('2')
 
-        c_installed = spack.spec.Spec.from_dict(c_spec.to_dict())
+    installed_names = [s.name for s in c_spec.traverse()]
-        installed_names = [s.name for s in c_installed.traverse()]
 
     def _mock_installed(self):
         return self.name in installed_names
 
     monkeypatch.setattr(Spec, 'installed', _mock_installed)
-        a_spec = Spec('a')
-        a_spec._add_dependency(c_installed, default)
-        a_spec.concretize()
 
-        assert a_spec['d'].version == spack.version.Version('3')
+    # install A, which depends on B, C, D, and E, and force A to
-        assert a_spec['e'].version == spack.version.Version('2')
+    # use the installed C.  It should *not* force A to use the installed D
+    # *if* we're doing a fresh installation.
+    a_spec = Spec(a)
+    a_spec._add_dependency(c_spec, ("build", "link"))
+    a_spec.concretize()
+    assert spack.version.Version('2') == a_spec[c][d].version
+    assert spack.version.Version('2') == a_spec[e].version
+    assert spack.version.Version('3') == a_spec[b][d].version
+    assert spack.version.Version('3') == a_spec[d].version
+
+    # TODO: with reuse, this will be different -- verify the reuse case
 
 
 @pytest.mark.usefixtures('config')

var/spack/repos/builtin.mock/packages/installed-deps-a/package.py

@@ -0,0 +1,26 @@
+# Copyright 2013-2022 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack import *
+
+
+class InstalledDepsA(Package):
+    """Used by test_installed_deps test case."""
+    #     a
+    #    / \
+    #   b   c   b --> d build/link
+    #   |\ /|   b --> e build/link
+    #   |/ \|   c --> d build
+    #   d   e   c --> e build/link
+
+    homepage = "http://www.example.com"
+    url      = "http://www.example.com/a-1.0.tar.gz"
+
+    version("1", "0123456789abcdef0123456789abcdef")
+    version("2", "abcdef0123456789abcdef0123456789")
+    version("3", "def0123456789abcdef0123456789abc")
+
+    depends_on("installed-deps-b", type=("build", "link"))
+    depends_on("installed-deps-c", type=("build", "link"))

var/spack/repos/builtin.mock/packages/installed-deps-b/package.py

@@ -0,0 +1,26 @@
+# Copyright 2013-2022 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack import *
+
+
+class InstalledDepsB(Package):
+    """Used by test_installed_deps test case."""
+    #     a
+    #    / \
+    #   b   c   b --> d build/link
+    #   |\ /|   b --> e build/link
+    #   |/ \|   c --> d build
+    #   d   e   c --> e build/link
+
+    homepage = "http://www.example.com"
+    url      = "http://www.example.com/b-1.0.tar.gz"
+
+    version("1", "0123456789abcdef0123456789abcdef")
+    version("2", "abcdef0123456789abcdef0123456789")
+    version("3", "def0123456789abcdef0123456789abc")
+
+    depends_on("installed-deps-d", type=("build", "link"))
+    depends_on("installed-deps-e", type=("build", "link"))

var/spack/repos/builtin.mock/packages/installed-deps-c/package.py

@@ -0,0 +1,26 @@
+# Copyright 2013-2022 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack import *
+
+
+class InstalledDepsC(Package):
+    """Used by test_installed_deps test case."""
+    #     a
+    #    / \
+    #   b   c   b --> d build/link
+    #   |\ /|   b --> e build/link
+    #   |/ \|   c --> d build
+    #   d   e   c --> e build/link
+
+    homepage = "http://www.example.com"
+    url      = "http://www.example.com/c-1.0.tar.gz"
+
+    version("1", "0123456789abcdef0123456789abcdef")
+    version("2", "abcdef0123456789abcdef0123456789")
+    version("3", "def0123456789abcdef0123456789abc")
+
+    depends_on("installed-deps-d@2", type="build")
+    depends_on("installed-deps-e@2", type=("build", "link"))

var/spack/repos/builtin.mock/packages/installed-deps-d/package.py

@@ -0,0 +1,23 @@
+# Copyright 2013-2022 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack import *
+
+
+class InstalledDepsD(Package):
+    """Used by test_installed_deps test case."""
+    #     a
+    #    / \
+    #   b   c   b --> d build/link
+    #   |\ /|   b --> e build/link
+    #   |/ \|   c --> d build
+    #   d   e   c --> e build/link
+
+    homepage = "http://www.example.com"
+    url      = "http://www.example.com/d-1.0.tar.gz"
+
+    version("1", "0123456789abcdef0123456789abcdef")
+    version("2", "abcdef0123456789abcdef0123456789")
+    version("3", "def0123456789abcdef0123456789abc")

var/spack/repos/builtin.mock/packages/installed-deps-e/package.py

@@ -0,0 +1,23 @@
+# Copyright 2013-2022 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack import *
+
+
+class InstalledDepsE(Package):
+    """Used by test_installed_deps test case."""
+    #     a
+    #    / \
+    #   b   c   b --> d build/link
+    #   |\ /|   b --> e build/link
+    #   |/ \|   c --> d build
+    #   d   e   c --> e build/link
+
+    homepage = "http://www.example.com"
+    url      = "http://www.example.com/e-1.0.tar.gz"
+
+    version("1", "0123456789abcdef0123456789abcdef")
+    version("2", "abcdef0123456789abcdef0123456789")
+    version("3", "def0123456789abcdef0123456789abc")