Improve error message for inconsistencies in package.py (#21811)

alalazo · web-flow · commit 7226bd64dc3b · 2021-02-22T19:09:43.000-08:00
* Improve error message for inconsistencies in package.py

Sometimes directives refer to variants that do not exist.
Make it such that:

1. The name of the variant
2. The name of the package which is supposed to have
   such variant
3. The name of the package making this assumption

are all printed in the error message for easier debugging.

* Add unit tests
diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
@@ -647,11 +647,15 @@ def _condition_facts(
         self.gen.fact(cond_fn(condition_id, pkg_name, dep_spec.name))
 
         # conditions that trigger the condition
-        conditions = self.spec_clauses(named_cond, body=True)
+        conditions = self.checked_spec_clauses(
+            named_cond, body=True, required_from=pkg_name
+        )
         for pred in conditions:
             self.gen.fact(require_fn(condition_id, pred.name, *pred.args))
 
-        imposed_constraints = self.spec_clauses(dep_spec)
+        imposed_constraints = self.checked_spec_clauses(
+            dep_spec, required_from=pkg_name
+        )
         for pred in imposed_constraints:
             # imposed "node"-like conditions are no-ops
             if pred.name in ("node", "virtual_node"):
@@ -857,6 +861,20 @@ def flag_defaults(self):
                     self.gen.fact(fn.compiler_version_flag(
                         compiler.name, compiler.version, name, flag))
 
+    def checked_spec_clauses(self, *args, **kwargs):
+        """Wrap a call to spec clauses into a try/except block that raise
+        a comprehensible error message in case of failure.
+        """
+        requestor = kwargs.pop('required_from', None)
+        try:
+            clauses = self.spec_clauses(*args, **kwargs)
+        except RuntimeError as exc:
+            msg = str(exc)
+            if requestor:
+                msg += ' [required from package "{0}"]'.format(requestor)
+            raise RuntimeError(msg)
+        return clauses
+
     def spec_clauses(self, spec, body=False, transitive=True):
         """Return a list of clauses for a spec mandates are true.
 
@@ -925,9 +943,14 @@ class Body(object):
 
                 # validate variant value
                 reserved_names = spack.directives.reserved_names
-                if (not spec.virtual and vname not in reserved_names):
-                    variant_def = spec.package.variants[vname]
-                    variant_def.validate_or_raise(variant, spec.package)
+                if not spec.virtual and vname not in reserved_names:
+                    try:
+                        variant_def = spec.package.variants[vname]
+                    except KeyError:
+                        msg = 'variant "{0}" not found in package "{1}"'
+                        raise RuntimeError(msg.format(vname, spec.name))
+                    else:
+                        variant_def.validate_or_raise(variant, spec.package)
 
                 clauses.append(f.variant_value(spec.name, vname, value))
 
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
@@ -1098,3 +1098,15 @@ def test_concretization_of_test_dependencies(self):
         # dependency type declared to infer that the dependency holds.
         s = Spec('test-dep-with-imposed-conditions').concretized()
         assert 'c' not in s
+
+    @pytest.mark.parametrize('spec_str', [
+        'wrong-variant-in-conflicts',
+        'wrong-variant-in-depends-on'
+    ])
+    def test_error_message_for_inconsistent_variants(self, spec_str):
+        if spack.config.get('config:concretizer') == 'original':
+            pytest.xfail('Known failure of the original concretizer')
+
+        s = Spec(spec_str)
+        with pytest.raises(RuntimeError, match='not found in package'):
+            s.concretize()
diff --git a/var/spack/repos/builtin.mock/packages/wrong-variant-in-conflicts/package.py b/var/spack/repos/builtin.mock/packages/wrong-variant-in-conflicts/package.py
@@ -0,0 +1,13 @@
+# Copyright 2013-2021 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+class WrongVariantInConflicts(Package):
+    """This package has a wrong variant spelled in a conflict."""
+
+    homepage = "http://www.example.com"
+    url = "http://www.example.com/b-1.0.tar.gz"
+
+    version('1.0', '0123456789abcdef0123456789abcdef')
+
+    conflicts('+foo', when='@1.0')
diff --git a/var/spack/repos/builtin.mock/packages/wrong-variant-in-depends-on/package.py b/var/spack/repos/builtin.mock/packages/wrong-variant-in-depends-on/package.py
@@ -0,0 +1,13 @@
+# Copyright 2013-2021 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+class WrongVariantInDependsOn(Package):
+    """This package has a wrong variant spelled in a depends_on."""
+
+    homepage = "http://www.example.com"
+    url = "http://www.example.com/b-1.0.tar.gz"
+
+    version('1.0', '0123456789abcdef0123456789abcdef')
+
+    depends_on('b+doesnotexist')
