Spec.from_detection now accounts for external prefix (#46063)

Change the signature of Spec.from_detection to set the
external prefix, and the external modules, if they are
present.

Delete "spack.package_prefs.spec_externals" since it
is unused.

9 files changed, 43 insertions, 74 deletions

diff --git a/lib/spack/spack/detection/common.py b/lib/spack/spack/detection/common.py
index 596d2ccd7e..eda7fda528 100644
--- a/lib/spack/spack/detection/common.py
+++ b/lib/spack/spack/detection/common.py
@@ -45,7 +45,9 @@ class DetectedPackage(NamedTuple):
     def restore(
         spec_str: str, prefix: str, extra_attributes: Optional[Dict[str, str]]
     ) -> "DetectedPackage":
-        spec = spack.spec.Spec.from_detection(spec_str=spec_str, extra_attributes=extra_attributes)
+        spec = spack.spec.Spec.from_detection(
+            spec_str=spec_str, external_path=prefix, extra_attributes=extra_attributes
+        )
         return DetectedPackage(spec=spec, prefix=prefix)
 
 
diff --git a/lib/spack/spack/detection/test.py b/lib/spack/spack/detection/test.py
index 353a0a5660..c2f0a5394b 100644
--- a/lib/spack/spack/detection/test.py
+++ b/lib/spack/spack/detection/test.py
@@ -104,7 +104,9 @@ class Runner:
     @property
     def expected_specs(self) -> List[spack.spec.Spec]:
         return [
-            spack.spec.Spec.from_detection(item.spec, extra_attributes=item.extra_attributes)
+            spack.spec.Spec.from_detection(
+                item.spec, external_path=self.tmpdir.name, extra_attributes=item.extra_attributes
+            )
             for item in self.test.results
         ]
 
diff --git a/lib/spack/spack/package_base.py b/lib/spack/spack/package_base.py
index 3fa4a86143..40f71c4e9f 100644
--- a/lib/spack/spack/package_base.py
+++ b/lib/spack/spack/package_base.py
@@ -246,10 +246,7 @@ class DetectablePackageMeta(type):
                         if version_str:
                             objs_by_version[version_str].append(obj)
                     except Exception as e:
-                        msg = (
-                            "An error occurred when trying to detect " 'the version of "{0}" [{1}]'
-                        )
-                        tty.debug(msg.format(obj, str(e)))
+                        tty.debug(f"Cannot detect the version of '{obj}' [{str(e)}]")
 
                 specs = []
                 for version_str, objs in objs_by_version.items():
@@ -262,27 +259,23 @@ class DetectablePackageMeta(type):
                         if isinstance(variant, str):
                             variant = (variant, {})
                         variant_str, extra_attributes = variant
-                        spec_str = "{0}@{1} {2}".format(cls.name, version_str, variant_str)
+                        spec_str = f"{cls.name}@{version_str} {variant_str}"
 
                         # Pop a few reserved keys from extra attributes, since
                         # they have a different semantics
                         external_path = extra_attributes.pop("prefix", None)
                         external_modules = extra_attributes.pop("modules", None)
                         try:
-                            spec = spack.spec.Spec(
+                            spec = spack.spec.Spec.from_detection(
                                 spec_str,
                                 external_path=external_path,
                                 external_modules=external_modules,
+                                extra_attributes=extra_attributes,
                             )
                         except Exception as e:
-                            msg = 'Parsing failed [spec_str="{0}", error={1}]'
-                            tty.debug(msg.format(spec_str, str(e)))
+                            tty.debug(f'Parsing failed [spec_str="{spec_str}", error={str(e)}]')
                         else:
-                            specs.append(
-                                spack.spec.Spec.from_detection(
-                                    spec, extra_attributes=extra_attributes
-                                )
-                            )
+                            specs.append(spec)
 
                 return sorted(specs)
 
diff --git a/lib/spack/spack/package_prefs.py b/lib/spack/spack/package_prefs.py
index 55cad78392..2c8644e0b2 100644
--- a/lib/spack/spack/package_prefs.py
+++ b/lib/spack/spack/package_prefs.py
@@ -10,7 +10,6 @@ import spack.error
 import spack.repo
 import spack.spec
 from spack.config import ConfigError
-from spack.util.path import canonicalize_path
 from spack.version import Version
 
 _lesser_spec_types = {"compiler": spack.spec.CompilerSpec, "version": Version}
@@ -156,44 +155,6 @@ class PackagePrefs:
         )
 
 
-def spec_externals(spec):
-    """Return a list of external specs (w/external directory path filled in),
-    one for each known external installation.
-    """
-    # break circular import.
-    from spack.util.module_cmd import path_from_modules  # noqa: F401
-
-    def _package(maybe_abstract_spec):
-        pkg_cls = spack.repo.PATH.get_pkg_class(spec.name)
-        return pkg_cls(maybe_abstract_spec)
-
-    allpkgs = spack.config.get("packages")
-    names = set([spec.name])
-    names |= set(vspec.name for vspec in _package(spec).virtuals_provided)
-
-    external_specs = []
-    for name in names:
-        pkg_config = allpkgs.get(name, {})
-        pkg_externals = pkg_config.get("externals", [])
-        for entry in pkg_externals:
-            spec_str = entry["spec"]
-            external_path = entry.get("prefix", None)
-            if external_path:
-                external_path = canonicalize_path(external_path)
-            external_modules = entry.get("modules", None)
-            external_spec = spack.spec.Spec.from_detection(
-                spack.spec.Spec(
-                    spec_str, external_path=external_path, external_modules=external_modules
-                ),
-                extra_attributes=entry.get("extra_attributes", {}),
-            )
-            if external_spec.intersects(spec):
-                external_specs.append(external_spec)
-
-    # Defensively copy returned specs
-    return [s.copy() for s in external_specs]
-
-
 def is_spec_buildable(spec):
     """Return true if the spec is configured as buildable"""
     allpkgs = spack.config.get("packages")
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 5e0abcf20c..1fb21b348b 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -2578,18 +2578,23 @@ class Spec:
         return Spec.from_dict(extracted_json)
 
     @staticmethod
-    def from_detection(spec_str, extra_attributes=None):
+    def from_detection(
+        spec_str: str,
+        *,
+        external_path: str,
+        external_modules: Optional[List[str]] = None,
+        extra_attributes: Optional[Dict] = None,
+    ) -> "Spec":
         """Construct a spec from a spec string determined during external
         detection and attach extra attributes to it.
 
         Args:
-            spec_str (str): spec string
-            extra_attributes (dict): dictionary containing extra attributes
-
-        Returns:
-            spack.spec.Spec: external spec
+            spec_str: spec string
+            external_path: prefix of the external spec
+            external_modules: optional module files to be loaded when the external spec is used
+            extra_attributes: dictionary containing extra attributes
         """
-        s = Spec(spec_str)
+        s = Spec(spec_str, external_path=external_path, external_modules=external_modules)
         extra_attributes = syaml.sorted_dict(extra_attributes or {})
         # This is needed to be able to validate multi-valued variants,
         # otherwise they'll still be abstract in the context of detection.
diff --git a/lib/spack/spack/test/cmd/external.py b/lib/spack/spack/test/cmd/external.py
index 86e6f41389..a55b621de7 100644
--- a/lib/spack/spack/test/cmd/external.py
+++ b/lib/spack/spack/test/cmd/external.py
@@ -72,8 +72,12 @@ def test_find_external_two_instances_same_package(mock_executable):
 
 def test_find_external_update_config(mutable_config):
     entries = [
-        spack.detection.DetectedPackage(Spec.from_detection("cmake@1.foo"), "/x/y1/"),
-        spack.detection.DetectedPackage(Spec.from_detection("cmake@3.17.2"), "/x/y2/"),
+        spack.detection.DetectedPackage(
+            Spec.from_detection("cmake@1.foo", external_path="/x/y1/"), "/x/y1/"
+        ),
+        spack.detection.DetectedPackage(
+            Spec.from_detection("cmake@3.17.2", external_path="/x/y2/"), "/x/y2/"
+        ),
     ]
     pkg_to_entries = {"cmake": entries}
 
@@ -221,10 +225,8 @@ def test_find_external_manifest_failure(mutable_config, mutable_mock_repo, tmpdi
     assert "Skipping manifest and continuing" in output
 
 
-def test_find_external_merge(mutable_config, mutable_mock_repo):
-    """Check that 'spack find external' doesn't overwrite an existing spec
-    entry in packages.yaml.
-    """
+def test_find_external_merge(mutable_config, mutable_mock_repo, tmp_path):
+    """Checks that 'spack find external' doesn't overwrite an existing spec in packages.yaml."""
     pkgs_cfg_init = {
         "find-externals1": {
             "externals": [{"spec": "find-externals1@1.1", "prefix": "/preexisting-prefix/"}],
@@ -234,8 +236,12 @@ def test_find_external_merge(mutable_config, mutable_mock_repo):
 
     mutable_config.update_config("packages", pkgs_cfg_init)
     entries = [
-        spack.detection.DetectedPackage(Spec.from_detection("find-externals1@1.1"), "/x/y1/"),
-        spack.detection.DetectedPackage(Spec.from_detection("find-externals1@1.2"), "/x/y2/"),
+        spack.detection.DetectedPackage(
+            Spec.from_detection("find-externals1@1.1", external_path="/x/y1/"), "/x/y1/"
+        ),
+        spack.detection.DetectedPackage(
+            Spec.from_detection("find-externals1@1.2", external_path="/x/y2/"), "/x/y2/"
+        ),
     ]
     pkg_to_entries = {"find-externals1": entries}
     scope = spack.config.default_modify_scope("packages")
diff --git a/var/spack/repos/builtin.mock/packages/find-externals1/package.py b/var/spack/repos/builtin.mock/packages/find-externals1/package.py
index 07d16a8f22..b8adbac19c 100644
--- a/var/spack/repos/builtin.mock/packages/find-externals1/package.py
+++ b/var/spack/repos/builtin.mock/packages/find-externals1/package.py
@@ -32,4 +32,4 @@ class FindExternals1(AutotoolsPackage):
             match = re.search(r"find-externals1.*version\s+(\S+)", output)
             if match:
                 version_str = match.group(1)
-                return Spec.from_detection("find-externals1@{0}".format(version_str))
+                return Spec.from_detection(f"find-externals1@{version_str}", external_path=prefix)
diff --git a/var/spack/repos/builtin/packages/lhapdfsets/package.py b/var/spack/repos/builtin/packages/lhapdfsets/package.py
index deb654c067..2dbe00a9a9 100644
--- a/var/spack/repos/builtin/packages/lhapdfsets/package.py
+++ b/var/spack/repos/builtin/packages/lhapdfsets/package.py
@@ -74,8 +74,8 @@ class Lhapdfsets(BundlePackage):
     @classmethod
     def determine_spec_details(cls, prefix, exes_in_prefix):
         path = os.environ.get("LHAPDF_DATA_PATH", None)
+        if not path:
+            return None
         # unfortunately the sets are not versioned -
         # just hardcode the current version and hope it is fine
-        s = Spec.from_detection("lhapdfsets@6.3.0")
-        s.external_path = path
-        return s if path else None
+        return Spec.from_detection("lhapdfsets@6.3.0", external_path=path)
diff --git a/var/spack/repos/builtin/packages/rust/package.py b/var/spack/repos/builtin/packages/rust/package.py
index 230d64ca05..b715573f1c 100644
--- a/var/spack/repos/builtin/packages/rust/package.py
+++ b/var/spack/repos/builtin/packages/rust/package.py
@@ -113,7 +113,7 @@ class Rust(Package):
         match = re.match(r"rustc (\S+)", output)
         if match:
             version_str = match.group(1)
-            return Spec.from_detection(f"rust@{version_str}")
+            return Spec.from_detection(f"rust@{version_str}", external_path=prefix)
 
     def setup_dependent_package(self, module, dependent_spec):
         module.cargo = Executable(os.path.join(self.spec.prefix.bin, "cargo"))