database: remove a few class properties (#46175)

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
+                # It is important that we always check upstream installations in the same order,
-                # in the same order, and that we always check the local
+                # and that we always check the local installation first: if a downstream Spack
-                # installation first: if a downstream Spack installs a package
+                # installs a package then dependents in that installation could be using it. If a
-                # then dependents in that installation could be using it.
+                # hash is installed locally and upstream, there isn't enough information to
-                # If a hash is installed locally and upstream, there isn't
+                # determine which one a local package depends on, so the convention ensures that
-                # enough information to determine which one a local package
+                # this isn't an issue.
-                # depends on, so the convention ensures that this isn't an
+                _, record = self.query_by_spec_hash(dhash, data=data)
-                # issue.
-                upstream, 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" % (
+                    tty.warn(
-                        spec.cformat("{name}{/hash:7}"),
+                        f"Missing dependency not in database: "
-                        dname,
+                        f"{spec.cformat('{name}{/hash:7}')} needs {dname}-{dhash[:7]}"
-                        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 = {}
+        data: Dict[str, InstallRecord] = {}
-        installed_prefixes = set()
+        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:
+            if error is not None:
-                tty.warn("Spack database was corrupt. Will rebuild. Error was:", str(self._error))
+                tty.warn(f"Spack database was corrupt. Will rebuild. Error was: {error}")
-                self._error = None
 
             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"
+                    tty.warn(
-                    msg = "Inconsistent state! %s %s of %s not in DB" % (
+                        f"Inconsistent state: "
-                        reltype,
+                        f"{'dependent' if direction == 'parents' else 'dependency'} {hash_key} of "
-                        hash_key,
+                        f"{spec.dag_hash()} not in DB"
-                        spec.dag_hash(),
                     )
-                    if self._fail_when_missing_deps:
-                        raise MissingDependenciesError(msg)
-                    tty.warn(msg)
                     continue
 
                 if not record.installed:

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

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)
 

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, capsys, config):
-def test_removed_upstream_dep(upstream_and_downstream_db, tmpdir):
     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()
+        # then rereading the downstream DB should warn about the missing dep
-        downstream_db.add(new_spec, downstream_layout)
+        downstream_db._read_from_file(downstream_db._index_path)
-
+        assert (
-        upstream_write_db.remove(new_spec["z"])
+            f"Missing dependency not in database: y/{y.dag_hash(7)} needs z"
-        upstream_db._read()
+            in capsys.readouterr().err
-
+        )
-        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()
 
 
 @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