environment: key by dag_hash instead of build_hash

5 files changed, 68 insertions, 45 deletions

diff --git a/lib/spack/spack/ci.py b/lib/spack/spack/ci.py
index 2ad8fd5924..9419869232 100644
--- a/lib/spack/spack/ci.py
+++ b/lib/spack/spack/ci.py
@@ -202,7 +202,7 @@ def format_root_spec(spec, main_phase, strip_compiler):
         return '{0}@{1} arch={2}'.format(
             spec.name, spec.version, spec.architecture)
     else:
-        return spec.build_hash()
+        return spec.dag_hash()
 
 
 def spec_deps_key(s):
diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
index 5ef53f5b72..7359bdd591 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -94,7 +94,7 @@ spack:
 valid_environment_name_re = r'^\w[\w-]*$'
 
 #: version of the lockfile format. Must increase monotonically.
-lockfile_format_version = 3
+lockfile_format_version = 4
 
 # Magic names
 # The name of the standalone spec list in the manifest yaml
@@ -439,7 +439,7 @@ class ViewDescriptor(object):
     def content_hash(self, specs):
         d = syaml.syaml_dict([
             ('descriptor', self.to_dict()),
-            ('specs', [(spec.full_hash(), spec.prefix) for spec in sorted(specs)])
+            ('specs', [(spec.dag_hash(), spec.prefix) for spec in sorted(specs)])
         ])
         contents = sjson.dump(d)
         return spack.util.hash.b32_hash(contents)
@@ -1010,14 +1010,9 @@ class Environment(object):
 
         if not matches:
             # concrete specs match against concrete specs in the env
