gclient: add support for native boolean variables

Bug: 570091
Change-Id: I195f5f798d9869f385437db4e0bc5f4c5d4853ae
Reviewed-on: https://chromium-review.googlesource.com/687496
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>

gclient.py

@@ -965,7 +965,9 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
     lines = ['# Generated from %r' % self.deps_file]
     variables = self.get_vars()
     for arg in self._gn_args:
-      value = gclient_eval.EvaluateCondition(variables[arg], variables)
+      value = variables[arg]
+      if isinstance(value, basestring):
+        value = gclient_eval.EvaluateCondition(value, variables)
       lines.append('%s = %s' % (arg, ToGNString(value)))
     with open(os.path.join(self.root.root_dir, self._gn_args_file), 'w') as f:
       f.write('\n'.join(lines))

gclient_eval.py

@@ -120,7 +120,9 @@ _GCLIENT_SCHEMA = schema.Schema({
     schema.Optional('use_relative_paths'): bool,
 
     # Variables that can be referenced using Var() - see 'deps'.
-    schema.Optional('vars'): {schema.Optional(basestring): basestring},
+    schema.Optional('vars'): {
+        schema.Optional(basestring): schema.Or(basestring, bool),
+    },
 })
 
 
@@ -237,27 +239,58 @@ def EvaluateCondition(condition, variables, referenced_variables=None):
       elif node.id in _allowed_names:
         return _allowed_names[node.id]
       elif node.id in variables:
+        value = variables[node.id]
+
+        # Allow using "native" types, without wrapping everything in strings.
+        # Note that schema constraints still apply to variables.
+        if not isinstance(value, basestring):
+          return value
+
+        # Recursively evaluate the variable reference.
         return EvaluateCondition(
             variables[node.id],
             variables,
             referenced_variables.union([node.id]))
       else:
-        raise ValueError(
+        # Implicitly convert unrecognized names to strings.
-            'invalid name %r (inside %r)' % (node.id, condition))
+        # If we want to change this, we'll need to explicitly distinguish
+        # between arguments for GN to be passed verbatim, and ones to
+        # be evaluated.
+        return node.id
     elif isinstance(node, ast.BoolOp) and isinstance(node.op, ast.Or):
       if len(node.values) != 2:
         raise ValueError(
             'invalid "or": exactly 2 operands required (inside %r)' % (
                 condition))
-      return _convert(node.values[0]) or _convert(node.values[1])
+      left = _convert(node.values[0])
+      right = _convert(node.values[1])
+      if not isinstance(left, bool):
+        raise ValueError(
+            'invalid "or" operand %r (inside %r)' % (left, condition))
+      if not isinstance(right, bool):
+        raise ValueError(
+            'invalid "or" operand %r (inside %r)' % (right, condition))
+      return left or right
     elif isinstance(node, ast.BoolOp) and isinstance(node.op, ast.And):
       if len(node.values) != 2:
         raise ValueError(
             'invalid "and": exactly 2 operands required (inside %r)' % (
                 condition))
-      return _convert(node.values[0]) and _convert(node.values[1])
+      left = _convert(node.values[0])
+      right = _convert(node.values[1])
+      if not isinstance(left, bool):
+        raise ValueError(
+            'invalid "and" operand %r (inside %r)' % (left, condition))
+      if not isinstance(right, bool):
+        raise ValueError(
+            'invalid "and" operand %r (inside %r)' % (right, condition))
+      return left and right
     elif isinstance(node, ast.UnaryOp) and isinstance(node.op, ast.Not):
