From b12d65ce926ebce0f4525f8bda41a0ea250f3deb Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Sun, 8 Dec 2024 13:44:11 +0100 Subject: [PATCH] spec lookup: separate module --- lib/spack/spack/cmd/__init__.py | 4 +- lib/spack/spack/cmd/diff.py | 3 +- lib/spack/spack/concretize.py | 4 +- lib/spack/spack/error.py | 7 +++ lib/spack/spack/solver/asp.py | 7 ++- lib/spack/spack/spec.py | 81 +------------------------- lib/spack/spack/spec_list.py | 3 +- lib/spack/spack/spec_lookup.py | 79 +++++++++++++++++++++++++ lib/spack/spack/test/spec_semantics.py | 4 +- lib/spack/spack/test/spec_syntax.py | 49 ++++++++-------- 10 files changed, 128 insertions(+), 113 deletions(-) create mode 100644 lib/spack/spack/spec_lookup.py diff --git a/lib/spack/spack/cmd/__init__.py b/lib/spack/spack/cmd/__init__.py index 221b6c60b76..52b3124258a 100644 --- a/lib/spack/spack/cmd/__init__.py +++ b/lib/spack/spack/cmd/__init__.py @@ -26,6 +26,7 @@ import spack.paths import spack.repo import spack.spec +import spack.spec_lookup import spack.spec_parser import spack.store import spack.traverse as traverse @@ -211,7 +212,8 @@ def _concretize_spec_pairs( ): # Get all the concrete specs ret = [ - concrete or (abstract if abstract.concrete else abstract.lookup_hash()) + concrete + or (abstract if abstract.concrete else spack.spec_lookup.lookup_hash(abstract)) for abstract, concrete in to_concretize ] diff --git a/lib/spack/spack/cmd/diff.py b/lib/spack/spack/cmd/diff.py index 28c8b3944ff..018086c9e69 100644 --- a/lib/spack/spack/cmd/diff.py +++ b/lib/spack/spack/cmd/diff.py @@ -11,6 +11,7 @@ import spack.cmd import spack.environment as ev import spack.solver.asp as asp +import spack.spec_lookup import spack.util.spack_json as sjson from spack.cmd.common import arguments @@ -210,7 +211,7 @@ def diff(parser, args): specs = [] for spec in spack.cmd.parse_specs(args.specs): # If the spec has a hash, check it before disambiguating - spec.replace_hash() + spack.spec_lookup.replace_hash(spec) if spec.concrete: specs.append(spec) else: diff --git a/lib/spack/spack/concretize.py b/lib/spack/spack/concretize.py index 78a46a33ab8..4e7f3ba91c0 100644 --- a/lib/spack/spack/concretize.py +++ b/lib/spack/spack/concretize.py @@ -199,10 +199,12 @@ def concretize_one(spec: Union[str, Spec], tests: TestsType = False) -> Spec: the packages in the list, if True activate 'test' dependencies for all packages. """ from spack.solver.asp import Solver, SpecBuilder + from spack.spec_lookup import replace_hash if isinstance(spec, str): spec = Spec(spec) - spec = spec.lookup_hash() + + replace_hash(spec) if spec.concrete: return spec.copy() diff --git a/lib/spack/spack/error.py b/lib/spack/spack/error.py index 92eb5f951ab..5713962e58d 100644 --- a/lib/spack/spack/error.py +++ b/lib/spack/spack/error.py @@ -202,3 +202,10 @@ class MirrorError(SpackError): def __init__(self, msg, long_msg=None): super().__init__(msg, long_msg) + + +class InvalidHashError(SpecError): + def __init__(self, spec, hash): + msg = f"No spec with hash {hash} could be found to match {spec}." + msg += " Either the hash does not exist, or it does not match other spec constraints." + super().__init__(msg) diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py index ce0f0a48c58..07fcf852f9c 100644 --- a/lib/spack/spack/solver/asp.py +++ b/lib/spack/spack/solver/asp.py @@ -39,6 +39,7 @@ import spack.repo import spack.solver.splicing import spack.spec +import spack.spec_lookup import spack.store import spack.util.crypto import spack.util.libc @@ -3774,7 +3775,7 @@ def execute_explicit_splices(self): # The first iteration, we need to replace the abstract hash if not replacement.concrete: - replacement.replace_hash() + spack.spec_lookup.replace_hash(replacement) current_spec = current_spec.splice(replacement, transitive) new_key = NodeArgument(id=key.id, pkg=current_spec.name) specs[new_key] = current_spec @@ -4133,7 +4134,7 @@ def solve_with_stats( setup_only (bool): if True, stop after setup and don't solve (default False). allow_deprecated (bool): allow deprecated version in the solve """ - specs = [s.lookup_hash() for s in specs] + specs = [spack.spec_lookup.lookup_hash(s) for s in specs] reusable_specs = self._check_input_and_extract_concrete_specs(specs) reusable_specs.extend(self.selector.reusable_specs(specs)) setup = SpackSolverSetup(tests=tests) @@ -4170,7 +4171,7 @@ def solve_in_rounds( tests (bool): add test dependencies to the solve allow_deprecated (bool): allow deprecated version in the solve """ - specs = [s.lookup_hash() for s in specs] + specs = [spack.spec_lookup.lookup_hash(s) for s in specs] reusable_specs = self._check_input_and_extract_concrete_specs(specs) reusable_specs.extend(self.selector.reusable_specs(specs)) setup = SpackSolverSetup(tests=tests) diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index 3391cf307a9..9975708b5c3 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -106,8 +106,6 @@ import spack.version as vn import spack.version.git_ref_lookup -from .enums import InstallRecordStatus - __all__ = [ "CompilerSpec", "Spec", @@ -128,8 +126,6 @@ "UnsatisfiableArchitectureSpecError", "UnsatisfiableProviderSpecError", "UnsatisfiableDependencySpecError", - "AmbiguousHashError", - "InvalidHashError", "SpecDeprecatedError", ] @@ -2170,66 +2166,6 @@ def process_hash_bit_prefix(self, bits): """Get the first bits of the DAG hash as an integer type.""" return spack.util.hash.base32_prefix_bits(self.process_hash(), bits) - def _lookup_hash(self): - """Lookup just one spec with an abstract hash, returning a spec from the the environment, - store, or finally, binary caches.""" - import spack.binary_distribution - import spack.environment - - active_env = spack.environment.active_environment() - - # First env, then store, then binary cache - matches = ( - (active_env.all_matching_specs(self) if active_env else []) - or spack.store.STORE.db.query(self, installed=InstallRecordStatus.ANY) - or spack.binary_distribution.BinaryCacheQuery(True)(self) - ) - - if not matches: - raise InvalidHashError(self, self.abstract_hash) - - if len(matches) != 1: - raise AmbiguousHashError( - f"Multiple packages specify hash beginning '{self.abstract_hash}'.", *matches - ) - - return matches[0] - - def lookup_hash(self): - """Given a spec with an abstract hash, return a copy of the spec with all properties and - dependencies by looking up the hash in the environment, store, or finally, binary caches. - This is non-destructive.""" - if self.concrete or not any(node.abstract_hash for node in self.traverse()): - return self - - spec = self.copy(deps=False) - # root spec is replaced - if spec.abstract_hash: - spec._dup(self._lookup_hash()) - return spec - - # Get dependencies that need to be replaced - for node in self.traverse(root=False): - if node.abstract_hash: - spec._add_dependency(node._lookup_hash(), depflag=0, virtuals=()) - - # reattach nodes that were not otherwise satisfied by new dependencies - for node in self.traverse(root=False): - if not any(n.satisfies(node) for n in spec.traverse()): - spec._add_dependency(node.copy(), depflag=0, virtuals=()) - - return spec - - def replace_hash(self): - """Given a spec with an abstract hash, attempt to populate all properties and dependencies - by looking up the hash in the environment, store, or finally, binary caches. - This is destructive.""" - - if not any(node for node in self.traverse(order="post") if node.abstract_hash): - return - - self._dup(self.lookup_hash()) - def to_node_dict(self, hash=ht.dag_hash): """Create a dictionary representing the state of this Spec. @@ -3132,7 +3068,7 @@ def constrain(self, other, deps=True): if not self.abstract_hash or other.abstract_hash.startswith(self.abstract_hash): self.abstract_hash = other.abstract_hash elif not self.abstract_hash.startswith(other.abstract_hash): - raise InvalidHashError(self, other.abstract_hash) + raise spack.error.InvalidHashError(self, other.abstract_hash) if not (self.name == other.name or (not self.name) or (not other.name)): raise UnsatisfiableSpecNameError(self.name, other.name) @@ -5339,21 +5275,6 @@ def __init__(self, spec): super().__init__(msg) -class AmbiguousHashError(spack.error.SpecError): - def __init__(self, msg, *specs): - spec_fmt = "{namespace}.{name}{@version}{%compiler}{compiler_flags}" - spec_fmt += "{variants}{ arch=architecture}{/hash:7}" - specs_str = "\n " + "\n ".join(spec.format(spec_fmt) for spec in specs) - super().__init__(msg + specs_str) - - -class InvalidHashError(spack.error.SpecError): - def __init__(self, spec, hash): - msg = f"No spec with hash {hash} could be found to match {spec}." - msg += " Either the hash does not exist, or it does not match other spec constraints." - super().__init__(msg) - - class SpecFilenameError(spack.error.SpecError): """Raised when a spec file name is invalid.""" diff --git a/lib/spack/spack/spec_list.py b/lib/spack/spack/spec_list.py index eadc28fa8f8..ac51a3b111c 100644 --- a/lib/spack/spack/spec_list.py +++ b/lib/spack/spack/spec_list.py @@ -5,6 +5,7 @@ from typing import List import spack.spec +import spack.spec_lookup import spack.variant from spack.error import SpackError from spack.spec import Spec @@ -230,7 +231,7 @@ def _expand_matrix_constraints(matrix_config): pass # Resolve abstract hashes for exclusion criteria - if any(test_spec.lookup_hash().satisfies(x) for x in excludes): + if any(spack.spec_lookup.lookup_hash(test_spec).satisfies(x) for x in excludes): continue if sigil: diff --git a/lib/spack/spack/spec_lookup.py b/lib/spack/spack/spec_lookup.py new file mode 100644 index 00000000000..e98544d9fa9 --- /dev/null +++ b/lib/spack/spack/spec_lookup.py @@ -0,0 +1,79 @@ +# Copyright Spack Project Developers. See COPYRIGHT file for details. +# +# SPDX-License-Identifier: (Apache-2.0 OR MIT) +import spack.binary_distribution +import spack.environment +import spack.error +import spack.spec +import spack.store + +from .enums import InstallRecordStatus + + +def _lookup_hash(spec: spack.spec.Spec): + """Lookup just one spec with an abstract hash, returning a spec from the the environment, + store, or finally, binary caches.""" + + active_env = spack.environment.active_environment() + + # First env, then store, then binary cache + matches = ( + (active_env.all_matching_specs(spec) if active_env else []) + or spack.store.STORE.db.query(spec, installed=InstallRecordStatus.ANY) + or spack.binary_distribution.BinaryCacheQuery(True)(spec) + ) + + if not matches: + raise spack.error.InvalidHashError(spec, spec.abstract_hash) + + if len(matches) != 1: + raise AmbiguousHashError( + f"Multiple packages specify hash beginning '{spec.abstract_hash}'.", *matches + ) + + return matches[0] + + +def lookup_hash(spec: spack.spec.Spec) -> spack.spec.Spec: + """Given a spec with an abstract hash, return a copy of the spec with all properties and + dependencies by looking up the hash in the environment, store, or finally, binary caches. + This is non-destructive.""" + if spec.concrete or not any(node.abstract_hash for node in spec.traverse()): + return spec + + spec = spec.copy(deps=False) + # root spec is replaced + if spec.abstract_hash: + spec._dup(_lookup_hash(spec)) + return spec + + # Get dependencies that need to be replaced + for node in spec.traverse(root=False): + if node.abstract_hash: + spec._add_dependency(_lookup_hash(node), depflag=0, virtuals=()) + + # reattach nodes that were not otherwise satisfied by new dependencies + for node in spec.traverse(root=False): + if not any(n.satisfies(node) for n in spec.traverse()): + spec._add_dependency(node.copy(), depflag=0, virtuals=()) + + return spec + + +def replace_hash(spec: spack.spec.Spec) -> None: + """Given a spec with an abstract hash, attempt to populate all properties and dependencies + by looking up the hash in the environment, store, or finally, binary caches. + This is destructive.""" + + if not any(node for node in spec.traverse(order="post") if node.abstract_hash): + return + + spec._dup(lookup_hash(spec)) + + +class AmbiguousHashError(spack.error.SpecError): + def __init__(self, msg, *specs): + spec_fmt = "{namespace}.{name}{@version}{%compiler}{compiler_flags}" + spec_fmt += "{variants}{ arch=architecture}{/hash:7}" + specs_str = "\n " + "\n ".join(spec.format(spec_fmt) for spec in specs) + super().__init__(msg + specs_str) diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py index df3c1ddfacb..4c2ce0cccec 100644 --- a/lib/spack/spack/test/spec_semantics.py +++ b/lib/spack/spack/test/spec_semantics.py @@ -427,9 +427,9 @@ def test_mismatched_constrain_spec_by_hash(self, default_mock_concretization, da """Test that Specs specified only by their incompatible hashes fail appropriately.""" lhs = "/" + database.query_one("callpath ^mpich").dag_hash() rhs = "/" + database.query_one("callpath ^mpich2").dag_hash() - with pytest.raises(spack.spec.InvalidHashError): + with pytest.raises(spack.error.InvalidHashError): Spec(lhs).constrain(Spec(rhs)) - with pytest.raises(spack.spec.InvalidHashError): + with pytest.raises(spack.error.InvalidHashError): Spec(lhs[:7]).constrain(Spec(rhs)) @pytest.mark.parametrize( diff --git a/lib/spack/spack/test/spec_syntax.py b/lib/spack/spack/test/spec_syntax.py index e00fb3a7eee..7aff8f1b6a1 100644 --- a/lib/spack/spack/test/spec_syntax.py +++ b/lib/spack/spack/test/spec_syntax.py @@ -11,9 +11,11 @@ import spack.binary_distribution import spack.cmd import spack.concretize +import spack.error import spack.platforms.test import spack.repo import spack.spec +from spack.spec_lookup import AmbiguousHashError, lookup_hash, replace_hash from spack.spec_parser import ( UNIX_FILENAME, WINDOWS_FILENAME, @@ -26,7 +28,7 @@ FAIL_ON_WINDOWS = pytest.mark.xfail( sys.platform == "win32", - raises=(SpecTokenizationError, spack.spec.InvalidHashError), + raises=(SpecTokenizationError, spack.error.InvalidHashError), reason="Unix style path on Windows", ) @@ -782,22 +784,22 @@ def test_spec_by_hash(database, monkeypatch, config): hash_str = f"/{mpileaks.dag_hash()}" parsed_spec = SpecParser(hash_str).next_spec() - parsed_spec.replace_hash() + replace_hash(parsed_spec) assert parsed_spec == mpileaks short_hash_str = f"/{mpileaks.dag_hash()[:5]}" parsed_spec = SpecParser(short_hash_str).next_spec() - parsed_spec.replace_hash() + replace_hash(parsed_spec) assert parsed_spec == mpileaks name_version_and_hash = f"{mpileaks.name}@{mpileaks.version} /{mpileaks.dag_hash()[:5]}" parsed_spec = SpecParser(name_version_and_hash).next_spec() - parsed_spec.replace_hash() + replace_hash(parsed_spec) assert parsed_spec == mpileaks b_hash = f"/{b.dag_hash()}" parsed_spec = SpecParser(b_hash).next_spec() - parsed_spec.replace_hash() + replace_hash(parsed_spec) assert parsed_spec == b @@ -811,7 +813,7 @@ def test_dep_spec_by_hash(database, config): assert "zmpi" in mpileaks_zmpi mpileaks_hash_fake = SpecParser(f"mpileaks ^/{fake.dag_hash()} ^zmpi").next_spec() - mpileaks_hash_fake.replace_hash() + replace_hash(mpileaks_hash_fake) assert "fake" in mpileaks_hash_fake assert mpileaks_hash_fake["fake"] == fake assert "zmpi" in mpileaks_hash_fake @@ -820,7 +822,7 @@ def test_dep_spec_by_hash(database, config): mpileaks_hash_zmpi = SpecParser( f"mpileaks %{mpileaks_zmpi.compiler} ^ /{zmpi.dag_hash()}" ).next_spec() - mpileaks_hash_zmpi.replace_hash() + replace_hash(mpileaks_hash_zmpi) assert "zmpi" in mpileaks_hash_zmpi assert mpileaks_hash_zmpi["zmpi"] == zmpi assert mpileaks_zmpi.compiler.satisfies(mpileaks_hash_zmpi.compiler) @@ -828,7 +830,7 @@ def test_dep_spec_by_hash(database, config): mpileaks_hash_fake_and_zmpi = SpecParser( f"mpileaks ^/{fake.dag_hash()[:4]} ^ /{zmpi.dag_hash()[:5]}" ).next_spec() - mpileaks_hash_fake_and_zmpi.replace_hash() + replace_hash(mpileaks_hash_fake_and_zmpi) assert "zmpi" in mpileaks_hash_fake_and_zmpi assert mpileaks_hash_fake_and_zmpi["zmpi"] == zmpi @@ -888,13 +890,13 @@ def test_ambiguous_hash(mutable_database): # ambiguity in first hash character s1 = SpecParser("/x").next_spec() - with pytest.raises(spack.spec.AmbiguousHashError): - s1.lookup_hash() + with pytest.raises(AmbiguousHashError): + lookup_hash(s1) # ambiguity in first hash character AND spec name s2 = SpecParser("pkg-a/x").next_spec() - with pytest.raises(spack.spec.AmbiguousHashError): - s2.lookup_hash() + with pytest.raises(AmbiguousHashError): + lookup_hash(s2) @pytest.mark.db @@ -903,24 +905,24 @@ def test_invalid_hash(database, config): mpich = database.query_one("mpich") # name + incompatible hash - with pytest.raises(spack.spec.InvalidHashError): + with pytest.raises(spack.error.InvalidHashError): parsed_spec = SpecParser(f"zmpi /{mpich.dag_hash()}").next_spec() - parsed_spec.replace_hash() - with pytest.raises(spack.spec.InvalidHashError): + replace_hash(parsed_spec) + with pytest.raises(spack.error.InvalidHashError): parsed_spec = SpecParser(f"mpich /{zmpi.dag_hash()}").next_spec() - parsed_spec.replace_hash() + replace_hash(parsed_spec) # name + dep + incompatible hash - with pytest.raises(spack.spec.InvalidHashError): + with pytest.raises(spack.error.InvalidHashError): parsed_spec = SpecParser(f"mpileaks ^zmpi /{mpich.dag_hash()}").next_spec() - parsed_spec.replace_hash() + replace_hash(parsed_spec) def test_invalid_hash_dep(database, config): mpich = database.query_one("mpich") hash = mpich.dag_hash() - with pytest.raises(spack.spec.InvalidHashError): - spack.spec.Spec(f"callpath ^zlib/{hash}").replace_hash() + with pytest.raises(spack.error.InvalidHashError): + replace_hash(spack.spec.Spec(f"callpath ^zlib/{hash}")) @pytest.mark.db @@ -933,9 +935,8 @@ def test_nonexistent_hash(database, config): hashes = [s._hash for s in specs] assert no_such_hash not in [h[: len(no_such_hash)] for h in hashes] - with pytest.raises(spack.spec.InvalidHashError): - parsed_spec = SpecParser(f"/{no_such_hash}").next_spec() - parsed_spec.replace_hash() + with pytest.raises(spack.error.InvalidHashError): + replace_hash(SpecParser(f"/{no_such_hash}").next_spec()) @pytest.mark.parametrize( @@ -966,7 +967,7 @@ def test_disambiguate_hash_by_spec(spec1, spec2, constraint, mock_packages, monk else: spec = spack.spec.Spec("/spec" + constraint) - assert spec.lookup_hash() == spec1_concrete + assert lookup_hash(spec) == spec1_concrete @pytest.mark.parametrize(