-            # by *dag hash*, not build hash.
-            dag_hashes_in_order = [
-                self.specs_by_hash[build_hash].dag_hash()
-                for build_hash in self.concretized_order
-            ]
-
+            # by dag hash.
             specs_hashes = zip(
-                self.concretized_user_specs, dag_hashes_in_order
+                self.concretized_user_specs, self.concretized_order
             )
 
             matches = [
@@ -1331,7 +1326,7 @@ class Environment(object):
             spec = next(
                 s for s in self.user_specs if s.satisfies(user_spec)
             )
-            concrete = self.specs_by_hash.get(spec.build_hash())
+            concrete = self.specs_by_hash.get(spec.dag_hash())
             if not concrete:
                 concrete = spec.concretized(tests=tests)
                 self._add_concrete_spec(spec, concrete)
@@ -1499,7 +1494,7 @@ class Environment(object):
         # update internal lists of specs
         self.concretized_user_specs.append(spec)
 
-        h = concrete.build_hash()
+        h = concrete.dag_hash()
         self.concretized_order.append(h)
         self.specs_by_hash[h] = concrete
 
@@ -1621,9 +1616,7 @@ class Environment(object):
         return sorted(all_specs)
 
     def all_hashes(self):
-        """Return hashes of all specs.
-
-        Note these hashes exclude build dependencies."""
+        """Return hashes of all specs."""
         return list(set(s.dag_hash() for s in self.all_specs()))
 
     def roots(self):
@@ -1695,11 +1688,10 @@ class Environment(object):
         for user_spec, concretized_user_spec in self.concretized_specs():
             # Deal with concrete specs differently
             if spec.concrete:
-                # Matching a concrete spec is more restrictive
-                # than just matching the dag hash
+                # TODO: do we still need the extra check comparing dag hashes?
                 is_match = (
                     spec in concretized_user_spec and
-                    concretized_user_spec[spec.name].build_hash() == spec.build_hash()
+                    concretized_user_spec[spec.name].dag_hash() == spec.dag_hash()
                 )
                 if is_match:
                     matches[spec] = spec
@@ -1781,12 +1773,12 @@ class Environment(object):
         concrete_specs = {}
         for spec in self.specs_by_hash.values():
             for s in spec.traverse():
-                build_hash = s.build_hash()
-                if build_hash not in concrete_specs:
-                    spec_dict = s.to_node_dict(hash=ht.build_hash)
+                dag_hash = s.dag_hash()
+                if dag_hash not in concrete_specs:
+                    spec_dict = s.node_dict_with_hashes(hash=ht.dag_hash)
                     # Assumes no legacy formats, since this was just created.
                     spec_dict[ht.dag_hash.name] = s.dag_hash()
-                    concrete_specs[build_hash] = spec_dict
+                    concrete_specs[dag_hash] = spec_dict
 
         hash_spec_list = zip(
             self.concretized_order, self.concretized_user_specs)
@@ -1828,33 +1820,53 @@ class Environment(object):
         root_hashes = set(self.concretized_order)
 
         specs_by_hash = {}
-        for build_hash, node_dict in json_specs_by_hash.items():
+        for dag_hash, node_dict in json_specs_by_hash.items():
             spec = Spec.from_node_dict(node_dict)
             if d['_meta']['lockfile-version'] > 1:
                 # Build hash is stored as a key, but not as part of the node dict
                 # To ensure build hashes are not recomputed, we reattach here
-                setattr(spec, ht.build_hash.attr, build_hash)
-            specs_by_hash[build_hash] = spec
+                setattr(spec, ht.dag_hash.attr, dag_hash)
+            specs_by_hash[dag_hash] = spec
 
-        for build_hash, node_dict in json_specs_by_hash.items():
+        for dag_hash, node_dict in json_specs_by_hash.items():
             for _, dep_hash, deptypes, _ in (
                     Spec.dependencies_from_node_dict(node_dict)):
-                specs_by_hash[build_hash]._add_dependency(
+                specs_by_hash[dag_hash]._add_dependency(
                     specs_by_hash[dep_hash], deptypes)
 
-        # If we are reading an older lockfile format (which uses dag hashes
-        # that exclude build deps), we use this to convert the old
-        # concretized_order to the full hashes (preserving the order)
+        # Current lockfile key: dag_hash() (dag_hash() == full_hash())
+        # Previous lockfile keys from most recent to least:
+        #   1. build_hash
+        #   2. dag_hash (computed *without* build deps)
+
+        # If we are reading an older lockfile format, the key may have been computed
+        # using a different hash type than the one spack uses currently (which
+        # includes build deps as well as the package hash).  If this is the case
+        # the following code updates the keys in in 'concretized_order' to be computed
+        # using the hash type spack currently uses, while maintaining the order of the
+        # list.
         old_hash_to_new = {}
         self.specs_by_hash = {}
         for _, spec in specs_by_hash.items():
-            dag_hash = spec.dag_hash()
+            # - to get old dag_hash() (w/out build deps) use runtime_hash() now
+            # - dag_hash() now includes build deps and package hash
+            #     - i.e. dag_hash() == full_hash()
+            # - regardless of what hash type keyed the lockfile we're reading,
+            #   the dag_hash we read from the file may appear appear in install
+            #   trees and binary mirrors, and as such, must be considered the
+            #   permanent id of the spec.
+            dag_hash = spec.dag_hash()          # == full_hash()
             build_hash = spec.build_hash()
-            if dag_hash in root_hashes:
-                old_hash_to_new[dag_hash] = build_hash
+            runtime_hash = spec.runtime_hash()  # == old dag_hash()
+
+            if runtime_hash in root_hashes:
+                old_hash_to_new[runtime_hash] = dag_hash
+            elif build_hash in root_hashes:
+                old_hash_to_new[build_hash] = dag_hash
 
-            if (dag_hash in root_hashes or build_hash in root_hashes):
-                self.specs_by_hash[build_hash] = spec
+            if (runtime_hash in root_hashes or
+                    build_hash in root_hashes or dag_hash in root_hashes):
+                self.specs_by_hash[dag_hash] = spec
 
         if old_hash_to_new:
             # Replace any older hashes in concretized_order with hashes
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index ef75eaff2c..46ee04ae90 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -2103,6 +2103,8 @@ class Spec(object):
             node[hash.name] = self.build_hash()
         elif hash.name == 'process_hash':
             node[hash.name] = self.process_hash()
+        elif hash.name == 'runtime_hash':
+            node[hash.name] = self.runtime_hash()
 
         return node
 
@@ -2244,11 +2246,17 @@ class Spec(object):
                 dep_hash, deptypes = elt
             elif isinstance(elt, dict):
                 # new format: elements of dependency spec are keyed.
-                for key in (ht.full_hash.name,
+                for key in (ht.dag_hash.name,
+                            ht.full_hash.name,
                             ht.build_hash.name,
-                            ht.dag_hash.name,
+                            ht.runtime_hash.name,
                             ht.process_hash.name):
                     if key in elt:
+                        # FIXME: if the key is 'hash' it could mean the old
+                        # dag hash without build deps, or the new dag hash which
+                        # is equivalent to the full hash.  If this was the old
+                        # dag hash, we need to keep the hash value but set the
+                        # key hash type to "runtime_hash".
                         dep_hash, deptypes = elt[key], elt['type']
                         hash_type = key
                         break
@@ -3793,6 +3801,7 @@ class Spec(object):
             self._dunder_hash = other._dunder_hash
             self._normal = True
             self._full_hash = other._full_hash
+            self._runtime_hash = other._runtime_hash
             self._package_hash = other._package_hash
         else:
             self._hash = None
@@ -3802,6 +3811,7 @@ class Spec(object):
             # always set it False here to avoid the complexity of checking
             self._normal = False
             self._full_hash = None
+            self._runtime_hash = None
             self._package_hash = None
 
         return changed
@@ -4741,6 +4751,7 @@ class Spec(object):
         for dep in ret.traverse(root=True, order='post'):
             opposite = other_nodes if dep.name in self_nodes else self_nodes
             if any(name in dep for name in opposite.keys()):
+
                 # package hash cannot be affected by splice
                 dep.clear_cached_hashes(ignore=['package_hash'])
 
diff --git a/lib/spack/spack/test/ci.py b/lib/spack/spack/test/ci.py
index 5dc094ae7a..8bd59124bf 100644
--- a/lib/spack/spack/test/ci.py
+++ b/lib/spack/spack/test/ci.py
@@ -96,9 +96,9 @@ def test_get_concrete_specs(config, mutable_mock_env_path, mock_packages):
 
     with e as active_env:
         for s in active_env.all_specs():
-            hash_dict[s.name] = s.build_hash()
+            hash_dict[s.name] = s.dag_hash()
             if s.name == 'dyninst':
-                dyninst_hash = s.build_hash()
+                dyninst_hash = s.dag_hash()
 
         assert(dyninst_hash)
 
@@ -107,7 +107,7 @@ def test_get_concrete_specs(config, mutable_mock_env_path, mock_packages):
         assert 'root' in spec_map
 
         concrete_root = spec_map['root']
-        assert(concrete_root.build_hash() == dyninst_hash)
+        assert(concrete_root.dag_hash() == dyninst_hash)
 
         s = spec.Spec('dyninst')
         print('nonconc spec name: {0}'.format(s.name))
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index 35276054aa..f698cd2715 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -975,14 +975,14 @@ def create_v1_lockfile_dict(roots, all_specs):
         },
         "roots": list(
             {
-                "hash": s.dag_hash(),
+                "hash": s.runtime_hash(),
                 "spec": s.name
             } for s in roots
         ),
         # Version one lockfiles use the dag hash without build deps as keys,
         # but they write out the full node dict (including build deps)
         "concrete_specs": dict(
-            (s.dag_hash(), s.to_node_dict(hash=ht.build_hash))
+            (s.runtime_hash(), s.to_node_dict(hash=ht.build_hash))
             for s in all_specs
         )
     }
@@ -1016,8 +1016,8 @@ def test_read_old_lock_and_write_new(tmpdir):
         # When the lockfile is rewritten, it should adopt the new hash scheme
         # which accounts for all dependencies, including build dependencies
         assert hashes == set([
-            x.build_hash(),
-            y.build_hash()])
+            x.dag_hash(),
+            y.dag_hash()])
 
 
 @pytest.mark.usefixtures('config')
@@ -1118,7 +1118,7 @@ def test_store_different_build_deps():
         # according to the DAG hash (since build deps are excluded from
         # comparison by default). Although the dag hashes are equal, the specs
         # are not considered equal because they compare build deps.
-        assert x_concretized['y'].dag_hash() == y_concretized.dag_hash()
+        assert x_concretized['y'].runtime_hash() == y_concretized.runtime_hash()
 
         _env_create('test', with_view=False)
         e = ev.read('test')