From acb02326aa6a0c03193ea6213657d7921fb1cd95 Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Thu, 31 Aug 2023 10:09:37 +0200 Subject: ASP-based solver: add hidden mode to ignore versions that are moving targets, use that in CI (#39611) Setting the undocumented variable SPACK_CONCRETIZER_REQUIRE_CHECKSUM now causes the solver to avoid accounting for versions that are not checksummed. This feature is used in CI to avoid spurious concretization against e.g. develop branches. --- lib/spack/spack/directives.py | 6 +- lib/spack/spack/solver/asp.py | 264 ++++++++++++------------ lib/spack/spack/test/concretize.py | 47 ++++- lib/spack/spack/test/concretize_requirements.py | 8 +- lib/spack/spack/util/crypto.py | 3 +- 5 files changed, 194 insertions(+), 134 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py index d6d7d81298..ccc913a1fe 100644 --- a/lib/spack/spack/directives.py +++ b/lib/spack/spack/directives.py @@ -42,6 +42,7 @@ import spack.error import spack.patch import spack.spec import spack.url +import spack.util.crypto import spack.variant from spack.dependency import Dependency, canonical_deptype, default_deptype from spack.fetch_strategy import from_kwargs @@ -407,10 +408,7 @@ def version( def _execute_version(pkg, ver, **kwargs): if ( - any( - s in kwargs - for s in ("sha256", "sha384", "sha512", "md5", "sha1", "sha224", "checksum") - ) + (any(s in kwargs for s in spack.util.crypto.hashes) or "checksum" in kwargs) and hasattr(pkg, "has_code") and not pkg.has_code ): diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py index c596b81464..7b13b4baa3 100644 --- a/lib/spack/spack/solver/asp.py +++ b/lib/spack/spack/solver/asp.py @@ -13,7 +13,7 @@ import pprint import re import types import warnings -from typing import List, NamedTuple +from typing import List, NamedTuple, Tuple, Union import archspec.cpu @@ -44,15 +44,18 @@ import spack.platforms import spack.repo import spack.spec import spack.store -import spack.traverse +import spack.util.crypto import spack.util.path import spack.util.timer import spack.variant import spack.version as vn import spack.version.git_ref_lookup +from spack import traverse from .counter import FullDuplicatesCounter, MinimalDuplicatesCounter, NoDuplicatesCounter +GitOrStandardVersion = Union[spack.version.GitVersion, spack.version.StandardVersion] + # these are from clingo.ast and bootstrapped later ASTType = None parse_files = None @@ -569,6 +572,41 @@ def _normalize_packages_yaml(packages_yaml): return normalized_yaml +def _is_checksummed_git_version(v): + return isinstance(v, vn.GitVersion) and v.is_commit + + +def _is_checksummed_version(version_info: Tuple[GitOrStandardVersion, dict]): + """Returns true iff the version is not a moving target""" + version, info = version_info + if isinstance(version, spack.version.StandardVersion): + if any(h in info for h in spack.util.crypto.hashes.keys()) or "checksum" in info: + return True + return "commit" in info and len(info["commit"]) == 40 + return _is_checksummed_git_version(version) + + +def _concretization_version_order(version_info: Tuple[GitOrStandardVersion, dict]): + """Version order key for concretization, where preferred > not preferred, + not deprecated > deprecated, finite > any infinite component; only if all are + the same, do we use default version ordering.""" + version, info = version_info + return ( + info.get("preferred", False), + not info.get("deprecated", False), + not version.isdevelop(), + version, + ) + + +def _spec_with_default_name(spec_str, name): + """Return a spec with a default name if none is provided, used for requirement specs""" + spec = spack.spec.Spec(spec_str) + if not spec.name: + spec.name = name + return spec + + def bootstrap_clingo(): global clingo, ASTType, parse_files @@ -1855,30 +1893,27 @@ class SpackSolverSetup: return clauses - def build_version_dict(self, possible_pkgs): + def define_package_versions_and_validate_preferences( + self, possible_pkgs, require_checksum: bool + ): """Declare any versions in specs not declared in packages.""" packages_yaml = spack.config.get("packages") - packages_yaml = _normalize_packages_yaml(packages_yaml) for pkg_name in possible_pkgs: pkg_cls = spack.repo.PATH.get_pkg_class(pkg_name) # All the versions from the corresponding package.py file. Since concepts # like being a "develop" version or being preferred exist only at a # package.py level, sort them in this partial list here - def key_fn(item): - version, info = item - # When COMPARING VERSIONS, the '@develop' version is always - # larger than other versions. BUT when CONCRETIZING, the largest - # NON-develop version is selected by default. - return ( - info.get("preferred", False), - not info.get("deprecated", False), - not version.isdevelop(), - version, - ) + package_py_versions = sorted( + pkg_cls.versions.items(), key=_concretization_version_order, reverse=True + ) - for idx, item in enumerate(sorted(pkg_cls.versions.items(), key=key_fn, reverse=True)): - v, version_info = item + if require_checksum and pkg_cls.has_code: + package_py_versions = [ + x for x in package_py_versions if _is_checksummed_version(x) + ] + + for idx, (v, version_info) in enumerate(package_py_versions): self.possible_versions[pkg_name].add(v) self.declared_versions[pkg_name].append( DeclaredVersion(version=v, idx=idx, origin=Provenance.PACKAGE_PY) @@ -1887,22 +1922,26 @@ class SpackSolverSetup: if deprecated: self.deprecated_versions[pkg_name].add(v) - # All the preferred version from packages.yaml, versions in external - # specs will be computed later - version_preferences = packages_yaml.get(pkg_name, {}).get("version", []) + if pkg_name not in packages_yaml or "version" not in packages_yaml[pkg_name]: + continue + version_defs = [] - pkg_class = spack.repo.PATH.get_pkg_class(pkg_name) - for vstr in version_preferences: + + for vstr in packages_yaml[pkg_name]["version"]: v = vn.ver(vstr) + if isinstance(v, vn.GitVersion): - version_defs.append(v) + if not require_checksum or v.is_commit: + version_defs.append(v) else: - satisfying_versions = self._check_for_defined_matching_versions(pkg_class, v) - # Amongst all defined versions satisfying this specific - # preference, the highest-numbered version is the - # most-preferred: therefore sort satisfying versions - # from greatest to least - version_defs.extend(sorted(satisfying_versions, reverse=True)) + matches = [x for x in self.possible_versions[pkg_name] if x.satisfies(v)] + matches.sort(reverse=True) + if not matches: + raise spack.config.ConfigError( + f"Preference for version {v} does not match any known " + f"version of {pkg_name} (in its package.py or any external)" + ) + version_defs.extend(matches) for weight, vdef in enumerate(llnl.util.lang.dedupe(version_defs)): self.declared_versions[pkg_name].append( @@ -1910,31 +1949,9 @@ class SpackSolverSetup: ) self.possible_versions[pkg_name].add(vdef) - def _check_for_defined_matching_versions(self, pkg_class, v): - """Given a version specification (which may be a concrete version, - range, etc.), determine if any package.py version declarations - or externals define a version which satisfies it. - - This is primarily for determining whether a version request (e.g. - version preferences, which should not themselves define versions) - refers to a defined version. - - This function raises an exception if no satisfying versions are - found. - """ - pkg_name = pkg_class.name - satisfying_versions = list(x for x in pkg_class.versions if x.satisfies(v)) - satisfying_versions.extend(x for x in self.possible_versions[pkg_name] if x.satisfies(v)) - if not satisfying_versions: - raise spack.config.ConfigError( - "Preference for version {0} does not match any version" - " defined for {1} (in its package.py or any external)".format(str(v), pkg_name) - ) - return satisfying_versions - - def add_concrete_versions_from_specs(self, specs, origin): + def define_ad_hoc_versions_from_specs(self, specs, origin, require_checksum: bool): """Add concrete versions to possible versions from lists of CLI/dev specs.""" - for s in spack.traverse.traverse_nodes(specs): + for s in traverse.traverse_nodes(specs): # If there is a concrete version on the CLI *that we know nothing # about*, add it to the known versions. Use idx=0, which is the # best possible, so they're guaranteed to be used preferentially. @@ -1943,9 +1960,13 @@ class SpackSolverSetup: if version is None or any(v == version for v in self.possible_versions[s.name]): continue - self.declared_versions[s.name].append( - DeclaredVersion(version=version, idx=0, origin=origin) - ) + if require_checksum and not _is_checksummed_git_version(version): + raise UnsatisfiableSpecError( + s.format("No matching version for constraint {name}{@versions}") + ) + + declared = DeclaredVersion(version=version, idx=0, origin=origin) + self.declared_versions[s.name].append(declared) self.possible_versions[s.name].add(version) def _supported_targets(self, compiler_name, compiler_version, targets): @@ -2142,7 +2163,7 @@ class SpackSolverSetup: # add compiler specs from the input line to possibilities if we # don't require compilers to exist. strict = spack.concretize.Concretizer().check_for_compiler_existence - for s in spack.traverse.traverse_nodes(specs): + for s in traverse.traverse_nodes(specs): # we don't need to validate compilers for already-built specs if s.concrete or not s.compiler: continue @@ -2392,13 +2413,12 @@ class SpackSolverSetup: self.provider_requirements() self.external_packages() - # traverse all specs and packages to build dict of possible versions - self.build_version_dict(self.pkgs) - self.add_concrete_versions_from_specs(specs, Provenance.SPEC) - self.add_concrete_versions_from_specs(dev_specs, Provenance.DEV_SPEC) - - req_version_specs = self._get_versioned_specs_from_pkg_requirements() - self.add_concrete_versions_from_specs(req_version_specs, Provenance.PACKAGE_REQUIREMENT) + # TODO: make a config option for this undocumented feature + require_checksum = "SPACK_CONCRETIZER_REQUIRE_CHECKSUM" in os.environ + self.define_package_versions_and_validate_preferences(self.pkgs, require_checksum) + self.define_ad_hoc_versions_from_specs(specs, Provenance.SPEC, require_checksum) + self.define_ad_hoc_versions_from_specs(dev_specs, Provenance.DEV_SPEC, require_checksum) + self.validate_and_define_versions_from_requirements(require_checksum) self.gen.h1("Package Constraints") for pkg in sorted(self.pkgs): @@ -2447,78 +2467,68 @@ class SpackSolverSetup: if self.concretize_everything: self.gen.fact(fn.solve_literal(idx)) - def _get_versioned_specs_from_pkg_requirements(self): - """If package requirements mention versions that are not mentioned + def validate_and_define_versions_from_requirements(self, require_checksum: bool): + """If package requirements mention concrete versions that are not mentioned elsewhere, then we need to collect those to mark them as possible - versions. - """ - req_version_specs = list() - config = spack.config.get("packages") - for pkg_name, d in config.items(): - if pkg_name == "all": + versions. If they are abstract and statically have no match, then we + need to throw an error. This function assumes all possible versions are already + registered in self.possible_versions.""" + for pkg_name, d in spack.config.get("packages").items(): + if pkg_name == "all" or "require" not in d: continue - if "require" in d: - req_version_specs.extend(self._specs_from_requires(pkg_name, d["require"])) - return req_version_specs + + for s in traverse.traverse_nodes(self._specs_from_requires(pkg_name, d["require"])): + name, versions = s.name, s.versions + + if name not in self.pkgs or versions == spack.version.any_version: + continue + + s.attach_git_version_lookup() + v = versions.concrete + + if not v: + # If the version is not concrete, check it's statically concretizable. If + # not throw an error, which is just so that users know they need to change + # their config, instead of getting a hard to decipher concretization error. + if not any(x for x in self.possible_versions[name] if x.satisfies(versions)): + raise spack.config.ConfigError( + f"Version requirement {versions} on {pkg_name} for {name} " + f"cannot match any known version from package.py or externals" + ) + continue + + if v in self.possible_versions[name]: + continue + + # If concrete an not yet defined, conditionally define it, like we do for specs + # from the command line. + if not require_checksum or _is_checksummed_git_version(v): + self.declared_versions[name].append( + DeclaredVersion(version=v, idx=0, origin=Provenance.PACKAGE_REQUIREMENT) + ) + self.possible_versions[name].add(v) def _specs_from_requires(self, pkg_name, section): - """Collect specs from requirements which define versions (i.e. those that - have a concrete version). Requirements can define *new* versions if - they are included as part of an equivalence (hash=number) but not - otherwise. - """ + """Collect specs from a requirement rule""" if isinstance(section, str): - spec = spack.spec.Spec(section) - if not spec.name: - spec.name = pkg_name - extracted_specs = [spec] - else: - spec_strs = [] - for spec_group in section: - if isinstance(spec_group, str): - spec_strs.append(spec_group) - else: - # Otherwise it is an object. The object can contain a single - # "spec" constraint, or a list of them with "any_of" or - # "one_of" policy. - if "spec" in spec_group: - new_constraints = [spec_group["spec"]] - else: - key = "one_of" if "one_of" in spec_group else "any_of" - new_constraints = spec_group[key] - spec_strs.extend(new_constraints) - - extracted_specs = [] - for spec_str in spec_strs: - spec = spack.spec.Spec(spec_str) - if not spec.name: - spec.name = pkg_name - extracted_specs.append(spec) + yield _spec_with_default_name(section, pkg_name) + return - version_specs = [] - for spec in extracted_specs: - if spec.versions.concrete: - # Note: this includes git versions - version_specs.append(spec) + for spec_group in section: + if isinstance(spec_group, str): + yield _spec_with_default_name(spec_group, pkg_name) continue - # Prefer spec's name if it exists, in case the spec is - # requiring a specific implementation inside of a virtual section - # e.g. packages:mpi:require:openmpi@4.0.1 - pkg_class = spack.repo.PATH.get_pkg_class(spec.name or pkg_name) - satisfying_versions = self._check_for_defined_matching_versions( - pkg_class, spec.versions - ) - - # Version ranges ("@1.3" without the "=", "@1.2:1.4") and lists - # will end up here - ordered_satisfying_versions = sorted(satisfying_versions, reverse=True) - vspecs = list(spack.spec.Spec("@{0}".format(x)) for x in ordered_satisfying_versions) - version_specs.extend(vspecs) + # Otherwise it is an object. The object can contain a single + # "spec" constraint, or a list of them with "any_of" or + # "one_of" policy. + if "spec" in spec_group: + yield _spec_with_default_name(spec_group["spec"], pkg_name) + continue - for spec in version_specs: - spec.attach_git_version_lookup() - return version_specs + key = "one_of" if "one_of" in spec_group else "any_of" + for s in spec_group[key]: + yield _spec_with_default_name(s, pkg_name) class SpecBuilder: diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py index 4cd22249ca..95c9fe8752 100644 --- a/lib/spack/spack/test/concretize.py +++ b/lib/spack/spack/test/concretize.py @@ -21,10 +21,11 @@ import spack.error import spack.hash_types as ht import spack.platforms import spack.repo +import spack.solver.asp import spack.variant as vt from spack.concretize import find_spec from spack.spec import CompilerSpec, Spec -from spack.version import ver +from spack.version import Version, ver def check_spec(abstract, concrete): @@ -2114,6 +2115,14 @@ class TestConcretize: assert len(mpi_edges) == 1 assert "mpi" in mpi_edges[0].virtuals + @pytest.mark.only_clingo("Use case not supported by the original concretizer") + def test_dont_define_new_version_from_input_if_checksum_required(self, working_env): + os.environ["SPACK_CONCRETIZER_REQUIRE_CHECKSUM"] = "yes" + with pytest.raises(spack.error.UnsatisfiableSpecError): + # normally spack concretizes to @=3.0 if it's not defined in package.py, except + # when checksums are required + Spec("a@=3.0").concretized() + @pytest.fixture() def duplicates_test_repository(): @@ -2208,3 +2217,39 @@ class TestConcretizeSeparately: s = Spec("cycle-b").concretized() assert s["cycle-a"].satisfies("~cycle") assert s["cycle-b"].satisfies("+cycle") + + +@pytest.mark.parametrize( + "v_str,v_opts,checksummed", + [ + ("1.2.3", {"sha256": f"{1:064x}"}, True), + # it's not about the version being "infinite", + # but whether it has a digest + ("develop", {"sha256": f"{1:064x}"}, True), + # other hash types + ("1.2.3", {"checksum": f"{1:064x}"}, True), + ("1.2.3", {"md5": f"{1:032x}"}, True), + ("1.2.3", {"sha1": f"{1:040x}"}, True), + ("1.2.3", {"sha224": f"{1:056x}"}, True), + ("1.2.3", {"sha384": f"{1:096x}"}, True), + ("1.2.3", {"sha512": f"{1:0128x}"}, True), + # no digest key + ("1.2.3", {"bogus": f"{1:064x}"}, False), + # git version with full commit sha + ("1.2.3", {"commit": f"{1:040x}"}, True), + (f"{1:040x}=1.2.3", {}, True), + # git version with short commit sha + ("1.2.3", {"commit": f"{1:07x}"}, False), + (f"{1:07x}=1.2.3", {}, False), + # git tag is a moving target + ("1.2.3", {"tag": "v1.2.3"}, False), + ("1.2.3", {"tag": "v1.2.3", "commit": f"{1:07x}"}, False), + # git branch is a moving target + ("1.2.3", {"branch": "releases/1.2"}, False), + # git ref is a moving target + ("git.branch=1.2.3", {}, False), + ], +) +def test_drop_moving_targets(v_str, v_opts, checksummed): + v = Version(v_str) + assert spack.solver.asp._is_checksummed_version((v, v_opts)) == checksummed diff --git a/lib/spack/spack/test/concretize_requirements.py b/lib/spack/spack/test/concretize_requirements.py index 20b89d9e5f..d7c1c88359 100644 --- a/lib/spack/spack/test/concretize_requirements.py +++ b/lib/spack/spack/test/concretize_requirements.py @@ -2,6 +2,7 @@ # Spack Project Developers. See the top-level COPYRIGHT file for details. # # SPDX-License-Identifier: (Apache-2.0 OR MIT) +import os import pathlib import pytest @@ -299,9 +300,14 @@ packages: assert s1.satisfies("@2.2") +@pytest.mark.parametrize("require_checksum", (True, False)) def test_requirement_adds_git_hash_version( - concretize_scope, test_repo, mock_git_version_info, monkeypatch + require_checksum, concretize_scope, test_repo, mock_git_version_info, monkeypatch, working_env ): + # A full commit sha is a checksummed version, so this test should pass in both cases + if require_checksum: + os.environ["SPACK_CONCRETIZER_REQUIRE_CHECKSUM"] = "yes" + repo_path, filename, commits = mock_git_version_info monkeypatch.setattr( spack.package_base.PackageBase, "git", path_to_file_url(repo_path), raising=False diff --git a/lib/spack/spack/util/crypto.py b/lib/spack/spack/util/crypto.py index df8102352e..8eebcc92bc 100644 --- a/lib/spack/spack/util/crypto.py +++ b/lib/spack/spack/util/crypto.py @@ -9,7 +9,8 @@ from typing import Any, Callable, Dict # novm import llnl.util.tty as tty #: Set of hash algorithms that Spack can use, mapped to digest size in bytes -hashes = {"md5": 16, "sha1": 20, "sha224": 28, "sha256": 32, "sha384": 48, "sha512": 64} +hashes = {"sha256": 32, "md5": 16, "sha1": 20, "sha224": 28, "sha384": 48, "sha512": 64} +# Note: keys are ordered by popularity for earliest return in ``hash_key in version_dict`` checks. #: size of hash digests in bytes, mapped to algoritm names -- cgit v1.2.3-60-g2f50