-      return not _convert(node.operand)
+      value = _convert(node.operand)
+      if not isinstance(value, bool):
+        raise ValueError(
+            'invalid "not" operand %r (inside %r)' % (value, condition))
+      return not value
     elif isinstance(node, ast.Compare):
       if len(node.ops) != 1:
         raise ValueError(

testing_support/fake_repos.py

@@ -329,15 +329,21 @@ class FakeRepos(FakeReposBase):
       'DEPS': """
 vars = {
   'DummyVariable': 'repo',
-  'false_var': 'False',
+  'false_var': False,
-  'true_var': 'True',
+  'false_str_var': 'False',
-  'str_var': '"abc"',
+  'true_var': True,
+  'true_str_var': 'True',
+  'str_var': 'abc',
+  'cond_var': 'false_str_var and true_var',
 }
 gclient_gn_args_file = 'src/gclient.args'
 gclient_gn_args = [
   'false_var',
+  'false_str_var',
   'true_var',
+  'true_str_var',
   'str_var',
+  'cond_var',
 ]
 deps = {
   'src/repo2': {
@@ -475,16 +481,22 @@ vars = {
   'hook1_contents': 'git_hooked1',
   'repo5_var': '/repo_5',
 
-  'false_var': 'False',
+  'false_var': False,
-  'true_var': 'True',
+  'false_str_var': 'False',
-  'str_var': '"abc"',
+  'true_var': True,
+  'true_str_var': 'True',
+  'str_var': 'abc',
+  'cond_var': 'false_str_var and true_var',
 }
 
 gclient_gn_args_file = 'src/gclient.args'
 gclient_gn_args = [
   'false_var',
+  'false_str_var',
   'true_var',
+  'true_str_var',
   'str_var',
+  'cond_var',
 ]
 
 allowed_hosts = [

tests/gclient_eval_unittest.py

@@ -145,6 +145,21 @@ class EvaluateConditionTest(unittest.TestCase):
     self.assertFalse(gclient_eval.EvaluateCondition(
         'foo == "bar"', {'foo': '"baz"'}))
 
+  def test_string_bool(self):
+    self.assertFalse(gclient_eval.EvaluateCondition(
+        'false_str_var and true_var',
+        {'false_str_var': 'False', 'true_var': True}))
+
+  def test_string_bool_typo(self):
+    with self.assertRaises(ValueError) as cm:
+      gclient_eval.EvaluateCondition(
+          'false_var_str and true_var',
+          {'false_str_var': 'False', 'true_var': True})
+    self.assertIn(
+        'invalid "and" operand \'false_var_str\' '
+            '(inside \'false_var_str and true_var\')',
+        str(cm.exception))
+
 
 if __name__ == '__main__':
   level = logging.DEBUG if '-v' in sys.argv else logging.FATAL

tests/gclient_smoketest.py

@@ -327,8 +327,11 @@ class GClientSmokeGIT(GClientSmokeBase):
     tree['src/gclient.args'] = '\n'.join([
         '# Generated from \'DEPS\'',
         'false_var = false',
+        'false_str_var = false',
         'true_var = true',
+        'true_str_var = true',
         'str_var = "abc"',
+        'cond_var = false',
     ])
     self.assertTree(tree)
     # Test incremental sync: delete-unversioned_trees isn't there.
@@ -345,8 +348,11 @@ class GClientSmokeGIT(GClientSmokeBase):
     tree['src/gclient.args'] = '\n'.join([
         '# Generated from \'DEPS\'',
         'false_var = false',
+        'false_str_var = false',
         'true_var = true',
+        'true_str_var = true',
         'str_var = "abc"',
+        'cond_var = false',
     ])
     self.assertTree(tree)
 
@@ -384,8 +390,11 @@ class GClientSmokeGIT(GClientSmokeBase):
     tree['src/gclient.args'] = '\n'.join([
         '# Generated from \'DEPS\'',
         'false_var = false',
+        'false_str_var = false',
         'true_var = true',
+        'true_str_var = true',
         'str_var = "abc"',
+        'cond_var = false',
     ])
     self.assertTree(tree)
 
@@ -426,8 +435,11 @@ class GClientSmokeGIT(GClientSmokeBase):
     tree['src/gclient.args'] = '\n'.join([
         '# Generated from \'DEPS\'',
         'false_var = false',
+        'false_str_var = false',
         'true_var = true',
+        'true_str_var = true',
         'str_var = "abc"',
+        'cond_var = false',
     ])
     self.assertTree(tree)
     # Test incremental sync: delete-unversioned_trees isn't there.
@@ -445,8 +457,11 @@ class GClientSmokeGIT(GClientSmokeBase):
     tree['src/gclient.args'] = '\n'.join([
         '# Generated from \'DEPS\'',
         'false_var = false',
+        'false_str_var = false',
         'true_var = true',
+        'true_str_var = true',
         'str_var = "abc"',
+        'cond_var = false',
     ])
     self.assertTree(tree)
 
@@ -626,7 +641,8 @@ class GClientSmokeGIT(GClientSmokeBase):
     self.maxDiff = None
     self.assertEqual([
         'gclient_gn_args_file = "src/gclient.args"',
-        'gclient_gn_args = [\'false_var\', \'true_var\', \'str_var\']',
+        'gclient_gn_args = [\'false_var\', \'false_str_var\', \'true_var\', '
+            '\'true_str_var\', \'str_var\', \'cond_var\']',
         'allowed_hosts = [',
         '  "git://127.0.0.1:20000/git/",',
         ']',
@@ -749,7 +765,13 @@ class GClientSmokeGIT(GClientSmokeBase):
         '  "DummyVariable": \'repo\',',
         '',
         '  # src',
-        '  "false_var": \'False\',',
+        '  "cond_var": \'false_str_var and true_var\',',
+        '',
+        '  # src',
+        '  "false_str_var": \'False\',',
+        '',
+        '  # src',
+        '  "false_var": False,',
         '',
         '  # src',
         '  "git_base": \'git://127.0.0.1:20000/git/\',',
@@ -761,10 +783,13 @@ class GClientSmokeGIT(GClientSmokeBase):
         '  "repo5_var": \'/repo_5\',',
         '',
         '  # src',
-        '  "str_var": \'"abc"\',',
+        '  "str_var": \'abc\',',
+        '',
+        '  # src',
+        '  "true_str_var": \'True\',',
         '',
         '  # src',
-        '  "true_var": \'True\',',
+        '  "true_var": True,',
         '',
         '}',
         '',
@@ -790,7 +815,8 @@ class GClientSmokeGIT(GClientSmokeBase):
 
     self.assertEqual([
         'gclient_gn_args_file = "src/gclient.args"',
-        'gclient_gn_args = [\'false_var\', \'true_var\', \'str_var\']',
+        'gclient_gn_args = [\'false_var\', \'false_str_var\', \'true_var\', '
+            '\'true_str_var\', \'str_var\', \'cond_var\']',
         'allowed_hosts = [',
         '  "git://127.0.0.1:20000/git/",',
         ']',
@@ -914,7 +940,13 @@ class GClientSmokeGIT(GClientSmokeBase):
         '  "DummyVariable": \'repo\',',
         '',
         '  # src',
-        '  "false_var": \'False\',',
+        '  "cond_var": \'false_str_var and true_var\',',
+        '',
+        '  # src',
+        '  "false_str_var": \'False\',',
+        '',
+        '  # src',
+        '  "false_var": False,',
         '',
         '  # src',
         '  "git_base": \'git://127.0.0.1:20000/git/\',',
@@ -926,10 +958,13 @@ class GClientSmokeGIT(GClientSmokeBase):
         '  "repo5_var": \'/repo_5\',',
         '',
         '  # src',
-        '  "str_var": \'"abc"\',',
+        '  "str_var": \'abc\',',
+        '',
+        '  # src',
+        '  "true_str_var": \'True\',',
         '',
         '  # src',
-        '  "true_var": \'True\',',
+        '  "true_var": True,',
         '',
         '}',
         '',