diff --git a/gclient.py b/gclient.py index ea58e96f4..c2f4040e5 100755 --- a/gclient.py +++ b/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)) diff --git a/gclient_eval.py b/gclient_eval.py index c1dee2aee..110cdacee 100644 --- a/gclient_eval.py +++ b/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( - 'invalid name %r (inside %r)' % (node.id, condition)) + # Implicitly convert unrecognized names to strings. + # 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( diff --git a/testing_support/fake_repos.py b/testing_support/fake_repos.py index b6c2b8766..ebaccc680 100755 --- a/testing_support/fake_repos.py +++ b/testing_support/fake_repos.py @@ -329,15 +329,21 @@ class FakeRepos(FakeReposBase): 'DEPS': """ vars = { 'DummyVariable': 'repo', - 'false_var': 'False', - 'true_var': 'True', - 'str_var': '"abc"', + 'false_var': False, + 'false_str_var': 'False', + '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', - 'true_var': 'True', - 'str_var': '"abc"', + 'false_var': False, + 'false_str_var': 'False', + '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 = [ diff --git a/tests/gclient_eval_unittest.py b/tests/gclient_eval_unittest.py index d8579b8ef..3fe307791 100755 --- a/tests/gclient_eval_unittest.py +++ b/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 diff --git a/tests/gclient_smoketest.py b/tests/gclient_smoketest.py index e924a386d..03065b8e5 100755 --- a/tests/gclient_smoketest.py +++ b/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,', '', '}', '',