variants: Unify metadata dictionaries to index by when (spack#44425)

Continuing the work started in spack#40326, his changes the structure
of Variant metadata on Packages from a single variant definition
per name with a list of `when` specs:

```
name: (Variant, [when_spec, ...])
```

to a Variant definition per `when_spec` per name:

```
when_spec: { name: Variant }
```

With this change, everything on a package *except* versions is
 keyed by `when` spec. This:

1. makes things consistent, in that conditional things are (nearly)
   all modeled in the same way; and

2. fixes an issue where we would lose information about multiple
   variant definitions in a package (see spack#38302). We can now have,
   e.g., different defaults for the same variant in different
   versions of a package.

Some notes:

1. This required some pretty deep changes to the solver. Previously,
   the solver's job was to select value(s) for a single variant definition
   per name per package. Now, the solver needs to:

   a. Determine which variant definition should be used for a given node,
      which can depend on the node's version, compiler, target, other variants, etc.
   b. Select valid value(s) for variants for each node based on the selected
      variant definition.

   When multiple variant definitions are enabled via their `when=` clause, we will
   always prefer the *last* matching definition, by declaration order in packages. This
   is implemented by adding a `precedence` to each variant at definition time, and we
   ensure they are added to the solver in order of precedence.

   This has the effect that variant definitions from derived classes are preferred over
   definitions from superclasses, and the last definition within the same class sticks.
   This matches python semantics. Some examples:

    ```python
    class ROCmPackage(PackageBase):
        variant("amdgpu_target", ..., when="+rocm")

    class Hipblas(ROCmPackage):
        variant("amdgpu_target", ...)
    ```

   The global variant in `hipblas` will always supersede the `when="+rocm"` variant in
   `ROCmPackage`. If `hipblas`'s variant was also conditional on `+rocm` (as it probably
   should be), we would again filter out the definition from `ROCmPackage` because it
   could never be activated. If you instead have:

    ```python
    class ROCmPackage(PackageBase):
        variant("amdgpu_target", ..., when="+rocm")

    class Hipblas(ROCmPackage):
        variant("amdgpu_target", ..., when="+rocm+foo")
    ```

   The variant on `hipblas` will win for `+rocm+foo` but the one on `ROCmPackage` will
   win with `rocm~foo`.

   So, *if* we can statically determine if a variant is overridden, we filter it out.
   This isn't strictly necessary, as the solver can handle many definitions fine, but
   this reduces the complexity of the problem instance presented to `clingo`, and
   simplifies output in `spack info` for derived packages. e.g., `spack info hipblas`
   now shows only one definition of `amdgpu_target` where before it showed two, one of
   which would never be used.

2. Nearly all access to the `variants` dictionary on packages has been refactored to
   use the following class methods on `PackageBase`:
    * `variant_names(cls) -> List[str]`: get all variant names for a package
    * `has_variant(cls, name) -> bool`: whether a package has a variant with a given name
    * `variant_definitions(cls, name: str) -> List[Tuple[Spec, Variant]]`: all definitions
      of variant `name` that are possible, along with their `when` specs.
    * `variant_items() -> `: iterate over `pkg.variants.items()`, with impossible variants
      filtered out.

   Consolidating to these methods seems to simplify the code a lot.

3. The solver does a lot more validation on variant values at setup time now. In
   particular, it checks whether a variant value on a spec is valid given the other
   constraints on that spec. This allowed us to remove the crufty logic in
   `update_variant_validate`, which was needed because we previously didn't *know* after
   a solve which variant definition had been used. Now, variant values from solves are
   constructed strictly based on which variant definition was selected -- no more
   heuristics.

4. The same prevalidation can now be done in package audits, and you can run:

   ```
   spack audit packages --strict-variants
   ```

   This turns up around 18 different places where a variant specification isn't valid
   given the conditions on variant definitions in packages. I haven't fixed those here
   but will open a separate PR to iterate on them. I plan to make strict checking the
   defaults once all existing package issues are resolved. It's not clear to me that
   strict checking should be the default for the prevalidation done at solve time.

There are a few other changes here that might be of interest:

  1. The `generator` variant in `CMakePackage` is now only defined when `build_system=cmake`.
  2. `spack info` has been updated to support the new metadata layout.
  3.  split out variant propagation into its own `.lp` file in the `solver` code.
  4. Add better typing and clean up code for variant types in `variant.py`.
  5. Add tests for new variant behavior.

lib/spack/spack/audit.py

@@ -282,7 +282,7 @@ def _avoid_mismatched_variants(error_cls):
             pkg_cls = spack.repo.PATH.get_pkg_class(pkg_name)
             for variant in current_spec.variants.values():
                 # Variant does not exist at all
-                if variant.name not in pkg_cls.variants:
+                if variant.name not in pkg_cls.variant_names():
                     summary = (
                         f"Setting a preference for the '{pkg_name}' package to the "
                         f"non-existing variant '{variant.name}'"
@@ -291,9 +291,8 @@ def _avoid_mismatched_variants(error_cls):
                     continue
 
                 # Variant cannot accept this value
-                s = spack.spec.Spec(pkg_name)
                 try:
-                    s.update_variant_validate(variant.name, variant.value)
+                    spack.variant.prevalidate_variant_value(pkg_cls, variant, strict=True)
                 except Exception:
                     summary = (
                         f"Setting the variant '{variant.name}' of the '{pkg_name}' package "
@@ -663,9 +662,15 @@ def _ensure_env_methods_are_ported_to_builders(pkgs, error_cls):
     errors = []
     for pkg_name in pkgs:
         pkg_cls = spack.repo.PATH.get_pkg_class(pkg_name)
-        buildsystem_variant, _ = pkg_cls.variants["build_system"]
-        buildsystem_names = [getattr(x, "value", x) for x in buildsystem_variant.values]
-        builder_cls_names = [spack.builder.BUILDER_CLS[x].__name__ for x in buildsystem_names]
+
+        # values are either Value objects (for conditional values) or the values themselves
+        build_system_names = set(
+            v.value if isinstance(v, spack.variant.Value) else v
+            for _, variant in pkg_cls.variant_definitions("build_system")
+            for v in variant.values
+        )
+        builder_cls_names = [spack.builder.BUILDER_CLS[x].__name__ for x in build_system_names]
+
         module = pkg_cls.module
         has_builders_in_package_py = any(
             getattr(module, name, False) for name in builder_cls_names
@@ -932,20 +937,22 @@ def check_virtual_with_variants(spec, msg):
 
                 # check variants
                 dependency_variants = dep.spec.variants
-                for name, value in dependency_variants.items():
+                for name, variant in dependency_variants.items():
                     try:
-                        v, _ = dependency_pkg_cls.variants[name]
-                        v.validate_or_raise(value, pkg_cls=dependency_pkg_cls)
+                        spack.variant.prevalidate_variant_value(
+                            dependency_pkg_cls, variant, dep.spec, strict=True
+                        )
                     except Exception as e:
                         summary = (
                             f"{pkg_name}: wrong variant used for dependency in 'depends_on()'"
                         )
 
+                        error_msg = str(e)
                         if isinstance(e, KeyError):
                             error_msg = (
                                 f"variant {str(e).strip()} does not exist in package {dep_name}"
+                                f" in package '{dep_name}'"
                             )
-                        error_msg += f" in package '{dep_name}'"
 
                         errors.append(
                             error_cls(summary=summary, details=[error_msg, f"in {filename}"])
@@ -957,39 +964,38 @@ def check_virtual_with_variants(spec, msg):
 @package_directives
 def _ensure_variant_defaults_are_parsable(pkgs, error_cls):
     """Ensures that variant defaults are present and parsable from cli"""
-    errors = []
-    for pkg_name in pkgs:
-        pkg_cls = spack.repo.PATH.get_pkg_class(pkg_name)
-        for variant_name, entry in pkg_cls.variants.items():
-            variant, _ = entry
-            default_is_parsable = (
-                # Permitting a default that is an instance on 'int' permits
-                # to have foo=false or foo=0. Other falsish values are
-                # not allowed, since they can't be parsed from cli ('foo=')
-                isinstance(variant.default, int)
-                or variant.default
-            )
-            if not default_is_parsable:
-                error_msg = "Variant '{}' of package '{}' has a bad default value"
-                errors.append(error_cls(error_msg.format(variant_name, pkg_name), []))
-                continue
 
-            try:
-                vspec = variant.make_default()
-            except spack.variant.MultipleValuesInExclusiveVariantError:
-                error_msg = "Cannot create a default value for the variant '{}' in package '{}'"
-                errors.append(error_cls(error_msg.format(variant_name, pkg_name), []))
-                continue
+    def check_variant(pkg_cls, variant, vname):
+        # bool is a subclass of int in python. Permitting a default that is an instance
+        # of 'int' means both foo=false and foo=0 are accepted. Other falsish values are
+        # not allowed, since they can't be parsed from CLI ('foo=')
+        default_is_parsable = isinstance(variant.default, int) or variant.default
 
-            try:
-                variant.validate_or_raise(vspec, pkg_cls=pkg_cls)
-            except spack.variant.InvalidVariantValueError:
-                error_msg = (
-                    "The default value of the variant '{}' in package '{}' failed validation"
-                )
-                question = "Is it among the allowed values?"
-                errors.append(error_cls(error_msg.format(variant_name, pkg_name), [question]))
+        if not default_is_parsable:
+            msg = f"Variant '{vname}' of package '{pkg_cls.name}' has an unparsable default value"
+            return [error_cls(msg, [])]
+
+        try:
+            vspec = variant.make_default()
+        except spack.variant.MultipleValuesInExclusiveVariantError:
+            msg = f"Can't create default value for variant '{vname}' in package '{pkg_cls.name}'"
+            return [error_cls(msg, [])]
+
+        try:
+            variant.validate_or_raise(vspec, pkg_cls.name)
+        except spack.variant.InvalidVariantValueError:
+            msg = "Default value of variant '{vname}' in package '{pkg.name}' is invalid"
+            question = "Is it among the allowed values?"
+            return [error_cls(msg, [question])]
+
+        return []
 
+    errors = []
+    for pkg_name in pkgs:
+        pkg_cls = spack.repo.PATH.get_pkg_class(pkg_name)
+        for vname in pkg_cls.variant_names():
+            for _, variant_def in pkg_cls.variant_definitions(vname):
+                errors.extend(check_variant(pkg_cls, variant_def, vname))
     return errors
 
 
@@ -999,11 +1005,11 @@ def _ensure_variants_have_descriptions(pkgs, error_cls):
     errors = []
     for pkg_name in pkgs:
         pkg_cls = spack.repo.PATH.get_pkg_class(pkg_name)
-        for variant_name, entry in pkg_cls.variants.items():
-            variant, _ = entry
-            if not variant.description:
-                error_msg = "Variant '{}' in package '{}' is missing a description"
-                errors.append(error_cls(error_msg.format(variant_name, pkg_name), []))
+        for name in pkg_cls.variant_names():
+            for when, variant in pkg_cls.variant_definitions(name):
+                if not variant.description:
+                    msg = f"Variant '{name}' in package '{pkg_name}' is missing a description"
+                    errors.append(error_cls(msg, []))
 
     return errors
 
@@ -1060,29 +1066,26 @@ def _version_constraints_are_satisfiable_by_some_version_in_repo(pkgs, error_cls
 
 
 def _analyze_variants_in_directive(pkg, constraint, directive, error_cls):
-    variant_exceptions = (
-        spack.variant.InconsistentValidationError,
-        spack.variant.MultipleValuesInExclusiveVariantError,
-        spack.variant.InvalidVariantValueError,
-        KeyError,
-    )
     errors = []
-    for name, v in constraint.variants.items():
-        try:
-            variant, _ = pkg.variants[name]
-            variant.validate_or_raise(v, pkg_cls=pkg)
-        except variant_exceptions as e:
-            summary = pkg.name + ': wrong variant in "{0}" directive'
-            summary = summary.format(directive)
-            filename = spack.repo.PATH.filename_for_package_name(pkg.name)
-
-            error_msg = str(e).strip()
-            if isinstance(e, KeyError):
-                error_msg = "the variant {0} does not exist".format(error_msg)
+    variant_names = pkg.variant_names()
+    summary = f"{pkg.name}: wrong variant in '{directive}' directive"
+    filename = spack.repo.PATH.filename_for_package_name(pkg.name)
 
-            err = error_cls(summary=summary, details=[error_msg, "in " + filename])
+    for name, v in constraint.variants.items():
+        if name not in variant_names:
+            msg = f"variant {name} does not exist in {pkg.name}"
+            errors.append(error_cls(summary=summary, details=[msg, f"in {filename}"]))
+            continue
 
-            errors.append(err)
+        try:
+            spack.variant.prevalidate_variant_value(pkg, v, constraint, strict=True)
+        except (
+            spack.variant.InconsistentValidationError,
+            spack.variant.MultipleValuesInExclusiveVariantError,
+            spack.variant.InvalidVariantValueError,
+        ) as e:
+            msg = str(e).strip()
+            errors.append(error_cls(summary=summary, details=[msg, f"in {filename}"]))
 
     return errors
 
@@ -1120,9 +1123,10 @@ def _extracts_errors(triggers, summary):
                 for dname in dnames
             )
 
-        for vname, (variant, triggers) in pkg_cls.variants.items():
-            summary = f"{pkg_name}: wrong 'when=' condition for the '{vname}' variant"
-            errors.extend(_extracts_errors(triggers, summary))
+        for when, variants_by_name in pkg_cls.variants.items():
+            for vname, variant in variants_by_name.items():
+                summary = f"{pkg_name}: wrong 'when=' condition for the '{vname}' variant"
+                errors.extend(_extracts_errors([when], summary))
 
         for when, providers, details in _error_items(pkg_cls.provided):
             errors.extend(

lib/spack/spack/build_systems/autotools.py

@@ -687,9 +687,8 @@ def _activate_or_not(
 
         variant = variant or name
 
-        # Defensively look that the name passed as argument is among
-        # variants
-        if variant not in self.pkg.variants:
+        # Defensively look that the name passed as argument is among variants
+        if not self.pkg.has_variant(variant):
             msg = '"{0}" is not a variant of "{1}"'
             raise KeyError(msg.format(variant, self.pkg.name))
 
@@ -698,27 +697,19 @@ def _activate_or_not(
 
         # Create a list of pairs. Each pair includes a configuration
         # option and whether or not that option is activated
-        variant_desc, _ = self.pkg.variants[variant]
-        if set(variant_desc.values) == set((True, False)):
+        vdef = self.pkg.get_variant(variant)
+        if set(vdef.values) == set((True, False)):
             # BoolValuedVariant carry information about a single option.
             # Nonetheless, for uniformity of treatment we'll package them
             # in an iterable of one element.
-            condition = "+{name}".format(name=variant)
-            options = [(name, condition in spec)]
+            options = [(name, f"+{variant}" in spec)]
         else:
-            condition = "{variant}={value}"
             # "feature_values" is used to track values which correspond to
             # features which can be enabled or disabled as understood by the
             # package's build system. It excludes values which have special
             # meanings and do not correspond to features (e.g. "none")
-            feature_values = (
-                getattr(variant_desc.values, "feature_values", None) or variant_desc.values
-            )
-
-            options = [
-                (value, condition.format(variant=variant, value=value) in spec)
-                for value in feature_values
-            ]
+            feature_values = getattr(vdef.values, "feature_values", None) or vdef.values
+            options = [(value, f"{variant}={value}" in spec) for value in feature_values]
 
         # For each allowed value in the list of values
         for option_value, activated in options:

lib/spack/spack/build_systems/cached_cmake.py

@@ -89,7 +89,7 @@ def define_cmake_cache_from_variant(self, cmake_var, variant=None, comment=""):
         if variant is None:
             variant = cmake_var.lower()
 
-        if variant not in self.pkg.variants:
+        if not self.pkg.has_variant(variant):
             raise KeyError('"{0}" is not a variant of "{1}"'.format(variant, self.pkg.name))
 
         if variant not in self.pkg.spec.variants:

lib/spack/spack/build_systems/cmake.py

@@ -146,6 +146,7 @@ def _values(x):
         default=default,
         values=_values,
         description="the build system generator to use",
+        when="build_system=cmake",
     )
     for x in not_used:
         conflicts(f"generator={x}")
@@ -505,7 +506,7 @@ def define_from_variant(self, cmake_var, variant=None):
         if variant is None:
             variant = cmake_var.lower()
 
-        if variant not in self.pkg.variants:
+        if not self.pkg.has_variant(variant):
             raise KeyError('"{0}" is not a variant of "{1}"'.format(variant, self.pkg.name))
 
         if variant not in self.pkg.spec.variants:

lib/spack/spack/cmd/config.py

@@ -536,11 +536,11 @@ def config_prefer_upstream(args):
         # Get and list all the variants that differ from the default.
         variants = []
         for var_name, variant in spec.variants.items():
-            if var_name in ["patches"] or var_name not in spec.package.variants:
+            if var_name in ["patches"] or not spec.package.has_variant(var_name):
                 continue
 
-            variant_desc, _ = spec.package.variants[var_name]
-            if variant.value != variant_desc.default:
+            vdef = spec.package.get_variant(var_name)
+            if variant.value != vdef.default:
                 variants.append(str(variant))
         variants.sort()
         variants = " ".join(variants)

lib/spack/spack/cmd/info.py

@@ -334,26 +334,6 @@ def _fmt_variant(variant, max_name_default_len, indent, when=None, out=None):
     out.write("\n")
 
 
-def _variants_by_name_when(pkg):
-    """Adaptor to get variants keyed by { name: { when: { [Variant...] } }."""
-    # TODO: replace with pkg.variants_by_name(when=True) when unified directive dicts are merged.
-    variants = {}
-    for name, (variant, whens) in sorted(pkg.variants.items()):
-        for when in whens:
-            variants.setdefault(name, {}).setdefault(when, []).append(variant)
-    return variants
-
-
-def _variants_by_when_name(pkg):
-    """Adaptor to get variants keyed by { when: { name: Variant } }"""
-    # TODO: replace with pkg.variants when unified directive dicts are merged.
-    variants = {}
-    for name, (variant, whens) in pkg.variants.items():
-        for when in whens:
-            variants.setdefault(when, {})[name] = variant
-    return variants
-
-
 def _print_variants_header(pkg):
     """output variants"""
 
@@ -364,32 +344,22 @@ def _print_variants_header(pkg):
     color.cprint("")
     color.cprint(section_title("Variants:"))
 
-    variants_by_name = _variants_by_name_when(pkg)
-
     # Calculate the max length of the "name [default]" part of the variant display
     # This lets us know where to print variant values.
     max_name_default_len = max(
         color.clen(_fmt_name_and_default(variant))
-        for name, when_variants in variants_by_name.items()
-        for variants in when_variants.values()
-        for variant in variants
+        for name in pkg.variant_names()
+        for _, variant in pkg.variant_definitions(name)
     )
 
-    return max_name_default_len, variants_by_name
-
-
-def _unconstrained_ver_first(item):
-    """sort key that puts specs with open version ranges first"""
-    spec, _ = item
-    return (spack.version.any_version not in spec.versions, spec)
+    return max_name_default_len
 
 
 def print_variants_grouped_by_when(pkg):
-    max_name_default_len, _ = _print_variants_header(pkg)
+    max_name_default_len = _print_variants_header(pkg)
 
     indent = 4
-    variants = _variants_by_when_name(pkg)
-    for when, variants_by_name in sorted(variants.items(), key=_unconstrained_ver_first):
+    for when, variants_by_name in pkg.variant_items():
         padded_values = max_name_default_len + 4
         start_indent = indent
 
@@ -407,15 +377,14 @@ def print_variants_grouped_by_when(pkg):
 
 
 def print_variants_by_name(pkg):
-    max_name_default_len, variants_by_name = _print_variants_header(pkg)
+    max_name_default_len = _print_variants_header(pkg)
     max_name_default_len += 4
 
     indent = 4
-    for name, when_variants in variants_by_name.items():
-        for when, variants in sorted(when_variants.items(), key=_unconstrained_ver_first):
-            for variant in variants:
-                _fmt_variant(variant, max_name_default_len, indent, when, out=sys.stdout)
-                sys.stdout.write("\n")
+    for name in pkg.variant_names():
+        for when, variant in pkg.variant_definitions(name):
+            _fmt_variant(variant, max_name_default_len, indent, when, out=sys.stdout)
+            sys.stdout.write("\n")
 
 
 def print_variants(pkg, args):

lib/spack/spack/cray_manifest.py

@@ -132,7 +132,7 @@ def spec_from_entry(entry):
         variant_strs = list()
         for name, value in entry["parameters"].items():
             # TODO: also ensure that the variant value is valid?
-            if not (name in pkg_cls.variants):
+            if not pkg_cls.has_variant(name):
                 tty.debug(
                     "Omitting variant {0} for entry {1}/{2}".format(
                         name, entry["name"], entry["hash"][:7]