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.

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

lib/spack/spack/solver/asp.py

@@ -1291,29 +1291,39 @@ def requirement_rules_from_packages_yaml(self, pkg):
             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)]
+            requirements = [requirements]
-        else:
+
         rules = []
         for requirement in requirements:
+            # A string is equivalent to a one_of group with a single element
             if isinstance(requirement, str):
-                    # A string represents a spec that must be satisfied. It is
+                requirement = {"one_of": [requirement]}
-                    # 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]
 
+            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,
@@ -1326,17 +1336,25 @@ def _rules_from_requirements(self, pkg_name: str, requirements, *, kind: Require
                 )
         return rules
 
-    def _rule_from_str(
+    def reject_requirement_constraint(
-        self, pkg_name: str, requirements: str, kind: RequirementKind
+        self, pkg_name: str, *, constraint: str, kind: RequirementKind
-    ) -> RequirementRule:
+    ) -> bool:
-        return RequirementRule(
+        """Returns True if a requirement constraint should be rejected"""
-            pkg_name=pkg_name,
+        if kind == RequirementKind.DEFAULT:
-            policy="one_of",
+            # Requirements under all: are applied only if they are satisfiable considering only
-            requirements=[requirements],
+            # package rules, so e.g. variants must exist etc. Otherwise, they are rejected.
-            kind=kind,
+            try:
-            condition=None,
+                s = spack.spec.Spec(pkg_name)
-            message=None,
+                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)

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 '
+        'trying to set variant "pipe" in package "gcc", but the package has no such variant'
-        'variant [happened during concretization of gcc cflags="-Os" ~pipe]'
     )
     assert cm.endswith('(1) cflags=-Os -pipe => cflags="-Os -pipe"')
 

lib/spack/spack/test/concretize_requirements.py

@@ -896,3 +896,134 @@ def test_requires_directive(concretize_scope, mock_packages):
     # 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

lib/spack/spack/variant.py

@@ -916,7 +916,7 @@ def __init__(self, spec, variants):
         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)