Spec.installed_upstream fix (spack#50938)

`Database.query_by_spec_hash` returns `upstream, record`, and
claims in its docstring that `upstream = True` if the spec is installed
upstream. However, the record only needs to be *present* upstream
for this to be true. `Spec.installed_upstream` was going by the docstring
(which meant it could be wrong in circumstances where the upstream
had a record, but the spec wasn't actually installed there).

I think it's useful for `query_by_spec_hash` to preserve its behavior, so I

* Updated the docstring
* Made `Spec.installed_upstream` check the record

I also added a test. More than just the behavior here, I didn't see an explicit
test for the case where a spec is added to the DB with no prefix - something
that can now happen fairly frequently with the compiler wrappers when installing
from a binary cache (which is also how a user found this problem).

Author: scheibelp

lib/spack/spack/database.py

@@ -727,8 +727,13 @@ def query_by_spec_hash(
 
         Return:
             (tuple): (bool, optional InstallRecord): bool tells us whether
-                the spec is installed upstream. Its InstallRecord is also
-                returned if it's installed at all; otherwise None.
+                the record is from an upstream. Its InstallRecord is also
+                returned if available (the record must be checked to know
+                whether the hash is installed).
+
+        If the record is available locally, this function will always have
+        a preference for returning that, even if it is not installed locally
+        and is installed upstream.
         """
         if data and hash_key in data:
             return False, data[hash_key]
@@ -1175,7 +1180,7 @@ def _add(
         key = spec.dag_hash()
         spec_pkg_hash = spec._package_hash  # type: ignore[attr-defined]
         upstream, record = self.query_by_spec_hash(key)
-        if upstream:
+        if upstream and record and record.installed:
             return
 
         installation_time = installation_time or _now()
@@ -1264,7 +1269,7 @@ def add(self, spec: "spack.spec.Spec", *, explicit: bool = False, allow_missing=
     def _get_matching_spec_key(self, spec: "spack.spec.Spec", **kwargs) -> str:
         """Get the exact spec OR get a single spec that matches."""
         key = spec.dag_hash()
-        upstream, record = self.query_by_spec_hash(key)
+        _, record = self.query_by_spec_hash(key)
         if not record:
             match = self.query_one(spec, **kwargs)
             if match:
@@ -1275,7 +1280,7 @@ def _get_matching_spec_key(self, spec: "spack.spec.Spec", **kwargs) -> str:
     @_autospec
     def get_record(self, spec: "spack.spec.Spec", **kwargs) -> Optional[InstallRecord]:
         key = self._get_matching_spec_key(spec, **kwargs)
-        upstream, record = self.query_by_spec_hash(key)
+        _, record = self.query_by_spec_hash(key)
         return record
 
     def _decrement_ref_count(self, spec: "spack.spec.Spec") -> None:
@@ -1783,7 +1788,7 @@ def query_one(
 
     def missing(self, spec):
         key = spec.dag_hash()
-        upstream, record = self.query_by_spec_hash(key)
+        _, record = self.query_by_spec_hash(key)
         return record and not record.installed
 
     def is_occupied_install_prefix(self, path):

lib/spack/spack/spec.py

@@ -2019,8 +2019,8 @@ def installed_upstream(self):
         if not self.concrete:
             return False
 
-        upstream, _ = spack.store.STORE.db.query_by_spec_hash(self.dag_hash())
-        return upstream
+        upstream, record = spack.store.STORE.db.query_by_spec_hash(self.dag_hash())
+        return upstream and record and record.installed
 
     @overload
     def traverse(

lib/spack/spack/test/database.py

@@ -16,6 +16,7 @@
 import pytest
 
 import spack.subprocess_context
+from spack.directory_layout import DirectoryLayoutError
 
 try:
     import uuid
@@ -186,6 +187,53 @@ def test_installed_upstream(upstream_and_downstream_db, tmpdir):
         downstream_db._check_ref_counts()
 
 
+def test_missing_upstream_build_dep(upstream_and_downstream_db, tmpdir, monkeypatch, config):
+    upstream_db, downstream_db = upstream_and_downstream_db
+
+    z_y_prefix = str(tmpdir.join("z-y"))
+
+    def fail_for_z(spec):
+        if spec.prefix == z_y_prefix:
+            raise DirectoryLayoutError("Fake layout error for z")
+
+    upstream_db.layout.ensure_installed = fail_for_z
+
+    builder = spack.repo.MockRepositoryBuilder(tmpdir.mkdir("mock.repo"))
+    builder.add_package("z")
+    builder.add_package("y", dependencies=[("z", "build", None)])
+
+    monkeypatch.setattr(spack.store.STORE, "db", downstream_db)
+
+    with spack.repo.use_repositories(builder.root):
+        y = spack.concretize.concretize_one("y")
+        z_y = y["z"]
+        z_y.set_prefix(z_y_prefix)
+
+        with writable(upstream_db):
+            upstream_db.add(y)
+        upstream_db._read()
+
+        upstream, record = downstream_db.query_by_spec_hash(z_y.dag_hash())
+        assert upstream
+        assert not record.installed
+
+        assert y.installed
+        assert y.installed_upstream
+
+        assert not z_y.installed
+        assert not z_y.installed_upstream
+
+        # Now add z to downstream with non-triggering prefix
+        # and make sure z *is* installed
+
+        z_new = z_y.copy()
+        z_new.set_prefix(str(tmpdir.join("z-new")))
+        downstream_db.add(z_new)
+
+        assert z_new.installed
+        assert not z_new.installed_upstream
+
+
 def test_removed_upstream_dep(upstream_and_downstream_db, tmpdir, capsys, config):
     upstream_db, downstream_db = upstream_and_downstream_db