From 210d221357abdd1c91e01f93bdee9444fe423eab Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Fri, 29 Sep 2023 10:24:42 +0200 Subject: Test package detection in a systematic way (#18175) This PR adds a new audit sub-command to check that detection of relevant packages is performed correctly in a few scenarios mocking real use-cases. The data for each package being tested is in a YAML file called detection_test.yaml alongside the corresponding package.py file. This is to allow encoding detection tests for compilers and other widely used tools, in preparation for compilers as dependencies. --- lib/spack/docs/packaging_guide.rst | 95 ++++++++++++++++- lib/spack/spack/audit.py | 76 ++++++++++++++ lib/spack/spack/cmd/audit.py | 25 ++++- lib/spack/spack/cmd/external.py | 16 ++- lib/spack/spack/detection/__init__.py | 2 + lib/spack/spack/detection/path.py | 13 ++- lib/spack/spack/detection/test.py | 187 ++++++++++++++++++++++++++++++++++ lib/spack/spack/repo.py | 51 +++++++--- lib/spack/spack/test/cmd/external.py | 3 +- lib/spack/spack/test/repo.py | 12 +++ 10 files changed, 457 insertions(+), 23 deletions(-) create mode 100644 lib/spack/spack/detection/test.py (limited to 'lib') diff --git a/lib/spack/docs/packaging_guide.rst b/lib/spack/docs/packaging_guide.rst index f433996ec7..d25009532a 100644 --- a/lib/spack/docs/packaging_guide.rst +++ b/lib/spack/docs/packaging_guide.rst @@ -6196,7 +6196,100 @@ follows: "foo-package@{0}".format(version_str) ) -.. _package-lifecycle: +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +Add detection tests to packages +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +To ensure that software is detected correctly for multiple configurations +and on different systems users can write a ``detection_test.yaml`` file and +put it in the package directory alongside the ``package.py`` file. +This YAML file contains enough information for Spack to mock an environment +and try to check if the detection logic yields the results that are expected. + +As a general rule, attributes at the top-level of ``detection_test.yaml`` +represent search mechanisms and they each map to a list of tests that should confirm +the validity of the package's detection logic. + +The detection tests can be run with the following command: + +.. code-block:: console + + $ spack audit externals + +Errors that have been detected are reported to screen. + +"""""""""""""""""""""""""" +Tests for PATH inspections +"""""""""""""""""""""""""" + +Detection tests insisting on ``PATH`` inspections are listed under +the ``paths`` attribute: + +.. code-block:: yaml + + paths: + - layout: + - executables: + - "bin/clang-3.9" + - "bin/clang++-3.9" + script: | + echo "clang version 3.9.1-19ubuntu1 (tags/RELEASE_391/rc2)" + echo "Target: x86_64-pc-linux-gnu" + echo "Thread model: posix" + echo "InstalledDir: /usr/bin" + results: + - spec: 'llvm@3.9.1 +clang~lld~lldb' + +Each test is performed by first creating a temporary directory structure as +specified in the corresponding ``layout`` and by then running +package detection and checking that the outcome matches the expected +``results``. The exact details on how to specify both the ``layout`` and the +``results`` are reported in the table below: + +.. list-table:: Test based on PATH inspections + :header-rows: 1 + + * - Option Name + - Description + - Allowed Values + - Required Field + * - ``layout`` + - Specifies the filesystem tree used for the test + - List of objects + - Yes + * - ``layout:[0]:executables`` + - Relative paths for the mock executables to be created + - List of strings + - Yes + * - ``layout:[0]:script`` + - Mock logic for the executable + - Any valid shell script + - Yes + * - ``results`` + - List of expected results + - List of objects (empty if no result is expected) + - Yes + * - ``results:[0]:spec`` + - A spec that is expected from detection + - Any valid spec + - Yes + +""""""""""""""""""""""""""""""" +Reuse tests from other packages +""""""""""""""""""""""""""""""" + +When using a custom repository, it is possible to customize a package that already exists in ``builtin`` +and reuse its external tests. To do so, just write a ``detection_tests.yaml`` alongside the customized +``package.py`` with an ``includes`` attribute. For instance the ``detection_tests.yaml`` for +``myrepo.llvm`` might look like: + +.. code-block:: yaml + + includes: + - "builtin.llvm" + +This YAML file instructs Spack to run the detection tests defined in ``builtin.llvm`` in addition to +those locally defined in the file. ----------------------------- Style guidelines for packages diff --git a/lib/spack/spack/audit.py b/lib/spack/spack/audit.py index c3a028a72b..176c45487f 100644 --- a/lib/spack/spack/audit.py +++ b/lib/spack/spack/audit.py @@ -38,10 +38,13 @@ as input. import ast import collections import collections.abc +import glob import inspect import itertools +import pathlib import pickle import re +import warnings from urllib.request import urlopen import llnl.util.lang @@ -798,3 +801,76 @@ def _analyze_variants_in_directive(pkg, constraint, directive, error_cls): errors.append(err) return errors + + +#: Sanity checks on package directives +external_detection = AuditClass( + group="externals", + tag="PKG-EXTERNALS", + description="Sanity checks for external software detection", + kwargs=("pkgs",), +) + + +def packages_with_detection_tests(): + """Return the list of packages with a corresponding detection_test.yaml file.""" + import spack.config + import spack.util.path + + to_be_tested = [] + for current_repo in spack.repo.PATH.repos: + namespace = current_repo.namespace + packages_dir = pathlib.PurePath(current_repo.packages_path) + pattern = packages_dir / "**" / "detection_test.yaml" + pkgs_with_tests = [ + f"{namespace}.{str(pathlib.PurePath(x).parent.name)}" for x in glob.glob(str(pattern)) + ] + to_be_tested.extend(pkgs_with_tests) + + return to_be_tested + + +@external_detection +def _test_detection_by_executable(pkgs, error_cls): + """Test drive external detection for packages""" + import spack.detection + + errors = [] + + # Filter the packages and retain only the ones with detection tests + pkgs_with_tests = packages_with_detection_tests() + selected_pkgs = [] + for current_package in pkgs_with_tests: + _, unqualified_name = spack.repo.partition_package_name(current_package) + # Check for both unqualified name and qualified name + if unqualified_name in pkgs or current_package in pkgs: + selected_pkgs.append(current_package) + selected_pkgs.sort() + + if not selected_pkgs: + summary = "No detection test to run" + details = [f' "{p}" has no detection test' for p in pkgs] + warnings.warn("\n".join([summary] + details)) + return errors + + for pkg_name in selected_pkgs: + for idx, test_runner in enumerate( + spack.detection.detection_tests(pkg_name, spack.repo.PATH) + ): + specs = test_runner.execute() + expected_specs = test_runner.expected_specs + + not_detected = set(expected_specs) - set(specs) + if not_detected: + summary = pkg_name + ": cannot detect some specs" + details = [f'"{s}" was not detected [test_id={idx}]' for s in sorted(not_detected)] + errors.append(error_cls(summary=summary, details=details)) + + not_expected = set(specs) - set(expected_specs) + if not_expected: + summary = pkg_name + ": detected unexpected specs" + msg = '"{0}" was detected, but was not expected [test_id={1}]' + details = [msg.format(s, idx) for s in sorted(not_expected)] + errors.append(error_cls(summary=summary, details=details)) + + return errors diff --git a/lib/spack/spack/cmd/audit.py b/lib/spack/spack/cmd/audit.py index cd56dfadd9..86eea9f7bc 100644 --- a/lib/spack/spack/cmd/audit.py +++ b/lib/spack/spack/cmd/audit.py @@ -3,6 +3,7 @@ # # SPDX-License-Identifier: (Apache-2.0 OR MIT) import llnl.util.tty as tty +import llnl.util.tty.colify import llnl.util.tty.color as cl import spack.audit @@ -20,6 +21,15 @@ def setup_parser(subparser): # Audit configuration files sp.add_parser("configs", help="audit configuration files") + # Audit package recipes + external_parser = sp.add_parser("externals", help="check external detection in packages") + external_parser.add_argument( + "--list", + action="store_true", + dest="list_externals", + help="if passed, list which packages have detection tests", + ) + # Https and other linting https_parser = sp.add_parser("packages-https", help="check https in packages") https_parser.add_argument( @@ -29,7 +39,7 @@ def setup_parser(subparser): # Audit package recipes pkg_parser = sp.add_parser("packages", help="audit package recipes") - for group in [pkg_parser, https_parser]: + for group in [pkg_parser, https_parser, external_parser]: group.add_argument( "name", metavar="PKG", @@ -62,6 +72,18 @@ def packages_https(parser, args): _process_reports(reports) +def externals(parser, args): + if args.list_externals: + msg = "@*{The following packages have detection tests:}" + tty.msg(cl.colorize(msg)) + llnl.util.tty.colify.colify(spack.audit.packages_with_detection_tests(), indent=2) + return + + pkgs = args.name or spack.repo.PATH.all_package_names() + reports = spack.audit.run_group(args.subcommand, pkgs=pkgs) + _process_reports(reports) + + def list(parser, args): for subcommand, check_tags in spack.audit.GROUPS.items(): print(cl.colorize("@*b{" + subcommand + "}:")) @@ -78,6 +100,7 @@ def list(parser, args): def audit(parser, args): subcommands = { "configs": configs, + "externals": externals, "packages": packages, "packages-https": packages_https, "list": list, diff --git a/lib/spack/spack/cmd/external.py b/lib/spack/spack/cmd/external.py index bf29787db9..081ec80394 100644 --- a/lib/spack/spack/cmd/external.py +++ b/lib/spack/spack/cmd/external.py @@ -5,6 +5,7 @@ import argparse import errno import os +import re import sys from typing import List, Optional @@ -156,11 +157,20 @@ def packages_to_search_for( ): result = [] for current_tag in tags: - result.extend(spack.repo.PATH.packages_with_tags(current_tag)) + result.extend(spack.repo.PATH.packages_with_tags(current_tag, full=True)) + if names: - result = [x for x in result if x in names] + # Match both fully qualified and unqualified + parts = [rf"(^{x}$|[.]{x}$)" for x in names] + select_re = re.compile("|".join(parts)) + result = [x for x in result if select_re.search(x)] + if exclude: - result = [x for x in result if x not in exclude] + # Match both fully qualified and unqualified + parts = [rf"(^{x}$|[.]{x}$)" for x in exclude] + select_re = re.compile("|".join(parts)) + result = [x for x in result if not select_re.search(x)] + return result diff --git a/lib/spack/spack/detection/__init__.py b/lib/spack/spack/detection/__init__.py index 73ae34ce63..7c54fb9d49 100644 --- a/lib/spack/spack/detection/__init__.py +++ b/lib/spack/spack/detection/__init__.py @@ -4,6 +4,7 @@ # SPDX-License-Identifier: (Apache-2.0 OR MIT) from .common import DetectedPackage, executable_prefix, update_configuration from .path import by_path, executables_in_path +from .test import detection_tests __all__ = [ "DetectedPackage", @@ -11,4 +12,5 @@ __all__ = [ "executables_in_path", "executable_prefix", "update_configuration", + "detection_tests", ] diff --git a/lib/spack/spack/detection/path.py b/lib/spack/spack/detection/path.py index 4a085aacd0..4de703ac97 100644 --- a/lib/spack/spack/detection/path.py +++ b/lib/spack/spack/detection/path.py @@ -2,7 +2,7 @@ # Spack Project Developers. See the top-level COPYRIGHT file for details. # # SPDX-License-Identifier: (Apache-2.0 OR MIT) -"""Detection of software installed in the system based on paths inspections +"""Detection of software installed in the system, based on paths inspections and running executables. """ import collections @@ -322,12 +322,14 @@ def by_path( path_hints: Optional[List[str]] = None, max_workers: Optional[int] = None, ) -> Dict[str, List[DetectedPackage]]: - """Return the list of packages that have been detected on the system, - searching by path. + """Return the list of packages that have been detected on the system, keyed by + unqualified package name. Args: - packages_to_search: list of package classes to be detected + packages_to_search: list of packages to be detected. Each package can be either unqualified + of fully qualified path_hints: initial list of paths to be searched + max_workers: maximum number of workers to search for packages in parallel """ # TODO: Packages should be able to define both .libraries and .executables in the future # TODO: determine_spec_details should get all relevant libraries and executables in one call @@ -355,7 +357,8 @@ def by_path( try: detected = future.result(timeout=DETECTION_TIMEOUT) if detected: - result[pkg_name].extend(detected) + _, unqualified_name = spack.repo.partition_package_name(pkg_name) + result[unqualified_name].extend(detected) except Exception: llnl.util.tty.debug( f"[EXTERNAL DETECTION] Skipping {pkg_name}: timeout reached" diff --git a/lib/spack/spack/detection/test.py b/lib/spack/spack/detection/test.py new file mode 100644 index 0000000000..f33040f292 --- /dev/null +++ b/lib/spack/spack/detection/test.py @@ -0,0 +1,187 @@ +# Copyright 2013-2023 Lawrence Livermore National Security, LLC and other +# Spack Project Developers. See the top-level COPYRIGHT file for details. +# +# SPDX-License-Identifier: (Apache-2.0 OR MIT) +"""Create and run mock e2e tests for package detection.""" +import collections +import contextlib +import pathlib +import tempfile +from typing import Any, Deque, Dict, Generator, List, NamedTuple, Tuple + +import jinja2 + +from llnl.util import filesystem + +import spack.repo +import spack.spec +from spack.util import spack_yaml + +from .path import by_path + + +class MockExecutables(NamedTuple): + """Mock executables to be used in detection tests""" + + #: Relative paths for mock executables to be created + executables: List[str] + #: Shell script for the mock executable + script: str + + +class ExpectedTestResult(NamedTuple): + """Data structure to model assertions on detection tests""" + + #: Spec to be detected + spec: str + + +class DetectionTest(NamedTuple): + """Data structure to construct detection tests by PATH inspection. + + Packages may have a YAML file containing the description of one or more detection tests + to be performed. Each test creates a few mock executable scripts in a temporary folder, + and checks that detection by PATH gives the expected results. + """ + + pkg_name: str + layout: List[MockExecutables] + results: List[ExpectedTestResult] + + +class Runner: + """Runs an external detection test""" + + def __init__(self, *, test: DetectionTest, repository: spack.repo.RepoPath) -> None: + self.test = test + self.repository = repository + self.tmpdir = tempfile.TemporaryDirectory() + + def execute(self) -> List[spack.spec.Spec]: + """Executes a test and returns the specs that have been detected. + + This function sets-up a test in a temporary directory, according to the prescriptions + in the test layout, then performs a detection by executables and returns the specs that + have been detected. + """ + with self._mock_layout() as path_hints: + entries = by_path([self.test.pkg_name], path_hints=path_hints) + _, unqualified_name = spack.repo.partition_package_name(self.test.pkg_name) + specs = set(x.spec for x in entries[unqualified_name]) + return list(specs) + + @contextlib.contextmanager + def _mock_layout(self) -> Generator[List[str], None, None]: + hints = set() + try: + for entry in self.test.layout: + exes = self._create_executable_scripts(entry) + + for mock_executable in exes: + hints.add(str(mock_executable.parent)) + + yield list(hints) + finally: + self.tmpdir.cleanup() + + def _create_executable_scripts(self, mock_executables: MockExecutables) -> List[pathlib.Path]: + relative_paths = mock_executables.executables + script = mock_executables.script + script_template = jinja2.Template("#!/bin/bash\n{{ script }}\n") + result = [] + for mock_exe_path in relative_paths: + rel_path = pathlib.Path(mock_exe_path) + abs_path = pathlib.Path(self.tmpdir.name) / rel_path + abs_path.parent.mkdir(parents=True, exist_ok=True) + abs_path.write_text(script_template.render(script=script)) + filesystem.set_executable(abs_path) + result.append(abs_path) + return result + + @property + def expected_specs(self) -> List[spack.spec.Spec]: + return [spack.spec.Spec(r.spec) for r in self.test.results] + + +def detection_tests(pkg_name: str, repository: spack.repo.RepoPath) -> List[Runner]: + """Returns a list of test runners for a given package. + + Currently, detection tests are specified in a YAML file, called ``detection_test.yaml``, + alongside the ``package.py`` file. + + This function reads that file to create a bunch of ``Runner`` objects. + + Args: + pkg_name: name of the package to test + repository: repository where the package lives + """ + result = [] + detection_tests_content = read_detection_tests(pkg_name, repository) + + tests_by_path = detection_tests_content.get("paths", []) + for single_test_data in tests_by_path: + mock_executables = [] + for layout in single_test_data["layout"]: + mock_executables.append( + MockExecutables(executables=layout["executables"], script=layout["script"]) + ) + expected_results = [] + for assertion in single_test_data["results"]: + expected_results.append(ExpectedTestResult(spec=assertion["spec"])) + + current_test = DetectionTest( + pkg_name=pkg_name, layout=mock_executables, results=expected_results + ) + result.append(Runner(test=current_test, repository=repository)) + + return result + + +def read_detection_tests(pkg_name: str, repository: spack.repo.RepoPath) -> Dict[str, Any]: + """Returns the normalized content of the detection_tests.yaml associated with the package + passed in input. + + The content is merged with that of any package that is transitively included using the + "includes" attribute. + + Args: + pkg_name: name of the package to test + repository: repository in which to search for packages + """ + content_stack, seen = [], set() + included_packages: Deque[str] = collections.deque() + + root_detection_yaml, result = _detection_tests_yaml(pkg_name, repository) + included_packages.extend(result.get("includes", [])) + seen |= set(result.get("includes", [])) + + while included_packages: + current_package = included_packages.popleft() + try: + current_detection_yaml, content = _detection_tests_yaml(current_package, repository) + except FileNotFoundError as e: + msg = ( + f"cannot read the detection tests from the '{current_package}' package, " + f"included by {root_detection_yaml}" + ) + raise FileNotFoundError(msg + f"\n\n\t{e}\n") + + content_stack.append((current_package, content)) + included_packages.extend(x for x in content.get("includes", []) if x not in seen) + seen |= set(content.get("includes", [])) + + result.setdefault("paths", []) + for pkg_name, content in content_stack: + result["paths"].extend(content.get("paths", [])) + + return result + + +def _detection_tests_yaml( + pkg_name: str, repository: spack.repo.RepoPath +) -> Tuple[pathlib.Path, Dict[str, Any]]: + pkg_dir = pathlib.Path(repository.filename_for_package_name(pkg_name)).parent + detection_tests_yaml = pkg_dir / "detection_test.yaml" + with open(str(detection_tests_yaml)) as f: + content = spack_yaml.load(f) + return detection_tests_yaml, content diff --git a/lib/spack/spack/repo.py b/lib/spack/spack/repo.py index 4391d9a9a2..a89b5dd407 100644 --- a/lib/spack/spack/repo.py +++ b/lib/spack/spack/repo.py @@ -24,7 +24,7 @@ import sys import traceback import types import uuid -from typing import Any, Dict, List, Union +from typing import Any, Dict, List, Tuple, Union import llnl.path import llnl.util.filesystem as fs @@ -745,10 +745,18 @@ class RepoPath: for name in self.all_package_names(): yield self.package_path(name) - def packages_with_tags(self, *tags): + def packages_with_tags(self, *tags, full=False): + """Returns a list of packages matching any of the tags in input. + + Args: + full: if True the package names in the output are fully-qualified + """ r = set() for repo in self.repos: - r |= set(repo.packages_with_tags(*tags)) + current = repo.packages_with_tags(*tags) + if full: + current = [f"{repo.namespace}.{x}" for x in current] + r |= set(current) return sorted(r) def all_package_classes(self): @@ -1124,7 +1132,8 @@ class Repo: def dirname_for_package_name(self, pkg_name): """Get the directory name for a particular package. This is the directory that contains its package.py file.""" - return os.path.join(self.packages_path, pkg_name) + _, unqualified_name = self.partition_package_name(pkg_name) + return os.path.join(self.packages_path, unqualified_name) def filename_for_package_name(self, pkg_name): """Get the filename for the module we should load for a particular @@ -1222,15 +1231,10 @@ class Repo: package. Then extracts the package class from the module according to Spack's naming convention. """ - namespace, _, pkg_name = pkg_name.rpartition(".") - if namespace and (namespace != self.namespace): - raise InvalidNamespaceError( - "Invalid namespace for %s repo: %s" % (self.namespace, namespace) - ) - + namespace, pkg_name = self.partition_package_name(pkg_name) class_name = nm.mod_to_class(pkg_name) + fullname = f"{self.full_namespace}.{pkg_name}" - fullname = "{0}.{1}".format(self.full_namespace, pkg_name) try: module = importlib.import_module(fullname) except ImportError: @@ -1241,7 +1245,7 @@ class Repo: cls = getattr(module, class_name) if not inspect.isclass(cls): - tty.die("%s.%s is not a class" % (pkg_name, class_name)) + tty.die(f"{pkg_name}.{class_name} is not a class") new_cfg_settings = ( spack.config.get("packages").get(pkg_name, {}).get("package_attributes", {}) @@ -1280,6 +1284,15 @@ class Repo: return cls + def partition_package_name(self, pkg_name: str) -> Tuple[str, str]: + namespace, pkg_name = partition_package_name(pkg_name) + if namespace and (namespace != self.namespace): + raise InvalidNamespaceError( + f"Invalid namespace for the '{self.namespace}' repo: {namespace}" + ) + + return namespace, pkg_name + def __str__(self): return "[Repo '%s' at '%s']" % (self.namespace, self.root) @@ -1293,6 +1306,20 @@ class Repo: RepoType = Union[Repo, RepoPath] +def partition_package_name(pkg_name: str) -> Tuple[str, str]: + """Given a package name that might be fully-qualified, returns the namespace part, + if present and the unqualified package name. + + If the package name is unqualified, the namespace is an empty string. + + Args: + pkg_name: a package name, either unqualified like "llvl", or + fully-qualified, like "builtin.llvm" + """ + namespace, _, pkg_name = pkg_name.rpartition(".") + return namespace, pkg_name + + def create_repo(root, namespace=None, subdir=packages_dir_name): """Create a new repository in root with the specified namespace. diff --git a/lib/spack/spack/test/cmd/external.py b/lib/spack/spack/test/cmd/external.py index 7d54057b46..0b4eca124d 100644 --- a/lib/spack/spack/test/cmd/external.py +++ b/lib/spack/spack/test/cmd/external.py @@ -120,8 +120,9 @@ def test_find_external_cmd_not_buildable(mutable_config, working_env, mock_execu "names,tags,exclude,expected", [ # find --all - (None, ["detectable"], [], ["find-externals1"]), + (None, ["detectable"], [], ["builtin.mock.find-externals1"]), # find --all --exclude find-externals1 + (None, ["detectable"], ["builtin.mock.find-externals1"], []), (None, ["detectable"], ["find-externals1"], []), # find cmake (and cmake is not detectable) (["cmake"], ["detectable"], [], []), diff --git a/lib/spack/spack/test/repo.py b/lib/spack/spack/test/repo.py index 7314beebb5..eb6b123916 100644 --- a/lib/spack/spack/test/repo.py +++ b/lib/spack/spack/test/repo.py @@ -181,3 +181,15 @@ def test_repository_construction_doesnt_use_globals(nullify_globals, repo_paths, repo_path = spack.repo.RepoPath(*repo_paths) assert len(repo_path.repos) == len(namespaces) assert [x.namespace for x in repo_path.repos] == namespaces + + +@pytest.mark.parametrize("method_name", ["dirname_for_package_name", "filename_for_package_name"]) +def test_path_computation_with_names(method_name, mock_repo_path): + """Tests that repositories can compute the correct paths when using both fully qualified + names and unqualified names. + """ + repo_path = spack.repo.RepoPath(mock_repo_path) + method = getattr(repo_path, method_name) + unqualified = method("mpileaks") + qualified = method("builtin.mock.mpileaks") + assert qualified == unqualified -- cgit v1.2.3-70-g09d2