From 01471aee6ba70cfdd50cdf5c5adb69d239dfba8e Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Wed, 2 Apr 2025 09:32:25 +0200 Subject: [PATCH] solver: don't use tags to compute injected deps (#49723) This commit reorders ASP setup, so that rules from possible compilers are collected first. This allows us to know the dependencies that may be injected before counting the possible dependencies, so we can account for them too. Proceeding this way makes it easier to inject complex runtimes, like hip. Signed-off-by: Massimiliano Culpo --- lib/spack/spack/solver/asp.py | 83 ++++++++++--------- lib/spack/spack/solver/input_analysis.py | 14 +--- lib/spack/spack/test/cmd/dependencies.py | 3 +- lib/spack/spack/test/package_class.py | 7 +- .../builtin.mock/packages/corge/package.py | 4 +- .../duplicates.test/packages/compiler-wrapper | 1 + .../edges.test/packages/compiler-wrapper | 1 + 7 files changed, 56 insertions(+), 57 deletions(-) create mode 120000 var/spack/repos/duplicates.test/packages/compiler-wrapper create mode 120000 var/spack/repos/edges.test/packages/compiler-wrapper diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py index 66a2258983a..ec45dc47e2c 100644 --- a/lib/spack/spack/solver/asp.py +++ b/lib/spack/spack/solver/asp.py @@ -2997,8 +2997,36 @@ def setup( """ reuse = reuse or [] check_packages_exist(specs) + self.gen = ProblemInstanceBuilder() - node_counter = create_counter(specs, tests=self.tests, possible_graph=self.possible_graph) + # Compute possible compilers first, so we can record which dependencies they might inject + _ = spack.compilers.config.all_compilers(init_config=True) + + # Get compilers from buildcache only if injected through "reuse" specs + supported_compilers = spack.compilers.config.supported_compilers() + compilers_from_reuse = { + x for x in reuse if x.name in supported_compilers and not x.external + } + candidate_compilers, self.rejected_compilers = possible_compilers( + configuration=spack.config.CONFIG + ) + for x in candidate_compilers: + if x.external or x in reuse: + continue + reuse.append(x) + for dep in x.traverse(root=False, deptype="run"): + reuse.extend(dep.traverse(deptype=("link", "run"))) + + candidate_compilers.update(compilers_from_reuse) + self.possible_compilers = list(candidate_compilers) + self.possible_compilers.sort() # type: ignore[call-overload] + + self.gen.h1("Runtimes") + injected_dependencies = self.define_runtime_constraints() + + node_counter = create_counter( + specs + injected_dependencies, tests=self.tests, possible_graph=self.possible_graph + ) self.possible_virtuals = node_counter.possible_virtuals() self.pkgs = node_counter.possible_dependencies() self.libcs = sorted(all_libcs()) # type: ignore[type-var] @@ -3021,7 +3049,6 @@ def setup( if node.namespace is not None: self.explicitly_required_namespaces[node.name] = node.namespace - self.gen = ProblemInstanceBuilder() self.gen.h1("Generic information") if using_libc_compatibility(): for libc in self.libcs: @@ -3050,27 +3077,6 @@ def setup( specs = tuple(specs) # ensure compatible types to add - _ = spack.compilers.config.all_compilers(init_config=True) - - # Get compilers from buildcache only if injected through "reuse" specs - supported_compilers = spack.compilers.config.supported_compilers() - compilers_from_reuse = { - x for x in reuse if x.name in supported_compilers and not x.external - } - candidate_compilers, self.rejected_compilers = possible_compilers( - configuration=spack.config.CONFIG - ) - for x in candidate_compilers: - if x.external or x in reuse: - continue - reuse.append(x) - for dep in x.traverse(root=False, deptype="run"): - reuse.extend(dep.traverse(deptype=("link", "run"))) - - candidate_compilers.update(compilers_from_reuse) - self.possible_compilers = list(candidate_compilers) - self.possible_compilers.sort() # type: ignore[call-overload] - self.gen.h1("Reusable concrete specs") self.define_concrete_input_specs(specs, self.pkgs) if reuse: @@ -3142,9 +3148,6 @@ def setup( self.gen.h1("Variant Values defined in specs") self.define_variant_values() - self.gen.h1("Runtimes") - self.define_runtime_constraints() - self.gen.h1("Version Constraints") self.collect_virtual_constraints() self.define_version_constraints() @@ -3178,8 +3181,10 @@ def visit(node): path = os.path.join(parent_dir, "concretize.lp") parse_files([path], visit) - def define_runtime_constraints(self): - """Define the constraints to be imposed on the runtimes""" + def define_runtime_constraints(self) -> List[spack.spec.Spec]: + """Define the constraints to be imposed on the runtimes, and returns a list of + injected packages. + """ recorder = RuntimePropertyRecorder(self) for compiler in self.possible_compilers: @@ -3229,6 +3234,7 @@ def define_runtime_constraints(self): ) recorder.consume_facts() + return sorted(recorder.injected_dependencies) def literal_specs(self, specs): for spec in sorted(specs): @@ -3493,6 +3499,7 @@ def __init__(self, setup): self._setup = setup self.rules = [] self.runtime_conditions = set() + self.injected_dependencies = set() # State of this object set in the __call__ method, and reset after # each directive-like method self.current_package = None @@ -3531,6 +3538,7 @@ def depends_on(self, dependency_str: str, *, when: str, type: str, description: if dependency_spec.versions != vn.any_version: self._setup.version_constraints.add((dependency_spec.name, dependency_spec.versions)) + self.injected_dependencies.add(dependency_spec) body_str, node_variable = self.rule_body_from(when_spec) head_clauses = self._setup.spec_clauses(dependency_spec, body=False) @@ -3686,20 +3694,21 @@ def consume_facts(self): """Consume the facts collected by this object, and emits rules and facts for the runtimes. """ + self._setup.gen.h2("Runtimes: declarations") + runtime_pkgs = sorted( + {x.name for x in self.injected_dependencies if not spack.repo.PATH.is_virtual(x.name)} + ) + for runtime_pkg in runtime_pkgs: + self._setup.gen.fact(fn.runtime(runtime_pkg)) + self._setup.gen.newline() + self._setup.gen.h2("Runtimes: rules") self._setup.gen.newline() for rule in self.rules: self._setup.gen.append(rule) + self._setup.gen.newline() - self._setup.gen.h2("Runtimes: conditions") - for runtime_pkg in spack.repo.PATH.packages_with_tags("runtime"): - self._setup.gen.fact(fn.runtime(runtime_pkg)) - self._setup.gen.fact(fn.possible_in_link_run(runtime_pkg)) - self._setup.gen.newline() - # Inject version rules for runtimes (versions are declared based - # on the available compilers) - self._setup.pkg_version_rules(runtime_pkg) - + self._setup.gen.h2("Runtimes: requirements") for imposed_spec, when_spec in sorted(self.runtime_conditions): msg = f"{when_spec} requires {imposed_spec} at runtime" _ = self._setup.condition(when_spec, imposed_spec=imposed_spec, msg=msg) diff --git a/lib/spack/spack/solver/input_analysis.py b/lib/spack/spack/solver/input_analysis.py index d6dce8695d4..1cc3258fc7a 100644 --- a/lib/spack/spack/solver/input_analysis.py +++ b/lib/spack/spack/solver/input_analysis.py @@ -18,8 +18,6 @@ import spack.store from spack.error import SpackError -RUNTIME_TAG = "runtime" - class PossibleGraph(NamedTuple): real_pkgs: Set[str] @@ -50,7 +48,8 @@ def possible_dependencies( ) -> PossibleGraph: """Returns the set of possible dependencies, and the set of possible virtuals. - Both sets always include runtime packages, which may be injected by compilers. + Runtime packages, which may be injected by compilers, needs to be added to specs if + the dependency is not explicit in the package.py recipe. Args: transitive: return transitive dependencies if True, only direct dependencies if False @@ -70,14 +69,9 @@ class NoStaticAnalysis(PossibleDependencyGraph): def __init__(self, *, configuration: spack.config.Configuration, repo: spack.repo.RepoPath): self.configuration = configuration self.repo = repo - self.runtime_pkgs = set(self.repo.packages_with_tags(RUNTIME_TAG)) - self.runtime_virtuals = set() self._platform_condition = spack.spec.Spec( f"platform={spack.platforms.host()} target={archspec.cpu.host().family}:" ) - for x in self.runtime_pkgs: - pkg_class = self.repo.get_pkg_class(x) - self.runtime_virtuals.update(pkg_class.provided_virtual_names()) try: self.libc_pkgs = [x.name for x in self.providers_for("libc")] @@ -214,8 +208,6 @@ def possible_dependencies( for root, children in edges.items(): real_packages.update(x for x in children if self._is_possible(pkg_name=x)) - virtuals.update(self.runtime_virtuals) - real_packages = real_packages | self.runtime_pkgs return PossibleGraph(real_pkgs=real_packages, virtuals=virtuals, edges=edges) def _package_list(self, specs: Tuple[Union[spack.spec.Spec, str], ...]) -> List[str]: @@ -470,7 +462,7 @@ def possible_packages_facts(self, gen, fn): gen.fact(fn.max_dupes(package_name, 1)) gen.newline() - gen.h2("Packages with at multiple possible nodes (build-tools)") + gen.h2("Packages with multiple possible nodes (build-tools)") default = spack.config.CONFIG.get("concretizer:duplicates:max_dupes:default", 2) for package_name in sorted(self.possible_dependencies() & build_tools): max_dupes = spack.config.CONFIG.get( diff --git a/lib/spack/spack/test/cmd/dependencies.py b/lib/spack/spack/test/cmd/dependencies.py index 17b33945389..2abcd2eed5c 100644 --- a/lib/spack/spack/test/cmd/dependencies.py +++ b/lib/spack/spack/test/cmd/dependencies.py @@ -38,10 +38,9 @@ (["--transitive", "--deptype=link", "dtbuild1"], {"dtlink2"}), ], ) -def test_direct_dependencies(cli_args, expected, mock_runtimes): +def test_direct_dependencies(cli_args, expected, mock_packages): out = dependencies(*cli_args) result = set(re.split(r"\s+", out.strip())) - expected.update(mock_runtimes) assert expected == result diff --git a/lib/spack/spack/test/package_class.py b/lib/spack/spack/test/package_class.py index 3e014647d3e..6d4cdcbfe29 100644 --- a/lib/spack/spack/test/package_class.py +++ b/lib/spack/spack/test/package_class.py @@ -111,14 +111,13 @@ def mpi_names(mock_inspector): ("dtbuild1", {"allowed_deps": dt.LINK}, {"dtbuild1", "dtlink2"}), ], ) -def test_possible_dependencies(pkg_name, fn_kwargs, expected, mock_runtimes, mock_inspector): +def test_possible_dependencies(pkg_name, fn_kwargs, expected, mock_inspector): """Tests possible nodes of mpileaks, under different scenarios.""" - expected.update(mock_runtimes) result, *_ = mock_inspector.possible_dependencies(pkg_name, **fn_kwargs) assert expected == result -def test_possible_dependencies_virtual(mock_inspector, mock_packages, mock_runtimes, mpi_names): +def test_possible_dependencies_virtual(mock_inspector, mock_packages, mpi_names): expected = set(mpi_names) for name in mpi_names: expected.update( @@ -126,7 +125,6 @@ def test_possible_dependencies_virtual(mock_inspector, mock_packages, mock_runti for dep in mock_packages.get_pkg_class(name).dependencies_by_name() if not mock_packages.is_virtual(dep) ) - expected.update(mock_runtimes) expected.update(s.name for s in mock_packages.providers_for("c")) real_pkgs, *_ = mock_inspector.possible_dependencies( @@ -146,7 +144,6 @@ def test_possible_dependencies_with_multiple_classes( pkgs = ["dt-diamond", "mpileaks"] expected = set(mpileaks_possible_deps) expected.update({"dt-diamond", "dt-diamond-left", "dt-diamond-right", "dt-diamond-bottom"}) - expected.update(mock_packages.packages_with_tags("runtime")) real_pkgs, *_ = mock_inspector.possible_dependencies(*pkgs, allowed_deps=dt.ALL) assert set(expected) == real_pkgs diff --git a/var/spack/repos/builtin.mock/packages/corge/package.py b/var/spack/repos/builtin.mock/packages/corge/package.py index 26ff1350b52..12589805910 100644 --- a/var/spack/repos/builtin.mock/packages/corge/package.py +++ b/var/spack/repos/builtin.mock/packages/corge/package.py @@ -110,9 +110,9 @@ class Corge f.write(corge_h) with open("%s/corge/corgegator.cc" % self.stage.source_path, "w", encoding="utf-8") as f: f.write(corgegator_cc) - gpp = which("/usr/bin/g++") + gpp = which("g++") if sys.platform == "darwin": - gpp = which("/usr/bin/clang++") + gpp = which("clang++") gpp( "-Dcorge_EXPORTS", "-I%s" % self.stage.source_path, diff --git a/var/spack/repos/duplicates.test/packages/compiler-wrapper b/var/spack/repos/duplicates.test/packages/compiler-wrapper new file mode 120000 index 00000000000..7e103b3d9e2 --- /dev/null +++ b/var/spack/repos/duplicates.test/packages/compiler-wrapper @@ -0,0 +1 @@ +../../builtin.mock/packages/compiler-wrapper \ No newline at end of file diff --git a/var/spack/repos/edges.test/packages/compiler-wrapper b/var/spack/repos/edges.test/packages/compiler-wrapper new file mode 120000 index 00000000000..7e103b3d9e2 --- /dev/null +++ b/var/spack/repos/edges.test/packages/compiler-wrapper @@ -0,0 +1 @@ +../../builtin.mock/packages/compiler-wrapper \ No newline at end of file