Spec.satisfies, Spec.intersects: drop spack.repo

Currently Spec.satisfies is stateful both for concrete and abstract
specs, even after our changes where concrete specs list virtuals as edge
attributes.

Virtuals on edges basically provide too little information:

1. They don't specify what version of the virtual is provided, so you
   cannot know the return value of `s.satisfies("^mpi@3:")` without
   looking in the current state of the package.
2. It's not all about edges: `mpich.satisfies("mpi")` is expected to be
   true (difference is *can* provide versus *provides*).

This commit tries to reduce the statefulness by only letting satisfies
and intersects access `spec.package`, instead of looking up the package.
The idea is that it reduces the surface area with `spack.repo`, as in
principle `spec.package` can be set upon construction.

The commit is breaking for abstract specs. The idea is you cannot answer
queries about abstract specs unless you associate a repo.

lib/spack/spack/cmd/repo.py

@@ -8,6 +8,7 @@
 import llnl.util.tty as tty
 
 import spack.config
+import spack.error
 import spack.repo
 import spack.util.path
 from spack.cmd.common import arguments
@@ -129,7 +130,7 @@ def repo_remove(args):
                 spack.config.set("repos", repos, args.scope)
                 tty.msg("Removed repository %s with namespace '%s'." % (repo.root, repo.namespace))
                 return
-        except spack.repo.RepoError:
+        except spack.error.RepoError:
             continue
 
     tty.die("No repository with path or namespace: %s" % namespace_or_path)
@@ -142,7 +143,7 @@ def repo_list(args):
     for r in roots:
         try:
             repos.append(spack.repo.from_path(r))
-        except spack.repo.RepoError:
+        except spack.error.RepoError:
             continue
 
     if sys.stdout.isatty():

lib/spack/spack/error.py

@@ -202,3 +202,11 @@ class MirrorError(SpackError):
 
     def __init__(self, msg, long_msg=None):
         super().__init__(msg, long_msg)
+
+
+class RepoError(SpackError):
+    """Superclass for repository-related errors."""
+
+
+class UnknownEntityError(RepoError):
+    """Raised when we encounter a package spack doesn't have."""

lib/spack/spack/installer.py

@@ -560,7 +560,7 @@ def dump_packages(spec: "spack.spec.Spec", path: str) -> None:
             try:
                 source_repo = spack.repo.from_path(source_repo_root)
                 source_pkg_dir = source_repo.dirname_for_package_name(node.name)
-            except spack.repo.RepoError as err:
+            except spack.error.RepoError as err:
                 tty.debug(f"Failed to create source repo for {node.name}: {str(err)}")
                 source_pkg_dir = None
                 tty.warn(f"Warning: Couldn't copy in provenance for {node.name}")

lib/spack/spack/package_base.py

@@ -2109,7 +2109,7 @@ def uninstall_by_spec(spec, force=False, deprecator=None):
         # Try to get the package for the spec
         try:
             pkg = spec.package
-        except spack.repo.UnknownEntityError:
+        except spack.error.UnknownEntityError:
             pkg = None
 
         # Pre-uninstall hook runs first.

lib/spack/spack/repo.py

