env: environments index specs by full DAG w/build deps

scheibelp · becker33 · commit 0715b512a19d · 2019-07-22T13:45:34.000-05:00
- ensure that Spec._build_hash attr is defined
- add logic to compute _build_hash when it is not already computed (e.g. for specs prior to this PR)
- add test to ensure that different instance of a build dep are preserved
- test conversion of old env lockfile format to new format
- tests: avoid view creation, DAG display in tests using MockPackage
- add regression test for more-general bug also fixed by this PR
- update lockfile version since the way we are maintaining hashes has changed
- write out backup for version-1 lockfiles and test that
diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment.py
@@ -72,7 +72,7 @@
 valid_environment_name_re = r'^\w[\w-]*$'
 
 #: version of the lockfile format. Must increase monotonically.
-lockfile_format_version = 1
+lockfile_format_version = 2
 
 #: legal first keys in the spack.yaml manifest file
 env_schema_keys = ('spack', 'env')
@@ -533,11 +533,17 @@ def __init__(self, path, init_file=None, with_view=None):
 
             if os.path.exists(self.lock_path):
                 with open(self.lock_path) as f:
-                    self._read_lockfile(f)
+                    read_lock_version = self._read_lockfile(f)
                 if default_manifest:
                     # No manifest, set user specs from lockfile
                     self._set_user_specs_from_lockfile()
 
+                if read_lock_version == 1:
+                    tty.debug(
+                        "Storing backup of old lockfile {0} at {1}".format(
+                            self.lock_path, self._lock_backup_v1_path))
+                    shutil.copy(self.lock_path, self._lock_backup_v1_path)
+
         if with_view is False:
             self.views = {}
         elif with_view is True:
@@ -638,6 +644,11 @@ def lock_path(self):
         """Path to spack.lock file in this environment."""
         return os.path.join(self.path, lockfile_name)
 
+    @property
+    def _lock_backup_v1_path(self):
+        """Path to backup of v1 lockfile before conversion to v2"""
+        return self.lock_path + '.backup.v1'
+
     @property
     def env_subdir_path(self):
         """Path to directory where the env stores repos, logs, views."""
@@ -809,7 +820,7 @@ def remove(self, query_spec, list_name=user_speclist_name, force=False):
                 del self.concretized_order[i]
                 del self.specs_by_hash[dag_hash]
 
-    def concretize(self, force=False):
+    def concretize(self, force=False, _display=True):
         """Concretize user_specs in this environment.
 
         Only concretizes specs that haven't been concretized yet unless
@@ -850,12 +861,13 @@ def concretize(self, force=False):
                 concrete = _concretize_from_constraints(uspec_constraints)
                 self._add_concrete_spec(uspec, concrete)
 
-                # Display concretized spec to the user
-                sys.stdout.write(concrete.tree(
-                    recurse_dependencies=True,
-                    status_fn=spack.spec.Spec.install_status,
-                    hashlen=7, hashes=True)
-                )
+                if _display:
+                    # Display concretized spec to the user
+                    sys.stdout.write(concrete.tree(
+                        recurse_dependencies=True,
+                        status_fn=spack.spec.Spec.install_status,
+                        hashlen=7, hashes=True)
+                    )
 
     def install(self, user_spec, concrete_spec=None, **install_args):
         """Install a single spec into an environment.
@@ -872,7 +884,7 @@ def install(self, user_spec, concrete_spec=None, **install_args):
             # spec might be in the user_specs, but not installed.
             # TODO: Redo name-based comparison for old style envs
             spec = next(s for s in self.user_specs if s.satisfies(user_spec))
-            concrete = self.specs_by_hash.get(spec.dag_hash())
+            concrete = self.specs_by_hash.get(spec.dag_hash(all_deps=True))
             if not concrete:
                 concrete = spec.concretized()
                 self._add_concrete_spec(spec, concrete)
@@ -984,7 +996,7 @@ def _add_concrete_spec(self, spec, concrete, new=True):
         # update internal lists of specs
         self.concretized_user_specs.append(spec)
 
-        h = concrete.dag_hash()
+        h = concrete.dag_hash(all_deps=True)
         self.concretized_order.append(h)
         self.specs_by_hash[h] = concrete
 
@@ -1013,6 +1025,10 @@ def install_all(self, args=None):
 
     def all_specs_by_hash(self):
         """Map of hashes to spec for all specs in this environment."""
+        # Note this uses dag-hashes calculated without build deps as keys,
+        # whereas the environment tracks specs based on dag-hashes calculated
+        # with all dependencies. This function should not be used by an
+        # Environment object for management of its own data structures
         hashes = {}
         for h in self.concretized_order:
             specs = self.specs_by_hash[h].traverse(deptype=('link', 'run'))
@@ -1095,9 +1111,11 @@ def _to_lockfile_dict(self):
         concrete_specs = {}
         for spec in self.specs_by_hash.values():
             for s in spec.traverse():
-                dag_hash = s.dag_hash()
-                if dag_hash not in concrete_specs:
-                    concrete_specs[dag_hash] = s.to_node_dict(all_deps=True)
+                dag_hash_all = s.dag_hash(all_deps=True)
+                if dag_hash_all not in concrete_specs:
+                    spec_dict = s.to_node_dict(all_deps=True)
+                    spec_dict[s.name]['hash'] = s.dag_hash()
+                    concrete_specs[dag_hash_all] = spec_dict
 
         hash_spec_list = zip(
             self.concretized_order, self.concretized_user_specs)
