Improve semantic for packages:all:require (#41239)

An `all` requirement is emitted for a package if all variants referenced are defined by it. Otherwise, the constraint is rejected.

5 files changed, 227 insertions, 47 deletions

diff --git a/lib/spack/docs/packages_yaml.rst b/lib/spack/docs/packages_yaml.rst
index e08f51e612..af0acf0f9a 100644
--- a/lib/spack/docs/packages_yaml.rst
+++ b/lib/spack/docs/packages_yaml.rst
@@ -383,7 +383,33 @@ like this:
 
 which means every spec will be required to use ``clang`` as a compiler.
 
-Note that in this case ``all`` represents a *default set of requirements* -
+Requirements on variants for all packages are possible too, but note that they
+are only enforced for those packages that define these variants, otherwise they
+are disregarded. For example:
+
+.. code-block:: yaml
+
+   packages:
+     all:
+       require:
+       - "+shared"
+       - "+cuda"
+
+will just enforce ``+shared`` on ``zlib``, which has a boolean ``shared`` variant but
+no ``cuda`` variant.
+
+Constraints in a single spec literal are always considered as a whole, so in a case like:
+
+.. code-block:: yaml
+
+   packages:
+     all:
+       require: "+shared +cuda"
+
+the default requirement will be enforced only if a package has both a ``cuda`` and
+a ``shared`` variant, and will never be partially enforced.
+
+Finally, ``all`` represents a *default set of requirements* -
 if there are specific package requirements, then the default requirements
 under ``all`` are disregarded. For example, with a configuration like this:
 
@@ -391,12 +417,18 @@ under ``all`` are disregarded. For example, with a configuration like this:
 
    packages:
      all:
-       require: '%clang'
+       require:
+       - 'build_type=Debug'
+       - '%clang'
      cmake:
-       require: '%gcc'
+       require:
+       - 'build_type=Debug'
+       - '%gcc'
 
 Spack requires ``cmake`` to use ``gcc`` and all other nodes (including ``cmake``
-dependencies) to use ``clang``.
+dependencies) to use ``clang``. If enforcing ``build_type=Debug`` is needed also
+on ``cmake``, it must be repeated in the specific ``cmake`` requirements.
+
 
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 Setting requirements on virtual specs
diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index f723acb0e9..b41bcba228 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -1291,52 +1291,70 @@ class SpackSolverSetup:
             kind = RequirementKind.DEFAULT
         return self._rules_from_requirements(pkg_name, requirements, kind=kind)
 
-    def _rules_from_requirements(self, pkg_name: str, requirements, *, kind: RequirementKind):
+    def _rules_from_requirements(
+        self, pkg_name: str, requirements, *, kind: RequirementKind
+    ) -> List[RequirementRule]:
         """Manipulate requirements from packages.yaml, and return a list of tuples
         with a uniform structure (name, policy, requirements).
         """
         if isinstance(requirements, str):
-            rules = [self._rule_from_str(pkg_name, requirements, kind)]
-        else:
-            rules = []
-            for requirement in requirements:
-                if isinstance(requirement, str):
-                    # A string represents a spec that must be satisfied. It is
-                    # equivalent to a one_of group with a single element
-                    rules.append(self._rule_from_str(pkg_name, requirement, kind))
-                else:
-                    for policy in ("spec", "one_of", "any_of"):
-                        if policy in requirement:
-                            constraints = requirement[policy]
-
-                            # "spec" is for specifying a single spec
-                            if policy == "spec":
-                                constraints = [constraints]
-                                policy = "one_of"
-
-                            rules.append(
-                                RequirementRule(
-                                    pkg_name=pkg_name,
-                                    policy=policy,
-                                    requirements=constraints,
-                                    kind=kind,
-                                    message=requirement.get("message"),
-                                    condition=requirement.get("when"),
-                                )
-                            )
+            requirements = [requirements]
+
+        rules = []
+        for requirement in requirements:
+            # A string is equivalent to a one_of group with a single element
+            if isinstance(requirement, str):
+                requirement = {"one_of": [requirement]}
+
+            for policy in ("spec", "one_of", "any_of"):
+                if policy not in requirement:
+                    continue
+
+                constraints = requirement[policy]
+                # "spec" is for specifying a single spec
+                if policy == "spec":
+                    constraints = [constraints]
+                    policy = "one_of"
+
+                constraints = [
+                    x
+                    for x in constraints
+                    if not self.reject_requirement_constraint(pkg_name, constraint=x, kind=kind)
+                ]
+                if not constraints:
+                    continue
+
+                rules.append(
+                    RequirementRule(
+                        pkg_name=pkg_name,
+                        policy=policy,
+                        requirements=constraints,
+                        kind=kind,
+                        message=requirement.get("message"),
+                        condition=requirement.get("when"),
+                    )
+                )
         return rules
 
-    def _rule_from_str(
-        self, pkg_name: str, requirements: str, kind: RequirementKind
-    ) -> RequirementRule:
-        return RequirementRule(
-            pkg_name=pkg_name,
-            policy="one_of",
-            requirements=[requirements],
-            kind=kind,
-            condition=None,
-            message=None,
-        )
+    def reject_requirement_constraint(
+        self, pkg_name: str, *, constraint: str, kind: RequirementKind
+    ) -> bool:
+        """Returns True if a requirement constraint should be rejected"""
+        if kind == RequirementKind.DEFAULT:
+            # Requirements under all: are applied only if they are satisfiable considering only
+            # package rules, so e.g. variants must exist etc. Otherwise, they are rejected.
+            try:
+                s = spack.spec.Spec(pkg_name)
+                s.constrain(constraint)
+                s.validate_or_raise()
+            except spack.error.SpackError as e:
+                tty.debug(
+                    f"[SETUP] Rejecting the default '{constraint}' requirement "
+                    f"on '{pkg_name}': {str(e)}",
+                    level=2,
+                )
+                return True
+        return False
 
     def pkg_rules(self, pkg, tests):
         pkg = packagize(pkg)
