Move "installed" and "installed_upstream" from PackageBase to Spec (#30191)

This PR moves the `installed` and `installed_upstream` properties from `PackageBase` to `Spec` and is a step towards being able to reuse specs for which we don't have a `package.py` available. It _should_ be sufficient to complete the concretization step and see the spec in the concretized DAG.

To fully reuse a spec without a package.py though we need a way to serialize enough data to reconstruct the results of calls to:
- `Spec.libs`, `Spec.headers` and `Spec.ommand`
- `Package.setup_dependent_*_environment` and `Package.setup_run_environment`

- [x] Add stub methods to packages with warnings
- [x] Add a missing "root=False" in cmd/fetch.py
- [x] Assert that a spec is concrete before checking installation status

lib/spack/spack/binary_distribution.py

@@ -1568,12 +1568,11 @@ def install_root_node(spec, allow_root, unsigned=False, force=False, sha256=None
         sha256 (str): optional sha256 of the binary package, to be checked
             before installation
     """
-    package = spack.repo.get(spec)
     # Early termination
     if spec.external or spec.virtual:
         warnings.warn("Skipping external or virtual package {0}".format(spec.format()))
         return
-    elif spec.concrete and package.installed and not force:
+    elif spec.concrete and spec.installed and not force:
         warnings.warn("Package for spec {0} already installed.".format(spec.format()))
         return
 

lib/spack/spack/cmd/dev_build.py

@@ -91,8 +91,8 @@ def dev_build(self, args):
     spec.concretize()
     package = spack.repo.get(spec)
 
-    if package.installed:
+    if spec.installed:
-        tty.error("Already installed in %s" % package.prefix)
+        tty.error("Already installed in %s" % spec.prefix)
         tty.msg("Uninstall or try adding a version suffix for this dev build.")
         sys.exit(1)
 

lib/spack/spack/cmd/fetch.py

@@ -69,14 +69,10 @@ def fetch(parser, args):
 
     for spec in specs:
         if args.missing or args.dependencies:
-            for s in spec.traverse():
+            for s in spec.traverse(root=False):
-                package = spack.repo.get(s)
-
                 # Skip already-installed packages with --missing
-                if args.missing and package.installed:
+                if args.missing and s.installed:
                     continue
 
-                package.do_fetch()
+                s.package.do_fetch()
-
+        spec.package.do_fetch()
-        package = spack.repo.get(spec)
-        package.do_fetch()

lib/spack/spack/cmd/modules/__init__.py

@@ -273,7 +273,7 @@ def refresh(module_type, specs, args):
         return
 
     if not args.upstream_modules:
-        specs = list(s for s in specs if not s.package.installed_upstream)
+        specs = list(s for s in specs if not s.installed_upstream)
 
     if not args.yes_to_all:
         msg = 'You are about to regenerate {types} module files for:\n'

lib/spack/spack/environment/environment.py

@@ -302,7 +302,7 @@ def _is_dev_spec_and_has_changed(spec):
         return False
 
     # Now we can check whether the code changed since the last installation
-    if not spec.package.installed:
+    if not spec.installed:
         # Not installed -> nothing to compare against
         return False
 
@@ -315,7 +315,7 @@ def _spec_needs_overwrite(spec, changed_dev_specs):
     """Check whether the current spec needs to be overwritten because either it has
     changed itself or one of its dependencies have changed"""
     # if it's not installed, we don't need to overwrite it
-    if not spec.package.installed:
+    if not spec.installed:
         return False
 
     # If the spec itself has changed this is a trivial decision
@@ -330,7 +330,7 @@ def _spec_needs_overwrite(spec, changed_dev_specs):
     # If any dep needs overwrite, or any dep is missing and is a dev build then
     # overwrite this package
     if any(
-        ((not dep.package.installed) and dep.satisfies('dev_path=*')) or
+        ((not dep.installed) and dep.satisfies('dev_path=*')) or
         _spec_needs_overwrite(dep, changed_dev_specs)
         for dep in spec.traverse(root=False)
     ):
@@ -518,7 +518,7 @@ def specs_for_view(self, concretized_root_specs):
 
         # Filter selected, installed specs
         with spack.store.db.read_transaction():
-            specs = [s for s in specs if s in self and s.package.installed]
+            specs = [s for s in specs if s in self and s.installed]
 
         return specs
 
@@ -1380,9 +1380,10 @@ def check_views(self):
             # default view if they are installed.
             for view_name, view in self.views.items():
                 for _, spec in self.concretized_specs():
-                    if spec in view and spec.package.installed:
+                    if spec in view and spec.package and spec.installed:
-                        tty.debug(
+                        msg = '{0} in view "{1}"'
-                            'Spec %s in view %s' % (spec.name, view_name))
+                        tty.debug(msg.format(spec.name, view_name))
+
         except (spack.repo.UnknownPackageError,
                 spack.repo.UnknownNamespaceError) as e:
             tty.warn(e)
@@ -1398,7 +1399,8 @@ def _env_modifications_for_default_view(self, reverse=False):
 
         errors = []
         for _, root_spec in self.concretized_specs():
-            if root_spec in self.default_view and root_spec.package.installed:
+            if (root_spec in self.default_view and
+                    root_spec.installed and root_spec.package):
                 for spec in root_spec.traverse(deptype='run', root=True):
                     if spec.name in visited:
                         # It is expected that only one instance of the package
@@ -1537,7 +1539,7 @@ def uninstalled_specs(self):
         with spack.store.db.read_transaction():
             for concretized_hash in self.concretized_order:
                 spec = self.specs_by_hash[concretized_hash]
-                if not spec.package.installed or (
+                if not spec.installed or (
                         spec.satisfies('dev_path=*') or
                         spec.satisfies('^dev_path=*')
                 ):
@@ -1572,7 +1574,7 @@ def install_specs(self, specs=None, **install_args):
 
         # ensure specs already installed are marked explicit
         all_specs = specs or [cs for _, cs in self.concretized_specs()]
-        specs_installed = [s for s in all_specs if s.package.installed]
+        specs_installed = [s for s in all_specs if s.installed]
         with spack.store.db.write_transaction():  # do all in one transaction
             for spec in specs_installed:
                 spack.store.db.update_explicit(spec, True)
@@ -1599,7 +1601,7 @@ def install_specs(self, specs=None, **install_args):
         finally:
             # Ensure links are set appropriately
             for spec in specs_to_install:
-                if spec.package.installed:
+                if spec.installed:
                     self.new_installs.append(spec)
                     try:
                         self._install_log_links(spec)
@@ -1649,7 +1651,7 @@ def added_specs(self):
                 concrete = concretized.get(spec)
                 if not concrete:
                     yield spec
-                elif not concrete.package.installed:
+                elif not concrete.installed:
                     yield concrete
 
     def concretized_specs(self):

lib/spack/spack/installer.py

@@ -140,7 +140,7 @@ def _handle_external_and_upstream(pkg, explicit):
                              .format(pkg.prefix, package_id(pkg)))
         return True
 
-    if pkg.installed_upstream:
+    if pkg.spec.installed_upstream:
         tty.verbose('{0} is installed in an upstream Spack instance at {1}'
                     .format(package_id(pkg), pkg.spec.prefix))
         _print_installed_pkg(pkg.prefix)
@@ -853,7 +853,7 @@ def _check_deps_status(self, request):
                 raise InstallError(err.format(request.pkg_id, msg))
 
             # Flag external and upstream packages as being installed
-            if dep_pkg.spec.external or dep_pkg.installed_upstream:
+            if dep_pkg.spec.external or dep_pkg.spec.installed_upstream:
                 self._flag_installed(dep_pkg)
                 continue
 
@@ -995,7 +995,7 @@ def _ensure_install_ready(self, pkg):
             raise ExternalPackageError('{0} {1}'.format(pre, 'is external'))
 
         # Upstream packages cannot be installed locally.
-        if pkg.installed_upstream:
+        if pkg.spec.installed_upstream:
             raise UpstreamPackageError('{0} {1}'.format(pre, 'is upstream'))
 
         # The package must have a prefix lock at this stage.

lib/spack/spack/modules/common.py

@@ -370,7 +370,7 @@ def get_module(
         available.
     """
     try:
-        upstream = spec.package.installed_upstream
+        upstream = spec.installed_upstream
     except spack.repo.UnknownPackageError:
         upstream, record = spack.store.db.query_by_spec_hash(spec.dag_hash())
     if upstream:

lib/spack/spack/package.py

@@ -26,6 +26,7 @@
 import time
 import traceback
 import types
+import warnings
 from typing import Any, Callable, Dict, List, Optional  # novm
 
 import six
@@ -790,15 +791,6 @@ def __init__(self, spec):
 
         super(PackageBase, self).__init__()
 
-    @property
-    def installed_upstream(self):
-        if not hasattr(self, '_installed_upstream'):
-            upstream, record = spack.store.db.query_by_spec_hash(
-                self.spec.dag_hash())
-            self._installed_upstream = upstream
-
-        return self._installed_upstream
-
     @classmethod
     def possible_dependencies(
             cls, transitive=True, expand_virtuals=True, deptype='all',
@@ -1267,6 +1259,20 @@ def install_test_root(self):
         """Return the install test root directory."""
         return os.path.join(self.metadata_dir, 'test')
 
+    @property
+    def installed(self):
+        msg = ('the "PackageBase.installed" property is deprecated and will be '
+               'removed in Spack v0.19, use "Spec.installed" instead')
+        warnings.warn(msg)
+        return self.spec.installed
+
+    @property
+    def installed_upstream(self):
+        msg = ('the "PackageBase.installed_upstream" property is deprecated and will '
+               'be removed in Spack v0.19, use "Spec.installed_upstream" instead')
+        warnings.warn(msg)
+        return self.spec.installed_upstream
+
     def _make_fetcher(self):
         # Construct a composite fetcher that always contains at least
         # one element (the root package). In case there are resources
@@ -1380,7 +1386,7 @@ def is_activated(self, view):
         if not self.is_extension:
             raise ValueError(
                 "is_activated called on package that is not an extension.")
-        if self.extendee_spec.package.installed_upstream:
+        if self.extendee_spec.installed_upstream:
             # If this extends an upstream package, it cannot be activated for
             # it. This bypasses construction of the extension map, which can
             # can fail when run in the context of a downstream Spack instance
@@ -1406,22 +1412,6 @@ def virtuals_provided(self):
         return [vspec for vspec, constraints in self.provided.items()
                 if any(self.spec.satisfies(c) for c in constraints)]
 
-    @property
-    def installed(self):
-        """Installation status of a package.
-
-        Returns:
-            True if the package has been installed, False otherwise.
-        """
-        try:
-            # If the spec is in the DB, check the installed
-            # attribute of the record
-            return spack.store.db.get_record(self.spec).installed
-        except KeyError:
-            # If the spec is not in the DB, the method
-            #  above raises a Key error
-            return False
-
     @property
     def prefix(self):
         """Get the prefix into which this package should be installed."""
@@ -2143,21 +2133,21 @@ def build_log_path(self):
         to the staging build file until the software is successfully installed,
         when it points to the file in the installation directory.
         """
-        return self.install_log_path if self.installed else self.log_path
+        return self.install_log_path if self.spec.installed else self.log_path
 
     @classmethod
     def inject_flags(cls, name, flags):
         """
         flag_handler that injects all flags through the compiler wrapper.
         """
-        return (flags, None, None)
+        return flags, None, None
 
     @classmethod
     def env_flags(cls, name, flags):
         """
         flag_handler that adds all flags to canonical environment variables.
         """
-        return (None, flags, None)
+        return None, flags, None
 
     @classmethod
     def build_system_flags(cls, name, flags):
@@ -2168,7 +2158,7 @@ def build_system_flags(cls, name, flags):
         implements it.  Currently, AutotoolsPackage and CMakePackage
         implement it.
         """
-        return (None, None, flags)
+        return None, None, flags
 
     def setup_build_environment(self, env):
         """Sets up the build environment for a package.
@@ -2465,10 +2455,10 @@ def _sanity_check_extension(self):
         extendee_package = self.extendee_spec.package
         extendee_package._check_extendable()
 
-        if not extendee_package.installed:
+        if not self.extendee_spec.installed:
             raise ActivationError(
                 "Can only (de)activate extensions for installed packages.")
-        if not self.installed:
+        if not self.spec.installed:
             raise ActivationError("Extensions must first be installed.")
         if self.extendee_spec.name not in self.extendees:
             raise ActivationError("%s does not extend %s!" %

lib/spack/spack/report.py

@@ -112,8 +112,7 @@ def __enter__(self):
             # Check which specs are already installed and mark them as skipped
             # only for install_task
             if self.do_fn == '_install_task':
-                for dep in filter(lambda x: x.package.installed,
+                for dep in filter(lambda x: x.installed, input_spec.traverse()):
-                                  input_spec.traverse()):
                     package = {
                         'name': dep.name,
                         'id': dep.dag_hash(),
@@ -140,7 +139,7 @@ def wrapper(instance, *args, **kwargs):
                     raise Exception
 
                 # We accounted before for what is already installed
-                installed_already = pkg.installed
+                installed_already = pkg.spec.installed
 
                 package = {
                     'name': pkg.name,

lib/spack/spack/rewiring.py

@@ -38,13 +38,13 @@ def rewire(spliced_spec):
     nodes in the DAG of that spec."""
     assert spliced_spec.spliced
     for spec in spliced_spec.traverse(order='post', root=True):
-        if not spec.build_spec.package.installed:
+        if not spec.build_spec.installed:
             # TODO: May want to change this at least for the root spec...
             # spec.build_spec.package.do_install(force=True)
             raise PackageNotInstalledError(spliced_spec,
                                            spec.build_spec,
                                            spec)
-        if spec.build_spec is not spec and not spec.package.installed:
+        if spec.build_spec is not spec and not spec.installed:
             explicit = spec is spliced_spec
             rewire_node(spec, explicit)
 

lib/spack/spack/spec.py

@@ -1197,6 +1197,7 @@ def __init__(self, spec_like=None, normal=False,
         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.
@@ -1554,6 +1555,33 @@ def spliced(self):
         """
         return any(s.build_spec is not s for s in self.traverse(root=True))
 
+    @property
+    def installed(self):
+        """Installation status of a package.
+
+        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
+        try:
+            # If the spec is in the DB, check the installed
+            # attribute of the record
+            return spack.store.db.get_record(self).installed
+        except KeyError:
+            # If the spec is not in the DB, the method
+            #  above raises a Key error
+            return False
+
+    @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
+
     def traverse(self, **kwargs):
         direction = kwargs.get('direction', 'children')
         depth = kwargs.get('depth', False)
@@ -2880,7 +2908,7 @@ def concretize(self, tests=False):
 
     def _mark_root_concrete(self, value=True):
         """Mark just this spec (not dependencies) concrete."""
-        if (not value) and self.concrete and self.package.installed:
+        if (not value) and self.concrete and self.installed:
             return
         self._normal = value
         self._concrete = value
@@ -2894,7 +2922,7 @@ def _mark_concrete(self, value=True):
         # if set to false, clear out all hashes (set to None or remove attr)
         # may need to change references to respect None
         for s in self.traverse():
-            if (not value) and s.concrete and s.package.installed:
+            if (not value) and s.concrete and s.installed:
                 continue
             elif not value:
                 s.clear_cached_hashes()
@@ -3159,7 +3187,7 @@ def _normalize_helper(self, visited, spec_deps, provider_index, tests):
         # Avoid recursively adding constraints for already-installed packages:
         # these may include build dependencies which are not needed for this
         # install (since this package is already installed).
-        if self.concrete and self.package.installed:
+        if self.concrete and self.installed:
             return False
 
         # Combine constraints from package deps with constraints from
@@ -4529,7 +4557,7 @@ def tree(self, **kwargs):
 
             if status_fn:
                 status = status_fn(node)
-                if node.package.installed_upstream:
+                if node.installed_upstream:
                     out += clr.colorize("@g{[^]}  ", color=color)
                 elif status is None:
                     out += clr.colorize("@K{ - }  ", color=color)  # !installed

lib/spack/spack/test/cmd/env.py

@@ -162,7 +162,7 @@ def test_env_install_all(install_mockery, mock_fetch):
     e.install_all()
     env_specs = e._get_environment_specs()
     spec = next(x for x in env_specs if x.name == 'cmake-client')
-    assert spec.package.installed
+    assert spec.installed
 
 
 def test_env_install_single_spec(install_mockery, mock_fetch):

lib/spack/spack/test/concretize.py

@@ -1422,7 +1422,7 @@ def test_concrete_specs_are_not_modified_on_reuse(
         # the answer set produced by clingo.
         with spack.config.override("concretizer:reuse", True):
             s = spack.spec.Spec(spec_str).concretized()
-        assert s.package.installed is expect_installed
+        assert s.installed is expect_installed
         assert s.satisfies(spec_str, strict=True)
 
     @pytest.mark.regression('26721,19736')

lib/spack/spack/test/database.py

@@ -726,11 +726,11 @@ def test_regression_issue_8036(mutable_database, usr_folder_exists):
     # do_install.
     s = spack.spec.Spec('externaltool@0.9')
     s.concretize()
-    assert not s.package.installed
+    assert not s.installed
 
     # Now install the external package and check again the `installed` property
     s.package.do_install(fake=True)
-    assert s.package.installed
+    assert s.installed
 
 
 @pytest.mark.regression('11118')
@@ -761,7 +761,7 @@ def test_old_external_entries_prefix(mutable_database):
 def test_uninstall_by_spec(mutable_database):
     with mutable_database.write_transaction():
         for spec in mutable_database.query():
-            if spec.package.installed:
+            if spec.installed:
                 spack.package.PackageBase.uninstall_by_spec(spec, force=True)
             else:
                 mutable_database.remove(spec)
@@ -1023,3 +1023,19 @@ def test_consistency_of_dependents_upon_remove(mutable_database):
     s = mutable_database.query_one('dyninst')
     parents = s.dependents(name='callpath')
     assert len(parents) == 2
+
+
+@pytest.mark.regression('30187')
+def test_query_installed_when_package_unknown(database):
+    """Test that we can query the installation status of a spec
+    when we don't know its package.py
+    """
+    with spack.repo.use_repositories(MockPackageMultiRepo()):
+        specs = database.query('mpileaks')
+        for s in specs:
+            # Assert that we can query the installation methods even though we
+            # don't have the package.py available
+            assert s.installed
+            assert not s.installed_upstream
+            with pytest.raises(spack.repo.UnknownNamespaceError):
+                s.package

lib/spack/spack/test/install.py

@@ -119,7 +119,7 @@ def test_partial_install_delete_prefix_and_stage(install_mockery, mock_fetch):
         pkg.do_install(restage=True)
         assert rm_prefix_checker.removed
         assert pkg.stage.test_destroyed
-        assert pkg.installed
+        assert pkg.spec.installed
 
     finally:
         pkg.remove_prefix = instance_rm_prefix
@@ -146,7 +146,7 @@ def test_failing_overwrite_install_should_keep_previous_installation(
     with pytest.raises(Exception):
         pkg.do_install(**kwargs)
 
-    assert pkg.installed
+    assert pkg.spec.installed
     assert os.path.exists(spec.prefix)
 
 
@@ -301,7 +301,7 @@ def test_installed_upstream(install_upstream, mock_fetch):
         dependent = spack.spec.Spec('dependent-install').concretized()
 
         new_dependency = dependent['dependency-install']
-        assert new_dependency.package.installed_upstream
+        assert new_dependency.installed_upstream
         assert (new_dependency.prefix ==
                 upstream_layout.path_for_spec(dependency))
 
@@ -333,7 +333,7 @@ def test_partial_install_keep_prefix(install_mockery, mock_fetch, monkeypatch):
     pkg.succeed = True   # make the build succeed
     pkg.stage = MockStage(pkg.stage)
     pkg.do_install(keep_prefix=True)
-    assert pkg.installed
+    assert pkg.spec.installed
     assert not pkg.stage.test_destroyed
 
 
@@ -344,7 +344,7 @@ def test_second_install_no_overwrite_first(install_mockery, mock_fetch, monkeypa
 
     pkg.succeed = True
     pkg.do_install()
-    assert pkg.installed
+    assert pkg.spec.installed
 
     # If Package.install is called after this point, it will fail
     pkg.succeed = False

lib/spack/spack/test/installer.py

@@ -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.package._installed_upstream = True
+    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.package._installed_upstream = False
+    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)
@@ -638,7 +638,7 @@ def test_check_deps_status_upstream(install_mockery, monkeypatch):
     request = installer.build_requests[0]
 
     # Mock the known dependent, b, as installed upstream
-    monkeypatch.setattr(spack.package.PackageBase, 'installed_upstream', True)
+    monkeypatch.setattr(spack.spec.Spec, 'installed_upstream', True)
     installer._check_deps_status(request)
     assert list(installer.installed)[0].startswith('b')
 

lib/spack/spack/test/modules/common.py

@@ -2,8 +2,6 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
-
-import collections
 import os
 import stat
 import sys
@@ -208,9 +206,7 @@ def test_get_module_upstream():
     )
     upstream_index = UpstreamModuleIndex(mock_db, module_indices)
 
-    MockPackage = collections.namedtuple('MockPackage', ['installed_upstream'])
+    setattr(s1, "installed_upstream", True)
-    setattr(s1, "package", MockPackage(True))
-
     try:
         old_index = spack.modules.common.upstream_module_index
         spack.modules.common.upstream_module_index = upstream_index

lib/spack/spack/test/spec_dag.py

@@ -84,7 +84,7 @@ def test_test_deptype():
 
 
 @pytest.mark.usefixtures('config')
-def test_installed_deps():
+def test_installed_deps(monkeypatch):
     """Preinstall a package P with a constrained build dependency D, then
     concretize a dependent package which also depends on P and D, specifying
     that the installed instance of P should be used. In this case, D should
@@ -120,9 +120,12 @@ def test_installed_deps():
         assert c_spec['d'].version == spack.version.Version('2')
 
         c_installed = spack.spec.Spec.from_dict(c_spec.to_dict())
-        for spec in c_installed.traverse():
+        installed_names = [s.name for s in c_installed.traverse()]
-            setattr(spec.package, 'installed', True)
 
+        def _mock_installed(self):
+            return self.name in installed_names
+
+        monkeypatch.setattr(Spec, 'installed', _mock_installed)
         a_spec = Spec('a')
         a_spec._add_dependency(c_installed, default)
         a_spec.concretize()

lib/spack/spack/test/test_activations.py

@@ -432,8 +432,7 @@ def test_is_activated_upstream_extendee(tmpdir, builtin_and_mock_packages,
     # Set the prefix on the package's spec reference because that is a copy of
     # the original spec
     extendee_spec.package.spec.prefix = python_prefix
-    monkeypatch.setattr(extendee_spec.package.__class__,
+    monkeypatch.setattr(extendee_spec.__class__, 'installed_upstream', True)
-                        'installed_upstream', True)
 
     ext_name = 'py-extension1'
     tmpdir.ensure(ext_name, dir=True)

lib/spack/spack/util/mock_package.py

@@ -32,7 +32,6 @@ def __init__(self, dependencies, dependency_types,
 
         """
         self.spec = None
-        self._installed_upstream = False
 
     def provides(self, vname):
         return vname in self.provided