fail if unknown variables are used in a manifest

Summary:
Check that all variable names are valid when loading manifest files.
This ensures that getdeps.py will error out if someone makes a typo in a
variable name, rather than treating it as never equal to anything.

Reviewed By: pkaush

Differential Revision: D16477397

fbshipit-source-id: 030e0642ff4a08db8eb74a0a0223e03d53e4880f

build/fbcode_builder/getdeps/expr.py

@@ -12,7 +12,7 @@ import re
 import shlex
 
 
-def parse_expr(expr_text):
+def parse_expr(expr_text, valid_variables):
     """ parses the simple criteria expression syntax used in
     dependency specifications.
     Returns an ExprNode instance that can be evaluated like this:
@@ -37,7 +37,7 @@ def parse_expr(expr_text):
                            # none of them evaluated true.
     """
 
-    p = Parser(expr_text)
+    p = Parser(expr_text, valid_variables)
     return p.parse()
 
 
@@ -112,9 +112,10 @@ class EqualExpr(ExprNode):
 
 
 class Parser(object):
-    def __init__(self, text):
+    def __init__(self, text, valid_variables):
         self.text = text
         self.lex = shlex.shlex(text)
+        self.valid_variables = valid_variables
 
     def parse(self):
         expr = self.top()
@@ -141,6 +142,8 @@ class Parser(object):
             return func()
 
         if op == "=":
+            if name not in self.valid_variables:
+                raise Exception("unknown variable %r in expression" % (name,))
             return EqualExpr(name, self.lex.get_token())
 
         raise Exception(

build/fbcode_builder/getdeps/manifest.py

@@ -88,10 +88,12 @@ ALLOWED_EXPR_SECTIONS = [
     "install.files",
 ]
 
+ALLOWED_VARIABLES = {"os", "distro", "distro_vers", "fb", "test"}
+
 
 def parse_conditional_section_name(name, section_def):
     expr = name[len(section_def) + 1 :]
-    return parse_expr(expr)
+    return parse_expr(expr, ALLOWED_VARIABLES)
 
 
 def validate_allowed_fields(file_name, section, config, allowed_fields):

build/fbcode_builder/getdeps/test/expr_test.py

@@ -15,28 +15,38 @@ from ..expr import parse_expr
 
 class ExprTest(unittest.TestCase):
     def test_equal(self):
-        e = parse_expr("foo=bar")
+        valid_variables = {"foo", "some_var", "another_var"}
+        e = parse_expr("foo=bar", valid_variables)
         self.assertTrue(e.eval({"foo": "bar"}))
         self.assertFalse(e.eval({"foo": "not-bar"}))
         self.assertFalse(e.eval({"not-foo": "bar"}))
 
     def test_not_equal(self):
-        e = parse_expr("not(foo=bar)")
+        valid_variables = {"foo"}
+        e = parse_expr("not(foo=bar)", valid_variables)
         self.assertFalse(e.eval({"foo": "bar"}))
         self.assertTrue(e.eval({"foo": "not-bar"}))
 
     def test_bad_not(self):
+        valid_variables = {"foo"}
         with self.assertRaises(Exception):
-            parse_expr("foo=not(bar)")
+            parse_expr("foo=not(bar)", valid_variables)
+
+    def test_bad_variable(self):
+        valid_variables = {"bar"}
+        with self.assertRaises(Exception):
+            parse_expr("foo=bar", valid_variables)
 
     def test_all(self):
-        e = parse_expr("all(foo = bar, baz = qux)")
+        valid_variables = {"foo", "baz"}
+        e = parse_expr("all(foo = bar, baz = qux)", valid_variables)
         self.assertTrue(e.eval({"foo": "bar", "baz": "qux"}))
         self.assertFalse(e.eval({"foo": "bar", "baz": "nope"}))
         self.assertFalse(e.eval({"foo": "nope", "baz": "nope"}))
 
     def test_any(self):
-        e = parse_expr("any(foo = bar, baz = qux)")
+        valid_variables = {"foo", "baz"}
+        e = parse_expr("any(foo = bar, baz = qux)", valid_variables)
         self.assertTrue(e.eval({"foo": "bar", "baz": "qux"}))
         self.assertTrue(e.eval({"foo": "bar", "baz": "nope"}))
         self.assertFalse(e.eval({"foo": "nope", "baz": "nope"}))

build/fbcode_builder/getdeps/test/manifest_test.py

@@ -142,16 +142,16 @@ a
 b
 c
 
-[dependencies.foo=bar]
+[dependencies.test=on]
 foo
 """,
         )
         self.assertEqual(p.get_section_as_args("dependencies"), ["a", "b", "c"])
         self.assertEqual(
-            p.get_section_as_args("dependencies", {"foo": "not-bar"}), ["a", "b", "c"]
+            p.get_section_as_args("dependencies", {"test": "off"}), ["a", "b", "c"]
         )
         self.assertEqual(
-            p.get_section_as_args("dependencies", {"foo": "bar"}),
+            p.get_section_as_args("dependencies", {"test": "on"}),
             ["a", "b", "c", "foo"],
         )
 
@@ -180,13 +180,13 @@ name = foo
 [cmake.defines]
 foo = bar
 
-[cmake.defines.bar=baz]
+[cmake.defines.test=on]
 foo = baz
 """,
         )
         self.assertEqual(p.get_section_as_dict("cmake.defines"), {"foo": "bar"})
         self.assertEqual(
-            p.get_section_as_dict("cmake.defines", {"bar": "baz"}), {"foo": "baz"}
+            p.get_section_as_dict("cmake.defines", {"test": "on"}), {"foo": "baz"}
         )
 
         p2 = ManifestParser(
@@ -195,7 +195,7 @@ foo = baz
 [manifest]
 name = foo
 
-[cmake.defines.bar=baz]
+[cmake.defines.test=on]
 foo = baz
 
 [cmake.defines]
@@ -203,7 +203,7 @@ foo = bar
 """,
         )
         self.assertEqual(
-            p2.get_section_as_dict("cmake.defines", {"bar": "baz"}),
+            p2.get_section_as_dict("cmake.defines", {"test": "on"}),
             {"foo": "bar"},
             msg="sections cascade in the order they appear in the manifest",
         )