diff --git a/lib/spack/spack/test/cmd/spec.py b/lib/spack/spack/test/cmd/spec.py
index 66dfce9308..763d83bf0a 100644
--- a/lib/spack/spack/test/cmd/spec.py
+++ b/lib/spack/spack/test/cmd/spec.py
@@ -104,8 +104,7 @@ def test_spec_parse_unquoted_flags_report():
         spec("gcc cflags=-Os -pipe")
     cm = str(cm.value)
     assert cm.startswith(
-        'trying to set variant "pipe" in package "gcc", but the package has no such '
-        'variant [happened during concretization of gcc cflags="-Os" ~pipe]'
+        'trying to set variant "pipe" in package "gcc", but the package has no such variant'
     )
     assert cm.endswith('(1) cflags=-Os -pipe => cflags="-Os -pipe"')
 
diff --git a/lib/spack/spack/test/concretize_requirements.py b/lib/spack/spack/test/concretize_requirements.py
index d5295691ce..529d481b2f 100644
--- a/lib/spack/spack/test/concretize_requirements.py
+++ b/lib/spack/spack/test/concretize_requirements.py
@@ -896,3 +896,134 @@ compilers::
     # This package can only be compiled with clang
     with pytest.raises(spack.error.SpackError, match="can only be compiled with Clang"):
         Spec("requires_clang").concretized()
+
+
+@pytest.mark.parametrize(
+    "packages_yaml",
+    [
+        # Simple string
+        """
+        packages:
+          all:
+            require: "+shared"
+    """,
+        # List of strings
+        """
+        packages:
+          all:
+            require:
+            - "+shared"
+    """,
+        # Objects with attributes
+        """
+        packages:
+          all:
+            require:
+            - spec: "+shared"
+    """,
+        """
+        packages:
+          all:
+            require:
+            - one_of: ["+shared"]
+    """,
+    ],
+)
+def test_default_requirements_semantic(packages_yaml, concretize_scope, mock_packages):
+    """Tests that requirements under 'all:' are by default applied only if the variant/property
+    required exists, but are strict otherwise.
+
+    For example:
+
+      packages:
+        all:
+          require: "+shared"
+
+    should enforce the value of "+shared" when a Boolean variant named "shared" exists. This is
+    not overridable from the command line, so with the configuration above:
+
+    > spack spec zlib~shared
+
+    is unsatisfiable.
+    """
+    update_packages_config(packages_yaml)
+
+    # Regular zlib concretize to +shared
+    s = Spec("zlib").concretized()
+    assert s.satisfies("+shared")
+
+    # If we specify the variant we can concretize only the one matching the constraint
+    s = Spec("zlib +shared").concretized()
+    assert s.satisfies("+shared")
+    with pytest.raises(UnsatisfiableSpecError):
+        Spec("zlib ~shared").concretized()
+
+    # A spec without the shared variant still concretize
+    s = Spec("a").concretized()
+    assert not s.satisfies("a +shared")
+    assert not s.satisfies("a ~shared")
+
+
+@pytest.mark.parametrize(
+    "packages_yaml,spec_str,expected,not_expected",
+    [
+        # The package has a 'libs' mv variant defaulting to 'libs=shared'
+        (
+            """
+        packages:
+          all:
+            require: "+libs"
+    """,
+            "multivalue-variant",
+            ["libs=shared"],
+            ["libs=static", "+libs"],
+        ),
+        (
+            """
+        packages:
+          all:
+            require: "libs=foo"
+    """,
+            "multivalue-variant",
+            ["libs=shared"],
+            ["libs=static", "libs=foo"],
+        ),
+        (
+            # (TODO): revisit this case when we'll have exact value semantic for mv variants
+            """
+        packages:
+          all:
+            require: "libs=static"
+    """,
+            "multivalue-variant",
+            ["libs=static", "libs=shared"],
+            [],
+        ),
+        (
+            # Constraint apply as a whole, so having a non-existing variant
+            # invalidate the entire constraint
+            """
+        packages:
+          all:
+            require: "libs=static +feefoo"
+    """,
+            "multivalue-variant",
+            ["libs=shared"],
+            ["libs=static"],
+        ),
+    ],
+)
+def test_default_requirements_semantic_with_mv_variants(
+    packages_yaml, spec_str, expected, not_expected, concretize_scope, mock_packages
+):
+    """Tests that requirements under 'all:' are behaving correctly under cases that could stem
+    from MV variants.
+    """
+    update_packages_config(packages_yaml)
+    s = Spec(spec_str).concretized()
+
+    for constraint in expected:
+        assert s.satisfies(constraint), constraint
+
+    for constraint in not_expected:
+        assert not s.satisfies(constraint), constraint
diff --git a/lib/spack/spack/variant.py b/lib/spack/spack/variant.py
index 7b045d6262..9bea903aac 100644
--- a/lib/spack/spack/variant.py
+++ b/lib/spack/spack/variant.py
@@ -916,7 +916,7 @@ class UnknownVariantError(error.SpecError):
         variant_str = "variant" if len(variants) == 1 else "variants"
         msg = (
             'trying to set {0} "{1}" in package "{2}", but the package'
-            " has no such {0} [happened during concretization of {3}]"
+            " has no such {0} [happened when validating '{3}']"
         )
         msg = msg.format(variant_str, comma_or(variants), spec.name, spec.root)
         super().__init__(msg)