@@ -1126,6 +1144,7 @@ def _read_lockfile(self, file_or_json):
         """Read a lockfile from a file or from a raw string."""
         lockfile_dict = sjson.load(file_or_json)
         self._read_lockfile_dict(lockfile_dict)
+        return lockfile_dict['_meta']['lockfile-version']
 
     def _read_lockfile_dict(self, d):
         """Read a lockfile dictionary into this environment."""
@@ -1146,8 +1165,25 @@ def _read_lockfile_dict(self, d):
                 specs_by_hash[dag_hash]._add_dependency(
                     specs_by_hash[dep_hash], deptypes)
 
-        self.specs_by_hash = dict(
-            (x, y) for x, y in specs_by_hash.items() if x in root_hashes)
+        # 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)
+        old_hash_to_new = {}
+        self.specs_by_hash = {}
+        for _, spec in specs_by_hash.items():
+            dag_hash = spec.dag_hash()
+            build_hash = spec.dag_hash(all_deps=True)
+            if dag_hash in root_hashes:
+                old_hash_to_new[dag_hash] = build_hash
+
+            if (dag_hash in root_hashes or build_hash in root_hashes):
+                self.specs_by_hash[build_hash] = spec
+
+        if old_hash_to_new:
+            # Replace any older hashes in concretized_order with hashes
+            # that include build deps
+            self.concretized_order = [
+                old_hash_to_new.get(h, h) for h in self.concretized_order]
 
     def write(self):
         """Writes an in-memory environment to its location on disk.
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
@@ -927,6 +927,7 @@ def __init__(self, spec_like=None,
         self.namespace = None
 
         self._hash = None
+        self._build_hash = None
         self._cmp_key_cache = None
         self._package = None
 
@@ -1285,22 +1286,35 @@ def prefix(self):
     def prefix(self, value):
         self._prefix = Prefix(value)
 
-    def dag_hash(self, length=None):
+    def dag_hash(self, length=None, all_deps=False):
         """Return a hash of the entire spec DAG, including connectivity."""
-        if self._hash:
-            return self._hash[:length]
-        else:
-            yaml_text = syaml.dump(
-                self.to_node_dict(), default_flow_style=True, width=maxint)
-            sha = hashlib.sha1(yaml_text.encode('utf-8'))
+        if not self.concrete:
+            h = self._dag_hash(all_deps=all_deps)
+            # An upper bound of None is equivalent to len(h). An upper bound of
+            # 0 produces the empty string
+            return h[:length]
 
-            b32_hash = base64.b32encode(sha.digest()).lower()
-            if sys.version_info[0] >= 3:
-                b32_hash = b32_hash.decode('utf-8')
+        if not self._hash:
+            self._hash = self._dag_hash(all_deps=False)
 
-            if self.concrete:
-                self._hash = b32_hash
-            return b32_hash[:length]
+        if not self._build_hash:
+            self._build_hash = self._dag_hash(all_deps=True)
+
+        h = self._build_hash if all_deps else self._hash
+        return h[:length]
+
+    def _dag_hash(self, all_deps=False):
+        yaml_text = syaml.dump(
+            self.to_node_dict(all_deps=all_deps),
+            default_flow_style=True,
+            width=maxint)
+        sha = hashlib.sha1(yaml_text.encode('utf-8'))
+
+        b32_hash = base64.b32encode(sha.digest()).lower()
+        if sys.version_info[0] >= 3:
+            b32_hash = b32_hash.decode('utf-8')
+
+        return b32_hash
 
     def dag_hash_bit_prefix(self, bits):
         """Get the first <bits> bits of the DAG hash as an integer type."""
@@ -1373,7 +1387,7 @@ def to_node_dict(self, hash_function=None, all_deps=False):
         deps = self.dependencies_dict(deptype=deptypes)
         if deps:
             if hash_function is None:
-                hash_function = lambda s: s.dag_hash()
+                hash_function = lambda s: s.dag_hash(all_deps=all_deps)
             d['dependencies'] = syaml_dict([
                 (name,
                  syaml_dict([
@@ -1393,6 +1407,8 @@ def to_dict(self, all_deps=False):
         for s in self.traverse(order='pre', deptype=deptypes):
             node = s.to_node_dict(all_deps=all_deps)
             node[s.name]['hash'] = s.dag_hash()
+            if all_deps:
+                node[s.name]['build_hash'] = s.dag_hash(all_deps=True)
             node_list.append(node)
 
         return syaml_dict([('spec', node_list)])
@@ -1412,6 +1428,7 @@ def from_node_dict(node):
         spec = Spec(name, full_hash=node.get('full_hash', None))
         spec.namespace = node.get('namespace', None)
         spec._hash = node.get('hash', None)
+        spec._build_hash = node.get('build_hash', None)
 
         if 'version' in node or 'versions' in node:
             spec.versions = VersionList.from_dict(node)
@@ -2812,11 +2829,13 @@ def _dup(self, other, deps=True, cleardeps=True, caches=None):
 
         if caches:
             self._hash = other._hash
+            self._build_hash = other._build_hash
             self._cmp_key_cache = other._cmp_key_cache
             self._normal = other._normal
             self._full_hash = other._full_hash
         else:
             self._hash = None
+            self._build_hash = None
             self._cmp_key_cache = None
             self._normal = False
             self._full_hash = None
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
