bugfix: `installed` and `installed_upstream` should not assert (#30271)

Fix bug introduced in #30191. `Spec.installed` and `Spec.installed_upstream` should just return
`False` for abstract specs, as they can be called in that context.

- [x] `Spec.installed` returns `False` now instead of asserting that the `Spec`
      is concrete.
- [x] `Spec.installed_upstream` returns `False` now instead of asserting that the `Spec`
      is concrete.
- [x] `Spec.installed_upstream` no longer caches its result, as install status seems
      like a bad thing to cache -- it can easily be invalidated. Calling code should
      use transactions if there are peformance issues, as with other places in Spack.
- [x] add tests for `Spec.installed` and `Spec.installed_upstream`

4 files changed, 63 insertions, 12 deletions

diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index e5505fb5a4..1283117e1d 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -1197,7 +1197,6 @@ class Spec(object):
         self._package_hash = None
         self._dunder_hash = None
         self._package = None
-        self._installed_upstream = None
 
         # Most of these are internal implementation details that can be
         # set by internal Spack calls in the constructor.
@@ -1562,8 +1561,9 @@ class Spec(object):
         Returns:
             True if the package has been installed, False otherwise.
         """
-        msg = "a spec must be concrete to be queried for installation status"
-        assert self.concrete, msg
+        if not self.concrete:
+            return False
+
         try:
             # If the spec is in the DB, check the installed
             # attribute of the record
@@ -1575,12 +1575,16 @@ class Spec(object):
 
     @property
     def installed_upstream(self):
-        msg = "a spec must be concrete to be queried for installation status"
-        assert self.concrete, msg
-        if getattr(self, '_installed_upstream', None) is None:
-            upstream, _ = spack.store.db.query_by_spec_hash(self.dag_hash())
-            self._installed_upstream = upstream
-        return self._installed_upstream
+        """Whether the spec is installed in an upstream repository.
+
+        Returns:
+            True if the package is installed in an upstream, False otherwise.
+        """
+        if not self.concrete:
+            return False
+
+        upstream, _ = spack.store.db.query_by_spec_hash(self.dag_hash())
+        return upstream
 
     def traverse(self, **kwargs):
         direction = kwargs.get('direction', 'children')
diff --git a/lib/spack/spack/test/database.py b/lib/spack/spack/test/database.py
index 4a5071e1a3..0315857f1d 100644
--- a/lib/spack/spack/test/database.py
+++ b/lib/spack/spack/test/database.py
@@ -66,6 +66,36 @@ def upstream_and_downstream_db(tmpdir_factory, gen_mock_layout):
 
 @pytest.mark.skipif(sys.platform == 'win32',
                     reason="Upstreams currently unsupported on Windows")
+def test_spec_installed_upstream(upstream_and_downstream_db, config, monkeypatch):
+    """Test whether Spec.installed_upstream() works."""
+    upstream_write_db, upstream_db, upstream_layout, \
+        downstream_db, downstream_layout = upstream_and_downstream_db
+
+    # a known installed spec should say that it's installed
+    mock_repo = MockPackageMultiRepo()
+    mock_repo.add_package('x', [], [])
+
+    with spack.repo.use_repositories(mock_repo):
+        spec = spack.spec.Spec("x").concretized()
+        assert not spec.installed
+        assert not spec.installed_upstream
+
+        upstream_write_db.add(spec, upstream_layout)
+        upstream_db._read()
+
+        monkeypatch.setattr(spack.store, "db", downstream_db)
+        assert spec.installed
+        assert spec.installed_upstream
+        assert spec.copy().installed
+
+    # an abstract spec should say it's not installed
+    spec = spack.spec.Spec("not-a-real-package")
+    assert not spec.installed
+    assert not spec.installed_upstream
+
+
+@pytest.mark.skipif(sys.platform == 'win32',
+                    reason="Upstreams currently unsupported on Windows")
 @pytest.mark.usefixtures('config')
 def test_installed_upstream(upstream_and_downstream_db):
     upstream_write_db, upstream_db, upstream_layout,\
diff --git a/lib/spack/spack/test/installer.py b/lib/spack/spack/test/installer.py
index 53f254d665..4fbafeb278 100644
--- a/lib/spack/spack/test/installer.py
+++ b/lib/spack/spack/test/installer.py
@@ -284,7 +284,7 @@ def test_check_last_phase_error(install_mockery):
     assert pkg.last_phase in err
 
 
-def test_installer_ensure_ready_errors(install_mockery):
+def test_installer_ensure_ready_errors(install_mockery, monkeypatch):
     const_arg = installer_args(['trivial-install-test-package'], {})
     installer = create_installer(const_arg)
     spec = installer.build_requests[0].pkg.spec
@@ -300,14 +300,14 @@ def test_installer_ensure_ready_errors(install_mockery):
 
     # Force an upstream package error
     spec.external_path, spec.external_modules = path, modules
-    spec._installed_upstream = True
+    monkeypatch.setattr(spack.spec.Spec, "installed_upstream", True)
     msg = fmt.format('is upstream')
     with pytest.raises(inst.UpstreamPackageError, match=msg):
         installer._ensure_install_ready(spec.package)
 
     # Force an install lock error, which should occur naturally since
     # we are calling an internal method prior to any lock-related setup
-    spec._installed_upstream = False
+    monkeypatch.setattr(spack.spec.Spec, "installed_upstream", False)
     assert len(installer.locks) == 0
     with pytest.raises(inst.InstallLockError, match=fmt.format('not locked')):
         installer._ensure_install_ready(spec.package)
diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py
index 621815494d..22a39d8f1f 100644
--- a/lib/spack/spack/test/spec_semantics.py
+++ b/lib/spack/spack/test/spec_semantics.py
@@ -1258,3 +1258,20 @@ def test_merge_anonymous_spec_with_named_spec(anonymous, named, expected):
     changed = s.constrain(named)
     assert changed
     assert s == Spec(expected)
+
+
+def test_spec_installed(install_mockery, database):
+    """Test whether Spec.installed works."""
+    # a known installed spec should say that it's installed
+    specs = database.query()
+    spec = specs[0]
+    assert spec.installed
+    assert spec.copy().installed
+
+    # an abstract spec should say it's not installed
+    spec = Spec("not-a-real-package")
+    assert not spec.installed
+
+    # 'a' is not in the mock DB and is not installed
+    spec = Spec("a").concretized()
+    assert not spec.installed