@@ -663,7 +663,7 @@ def __init__(
                     repo = Repo(repo, cache=cache, overrides=overrides)
                 repo.finder(self)
                 self.put_last(repo)
-            except RepoError as e:
+            except spack.error.RepoError as e:
                 tty.warn(
                     f"Failed to initialize repository: '{repo}'.",
                     e.message,
@@ -1241,7 +1241,7 @@ def get_pkg_class(self, pkg_name: str) -> Type["spack.package_base.PackageBase"]
             raise UnknownPackageError(fullname)
         except Exception as e:
             msg = f"cannot load package '{pkg_name}' from the '{self.namespace}' repository: {e}"
-            raise RepoError(msg) from e
+            raise spack.error.RepoError(msg) from e
 
         cls = getattr(module, class_name)
         if not isinstance(cls, type):
@@ -1501,27 +1501,19 @@ def recipe_filename(self, name):
         return os.path.join(self.root, "packages", name, "package.py")
 
 
-class RepoError(spack.error.SpackError):
+class NoRepoConfiguredError(spack.error.RepoError):
-    """Superclass for repository-related errors."""
-
-
-class NoRepoConfiguredError(RepoError):
     """Raised when there are no repositories configured."""
 
 
-class InvalidNamespaceError(RepoError):
+class InvalidNamespaceError(spack.error.RepoError):
     """Raised when an invalid namespace is encountered."""
 
 
-class BadRepoError(RepoError):
+class BadRepoError(spack.error.RepoError):
     """Raised when repo layout is invalid."""
 
 
-class UnknownEntityError(RepoError):
+class UnknownPackageError(spack.error.UnknownEntityError):
-    """Raised when we encounter a package spack doesn't have."""
-
-
-class UnknownPackageError(UnknownEntityError):
     """Raised when we encounter a package spack doesn't have."""
 
     def __init__(self, name, repo=None):
@@ -1558,7 +1550,7 @@ def __init__(self, name, repo=None):
         self.name = name
 
 
-class UnknownNamespaceError(UnknownEntityError):
+class UnknownNamespaceError(spack.error.UnknownEntityError):
     """Raised when we encounter an unknown namespace"""
 
     def __init__(self, namespace, name=None):
@@ -1568,7 +1560,7 @@ def __init__(self, namespace, name=None):
         super().__init__(msg, long_msg)
 
 
-class FailedConstructorError(RepoError):
+class FailedConstructorError(spack.error.RepoError):
     """Raised when a package's class constructor fails."""
 
     def __init__(self, name, exc_type, exc_obj, exc_tb):

lib/spack/spack/solver/asp.py

@@ -3831,7 +3831,7 @@ def _is_reusable(spec: spack.spec.Spec, packages, local: bool) -> bool:
 
     try:
         provided = spack.repo.PATH.get(spec).provided_virtual_names()
-    except spack.repo.RepoError:
+    except spack.error.RepoError:
         provided = []
 
     for name in {spec.name, *provided}:

lib/spack/spack/spec.py

@@ -91,7 +91,6 @@
 import spack.hash_types as ht
 import spack.paths
 import spack.platforms
-import spack.provider_index
 import spack.repo
 import spack.spec_parser
 import spack.store
@@ -1893,9 +1892,7 @@ def root(self):
 
     @property
     def package(self):
-        assert self.concrete, "{0}: Spec.package can only be called on concrete specs".format(
+        assert self.concrete, f"{self.name}: Spec.package can only be called on concrete specs"
-            self.name
-        )
         if not self._package:
             self._package = spack.repo.PATH.get(self)
         return self._package
@@ -3267,7 +3264,7 @@ def _constrain_dependencies(self, other):
 
     def common_dependencies(self, other):
         """Return names of dependencies that self an other have in common."""
-        common = set(s.name for s in self.traverse(root=False))
+        common = {s.name for s in self.traverse(root=False)}
         common.intersection_update(s.name for s in other.traverse(root=False))
         return common
 
@@ -3315,7 +3312,7 @@ def intersects(self, other: Union[str, "Spec"], deps: bool = True) -> bool:
         elif other.concrete:
             return other.satisfies(self)
 
-        # From here we know both self and other are not concrete
+        # From here we know both self and other are abstract
         self_hash = self.abstract_hash
         other_hash = other.abstract_hash
 
@@ -3326,32 +3323,8 @@ def intersects(self, other: Union[str, "Spec"], deps: bool = True) -> bool:
         ):
             return False
 
-        # If the names are different, we need to consider virtuals
+        # We do not consider virtuals for two abstract specs, since no package is associated
         if self.name != other.name and self.name and other.name:
-            if self.virtual and other.virtual:
-                # Two virtual specs intersect only if there are providers for both
-                lhs = spack.repo.PATH.providers_for(str(self))
-                rhs = spack.repo.PATH.providers_for(str(other))
-                intersection = [s for s in lhs if any(s.intersects(z) for z in rhs)]
-                return bool(intersection)
-
-            # A provider can satisfy a virtual dependency.
-            elif self.virtual or other.virtual:
-                virtual_spec, non_virtual_spec = (self, other) if self.virtual else (other, self)
-                try:
-                    # Here we might get an abstract spec
-                    pkg_cls = spack.repo.PATH.get_pkg_class(non_virtual_spec.fullname)
-                    pkg = pkg_cls(non_virtual_spec)
-                except spack.repo.UnknownEntityError:
-                    # If we can't get package info on this spec, don't treat
-                    # it as a provider of this vdep.
-                    return False
-
-                if pkg.provides(virtual_spec.name):
-                    for when_spec, provided in pkg.provided.items():
-                        if non_virtual_spec.intersects(when_spec, deps=False):
-                            if any(vpkg.intersects(virtual_spec) for vpkg in provided):
-                                return True
             return False
 
         # namespaces either match, or other doesn't require one.
@@ -3381,40 +3354,16 @@ def intersects(self, other: Union[str, "Spec"], deps: bool = True) -> bool:
             return False
 
         # If we need to descend into dependencies, do it, otherwise we're done.
-        if deps:
+        if not deps or not self._dependencies or not other._dependencies:
+            return True
+
         return self._intersects_dependencies(other)
 
-        return True
+    def _intersects_dependencies(self, other: "Spec") -> bool:
-
-    def _intersects_dependencies(self, other):
-        if not other._dependencies or not self._dependencies:
-            # one spec *could* eventually satisfy the other
-            return True
-
-        # Handle first-order constraints directly
         for name in self.common_dependencies(other):
             if not self[name].intersects(other[name], deps=False):
                 return False
 
-        # For virtual dependencies, we need to dig a little deeper.
-        self_index = spack.provider_index.ProviderIndex(
-            repository=spack.repo.PATH, specs=self.traverse(), restrict=True
-        )
-        other_index = spack.provider_index.ProviderIndex(
-            repository=spack.repo.PATH, specs=other.traverse(), restrict=True
-        )
-
-        # These two loops handle cases where there is an overly restrictive
-        # vpkg in one spec for a provider in the other (e.g., mpi@3: is not
-        # compatible with mpich2)
-        for spec in self.virtual_dependencies():
-            if spec.name in other_index and not other_index.providers_for(spec):
-                return False
-
-        for spec in other.virtual_dependencies():
-            if spec.name in self_index and not self_index.providers_for(spec):
-                return False
-
         return True
 
     def satisfies(self, other: Union[str, "Spec"], deps: bool = True) -> bool:
@@ -3439,24 +3388,22 @@ def satisfies(self, other: Union[str, "Spec"], deps: bool = True) -> bool:
             if not compare_hash or not compare_hash.startswith(other.abstract_hash):
                 return False
 
-        # If the names are different, we need to consider virtuals
+        # If the names are different, we need to consider virtuals. We return false for two
-        if self.name != other.name and self.name and other.name:
+        # abstract specs with different names because no package is associated with them.
-            # A concrete provider can satisfy a virtual dependency.
+        if other.name and self.name != other.name:
-            if not self.virtual and other.virtual:
             try:
-                    # Here we might get an abstract spec
+                pkg = self.package
-                    pkg_cls = spack.repo.PATH.get_pkg_class(self.fullname)
+            except (AssertionError, spack.error.RepoError):
-                    pkg = pkg_cls(self)
+                # If we can't get package info on this spec, don't treat it as a provider of this
-                except spack.repo.UnknownEntityError:
+                # vdep.
-                    # If we can't get package info on this spec, don't treat
-                    # it as a provider of this vdep.
                 return False
 
             if pkg.provides(other.name):
-                    for when_spec, provided in pkg.provided.items():
+                for when, provided in pkg.provided.items():
-                        if self.satisfies(when_spec, deps=False):
+                    if self.satisfies(when, deps=False):
-                            if any(vpkg.intersects(other) for vpkg in provided):
+                        if any(p.intersects(other) for p in provided):
                             return True
+
             return False
 
         # namespaces either match, or other doesn't require one.
@@ -3509,10 +3456,6 @@ def satisfies(self, other: Union[str, "Spec"], deps: bool = True) -> bool:
         # will be verified later.
         lhs_edges: Dict[str, Set[DependencySpec]] = collections.defaultdict(set)
         for rhs_edge in other.traverse_edges(root=False, cover="edges"):
-            # If we are checking for ^mpi we need to verify if there is any edge
-            if rhs_edge.spec.virtual:
-                rhs_edge.update_virtuals(virtuals=(rhs_edge.spec.name,))
-
             if not rhs_edge.virtuals:
                 continue
 
@@ -3554,10 +3497,6 @@ def satisfies(self, other: Union[str, "Spec"], deps: bool = True) -> bool:
             for rhs in other.traverse(root=False)
         )
 
