zhangyiss/spack
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Remove DetectedPackage class (#46071)

Browse Source 
This PR simplifies the code doing external spec detection by removing 
the `DetectedPackage` class. Now, functions accepting or returning lists 
of `DetectedPackage`, will accept or return list of specs.

Performance doesn't seem to change if we use `Spec.__reduce__` instead 
of `DetectionPackage.__reduce__`.
This commit is contained in:
Massimiliano Culpo 2024-08-30 10:11:17 +02:00 committed by GitHub
parent 199cbce5ef
commit c283fce487
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 55 additions and 95 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
lib/spack/spack/build_systems/python.py
View File

@ -315,9 +315,9 @@ def get_external_python_for_prefix(self):
) )
python_externals_detected = [ python_externals_detected = [
d.spec spec
for d in python_externals_detection.get("python", []) for spec in python_externals_detection.get("python", [])
if d.prefix == self.spec.external_path if spec.external_path == self.spec.external_path
] ]
if python_externals_detected: if python_externals_detected:
return python_externals_detected[0] return python_externals_detected[0]

14
lib/spack/spack/compilers/__init__.py
View File

@ -273,24 +273,24 @@ def find_compilers(
valid_compilers = {} valid_compilers = {}
for name, detected in detected_packages.items(): for name, detected in detected_packages.items():
compilers = [x for x in detected if CompilerConfigFactory.from_external_spec(x.spec)] compilers = [x for x in detected if CompilerConfigFactory.from_external_spec(x)]
if not compilers: if not compilers:
continue continue
valid_compilers[name] = compilers valid_compilers[name] = compilers
def _has_fortran_compilers(x): def _has_fortran_compilers(x):
if "compilers" not in x.spec.extra_attributes: if "compilers" not in x.extra_attributes:
return False return False
return "fortran" in x.spec.extra_attributes["compilers"] return "fortran" in x.extra_attributes["compilers"]
if mixed_toolchain: if mixed_toolchain:
gccs = [x for x in valid_compilers.get("gcc", []) if _has_fortran_compilers(x)] gccs = [x for x in valid_compilers.get("gcc", []) if _has_fortran_compilers(x)]
if gccs: if gccs:
best_gcc = sorted( best_gcc = sorted(
gccs, key=lambda x: spack.spec.parse_with_version_concrete(x.spec).version gccs, key=lambda x: spack.spec.parse_with_version_concrete(x).version
)[-1] )[-1]
gfortran = best_gcc.spec.extra_attributes["compilers"]["fortran"] gfortran = best_gcc.extra_attributes["compilers"]["fortran"]
for name in ("llvm", "apple-clang"): for name in ("llvm", "apple-clang"):
if name not in valid_compilers: if name not in valid_compilers:
continue continue
@ -298,11 +298,11 @@ def _has_fortran_compilers(x):
for candidate in candidates: for candidate in candidates:
if _has_fortran_compilers(candidate): if _has_fortran_compilers(candidate):
continue continue
candidate.spec.extra_attributes["compilers"]["fortran"] = gfortran candidate.extra_attributes["compilers"]["fortran"] = gfortran
new_compilers = [] new_compilers = []
for name, detected in valid_compilers.items(): for name, detected in valid_compilers.items():
for config in CompilerConfigFactory.from_specs([x.spec for x in detected]): for config in CompilerConfigFactory.from_specs(detected):
c = _compiler_from_config_entry(config["compiler"]) c = _compiler_from_config_entry(config["compiler"])
if c in known_compilers: if c in known_compilers:
continue continue

8
lib/spack/spack/detection/__init__.py
View File

@ -2,17 +2,11 @@
# Spack Project Developers. See the top-level COPYRIGHT file for details. # Spack Project Developers. See the top-level COPYRIGHT file for details.
# #
# SPDX-License-Identifier: (Apache-2.0 OR MIT) # SPDX-License-Identifier: (Apache-2.0 OR MIT)
from .common import ( from .common import executable_prefix, set_virtuals_nonbuildable, update_configuration
DetectedPackage,
executable_prefix,
set_virtuals_nonbuildable,
update_configuration,
)
from .path import by_path, executables_in_path from .path import by_path, executables_in_path
from .test import detection_tests from .test import detection_tests
__all__ = [ __all__ = [
"DetectedPackage",
"by_path", "by_path",
"executables_in_path", "executables_in_path",
"executable_prefix", "executable_prefix",

58
lib/spack/spack/detection/common.py
View File

@ -6,9 +6,9 @@
function to update packages.yaml given a list of detected packages. function to update packages.yaml given a list of detected packages.
Ideally, each detection method should be placed in a specific subpackage Ideally, each detection method should be placed in a specific subpackage
and implement at least a function that returns a list of DetectedPackage and implement at least a function that returns a list of specs.
objects. The update in packages.yaml can then be done using the function
provided here. The update in packages.yaml can then be done using the function provided here.
The module also contains other functions that might be useful across different The module also contains other functions that might be useful across different
detection mechanisms. detection mechanisms.
@ -17,9 +17,10 @@
import itertools import itertools
import os import os
import os.path import os.path
import pathlib
import re import re
import sys import sys
from typing import Dict, List, NamedTuple, Optional, Set, Tuple, Union from typing import Dict, List, Optional, Set, Tuple, Union
import llnl.util.tty import llnl.util.tty
@ -30,27 +31,6 @@
import spack.util.windows_registry import spack.util.windows_registry
class DetectedPackage(NamedTuple):
"""Information on a package that has been detected."""
#: Spec that was detected
spec: spack.spec.Spec
#: Prefix of the spec
prefix: str
def __reduce__(self):
return DetectedPackage.restore, (str(self.spec), self.prefix, self.spec.extra_attributes)
@staticmethod
def restore(
spec_str: str, prefix: str, extra_attributes: Optional[Dict[str, str]]
) -> "DetectedPackage":
spec = spack.spec.Spec.from_detection(
spec_str=spec_str, external_path=prefix, extra_attributes=extra_attributes
)
return DetectedPackage(spec=spec, prefix=prefix)
def _externals_in_packages_yaml() -> Set[spack.spec.Spec]: def _externals_in_packages_yaml() -> Set[spack.spec.Spec]:
"""Returns all the specs mentioned as externals in packages.yaml""" """Returns all the specs mentioned as externals in packages.yaml"""
packages_yaml = spack.config.get("packages") packages_yaml = spack.config.get("packages")
@ -65,7 +45,7 @@ def _externals_in_packages_yaml() -> Set[spack.spec.Spec]:
def _pkg_config_dict( def _pkg_config_dict(
external_pkg_entries: List[DetectedPackage], external_pkg_entries: List["spack.spec.Spec"],
) -> Dict[str, Union[bool, List[Dict[str, ExternalEntryType]]]]: ) -> Dict[str, Union[bool, List[Dict[str, ExternalEntryType]]]]:
"""Generate a package specific config dict according to the packages.yaml schema. """Generate a package specific config dict according to the packages.yaml schema.
@ -85,22 +65,19 @@ def _pkg_config_dict(
pkg_dict = spack.util.spack_yaml.syaml_dict() pkg_dict = spack.util.spack_yaml.syaml_dict()
pkg_dict["externals"] = [] pkg_dict["externals"] = []
for e in external_pkg_entries: for e in external_pkg_entries:
if not _spec_is_valid(e.spec): if not _spec_is_valid(e):
continue continue
external_items: List[Tuple[str, ExternalEntryType]] = [ external_items: List[Tuple[str, ExternalEntryType]] = [
("spec", str(e.spec)), ("spec", str(e)),
("prefix", e.prefix), ("prefix", pathlib.Path(e.external_path).as_posix()),
] ]
if e.spec.external_modules: if e.external_modules:
external_items.append(("modules", e.spec.external_modules)) external_items.append(("modules", e.external_modules))
if e.spec.extra_attributes: if e.extra_attributes:
external_items.append( external_items.append(
( ("extra_attributes", spack.util.spack_yaml.syaml_dict(e.extra_attributes.items()))
"extra_attributes",
spack.util.spack_yaml.syaml_dict(e.spec.extra_attributes.items()),
)
) )
# external_items.extend(e.spec.extra_attributes.items()) # external_items.extend(e.spec.extra_attributes.items())
@ -221,33 +198,32 @@ def library_prefix(library_dir: str) -> str:
def update_configuration( def update_configuration(
detected_packages: Dict[str, List[DetectedPackage]], detected_packages: Dict[str, List["spack.spec.Spec"]],
scope: Optional[str] = None, scope: Optional[str] = None,
buildable: bool = True, buildable: bool = True,
) -> List[spack.spec.Spec]: ) -> List[spack.spec.Spec]:
"""Add the packages passed as arguments to packages.yaml """Add the packages passed as arguments to packages.yaml
Args: Args:
detected_packages: list of DetectedPackage objects to be added detected_packages: list of specs to be added
scope: configuration scope where to add the detected packages scope: configuration scope where to add the detected packages
buildable: whether the detected packages are buildable or not buildable: whether the detected packages are buildable or not
""" """
predefined_external_specs = _externals_in_packages_yaml() predefined_external_specs = _externals_in_packages_yaml()
pkg_to_cfg, all_new_specs = {}, [] pkg_to_cfg, all_new_specs = {}, []
for package_name, entries in detected_packages.items(): for package_name, entries in detected_packages.items():
new_entries = [e for e in entries if (e.spec not in predefined_external_specs)] new_entries = [s for s in entries if s not in predefined_external_specs]
pkg_config = _pkg_config_dict(new_entries) pkg_config = _pkg_config_dict(new_entries)
external_entries = pkg_config.get("externals", []) external_entries = pkg_config.get("externals", [])
assert not isinstance(external_entries, bool), "unexpected value for external entry" assert not isinstance(external_entries, bool), "unexpected value for external entry"
all_new_specs.extend([x.spec for x in new_entries]) all_new_specs.extend(new_entries)
if buildable is False: if buildable is False:
pkg_config["buildable"] = False pkg_config["buildable"] = False
pkg_to_cfg[package_name] = pkg_config pkg_to_cfg[package_name] = pkg_config
pkgs_cfg = spack.config.get("packages", scope=scope) pkgs_cfg = spack.config.get("packages", scope=scope)
pkgs_cfg = spack.config.merge_yaml(pkgs_cfg, pkg_to_cfg) pkgs_cfg = spack.config.merge_yaml(pkgs_cfg, pkg_to_cfg)
spack.config.set("packages", pkgs_cfg, scope=scope) spack.config.set("packages", pkgs_cfg, scope=scope)

13
lib/spack/spack/detection/path.py
View File

@ -24,7 +24,6 @@
import spack.util.ld_so_conf import spack.util.ld_so_conf
from .common import ( from .common import (
DetectedPackage,
WindowsCompilerExternalPaths, WindowsCompilerExternalPaths,
WindowsKitExternalPaths, WindowsKitExternalPaths,
_convert_to_iterable, _convert_to_iterable,
@ -229,7 +228,7 @@ def prefix_from_path(self, *, path: str) -> str:
def detect_specs( def detect_specs(
self, *, pkg: Type["spack.package_base.PackageBase"], paths: List[str] self, *, pkg: Type["spack.package_base.PackageBase"], paths: List[str]
) -> List[DetectedPackage]: ) -> List["spack.spec.Spec"]:
"""Given a list of files matching the search patterns, returns a list of detected specs. """Given a list of files matching the search patterns, returns a list of detected specs.
Args: Args:
@ -295,16 +294,16 @@ def detect_specs(
warnings.warn(msg) warnings.warn(msg)
continue continue
if spec.external_path: if not spec.external_path:
prefix = spec.external_path spec.external_path = prefix
result.append(DetectedPackage(spec=spec, prefix=prefix)) result.append(spec)
return result return result
def find( def find(
self, *, pkg_name: str, repository, initial_guess: Optional[List[str]] = None self, *, pkg_name: str, repository, initial_guess: Optional[List[str]] = None
) -> List[DetectedPackage]: ) -> List["spack.spec.Spec"]:
"""For a given package, returns a list of detected specs. """For a given package, returns a list of detected specs.
Args: Args:
@ -388,7 +387,7 @@ def by_path(
*, *,
path_hints: Optional[List[str]] = None, path_hints: Optional[List[str]] = None,
max_workers: Optional[int] = None, max_workers: Optional[int] = None,
) -> Dict[str, List[DetectedPackage]]: ) -> Dict[str, List["spack.spec.Spec"]]:
"""Return the list of packages that have been detected on the system, keyed by """Return the list of packages that have been detected on the system, keyed by
unqualified package name. unqualified package name.

2
lib/spack/spack/detection/test.py
View File

@ -68,7 +68,7 @@ def execute(self) -> List[spack.spec.Spec]:
with self._mock_layout() as path_hints: with self._mock_layout() as path_hints:
entries = by_path([self.test.pkg_name], path_hints=path_hints) entries = by_path([self.test.pkg_name], path_hints=path_hints)
_, unqualified_name = spack.repo.partition_package_name(self.test.pkg_name) _, unqualified_name = spack.repo.partition_package_name(self.test.pkg_name)
specs = set(x.spec for x in entries[unqualified_name]) specs = set(entries[unqualified_name])
return list(specs) return list(specs)
@contextlib.contextmanager @contextlib.contextmanager

34
lib/spack/spack/test/cmd/external.py
View File

@ -44,7 +44,7 @@ def test_find_external_single_package(mock_executable):
assert len(specs_by_package) == 1 and "cmake" in specs_by_package assert len(specs_by_package) == 1 and "cmake" in specs_by_package
detected_spec = specs_by_package["cmake"] detected_spec = specs_by_package["cmake"]
assert len(detected_spec) == 1 and detected_spec[0].spec == Spec("cmake@1.foo") assert len(detected_spec) == 1 and detected_spec[0] == Spec("cmake@1.foo")
def test_find_external_two_instances_same_package(mock_executable): def test_find_external_two_instances_same_package(mock_executable):
@ -61,10 +61,10 @@ def test_find_external_two_instances_same_package(mock_executable):
) )
assert len(detected_specs) == 2 assert len(detected_specs) == 2
spec_to_path = {e.spec: e.prefix for e in detected_specs} spec_to_path = {s: s.external_path for s in detected_specs}
assert spec_to_path[Spec("cmake@1.foo")] == ( assert spec_to_path[Spec("cmake@1.foo")] == (
spack.detection.executable_prefix(str(cmake1.parent)) spack.detection.executable_prefix(str(cmake1.parent))
) ), spec_to_path
assert spec_to_path[Spec("cmake@3.17.2")] == ( assert spec_to_path[Spec("cmake@3.17.2")] == (
spack.detection.executable_prefix(str(cmake2.parent)) spack.detection.executable_prefix(str(cmake2.parent))
) )
@ -72,12 +72,8 @@ def test_find_external_two_instances_same_package(mock_executable):
def test_find_external_update_config(mutable_config): def test_find_external_update_config(mutable_config):
entries = [ entries = [
spack.detection.DetectedPackage( Spec.from_detection("cmake@1.foo", external_path="/x/y1"),
Spec.from_detection("cmake@1.foo", external_path="/x/y1/"), "/x/y1/" Spec.from_detection("cmake@3.17.2", external_path="/x/y2"),
),
spack.detection.DetectedPackage(
Spec.from_detection("cmake@3.17.2", external_path="/x/y2/"), "/x/y2/"
),
] ]
pkg_to_entries = {"cmake": entries} pkg_to_entries = {"cmake": entries}
@ -88,8 +84,8 @@ def test_find_external_update_config(mutable_config):
cmake_cfg = pkgs_cfg["cmake"] cmake_cfg = pkgs_cfg["cmake"]
cmake_externals = cmake_cfg["externals"] cmake_externals = cmake_cfg["externals"]
assert {"spec": "cmake@1.foo", "prefix": "/x/y1/"} in cmake_externals assert {"spec": "cmake@1.foo", "prefix": "/x/y1"} in cmake_externals
assert {"spec": "cmake@3.17.2", "prefix": "/x/y2/"} in cmake_externals assert {"spec": "cmake@3.17.2", "prefix": "/x/y2"} in cmake_externals
def test_get_executables(working_env, mock_executable): def test_get_executables(working_env, mock_executable):
@ -229,19 +225,15 @@ 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.""" """Checks that 'spack find external' doesn't overwrite an existing spec in packages.yaml."""
pkgs_cfg_init = { pkgs_cfg_init = {
"find-externals1": { "find-externals1": {
"externals": [{"spec": "find-externals1@1.1", "prefix": "/preexisting-prefix/"}], "externals": [{"spec": "find-externals1@1.1", "prefix": "/preexisting-prefix"}],
"buildable": False, "buildable": False,
} }
} }
mutable_config.update_config("packages", pkgs_cfg_init) mutable_config.update_config("packages", pkgs_cfg_init)
entries = [ entries = [
spack.detection.DetectedPackage( Spec.from_detection("find-externals1@1.1", external_path="/x/y1"),
Spec.from_detection("find-externals1@1.1", external_path="/x/y1/"), "/x/y1/" Spec.from_detection("find-externals1@1.2", external_path="/x/y2"),
),
spack.detection.DetectedPackage(
Spec.from_detection("find-externals1@1.2", external_path="/x/y2/"), "/x/y2/"
),
] ]
pkg_to_entries = {"find-externals1": entries} pkg_to_entries = {"find-externals1": entries}
scope = spack.config.default_modify_scope("packages") scope = spack.config.default_modify_scope("packages")
@ -251,8 +243,8 @@ def test_find_external_merge(mutable_config, mutable_mock_repo, tmp_path):
pkg_cfg = pkgs_cfg["find-externals1"] pkg_cfg = pkgs_cfg["find-externals1"]
pkg_externals = pkg_cfg["externals"] pkg_externals = pkg_cfg["externals"]
assert {"spec": "find-externals1@1.1", "prefix": "/preexisting-prefix/"} in pkg_externals assert {"spec": "find-externals1@1.1", "prefix": "/preexisting-prefix"} in pkg_externals
assert {"spec": "find-externals1@1.2", "prefix": "/x/y2/"} in pkg_externals assert {"spec": "find-externals1@1.2", "prefix": "/x/y2"} in pkg_externals
def test_list_detectable_packages(mutable_config, mutable_mock_repo): def test_list_detectable_packages(mutable_config, mutable_mock_repo):
@ -278,7 +270,7 @@ def _determine_variants(cls, exes, version_str):
assert len(detected_specs) == 1 assert len(detected_specs) == 1
gcc = detected_specs[0].spec gcc = detected_specs[0]
assert gcc.name == "gcc" assert gcc.name == "gcc"
assert gcc.external_path == os.path.sep + os.path.join("opt", "gcc", "bin") assert gcc.external_path == os.path.sep + os.path.join("opt", "gcc", "bin")

9
lib/spack/spack/test/concretize.py
View File

@ -2109,11 +2109,13 @@ def test_external_python_extension_find_dependency_from_detection(self, monkeypa
"""Test that python extensions have access to a python dependency """Test that python extensions have access to a python dependency
when python isn't otherwise in the DAG""" when python isn't otherwise in the DAG"""
python_spec = Spec("python@=detected")
prefix = os.path.sep + "fake" prefix = os.path.sep + "fake"
python_spec = Spec.from_detection("python@=detected", external_path=prefix)
def find_fake_python(classes, path_hints): def find_fake_python(classes, path_hints):
return {"python": [spack.detection.DetectedPackage(python_spec, prefix=path_hints[0])]} return {
"python": [Spec.from_detection("python@=detected", external_path=path_hints[0])]
}
monkeypatch.setattr(spack.detection, "by_path", find_fake_python) monkeypatch.setattr(spack.detection, "by_path", find_fake_python)
external_conf = { external_conf = {
@ -2128,7 +2130,8 @@ def find_fake_python(classes, path_hints):
assert "python" in spec["py-extension1"] assert "python" in spec["py-extension1"]
assert spec["python"].prefix == prefix assert spec["python"].prefix == prefix
assert spec["python"] == python_spec assert spec["python"].external
assert spec["python"].satisfies(python_spec)
def test_external_python_extension_find_unified_python(self): def test_external_python_extension_find_unified_python(self):
"""Test that python extensions use the same python as other specs in unified env""" """Test that python extensions use the same python as other specs in unified env"""

6
lib/spack/spack/test/detection.py
View File

@ -11,11 +11,7 @@
def test_detection_update_config(mutable_config): def test_detection_update_config(mutable_config):
# mock detected package # mock detected package
detected_packages = collections.defaultdict(list) detected_packages = collections.defaultdict(list)
detected_packages["cmake"] = [ detected_packages["cmake"] = [spack.spec.Spec("cmake@3.27.5", external_path="/usr/bin")]
spack.detection.common.DetectedPackage(
spec=spack.spec.Spec("cmake@3.27.5"), prefix="/usr/bin"
)
]
# update config for new package # update config for new package
spack.detection.common.update_configuration(detected_packages) spack.detection.common.update_configuration(detected_packages)