From acd47147a512caea9c7e1f9adf7618f725207370 Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Fri, 9 May 2025 23:08:21 +0200 Subject: [PATCH] solver: support concrete multivalued variants (#50325) The solves now supports key:=val syntax for multivalued variants in specs originating from input, externals, requirements, directives and when conditions Signed-off-by: Massimiliano Culpo --- lib/spack/spack/solver/asp.py | 17 +++- lib/spack/spack/solver/concretize.lp | 55 +++++++++-- lib/spack/spack/spec.py | 3 + lib/spack/spack/test/concretization/core.py | 96 +++++++++++++++++++ .../builtin/packages/gcc/detection_test.yaml | 6 +- .../builtin/packages/gcc/package.py | 6 +- .../gmt-concrete-mv-dependency/package.py | 13 +++ .../packages/mvdefaults/package.py | 4 + 8 files changed, 185 insertions(+), 15 deletions(-) create mode 100644 var/spack/test_repos/builtin.mock/packages/gmt-concrete-mv-dependency/package.py diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py index a3e04fcac03..ff75556c22c 100644 --- a/lib/spack/spack/solver/asp.py +++ b/lib/spack/spack/solver/asp.py @@ -2512,7 +2512,22 @@ def _spec_clauses( if self.pkg_class(spec.name).has_variant(vname): clauses.append(f.variant_value(spec.name, vname, value)) else: - clauses.append(f.variant_value(spec.name, vname, value)) + variant_clause = f.variant_value(spec.name, vname, value) + if ( + variant.concrete + and variant.type == vt.VariantType.MULTI + and not spec.concrete + ): + if body is False: + variant_clause.args = ( + f"concrete_{variant_clause.args[0]}", + *variant_clause.args[1:], + ) + else: + clauses.append( + fn.attr("concrete_variant_request", spec.name, vname, value) + ) + clauses.append(variant_clause) # compiler flags source = context.source if context else "none" diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp index 3ed05e81691..088a83d1c2c 100644 --- a/lib/spack/spack/solver/concretize.lp +++ b/lib/spack/spack/solver/concretize.lp @@ -159,10 +159,12 @@ unification_set(SetID, VirtualNode) % TODO: literals, at the moment, can only influence the "root" unification set. This needs to be extended later. -% Node attributes that have multiple node arguments (usually, only the first argument is a node) -multiple_nodes_attribute("depends_on"). -multiple_nodes_attribute("virtual_on_edge"). -multiple_nodes_attribute("provider_set"). +% Node attributes that need custom rules in ASP, e.g. because they involve multiple nodes +node_attributes_with_custom_rules("depends_on"). +node_attributes_with_custom_rules("virtual_on_edge"). +node_attributes_with_custom_rules("provider_set"). +node_attributes_with_custom_rules("concrete_variant_set"). +node_attributes_with_custom_rules("concrete_variant_request"). trigger_condition_holds(TriggerID, node(min_dupe_id, Package)) :- solve_literal(TriggerID), @@ -397,12 +399,26 @@ trigger_condition_holds(ID, RequestorNode) :- trigger_node(ID, PackageNode, RequestorNode); attr(Name, node(X, A1)) : condition_requirement(ID, Name, A1), condition_nodes(ID, PackageNode, node(X, A1)); attr(Name, node(X, A1), A2) : condition_requirement(ID, Name, A1, A2), condition_nodes(ID, PackageNode, node(X, A1)); - attr(Name, node(X, A1), A2, A3) : condition_requirement(ID, Name, A1, A2, A3), condition_nodes(ID, PackageNode, node(X, A1)), not multiple_nodes_attribute(Name); + attr(Name, node(X, A1), A2, A3) : condition_requirement(ID, Name, A1, A2, A3), condition_nodes(ID, PackageNode, node(X, A1)), not node_attributes_with_custom_rules(Name); attr(Name, node(X, A1), A2, A3, A4) : condition_requirement(ID, Name, A1, A2, A3, A4), condition_nodes(ID, PackageNode, node(X, A1)); % Special cases attr("depends_on", node(X, A1), node(Y, A2), A3) : condition_requirement(ID, "depends_on", A1, A2, A3), condition_nodes(ID, PackageNode, node(X, A1)), condition_nodes(ID, PackageNode, node(Y, A2)); not cannot_hold(ID, PackageNode). +condition_with_concrete_variant(ID, Package, Variant) :- condition_requirement(ID, "concrete_variant_request", Package, Variant, _). + +cannot_hold(ID, PackageNode) :- + not attr("variant_value", node(X, A1), Variant, Value), + condition_with_concrete_variant(ID, A1, Variant), + condition_requirement(ID, "concrete_variant_request", A1, Variant, Value), + condition_nodes(ID, PackageNode, node(X, A1)). + +cannot_hold(ID, PackageNode) :- + attr("variant_value", node(X, A1), Variant, Value), + condition_with_concrete_variant(ID, A1, Variant), + not condition_requirement(ID, "concrete_variant_request", A1, Variant, Value), + condition_nodes(ID, PackageNode, node(X, A1)). + condition_holds(ConditionID, node(X, Package)) :- pkg_fact(Package, condition_trigger(ConditionID, TriggerID)), trigger_condition_holds(TriggerID, node(X, Package)). @@ -449,8 +465,8 @@ imposed_nodes(ConditionID, PackageNode, node(X, A1)) % Conditions that hold impose may impose constraints on other specs attr(Name, node(X, A1)) :- impose(ID, PackageNode), imposed_constraint(ID, Name, A1), imposed_nodes(ID, PackageNode, node(X, A1)). -attr(Name, node(X, A1), A2) :- impose(ID, PackageNode), imposed_constraint(ID, Name, A1, A2), imposed_nodes(ID, PackageNode, node(X, A1)), not multiple_nodes_attribute(Name). -attr(Name, node(X, A1), A2, A3) :- impose(ID, PackageNode), imposed_constraint(ID, Name, A1, A2, A3), imposed_nodes(ID, PackageNode, node(X, A1)), not multiple_nodes_attribute(Name). +attr(Name, node(X, A1), A2) :- impose(ID, PackageNode), imposed_constraint(ID, Name, A1, A2), imposed_nodes(ID, PackageNode, node(X, A1)), not node_attributes_with_custom_rules(Name). +attr(Name, node(X, A1), A2, A3) :- impose(ID, PackageNode), imposed_constraint(ID, Name, A1, A2, A3), imposed_nodes(ID, PackageNode, node(X, A1)), not node_attributes_with_custom_rules(Name). attr(Name, node(X, A1), A2, A3, A4) :- impose(ID, PackageNode), imposed_constraint(ID, Name, A1, A2, A3, A4), imposed_nodes(ID, PackageNode, node(X, A1)). % Provider set is relevant only for literals, since it's the only place where `^[virtuals=foo] bar` @@ -471,6 +487,15 @@ provider(ProviderNode, VirtualNode) :- attr("provider_set", ProviderNode, Virtua imposed_constraint(ID, "depends_on", A1, A2, A3), internal_error("Build deps must land in exactly one duplicate"). +% For := we must keep track of the origin of the fact, since we need to check +% each condition separately, i.e. foo:=a,b in one place and foo:=c in another +% should not make foo:=a,b,c possible +attr("concrete_variant_set", node(X, A1), Variant, Value, ID) + :- impose(ID, PackageNode), + imposed_nodes(ID, PackageNode, node(X, A1)), + imposed_constraint(ID, "concrete_variant_set", A1, Variant, Value). + + % The rule below accounts for expressions like: % % root ^dep %compiler @@ -1149,6 +1174,22 @@ error(100, "No valid value for variant '{1}' of package '{0}'", Package, Variant % if a variant is set to anything, it is considered 'set'. attr("variant_set", PackageNode, Variant) :- attr("variant_set", PackageNode, Variant, _). +% Setting a concrete variant implies setting a variant +concrete_variant_value(PackageNode, Variant, Value, Origin) :- attr("concrete_variant_set", PackageNode, Variant, Value, Origin). + +attr("variant_set", PackageNode, Variant, Value) :- attr("concrete_variant_set", PackageNode, Variant, Value, _). + +% Concrete variant values must be in the answer set +:- concrete_variant_value(PackageNode, Variant, Value, _), not attr("variant_value", PackageNode, Variant, Value). + +% Extra variant values are not allowed, if the variant is concrete +variant_is_concrete(PackageNode, Variant, Origin) :- concrete_variant_value(PackageNode, Variant, _, Origin). + +error(100, "The variant {0} in package {1} specified as := has the extra value {2}", Variant, PackageNode, Value) +:- variant_is_concrete(PackageNode, Variant, Origin), + attr("variant_value", PackageNode, Variant, Value), + not concrete_variant_value(PackageNode, Variant, Value, Origin). + % A variant cannot have a value that is not also a possible value % This only applies to packages we need to build -- concrete packages may % have been built w/different variants from older/different package versions. diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index ef4eab6bd93..a5b1b0c8f03 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -4670,6 +4670,9 @@ def substitute_abstract_variants(spec: Spec): # in $spack/lib/spack/spack/spec_list.py unknown = [] for name, v in spec.variants.items(): + if v.concrete and v.type == vt.VariantType.MULTI: + continue + if name == "dev_path": v.type = vt.VariantType.SINGLE v.concrete = True diff --git a/lib/spack/spack/test/concretization/core.py b/lib/spack/spack/test/concretization/core.py index 722174072ed..6241f148e05 100644 --- a/lib/spack/spack/test/concretization/core.py +++ b/lib/spack/spack/test/concretization/core.py @@ -3470,3 +3470,99 @@ def test_installed_compiler_and_better_external( with spack.config.override("concretizer:reuse", False): mpileaks = spack.concretize.concretize_one("mpileaks") assert mpileaks.satisfies("%gcc@10") + + +@pytest.mark.regression("50006") +def test_concrete_multi_valued_variants_in_externals(mutable_config, mock_packages, tmp_path): + """Tests that concrete multivalued variants in externals cannot be extended with additional + values when concretizing. + """ + packages_yaml = syaml.load_config( + f""" +packages: + gcc: + buildable: false + externals: + - spec: gcc@12.1.0 languages:='c,c++' + prefix: {tmp_path / 'gcc-12'} + extra_attributes: + compilers: + c: {tmp_path / 'gcc-12'}/bin/gcc + cxx: {tmp_path / 'gcc-12'}/bin/g++ + + - spec: gcc@14.1.0 languages:=fortran + prefix: {tmp_path / 'gcc-14'} + extra_attributes: + compilers: + fortran: {tmp_path / 'gcc-14'}/bin/gfortran +""" + ) + mutable_config.set("packages", packages_yaml["packages"]) + + with pytest.raises(spack.solver.asp.UnsatisfiableSpecError): + spack.concretize.concretize_one("pkg-b %gcc@14") + + s = spack.concretize.concretize_one("pkg-b %gcc") + assert s["c"].satisfies("gcc@12.1.0"), s.tree() + assert s["c"].external + assert s["c"].satisfies("languages=c,c++") and not s["c"].satisfies("languages=fortran") + + +def test_concrete_multi_valued_in_input_specs(default_mock_concretization): + """Tests that we can use := to specify exactly multivalued variants in input specs.""" + s = default_mock_concretization("gcc languages:=fortran") + assert not s.external and s["c"].external + assert s.satisfies("languages:=fortran") + assert not s.satisfies("languages=c") and not s.satisfies("languages=c++") + + +def test_concrete_multi_valued_variants_in_requirements(mutable_config, mock_packages, tmp_path): + """Tests that concrete multivalued variants can be imposed by requirements.""" + packages_yaml = syaml.load_config( + """ +packages: + pkg-a: + require: + - libs:=static +""" + ) + mutable_config.set("packages", packages_yaml["packages"]) + + with pytest.raises(spack.solver.asp.UnsatisfiableSpecError): + spack.concretize.concretize_one("pkg-a libs=shared") + spack.concretize.concretize_one("pkg-a libs=shared,static") + + s = spack.concretize.concretize_one("pkg-a") + assert s.satisfies("libs:=static") + assert not s.satisfies("libs=shared") + + +def test_concrete_multi_valued_variants_in_depends_on(default_mock_concretization): + """Tests the use of := in depends_on directives""" + with pytest.raises(spack.solver.asp.UnsatisfiableSpecError): + default_mock_concretization("gmt-concrete-mv-dependency ^mvdefaults foo:=c") + default_mock_concretization("gmt-concrete-mv-dependency ^mvdefaults foo:=a,c") + default_mock_concretization("gmt-concrete-mv-dependency ^mvdefaults foo:=b,c") + + s = default_mock_concretization("gmt-concrete-mv-dependency") + assert s.satisfies("^mvdefaults foo:=a,b"), s.tree() + assert not s.satisfies("^mvdefaults foo=c") + + +def test_concrete_multi_valued_variants_when_args(default_mock_concretization): + """Tests the use of := in conflicts and when= arguments""" + # Check conflicts("foo:=a,b", when="@0.9") + with pytest.raises(spack.solver.asp.UnsatisfiableSpecError): + default_mock_concretization("mvdefaults@0.9 foo:=a,b") + + for c in ("foo:=a", "foo:=a,b,c", "foo:=a,c", "foo:=b,c"): + s = default_mock_concretization(f"mvdefaults@0.9 {c}") + assert s.satisfies(c) + + # Check depends_on("pkg-b", when="foo:=b,c") + s = default_mock_concretization("mvdefaults foo:=b,c") + assert s.satisfies("^pkg-b") + + for c in ("foo:=a", "foo:=a,b,c", "foo:=a,b", "foo:=a,c"): + s = default_mock_concretization(f"mvdefaults {c}") + assert not s.satisfies("^pkg-b") diff --git a/var/spack/repos/spack_repo/builtin/packages/gcc/detection_test.yaml b/var/spack/repos/spack_repo/builtin/packages/gcc/detection_test.yaml index 67b5bc1e129..6b0355a6a69 100644 --- a/var/spack/repos/spack_repo/builtin/packages/gcc/detection_test.yaml +++ b/var/spack/repos/spack_repo/builtin/packages/gcc/detection_test.yaml @@ -22,7 +22,7 @@ paths: fi platforms: ["darwin", "linux"] results: - - spec: "gcc@9.4.0 languages=c,c++" + - spec: "gcc@9.4.0 languages:=c,c++" extra_attributes: compilers: c: ".*/bin/gcc" @@ -45,7 +45,7 @@ paths: fi platforms: ["darwin", "linux"] results: - - spec: "gcc@5.5.0 languages=c,c++,fortran" + - spec: "gcc@5.5.0 languages:=c,c++,fortran" extra_attributes: compilers: c: ".*/bin/gcc-5$" @@ -115,7 +115,7 @@ paths: fi platforms: [darwin] results: - - spec: "gcc@14.1.0 languages=c" + - spec: "gcc@14.1.0 languages:=c" extra_attributes: compilers: c: ".*/bin/gcc-14$" diff --git a/var/spack/repos/spack_repo/builtin/packages/gcc/package.py b/var/spack/repos/spack_repo/builtin/packages/gcc/package.py index 42ff4ca8dfc..93332534450 100644 --- a/var/spack/repos/spack_repo/builtin/packages/gcc/package.py +++ b/var/spack/repos/spack_repo/builtin/packages/gcc/package.py @@ -672,15 +672,13 @@ def determine_variants(cls, exes, version_str): translation = {"cxx": "c++"} for lang, compiler in compilers.items(): languages.add(translation.get(lang, lang)) - variant_str = "languages={0}".format(",".join(languages)) + variant_str = "languages:={0}".format(",".join(languages)) return variant_str, {"compilers": compilers} @classmethod def validate_detected_spec(cls, spec, extra_attributes): # For GCC 'compilers' is a mandatory attribute - msg = 'the extra attribute "compilers" must be set for ' 'the detected spec "{0}"'.format( - spec - ) + msg = f'the extra attribute "compilers" must be set for the detected spec "{spec}"' assert "compilers" in extra_attributes, msg compilers = extra_attributes["compilers"] diff --git a/var/spack/test_repos/builtin.mock/packages/gmt-concrete-mv-dependency/package.py b/var/spack/test_repos/builtin.mock/packages/gmt-concrete-mv-dependency/package.py new file mode 100644 index 00000000000..ceee3503151 --- /dev/null +++ b/var/spack/test_repos/builtin.mock/packages/gmt-concrete-mv-dependency/package.py @@ -0,0 +1,13 @@ +# Copyright Spack Project Developers. See COPYRIGHT file for details. +# +# SPDX-License-Identifier: (Apache-2.0 OR MIT) +from spack.package import * + + +class GmtConcreteMvDependency(Package): + url = "http://www.example.com/" + + version("2.0", md5="abcdef1234567890abcdef1234567890") + version("1.0", md5="abcdef1234567890abcdef1234567890") + + depends_on("mvdefaults foo:=a,b") diff --git a/var/spack/test_repos/builtin.mock/packages/mvdefaults/package.py b/var/spack/test_repos/builtin.mock/packages/mvdefaults/package.py index 569d504c5c9..d747ec821ee 100644 --- a/var/spack/test_repos/builtin.mock/packages/mvdefaults/package.py +++ b/var/spack/test_repos/builtin.mock/packages/mvdefaults/package.py @@ -9,5 +9,9 @@ class Mvdefaults(Package): url = "http://www.example.com/mvdefaults-1.0.tar.gz" version("1.0", md5="abcdef1234567890abcdef1234567890") + version("0.9", md5="abcdef1234567890abcdef1234567890") variant("foo", values=("a", "b", "c"), default=("a", "b", "c"), multi=True, description="") + conflicts("foo:=a,b", when="@0.9") + + depends_on("pkg-b", when="foo:=b,c")