concretizer: refactor handling of special variants dev_build and patches

Other parts of the concretizer code build up lists of things we can't
know without traversing all specs and packages, and they output these
list at the very end.

The code for this for variant values from spec literals was intertwined
with the code for traversing the input specs. This only covers the input
specs and misses variant values that might come from directives in
packages.

- [x] move ad-hoc value handling code into spec_clauses so we do it in
  one place for CLI and packages

- [x] move handling of `variant_possible_value`, etc. into
  `concretize.lp`, where we can automatically infer variant existence
  more concisely.

- [x] simplify/clarify some of the code for variants in `spec_clauses()`

2 files changed, 55 insertions, 50 deletions

diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 890411e9e3..bf64efa3ec 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -34,6 +34,7 @@ import spack.cmd
 import spack.compilers
 import spack.config
 import spack.dependency
+import spack.directives
 import spack.error
 import spack.spec
 import spack.package
@@ -696,6 +697,7 @@ class SpackSolverSetup(object):
         self.possible_versions = {}
         self.possible_virtuals = None
         self.possible_compilers = []
+        self.variant_values_from_specs = set()
         self.version_constraints = set()
         self.target_constraints = set()
         self.providers_by_vspec_name = collections.defaultdict(list)
@@ -1161,7 +1163,7 @@ class SpackSolverSetup(object):
             node_platform = fn.node_platform_set
             node_os = fn.node_os_set
             node_target = fn.node_target_set
-            variant = fn.variant_set
+            variant_value = fn.variant_set
             node_compiler = fn.node_compiler_hard
             node_compiler_version = fn.node_compiler_version_hard
             node_flag = fn.node_flag_set
@@ -1171,7 +1173,7 @@ class SpackSolverSetup(object):
             node_platform = fn.node_platform
             node_os = fn.node_os
             node_target = fn.node_target
-            variant = fn.variant_value
+            variant_value = fn.variant_value
             node_compiler = fn.node_compiler
             node_compiler_version = fn.node_compiler_version
             node_flag = fn.node_flag
@@ -1196,14 +1198,26 @@ class SpackSolverSetup(object):
 
         # variants
         for vname, variant in sorted(spec.variants.items()):
-            value = variant.value
-            if isinstance(value, tuple):
-                for v in value:
-                    if v == '*':
-                        continue
-                    clauses.append(f.variant(spec.name, vname, v))
-            elif value != '*':
-                clauses.append(f.variant(spec.name, vname, variant.value))
+            values = variant.value
+            if not isinstance(values, (list, tuple)):
+                values = [values]
+
+            for value in values:
+                # * is meaningless for concretization -- just for matching
+                if value == '*':
+                    continue
+
+                # validate variant value
+                if vname not in spack.directives.reserved_names:
+                    variant_def = spec.package.variants[vname]
+                    variant_def.validate_or_raise(variant, spec.package)
+
+                clauses.append(f.variant_value(spec.name, vname, value))
+
+                # Tell the concretizer that this is a possible value for the
+                # variant, to account for things like int/str values where we
+                # can't enumerate the valid values
+                self.variant_values_from_specs.add((spec.name, vname, value))
 
         # compiler and compiler version
         if spec.compiler:
@@ -1513,6 +1527,18 @@ class SpackSolverSetup(object):
             )
             self.gen.newline()
 
+    def define_variant_values(self):
+        """Validate variant values from the command line.
+
+        Also add valid variant values from the command line to the
+        possible values for a variant.
+
+        """
+        # Tell the concretizer about possible values from specs we saw in
+        # spec_clauses()
+        for pkg, variant, value in sorted(self.variant_values_from_specs):
+            self.gen.fact(fn.variant_possible_value(pkg, variant, value))
+
     def setup(self, driver, specs, tests=False):
         """Generate an ASP program with relevant constraints for specs.
 
@@ -1581,51 +1607,20 @@ class SpackSolverSetup(object):
 
             for dep in spec.traverse():
                 self.gen.h2('Spec: %s' % str(dep))
+
                 # Inject dev_path from environment
                 _develop_specs_from_env(dep)
+
                 if dep.virtual:
                     for clause in self.virtual_spec_clauses(dep):
                         self.gen.fact(clause)
-                else:
-                    for clause in self.spec_clauses(dep):
-                        self.gen.fact(clause)
-                        # TODO: This might need to be moved somewhere else.
-                        # TODO: It's needed to account for open-ended variants
-                        # TODO: validated through a function. The rationale is
-                        # TODO: that if a value is set from cli and validated
-                        # TODO: then it's also a possible value.
-                        if clause.name == 'variant_set':
-                            variant_name = clause.args[1]
-                            # 'dev_path' and 'patches are treated in a
-                            # special way, as they are injected from cli
-                            # or files
-                            if variant_name == 'dev_path':
-                                pkg_name = clause.args[0]
-                                self.gen.fact(fn.variant(
-                                    pkg_name, variant_name
-                                ))
-                                self.gen.fact(fn.variant_single_value(
-                                    pkg_name, variant_name
-                                ))
-                            elif variant_name == 'patches':
-                                pkg_name = clause.args[0]
-                                self.gen.fact(fn.variant(
-                                    pkg_name, variant_name
-                                ))
-                            else:
-                                variant_def = dep.package.variants[
-                                    variant_name
-                                ]
-                                variant_def.validate_or_raise(
-                                    dep.variants[variant_name],
-                                    dep.package
-                                )
-                            # State that this variant is a possible value
-                            # to account for variant values that are not
-                            # enumerated explicitly
-                            self.gen.fact(
-                                fn.variant_possible_value(*clause.args)
-                            )
+                    continue
+
+                for clause in self.spec_clauses(dep):
+                    self.gen.fact(clause)
+
+        self.gen.h1("Variant Values defined in specs")
+        self.define_variant_values()
 
         self.gen.h1("Virtual Constraints")
         self.define_virtual_constraints()
diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
index acfed6f599..96bf23a45c 100644
--- a/lib/spack/spack/solver/concretize.lp
+++ b/lib/spack/spack/solver/concretize.lp
@@ -222,6 +222,16 @@ variant_default_value(Package, Variant, Value)
 :- 2 {variant_value(Package, Variant, Value): variant_possible_value(Package, Variant, Value)},
    variant_value(Package, Variant, "none").
 
+% patches and dev_path are special variants -- they don't have to be
+% declared in the package, so we just allow them to spring into existence
+% when assigned a value.
+auto_variant("dev_path").
+auto_variant("patches").
+variant(Package, "dev_path")
+  :- variant_set(Package, Variant, _), auto_variant(Variant).
+variant_single_value(Package, "dev_path")
+  :- variant_set(Package, "dev_path", _).
+
 % suppress warnings about this atom being unset.  It's only set if some
 % spec or some package sets it, and without this, clingo will give
 % warnings like 'info: atom does not occur in any rule head'.