From ddae696cf8f2a130773f9b21490c95ed7a30b5c4 Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Tue, 16 Jan 2024 11:47:32 +0100 Subject: [PATCH] Fix using fully-qualified namespaces from root specs (#41957) Explicitly requested namespaces are annotated during the setup phase, and used to retrieve the correct package class. An attribute for the namespace has been added for each node. Currently, a single namespace per package is allowed during concretization. --- lib/spack/spack/package_base.py | 35 +++++++++++----- lib/spack/spack/solver/asp.py | 41 ++++++++++++------- lib/spack/spack/solver/concretize.lp | 3 ++ lib/spack/spack/test/concretize.py | 27 ++++++++++++ .../builtin.mock/packages/gmake/package.py | 1 + 5 files changed, 82 insertions(+), 25 deletions(-) diff --git a/lib/spack/spack/package_base.py b/lib/spack/spack/package_base.py index 92c456f7fe..a2176565fe 100644 --- a/lib/spack/spack/package_base.py +++ b/lib/spack/spack/package_base.py @@ -24,6 +24,7 @@ import textwrap import time import traceback +import typing import warnings from typing import Any, Callable, Dict, Iterable, List, Optional, Set, Tuple, Type, TypeVar, Union @@ -732,13 +733,13 @@ def dependencies_by_name(cls, when: bool = False): @classmethod def possible_dependencies( cls, - transitive=True, - expand_virtuals=True, + transitive: bool = True, + expand_virtuals: bool = True, depflag: dt.DepFlag = dt.ALL, - visited=None, - missing=None, - virtuals=None, - ): + visited: Optional[dict] = None, + missing: Optional[dict] = None, + virtuals: Optional[set] = None, + ) -> Dict[str, Set[str]]: """Return dict of possible dependencies of this package. Args: @@ -2493,14 +2494,21 @@ def flatten_dependencies(spec, flat_dir): dep_files.merge(flat_dir + "/" + name) -def possible_dependencies(*pkg_or_spec, **kwargs): +def possible_dependencies( + *pkg_or_spec: Union[str, spack.spec.Spec, typing.Type[PackageBase]], + transitive: bool = True, + expand_virtuals: bool = True, + depflag: dt.DepFlag = dt.ALL, + missing: Optional[dict] = None, + virtuals: Optional[set] = None, +) -> Dict[str, Set[str]]: """Get the possible dependencies of a number of packages. See ``PackageBase.possible_dependencies`` for details. """ packages = [] for pos in pkg_or_spec: - if isinstance(pos, PackageMeta): + if isinstance(pos, PackageMeta) and issubclass(pos, PackageBase): packages.append(pos) continue @@ -2513,9 +2521,16 @@ def possible_dependencies(*pkg_or_spec, **kwargs): else: packages.append(pos.package_class) - visited = {} + visited: Dict[str, Set[str]] = {} for pkg in packages: - pkg.possible_dependencies(visited=visited, **kwargs) + pkg.possible_dependencies( + visited=visited, + transitive=transitive, + expand_virtuals=expand_virtuals, + depflag=depflag, + missing=missing, + virtuals=virtuals, + ) return visited diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py index 0f38331412..bc9a3e2fd7 100644 --- a/lib/spack/spack/solver/asp.py +++ b/lib/spack/spack/solver/asp.py @@ -13,6 +13,7 @@ import re import sys import types +import typing import warnings from typing import Callable, Dict, List, NamedTuple, Optional, Sequence, Set, Tuple, Union @@ -418,7 +419,7 @@ def check_packages_exist(specs): for spec in specs: for s in spec.traverse(): try: - check_passed = repo.exists(s.name) or repo.is_virtual(s.name) + check_passed = repo.repo_for_pkg(s).exists(s.name) or repo.is_virtual(s.name) except Exception as e: msg = "Cannot find package: {0}".format(str(e)) check_passed = False @@ -1175,6 +1176,7 @@ def __init__(self, tests=False): # Set during the call to setup self.pkgs = None + self.explicitly_required_namespaces = {} def pkg_version_rules(self, pkg): """Output declared versions of a package. @@ -1187,7 +1189,9 @@ def key_fn(version): # Origins are sorted by "provenance" first, see the Provenance enumeration above return version.origin, version.idx - pkg = packagize(pkg) + if isinstance(pkg, str): + pkg = self.pkg_class(pkg) + declared_versions = self.declared_versions[pkg.name] partially_sorted_versions = sorted(set(declared_versions), key=key_fn) @@ -1406,7 +1410,10 @@ def reject_requirement_constraint( return False def pkg_rules(self, pkg, tests): - pkg = packagize(pkg) + pkg = self.pkg_class(pkg) + + # Namespace of the package + self.gen.fact(fn.pkg_fact(pkg.name, fn.namespace(pkg.namespace))) # versions self.pkg_version_rules(pkg) @@ -2038,7 +2045,7 @@ class Body: if not spec.concrete: reserved_names = spack.directives.reserved_names if not spec.virtual and vname not in reserved_names: - pkg_cls = spack.repo.PATH.get_pkg_class(spec.name) + pkg_cls = self.pkg_class(spec.name) try: variant_def, _ = pkg_cls.variants[vname] except KeyError: @@ -2159,7 +2166,7 @@ def define_package_versions_and_validate_preferences( """Declare any versions in specs not declared in packages.""" packages_yaml = spack.config.get("packages") for pkg_name in possible_pkgs: - pkg_cls = spack.repo.PATH.get_pkg_class(pkg_name) + pkg_cls = self.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 @@ -2621,8 +2628,6 @@ def setup( """ check_packages_exist(specs) - self.possible_virtuals = set(x.name for x in specs if x.virtual) - node_counter = _create_counter(specs, tests=self.tests) self.possible_virtuals = node_counter.possible_virtuals() self.pkgs = node_counter.possible_dependencies() @@ -2638,6 +2643,10 @@ def setup( if missing_deps: raise spack.spec.InvalidDependencyError(spec.name, missing_deps) + for node in spack.traverse.traverse_nodes(specs): + if node.namespace is not None: + self.explicitly_required_namespaces[node.name] = node.namespace + # driver is used by all the functions below to add facts and # rules to generate an ASP program. self.gen = driver @@ -2866,6 +2875,13 @@ def _specs_from_requires(self, pkg_name, section): for s in spec_group[key]: yield _spec_with_default_name(s, pkg_name) + def pkg_class(self, pkg_name: str) -> typing.Type["spack.package_base.PackageBase"]: + request = pkg_name + if pkg_name in self.explicitly_required_namespaces: + namespace = self.explicitly_required_namespaces[pkg_name] + request = f"{namespace}.{pkg_name}" + return spack.repo.PATH.get_pkg_class(request) + class RuntimePropertyRecorder: """An object of this class is injected in callbacks to compilers, to let them declare @@ -3077,6 +3093,9 @@ def _arch(self, node): self._specs[node].architecture = arch return arch + def namespace(self, node, namespace): + self._specs[node].namespace = namespace + def node_platform(self, node, platform): self._arch(node).platform = platform @@ -3291,14 +3310,6 @@ def build_specs(self, function_tuples): action(*args) - # namespace assignment is done after the fact, as it is not - # currently part of the solve - for spec in self._specs.values(): - if spec.namespace: - continue - repo = spack.repo.PATH.repo_for_pkg(spec) - spec.namespace = repo.namespace - # fix flags after all specs are constructed self.reorder_flags() diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp index 21cdbbf848..c56f2bcc66 100644 --- a/lib/spack/spack/solver/concretize.lp +++ b/lib/spack/spack/solver/concretize.lp @@ -45,6 +45,9 @@ :- attr("depends_on", node(min_dupe_id, Package), node(ID, _), "link"), ID != min_dupe_id, unification_set("root", node(min_dupe_id, Package)), internal_error("link dependency out of the root unification set"). :- attr("depends_on", node(min_dupe_id, Package), node(ID, _), "run"), ID != min_dupe_id, unification_set("root", node(min_dupe_id, Package)), internal_error("run dependency out of the root unification set"). +% Namespaces are statically assigned by a package fact +attr("namespace", node(ID, Package), Namespace) :- attr("node", node(ID, Package)), pkg_fact(Package, namespace(Namespace)). + % Rules on "unification sets", i.e. on sets of nodes allowing a single configuration of any given package unify(SetID, PackageName) :- unification_set(SetID, node(_, PackageName)). :- 2 { unification_set(SetID, node(_, PackageName)) }, unify(SetID, PackageName). diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py index b6ddf6c4dd..70743d8704 100644 --- a/lib/spack/spack/test/concretize.py +++ b/lib/spack/spack/test/concretize.py @@ -2207,6 +2207,33 @@ def test_reuse_python_from_cli_and_extension_from_db(self, mutable_database): assert with_reuse.dag_hash() == without_reuse.dag_hash() + @pytest.mark.regression("35536") + @pytest.mark.parametrize( + "spec_str,expected_namespaces", + [ + # Single node with fully qualified namespace + ("builtin.mock.gmake", {"gmake": "builtin.mock"}), + # Dependency with fully qualified namespace + ("hdf5 ^builtin.mock.gmake", {"gmake": "builtin.mock", "hdf5": "duplicates.test"}), + ("hdf5 ^gmake", {"gmake": "duplicates.test", "hdf5": "duplicates.test"}), + ], + ) + @pytest.mark.only_clingo("Uses specs requiring multiple gmake specs") + def test_select_lower_priority_package_from_repository_stack( + self, spec_str, expected_namespaces + ): + """Tests that a user can explicitly select a lower priority, fully qualified dependency + from cli. + """ + # 'builtin.mock" and "duplicates.test" share a 'gmake' package + additional_repo = os.path.join(spack.paths.repos_path, "duplicates.test") + with spack.repo.use_repositories(additional_repo, override=False): + s = Spec(spec_str).concretized() + + for name, namespace in expected_namespaces.items(): + assert s[name].concrete + assert s[name].namespace == namespace + @pytest.fixture() def duplicates_test_repository(): diff --git a/var/spack/repos/builtin.mock/packages/gmake/package.py b/var/spack/repos/builtin.mock/packages/gmake/package.py index 5fc2748dee..4ab856a3ad 100644 --- a/var/spack/repos/builtin.mock/packages/gmake/package.py +++ b/var/spack/repos/builtin.mock/packages/gmake/package.py @@ -13,6 +13,7 @@ class Gmake(Package): url = "https://ftpmirror.gnu.org/make/make-4.4.tar.gz" version("4.4", sha256="ce35865411f0490368a8fc383f29071de6690cbadc27704734978221f25e2bed") + version("3.0", sha256="ce35865411f0490368a8fc383f29071de6690cbadc27704734978221f25e2bed") def do_stage(self): mkdirp(self.stage.source_path)