From d729b4e72bf85024bccf87f7a4fb7163cd2407ff Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Mon, 25 Apr 2022 00:46:21 -0700 Subject: 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` --- lib/spack/spack/spec.py | 22 +++++++++++++--------- lib/spack/spack/test/database.py | 30 ++++++++++++++++++++++++++++++ lib/spack/spack/test/installer.py | 6 +++--- lib/spack/spack/test/spec_semantics.py | 17 +++++++++++++++++ 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 @@ -64,6 +64,36 @@ def upstream_and_downstream_db(tmpdir_factory, gen_mock_layout): downstream_db, downstream_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') 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 -- cgit v1.2.3-70-g09d2