-    def virtual_dependencies(self):
-        """Return list of any virtual deps in this spec."""
-        return [spec for spec in self.traverse() if spec.virtual]
-
     @property  # type: ignore[misc] # decorated prop not supported in mypy
     def patches(self):
         """Return patch objects for any patch sha256 sums on this Spec.

lib/spack/spack/test/installer.py

@@ -457,7 +457,7 @@ def bpp_path(spec):
 
     def _repoerr(repo, name):
         if name == "cmake":
-            raise spack.repo.RepoError(repo_err_msg)
+            raise spack.error.RepoError(repo_err_msg)
         else:
             return orig_dirname(repo, name)
 
@@ -476,7 +476,7 @@ def _repoerr(repo, name):
     # Now try the error path, which requires the mock directory structure
     # above
     monkeypatch.setattr(spack.repo.Repo, "dirname_for_package_name", _repoerr)
-    with pytest.raises(spack.repo.RepoError, match=repo_err_msg):
+    with pytest.raises(spack.error.RepoError, match=repo_err_msg):
         inst.dump_packages(spec, path)
 
     out = str(capsys.readouterr()[1])

lib/spack/spack/test/spec_semantics.py

@@ -472,9 +472,6 @@ def test_concrete_specs_which_satisfies_abstract(self, lhs, rhs, default_mock_co
             ("mpileaks^mpich@2.0^callpath@1.7", "^mpich@1:3^callpath@1.4:1.6"),
             ("mpileaks^mpich@4.0^callpath@1.7", "^mpich@1:3^callpath@1.4:1.6"),
             ("mpileaks^mpi@3", "^mpi@1.2:1.6"),
-            ("mpileaks^mpi@3:", "^mpich2@1.4"),
-            ("mpileaks^mpi@3:", "^mpich2"),
-            ("mpileaks^mpi@3:", "^mpich@1.0"),
             ("mpich~foo", "mpich+foo"),
             ("mpich+foo", "mpich~foo"),
             ("mpich foo=True", "mpich foo=False"),
@@ -705,9 +702,9 @@ def test_copy_satisfies_transitive(self):
             assert copy[s.name].satisfies(s)
 
     def test_intersects_virtual(self):
-        assert Spec("mpich").intersects(Spec("mpi"))
+        assert spack.concretize.concretize_one("mpich").intersects("mpi")
-        assert Spec("mpich2").intersects(Spec("mpi"))
+        assert spack.concretize.concretize_one("mpich2").intersects("mpi")
-        assert Spec("zmpi").intersects(Spec("mpi"))
+        assert spack.concretize.concretize_one("zmpi").intersects("mpi")
 
     def test_intersects_virtual_providers(self):
         """Tests that we can always intersect virtual providers from abstract specs.
