From 069286bda74557f1e7dd823d7369106cc98c202a Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Tue, 3 Sep 2024 11:22:14 +0200 Subject: [PATCH] database: remove a few class properties (#46175) --- lib/spack/spack/database.py | 70 ++++++++++++++------------------ lib/spack/spack/spec.py | 4 ++ lib/spack/spack/store.py | 3 +- lib/spack/spack/test/database.py | 34 ++++++++-------- 4 files changed, 53 insertions(+), 58 deletions(-) diff --git a/lib/spack/spack/database.py b/lib/spack/spack/database.py index 02eae97f3f4..7ad8b11a81f 100644 --- a/lib/spack/spack/database.py +++ b/lib/spack/spack/database.py @@ -668,14 +668,6 @@ def __init__( self.upstream_dbs = list(upstream_dbs) if upstream_dbs else [] - # whether there was an error at the start of a read transaction - self._error = None - - # For testing: if this is true, an exception is thrown when missing - # dependencies are detected (rather than just printing a warning - # message) - self._fail_when_missing_deps = False - self._write_transaction_impl = lk.WriteTransaction self._read_transaction_impl = lk.ReadTransaction @@ -778,7 +770,13 @@ def query_local_by_spec_hash(self, hash_key): with self.read_transaction(): return self._data.get(hash_key, None) - def _assign_dependencies(self, spec_reader, hash_key, installs, data): + def _assign_dependencies( + self, + spec_reader: Type["spack.spec.SpecfileReaderBase"], + hash_key: str, + installs: dict, + data: Dict[str, InstallRecord], + ): # Add dependencies from other records in the install DB to # form a full spec. spec = data[hash_key].spec @@ -791,26 +789,20 @@ def _assign_dependencies(self, spec_reader, hash_key, installs, data): for dname, dhash, dtypes, _, virtuals in spec_reader.read_specfile_dep_specs( yaml_deps ): - # It is important that we always check upstream installations - # in the same order, and that we always check the local - # installation first: if a downstream Spack installs a package - # then dependents in that installation could be using it. - # If a hash is installed locally and upstream, there isn't - # enough information to determine which one a local package - # depends on, so the convention ensures that this isn't an - # issue. - upstream, record = self.query_by_spec_hash(dhash, data=data) + # It is important that we always check upstream installations in the same order, + # and that we always check the local installation first: if a downstream Spack + # installs a package then dependents in that installation could be using it. If a + # hash is installed locally and upstream, there isn't enough information to + # determine which one a local package depends on, so the convention ensures that + # this isn't an issue. + _, record = self.query_by_spec_hash(dhash, data=data) child = record.spec if record else None if not child: - msg = "Missing dependency not in database: " "%s needs %s-%s" % ( - spec.cformat("{name}{/hash:7}"), - dname, - dhash[:7], + tty.warn( + f"Missing dependency not in database: " + f"{spec.cformat('{name}{/hash:7}')} needs {dname}-{dhash[:7]}" ) - if self._fail_when_missing_deps: - raise MissingDependenciesError(msg) - tty.warn(msg) continue spec._add_dependency(child, depflag=dt.canonicalize(dtypes), virtuals=virtuals) @@ -877,8 +869,8 @@ def invalid_record(hash_key, error): # (i.e., its specs are a true Merkle DAG, unlike most specs.) # Pass 1: Iterate through database and build specs w/o dependencies - data = {} - installed_prefixes = set() + data: Dict[str, InstallRecord] = {} + installed_prefixes: Set[str] = set() for hash_key, rec in installs.items(): try: # This constructs a spec DAG from the list of all installs @@ -923,6 +915,8 @@ def reindex(self, directory_layout): if self.is_upstream: raise UpstreamDatabaseLockingError("Cannot reindex an upstream database") + error: Optional[CorruptDatabaseError] = None + # Special transaction to avoid recursive reindex calls and to # ignore errors if we need to rebuild a corrupt database. def _read_suppress_error(): @@ -930,7 +924,8 @@ def _read_suppress_error(): if os.path.isfile(self._index_path): self._read_from_file(self._index_path) except CorruptDatabaseError as e: - self._error = e + nonlocal error + error = e self._data = {} self._installed_prefixes = set() @@ -939,9 +934,8 @@ def _read_suppress_error(): ) with transaction: - if self._error: - tty.warn("Spack database was corrupt. Will rebuild. Error was:", str(self._error)) - self._error = None + if error is not None: + tty.warn(f"Spack database was corrupt. Will rebuild. Error was: {error}") old_data = self._data old_installed_prefixes = self._installed_prefixes @@ -1405,17 +1399,13 @@ def installed_relatives( for relative in to_add: hash_key = relative.dag_hash() - upstream, record = self.query_by_spec_hash(hash_key) + _, record = self.query_by_spec_hash(hash_key) if not record: - reltype = "Dependent" if direction == "parents" else "Dependency" - msg = "Inconsistent state! %s %s of %s not in DB" % ( - reltype, - hash_key, - spec.dag_hash(), + tty.warn( + f"Inconsistent state: " + f"{'dependent' if direction == 'parents' else 'dependency'} {hash_key} of " + f"{spec.dag_hash()} not in DB" ) - if self._fail_when_missing_deps: - raise MissingDependenciesError(msg) - tty.warn(msg) continue if not record.installed: diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index 0e7746603dc..700f3a74de4 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -4686,6 +4686,10 @@ def _load(cls, data): return hash_dict[root_spec_hash]["node_spec"] + @classmethod + def read_specfile_dep_specs(cls, deps, hash_type=ht.dag_hash.name): + raise NotImplementedError("Subclasses must implement this method.") + class SpecfileV1(SpecfileReaderBase): @classmethod diff --git a/lib/spack/spack/store.py b/lib/spack/spack/store.py index 8509d728aa5..b248e4fd615 100644 --- a/lib/spack/spack/store.py +++ b/lib/spack/spack/store.py @@ -261,7 +261,7 @@ def restore(token): def _construct_upstream_dbs_from_install_roots( - install_roots: List[str], _test: bool = False + install_roots: List[str], ) -> List[spack.database.Database]: accumulated_upstream_dbs: List[spack.database.Database] = [] for install_root in reversed(install_roots): @@ -271,7 +271,6 @@ def _construct_upstream_dbs_from_install_roots( is_upstream=True, upstream_dbs=upstream_dbs, ) - next_db._fail_when_missing_deps = _test next_db._read() accumulated_upstream_dbs.insert(0, next_db) diff --git a/lib/spack/spack/test/database.py b/lib/spack/spack/test/database.py index 91eab857f41..bbfbc92bb78 100644 --- a/lib/spack/spack/test/database.py +++ b/lib/spack/spack/test/database.py @@ -148,8 +148,7 @@ def test_installed_upstream(upstream_and_downstream_db, tmpdir): downstream_db._check_ref_counts() -@pytest.mark.usefixtures("config") -def test_removed_upstream_dep(upstream_and_downstream_db, tmpdir): +def test_removed_upstream_dep(upstream_and_downstream_db, tmpdir, capsys, config): upstream_write_db, upstream_db, upstream_layout, downstream_db, downstream_layout = ( upstream_and_downstream_db ) @@ -159,21 +158,24 @@ def test_removed_upstream_dep(upstream_and_downstream_db, tmpdir): builder.add_package("y", dependencies=[("z", None, None)]) with spack.repo.use_repositories(builder): - spec = spack.spec.Spec("y").concretized() + y = spack.spec.Spec("y").concretized() + z = y["z"] - upstream_write_db.add(spec["z"], upstream_layout) + # add dependency to upstream, dependents to downstream + upstream_write_db.add(z, upstream_layout) + upstream_db._read() + downstream_db.add(y, downstream_layout) + + # remove the dependency from the upstream DB + upstream_write_db.remove(z) upstream_db._read() - new_spec = spack.spec.Spec("y").concretized() - downstream_db.add(new_spec, downstream_layout) - - upstream_write_db.remove(new_spec["z"]) - upstream_db._read() - - new_downstream = spack.database.Database(downstream_db.root, upstream_dbs=[upstream_db]) - new_downstream._fail_when_missing_deps = True - with pytest.raises(spack.database.MissingDependenciesError): - new_downstream._read() + # then rereading the downstream DB should warn about the missing dep + downstream_db._read_from_file(downstream_db._index_path) + assert ( + f"Missing dependency not in database: y/{y.dag_hash(7)} needs z" + in capsys.readouterr().err + ) @pytest.mark.usefixtures("config") @@ -226,7 +228,7 @@ def test_cannot_write_upstream(tmpdir, gen_mock_layout): with upstream_db_independent.write_transaction(): pass - upstream_dbs = spack.store._construct_upstream_dbs_from_install_roots([roots[1]], _test=True) + upstream_dbs = spack.store._construct_upstream_dbs_from_install_roots([roots[1]]) with spack.repo.use_repositories(builder.root): spec = spack.spec.Spec("x") @@ -258,7 +260,7 @@ def test_recursive_upstream_dbs(tmpdir, gen_mock_layout): db_a.add(spec["x"], layouts[0]) upstream_dbs_from_scratch = spack.store._construct_upstream_dbs_from_install_roots( - [roots[1], roots[2]], _test=True + [roots[1], roots[2]] ) db_a_from_scratch = spack.database.Database( roots[0], upstream_dbs=upstream_dbs_from_scratch