From 0715b512a19d5d966f596559274ca7b8a3406701 Mon Sep 17 00:00:00 2001 From: Peter Josef Scheibel Date: Mon, 4 Mar 2019 19:04:47 -0800 Subject: env: environments index specs by full DAG w/build deps - 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 --- lib/spack/spack/environment.py | 68 ++++++++++++---- lib/spack/spack/spec.py | 47 +++++++---- lib/spack/spack/test/cmd/env.py | 172 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 257 insertions(+), 30 deletions(-) diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment.py index e6dcc08b22..c7f57cc61d 100644 --- a/lib/spack/spack/environment.py +++ b/lib/spack/spack/environment.py @@ -72,7 +72,7 @@ spack: 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 @@ class Environment(object): 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 @@ class Environment(object): """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 @@ class Environment(object): 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 @@ class Environment(object): 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 @@ class Environment(object): # 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 @@ class Environment(object): # 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 @@ class Environment(object): 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 @@ class Environment(object): 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 @@ class Environment(object): """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 @@ class Environment(object): 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 index 8db4c765ca..1a8d9016c0 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -927,6 +927,7 @@ class Spec(object): self.namespace = None self._hash = None + self._build_hash = None self._cmp_key_cache = None self._package = None @@ -1285,22 +1286,35 @@ class Spec(object): 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 of the DAG hash as an integer type.""" @@ -1373,7 +1387,7 @@ class Spec(object): 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 @@ class Spec(object): 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 @@ class Spec(object): 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 @@ class Spec(object): 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 index 1d2dc5a3ab..23db0e429d 100644 --- a/lib/spack/spack/test/cmd/env.py +++ b/lib/spack/spack/test/cmd/env.py @@ -15,7 +15,10 @@ import spack.environment as ev from spack.cmd.env import _env_create from spack.spec import Spec from spack.main import SpackCommand + from spack.spec_list import SpecListError +from spack.test.conftest import MockPackage, MockPackageMultiRepo +import spack.util.spack_json as sjson # everything here uses the mock_env_path @@ -625,6 +628,175 @@ def test_uninstall_removes_from_env(mock_stage, mock_fetch, install_mockery): assert not test.user_specs +def create_v1_lockfile_dict(roots, all_specs): + test_lockfile_dict = { + "_meta": { + "lockfile-version": 1, + "file-type": "spack-lockfile" + }, + "roots": list( + { + "hash": s.dag_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(all_deps=True)) for s in all_specs + ) + } + return test_lockfile_dict + + +@pytest.mark.usefixtures('config') +def test_read_old_lock_and_write_new(tmpdir): + build_only = ('build',) + + y = MockPackage('y', [], []) + x = MockPackage('x', [y], [build_only]) + + mock_repo = MockPackageMultiRepo([x, y]) + with spack.repo.swap(mock_repo): + x = Spec('x') + x.concretize() + + y = x['y'] + + test_lockfile_dict = create_v1_lockfile_dict([x], [x, y]) + + test_lockfile_path = str(tmpdir.join('test.lock')) + with open(test_lockfile_path, 'w') as f: + sjson.dump(test_lockfile_dict, stream=f) + + _env_create('test', test_lockfile_path, with_view=False) + + e = ev.read('test') + hashes = set(e._to_lockfile_dict()['concrete_specs']) + # When the lockfile is rewritten, it should adopt the new hash scheme + # which accounts for all dependencies, including build dependencies + assert hashes == set([ + x.dag_hash(all_deps=True), + y.dag_hash(all_deps=True)]) + + +@pytest.mark.usefixtures('config') +def test_read_old_lock_creates_backup(tmpdir): + """When reading a version-1 lockfile, make sure that a backup of that file + is created. + """ + y = MockPackage('y', [], []) + + mock_repo = MockPackageMultiRepo([y]) + with spack.repo.swap(mock_repo): + y = Spec('y') + y.concretize() + + test_lockfile_dict = create_v1_lockfile_dict([y], [y]) + + env_root = tmpdir.mkdir('test-root') + test_lockfile_path = str(env_root.join(ev.lockfile_name)) + with open(test_lockfile_path, 'w') as f: + sjson.dump(test_lockfile_dict, stream=f) + + e = ev.Environment(str(env_root)) + assert os.path.exists(e._lock_backup_v1_path) + with open(e._lock_backup_v1_path, 'r') as backup_v1_file: + lockfile_dict_v1 = sjson.load(backup_v1_file) + # Make sure that the backup file follows the v1 hash scheme + assert y.dag_hash() in lockfile_dict_v1['concrete_specs'] + + +@pytest.mark.usefixtures('config') +def test_indirect_build_dep(): + """Simple case of X->Y->Z where Y is a build/link dep and Z is a + build-only dep. Make sure this concrete DAG is preserved when writing the + environment out and reading it back. + """ + default = ('build', 'link') + build_only = ('build',) + + z = MockPackage('z', [], []) + y = MockPackage('y', [z], [build_only]) + x = MockPackage('x', [y], [default]) + + mock_repo = MockPackageMultiRepo([x, y, z]) + + def noop(*args): + pass + setattr(mock_repo, 'dump_provenance', noop) + + with spack.repo.swap(mock_repo): + x_spec = Spec('x') + x_concretized = x_spec.concretized() + + _env_create('test', with_view=False) + e = ev.read('test') + e.add(x_spec) + e.concretize(_display=False) + e.write() + + e_read = ev.read('test') + x_env_hash, = e_read.concretized_order + + x_env_spec = e_read.specs_by_hash[x_env_hash] + assert x_env_spec == x_concretized + + +@pytest.mark.usefixtures('config') +def test_store_different_build_deps(): + r"""Ensure that an environment can store two instances of a build-only +Dependency: + + x y + /| (l) | (b) + (b) | y z2 + \| (b) # noqa: W605 + z1 + + """ + default = ('build', 'link') + build_only = ('build',) + + z = MockPackage('z', [], []) + y = MockPackage('y', [z], [build_only]) + x = MockPackage('x', [y, z], [default, build_only]) + + mock_repo = MockPackageMultiRepo([x, y, z]) + + def noop(*args): + pass + setattr(mock_repo, 'dump_provenance', noop) + + with spack.repo.swap(mock_repo): + y_spec = Spec('y ^z@3') + y_concretized = y_spec.concretized() + + x_spec = Spec('x ^z@2') + x_concretized = x_spec.concretized() + + # Even though x chose a different 'z', it should choose the same y + # 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() + + _env_create('test', with_view=False) + e = ev.read('test') + e.add(y_spec) + e.add(x_spec) + e.concretize(_display=False) + e.write() + + e_read = ev.read('test') + y_env_hash, x_env_hash = e_read.concretized_order + + y_read = e_read.specs_by_hash[y_env_hash] + x_read = e_read.specs_by_hash[x_env_hash] + + assert x_read['z'] != y_read['z'] + + def test_env_updates_view_install( tmpdir, mock_stage, mock_fetch, install_mockery): view_dir = tmpdir.mkdir('view') -- cgit v1.2.3-70-g09d2