@@ -1829,9 +1826,6 @@ def test_abstract_contains_semantic(lhs, rhs, expected, mock_packages):
         (Spec, "cppflags=-foo", "cflags=-foo", (True, False, False)),
         # Versions
         (Spec, "@0.94h", "@:0.94i", (True, True, False)),
-        # Different virtuals intersect if there is at least package providing both
-        (Spec, "mpi", "lapack", (True, False, False)),
-        (Spec, "mpi", "pkgconfig", (False, False, False)),
         # Intersection among target ranges for different architectures
         (Spec, "target=x86_64:", "target=ppc64le:", (False, False, False)),
         (Spec, "target=x86_64:", "target=:power9", (False, False, False)),

lib/spack/spack/test/spec_syntax.py

@@ -11,6 +11,7 @@
 import spack.binary_distribution
 import spack.cmd
 import spack.concretize
+import spack.error
 import spack.platforms.test
 import spack.repo
 import spack.spec
@@ -1139,7 +1140,7 @@ def test_parse_filename_missing_slash_as_spec(specfile_for, tmpdir, filename):
 
     # Check that if we concretize this spec, we get a good error
     # message that mentions we might've meant a file.
-    with pytest.raises(spack.repo.UnknownEntityError) as exc_info:
+    with pytest.raises(spack.error.UnknownEntityError) as exc_info:
         spack.concretize.concretize_one(spec)
     assert exc_info.value.long_message
     assert (

lib/spack/spack/version/git_ref_lookup.py

@@ -10,6 +10,7 @@
 from llnl.util.filesystem import mkdirp, working_dir
 
 import spack.caches
+import spack.error
 import spack.fetch_strategy
 import spack.paths
 import spack.repo
@@ -78,7 +79,7 @@ def pkg(self):
             try:
                 pkg = spack.repo.PATH.get_pkg_class(self.pkg_name)
                 pkg.git
-            except (spack.repo.RepoError, AttributeError) as e:
+            except (spack.error.RepoError, AttributeError) as e:
                 raise VersionLookupError(f"Couldn't get the git repo for {self.pkg_name}") from e
             self._pkg = pkg
         return self._pkg