diff --git a/gclient.py b/gclient.py index c347fcb24..0f895c18f 100755 --- a/gclient.py +++ b/gclient.py @@ -716,8 +716,7 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): if deps_content: try: local_scope = gclient_eval.Parse( - deps_content, self._get_option('validate_syntax', False), - filepath, self.get_vars(), self.get_builtin_vars()) + deps_content, filepath, self.get_vars(), self.get_builtin_vars()) except SyntaxError as e: gclient_utils.SyntaxErrorToError(filepath, e) @@ -2698,12 +2697,6 @@ def CMDsync(parser, args): help='DEPRECATED: This is a no-op.') parser.add_option('-m', '--manually_grab_svn_rev', action='store_true', help='DEPRECATED: This is a no-op.') - # TODO(phajdan.jr): Remove validation options once default (crbug/570091). - parser.add_option('--validate-syntax', action='store_true', default=True, - help='Validate the .gclient and DEPS syntax') - parser.add_option('--disable-syntax-validation', action='store_false', - dest='validate_syntax', - help='Disable validation of .gclient and DEPS syntax.') parser.add_option('--no-rebase-patch-ref', action='store_false', dest='rebase_patch_ref', default=True, help='Bypass rebase of the patch ref after checkout.') @@ -2745,7 +2738,6 @@ CMDupdate = CMDsync def CMDvalidate(parser, args): """Validates the .gclient and DEPS syntax.""" options, args = parser.parse_args(args) - options.validate_syntax = True client = GClient.LoadCurrentConfig(options) rv = client.RunOnDeps('validate', args) if rv == 0: diff --git a/gclient_eval.py b/gclient_eval.py index c41ff398d..13abeb863 100644 --- a/gclient_eval.py +++ b/gclient_eval.py @@ -372,44 +372,10 @@ def Exec(content, filename='', vars_override=None, builtin_vars=None): value = _gclient_eval(node, filename, vars_dict) local_scope.SetNode(name, value, node) - return _GCLIENT_SCHEMA.validate(local_scope) - - -def ExecLegacy(content, filename='', vars_override=None, - builtin_vars=None): - """Executes a DEPS file |content| using exec.""" - local_scope = {} - global_scope = {'Var': lambda var_name: '{%s}' % var_name} - - # If we use 'exec' directly, it complains that 'Parse' contains a nested - # function with free variables. - # This is because on versions of Python < 2.7.9, "exec(a, b, c)" not the same - # as "exec a in b, c" (See https://bugs.python.org/issue21591). - eval(compile(content, filename, 'exec'), global_scope, local_scope) - - vars_dict = {} - vars_dict.update(local_scope.get('vars', {})) - if builtin_vars: - vars_dict.update(builtin_vars) - if vars_override: - vars_dict.update({k: v for k, v in vars_override.items() if k in vars_dict}) - - if not vars_dict: - return local_scope - - def _DeepFormat(node): - if isinstance(node, basestring): - return node.format(**vars_dict) - elif isinstance(node, dict): - return {k.format(**vars_dict): _DeepFormat(v) for k, v in node.items()} - elif isinstance(node, list): - return [_DeepFormat(elem) for elem in node] - elif isinstance(node, tuple): - return tuple(_DeepFormat(elem) for elem in node) - else: - return node - - return _DeepFormat(local_scope) + try: + return _GCLIENT_SCHEMA.validate(local_scope) + except schema.SchemaError as e: + raise gclient_utils.Error(str(e)) def _StandardizeDeps(deps_dict, vars_dict): @@ -477,8 +443,7 @@ def UpdateCondition(info_dict, op, new_condition): del info_dict['condition'] -def Parse(content, validate_syntax, filename, vars_override=None, - builtin_vars=None): +def Parse(content, filename, vars_override=None, builtin_vars=None): """Parses DEPS strings. Executes the Python-like string stored in content, resulting in a Python @@ -487,8 +452,6 @@ def Parse(content, validate_syntax, filename, vars_override=None, Args: content: str. DEPS file stored as a string. - validate_syntax: bool. Whether syntax should be validated using the schema - defined above. filename: str. The name of the DEPS file, or a string describing the source of the content, e.g. '', ''. vars_override: dict, optional. A dictionary with overrides for the variables @@ -500,10 +463,7 @@ def Parse(content, validate_syntax, filename, vars_override=None, A Python dict with the parsed contents of the DEPS file, as specified by the schema above. """ - if validate_syntax: - result = Exec(content, filename, vars_override, builtin_vars) - else: - result = ExecLegacy(content, filename, vars_override, builtin_vars) + result = Exec(content, filename, vars_override, builtin_vars) vars_dict = result.get('vars', {}) if 'deps' in result: diff --git a/tests/gclient_eval_unittest.py b/tests/gclient_eval_unittest.py index dfb2813d6..92c9ddfd5 100755 --- a/tests/gclient_eval_unittest.py +++ b/tests/gclient_eval_unittest.py @@ -20,6 +20,7 @@ metrics.DISABLE_METRICS_COLLECTION = True import gclient import gclient_eval +import gclient_utils class GClientEvalTest(unittest.TestCase): @@ -103,7 +104,7 @@ class ExecTest(unittest.TestCase): 'invalid assignment: overrides var \'a\'', str(cm.exception)) def test_schema_wrong_type(self): - with self.assertRaises(schema.SchemaError): + with self.assertRaises(gclient_utils.Error): gclient_eval.Exec('include_rules = {}') def test_recursedeps_list(self): @@ -787,7 +788,7 @@ class RevisionTest(unittest.TestCase): class ParseTest(unittest.TestCase): - def callParse(self, validate_syntax=True, vars_override=None): + def callParse(self, vars_override=None): return gclient_eval.Parse('\n'.join([ 'vars = {', ' "foo": "bar",', @@ -795,7 +796,7 @@ class ParseTest(unittest.TestCase): 'deps = {', ' "a_dep": "a{foo}b",', '}', - ]), validate_syntax, '', vars_override) + ]), '', vars_override) def test_supports_vars_inside_vars(self): deps_file = '\n'.join([ @@ -810,16 +811,14 @@ class ParseTest(unittest.TestCase): ' },', '}', ]) - for validate_syntax in False, True: - local_scope = gclient_eval.Parse( - deps_file, validate_syntax, '', None) - self.assertEqual({ - 'vars': {'foo': 'bar', - 'baz': '"bar" == "bar"'}, - 'deps': {'src/baz': {'url': 'baz_url', - 'dep_type': 'git', - 'condition': 'baz'}}, - }, local_scope) + local_scope = gclient_eval.Parse(deps_file, '', None) + self.assertEqual({ + 'vars': {'foo': 'bar', + 'baz': '"bar" == "bar"'}, + 'deps': {'src/baz': {'url': 'baz_url', + 'dep_type': 'git', + 'condition': 'baz'}}, + }, local_scope) def test_has_builtin_vars(self): builtin_vars = {'builtin_var': 'foo'} @@ -828,13 +827,11 @@ class ParseTest(unittest.TestCase): ' "a_dep": "a{builtin_var}b",', '}', ]) - for validate_syntax in False, True: - local_scope = gclient_eval.Parse( - deps_file, validate_syntax, '', None, builtin_vars) - self.assertEqual({ - 'deps': {'a_dep': {'url': 'afoob', - 'dep_type': 'git'}}, - }, local_scope) + local_scope = gclient_eval.Parse(deps_file, '', None, builtin_vars) + self.assertEqual({ + 'deps': {'a_dep': {'url': 'afoob', + 'dep_type': 'git'}}, + }, local_scope) def test_declaring_builtin_var_has_no_effect(self): builtin_vars = {'builtin_var': 'foo'} @@ -846,14 +843,12 @@ class ParseTest(unittest.TestCase): ' "a_dep": "a{builtin_var}b",', '}', ]) - for validate_syntax in False, True: - local_scope = gclient_eval.Parse( - deps_file, validate_syntax, '', None, builtin_vars) - self.assertEqual({ - 'vars': {'builtin_var': 'bar'}, - 'deps': {'a_dep': {'url': 'afoob', - 'dep_type': 'git'}}, - }, local_scope) + local_scope = gclient_eval.Parse(deps_file, '', None, builtin_vars) + self.assertEqual({ + 'vars': {'builtin_var': 'bar'}, + 'deps': {'a_dep': {'url': 'afoob', + 'dep_type': 'git'}}, + }, local_scope) def test_override_builtin_var(self): builtin_vars = {'builtin_var': 'foo'} @@ -863,32 +858,28 @@ class ParseTest(unittest.TestCase): ' "a_dep": "a{builtin_var}b",', '}', ]) - for validate_syntax in False, True: - local_scope = gclient_eval.Parse( - deps_file, validate_syntax, '', vars_override, builtin_vars) - self.assertEqual({ - 'deps': {'a_dep': {'url': 'aoverrideb', - 'dep_type': 'git'}}, - }, local_scope, str(local_scope)) + local_scope = gclient_eval.Parse( + deps_file, '', vars_override, builtin_vars) + self.assertEqual({ + 'deps': {'a_dep': {'url': 'aoverrideb', + 'dep_type': 'git'}}, + }, local_scope, str(local_scope)) def test_expands_vars(self): - for validate_syntax in True, False: - local_scope = self.callParse(validate_syntax=validate_syntax) - self.assertEqual({ - 'vars': {'foo': 'bar'}, - 'deps': {'a_dep': {'url': 'abarb', - 'dep_type': 'git'}}, - }, local_scope) + local_scope = self.callParse() + self.assertEqual({ + 'vars': {'foo': 'bar'}, + 'deps': {'a_dep': {'url': 'abarb', + 'dep_type': 'git'}}, + }, local_scope) def test_overrides_vars(self): - for validate_syntax in True, False: - local_scope = self.callParse(validate_syntax=validate_syntax, - vars_override={'foo': 'baz'}) - self.assertEqual({ - 'vars': {'foo': 'bar'}, - 'deps': {'a_dep': {'url': 'abazb', - 'dep_type': 'git'}}, - }, local_scope) + local_scope = self.callParse(vars_override={'foo': 'baz'}) + self.assertEqual({ + 'vars': {'foo': 'bar'}, + 'deps': {'a_dep': {'url': 'abazb', + 'dep_type': 'git'}}, + }, local_scope) def test_no_extra_vars(self): deps_file = '\n'.join([ @@ -901,101 +892,131 @@ class ParseTest(unittest.TestCase): ]) with self.assertRaises(KeyError) as cm: - gclient_eval.Parse( - deps_file, True, '', {'baz': 'lalala'}) + gclient_eval.Parse(deps_file, '', {'baz': 'lalala'}) self.assertIn('baz was used as a variable, but was not declared', str(cm.exception)) - with self.assertRaises(KeyError) as cm: - gclient_eval.Parse( - deps_file, False, '', {'baz': 'lalala'}) - self.assertIn('baz', str(cm.exception)) - def test_standardizes_deps_string_dep(self): - for validate_syntax in True, False: - local_scope = gclient_eval.Parse('\n'.join([ - 'deps = {', - ' "a_dep": "a_url@a_rev",', - '}', - ]), validate_syntax, '') - self.assertEqual({ - 'deps': {'a_dep': {'url': 'a_url@a_rev', - 'dep_type': 'git'}}, - }, local_scope) + local_scope = gclient_eval.Parse('\n'.join([ + 'deps = {', + ' "a_dep": "a_url@a_rev",', + '}', + ]), '') + self.assertEqual({ + 'deps': {'a_dep': {'url': 'a_url@a_rev', + 'dep_type': 'git'}}, + }, local_scope) def test_standardizes_deps_dict_dep(self): - for validate_syntax in True, False: - local_scope = gclient_eval.Parse('\n'.join([ - 'deps = {', - ' "a_dep": {', - ' "url": "a_url@a_rev",', - ' "condition": "checkout_android",', - ' },', - '}', - ]), validate_syntax, '') - self.assertEqual({ - 'deps': {'a_dep': {'url': 'a_url@a_rev', - 'dep_type': 'git', - 'condition': 'checkout_android'}}, - }, local_scope) + local_scope = gclient_eval.Parse('\n'.join([ + 'deps = {', + ' "a_dep": {', + ' "url": "a_url@a_rev",', + ' "condition": "checkout_android",', + ' },', + '}', + ]), '') + self.assertEqual({ + 'deps': {'a_dep': {'url': 'a_url@a_rev', + 'dep_type': 'git', + 'condition': 'checkout_android'}}, + }, local_scope) def test_ignores_none_in_deps_os(self): - for validate_syntax in True, False: - local_scope = gclient_eval.Parse('\n'.join([ - 'deps = {', - ' "a_dep": "a_url@a_rev",', - '}', - 'deps_os = {', - ' "mac": {', - ' "a_dep": None,', - ' },', - '}', - ]), validate_syntax, '') - self.assertEqual({ - 'deps': {'a_dep': {'url': 'a_url@a_rev', - 'dep_type': 'git'}}, - }, local_scope) + local_scope = gclient_eval.Parse('\n'.join([ + 'deps = {', + ' "a_dep": "a_url@a_rev",', + '}', + 'deps_os = {', + ' "mac": {', + ' "a_dep": None,', + ' },', + '}', + ]), '') + self.assertEqual({ + 'deps': {'a_dep': {'url': 'a_url@a_rev', + 'dep_type': 'git'}}, + }, local_scope) def test_merges_deps_os_extra_dep(self): - for validate_syntax in True, False: - local_scope = gclient_eval.Parse('\n'.join([ - 'deps = {', - ' "a_dep": "a_url@a_rev",', - '}', - 'deps_os = {', - ' "mac": {', - ' "b_dep": "b_url@b_rev"', - ' },', - '}', - ]), validate_syntax, '') - self.assertEqual({ - 'deps': {'a_dep': {'url': 'a_url@a_rev', - 'dep_type': 'git'}, - 'b_dep': {'url': 'b_url@b_rev', - 'dep_type': 'git', - 'condition': 'checkout_mac'}}, - }, local_scope) + local_scope = gclient_eval.Parse('\n'.join([ + 'deps = {', + ' "a_dep": "a_url@a_rev",', + '}', + 'deps_os = {', + ' "mac": {', + ' "b_dep": "b_url@b_rev"', + ' },', + '}', + ]), '') + self.assertEqual({ + 'deps': {'a_dep': {'url': 'a_url@a_rev', + 'dep_type': 'git'}, + 'b_dep': {'url': 'b_url@b_rev', + 'dep_type': 'git', + 'condition': 'checkout_mac'}}, + }, local_scope) def test_merges_deps_os_existing_dep_with_no_condition(self): - for validate_syntax in True, False: - local_scope = gclient_eval.Parse('\n'.join([ - 'deps = {', - ' "a_dep": "a_url@a_rev",', - '}', - 'deps_os = {', - ' "mac": {', - ' "a_dep": "a_url@a_rev"', - ' },', - '}', - ]), validate_syntax, '') - self.assertEqual({ - 'deps': {'a_dep': {'url': 'a_url@a_rev', - 'dep_type': 'git'}}, - }, local_scope) + local_scope = gclient_eval.Parse('\n'.join([ + 'deps = {', + ' "a_dep": "a_url@a_rev",', + '}', + 'deps_os = {', + ' "mac": {', + ' "a_dep": "a_url@a_rev"', + ' },', + '}', + ]), '') + self.assertEqual({ + 'deps': {'a_dep': {'url': 'a_url@a_rev', + 'dep_type': 'git'}}, + }, local_scope) def test_merges_deps_os_existing_dep_with_condition(self): - for validate_syntax in True, False: - local_scope = gclient_eval.Parse('\n'.join([ + local_scope = gclient_eval.Parse('\n'.join([ + 'deps = {', + ' "a_dep": {', + ' "url": "a_url@a_rev",', + ' "condition": "some_condition",', + ' },', + '}', + 'deps_os = {', + ' "mac": {', + ' "a_dep": "a_url@a_rev"', + ' },', + '}', + ]), '') + self.assertEqual({ + 'deps': { + 'a_dep': {'url': 'a_url@a_rev', + 'dep_type': 'git', + 'condition': '(checkout_mac) or (some_condition)'}, + }, + }, local_scope) + + def test_merges_deps_os_multiple_os(self): + local_scope = gclient_eval.Parse('\n'.join([ + 'deps_os = {', + ' "win": {' + ' "a_dep": "a_url@a_rev"', + ' },', + ' "mac": {', + ' "a_dep": "a_url@a_rev"', + ' },', + '}', + ]), '') + self.assertEqual({ + 'deps': { + 'a_dep': {'url': 'a_url@a_rev', + 'dep_type': 'git', + 'condition': '(checkout_mac) or (checkout_win)'}, + }, + }, local_scope) + + def test_fails_to_merge_same_dep_with_different_revisions(self): + with self.assertRaises(gclient_eval.gclient_utils.Error) as cm: + gclient_eval.Parse('\n'.join([ 'deps = {', ' "a_dep": {', ' "url": "a_url@a_rev",', @@ -1004,76 +1025,31 @@ class ParseTest(unittest.TestCase): '}', 'deps_os = {', ' "mac": {', - ' "a_dep": "a_url@a_rev"', + ' "a_dep": "a_url@b_rev"', ' },', '}', - ]), validate_syntax, '') - self.assertEqual({ - 'deps': { - 'a_dep': {'url': 'a_url@a_rev', - 'dep_type': 'git', - 'condition': '(checkout_mac) or (some_condition)'}, - }, - }, local_scope) - - def test_merges_deps_os_multiple_os(self): - for validate_syntax in True, False: - local_scope = gclient_eval.Parse('\n'.join([ - 'deps_os = {', - ' "win": {' - ' "a_dep": "a_url@a_rev"', - ' },', - ' "mac": {', - ' "a_dep": "a_url@a_rev"', - ' },', - '}', - ]), validate_syntax, '') - self.assertEqual({ - 'deps': { - 'a_dep': {'url': 'a_url@a_rev', - 'dep_type': 'git', - 'condition': '(checkout_mac) or (checkout_win)'}, - }, - }, local_scope) - - def test_fails_to_merge_same_dep_with_different_revisions(self): - for validate_syntax in True, False: - with self.assertRaises(gclient_eval.gclient_utils.Error) as cm: - gclient_eval.Parse('\n'.join([ - 'deps = {', - ' "a_dep": {', - ' "url": "a_url@a_rev",', - ' "condition": "some_condition",', - ' },', - '}', - 'deps_os = {', - ' "mac": {', - ' "a_dep": "a_url@b_rev"', - ' },', - '}', - ]), validate_syntax, '') - self.assertIn('conflicts with existing deps', str(cm.exception)) + ]), '') + self.assertIn('conflicts with existing deps', str(cm.exception)) def test_merges_hooks_os(self): - for validate_syntax in True, False: - local_scope = gclient_eval.Parse('\n'.join([ - 'hooks = [', - ' {', - ' "action": ["a", "action"],', - ' },', - ']', - 'hooks_os = {', - ' "mac": [', - ' {', - ' "action": ["b", "action"]', - ' },', - ' ]', - '}', - ]), validate_syntax, '') - self.assertEqual({ - "hooks": [{"action": ["a", "action"]}, - {"action": ["b", "action"], "condition": "checkout_mac"}], - }, local_scope) + local_scope = gclient_eval.Parse('\n'.join([ + 'hooks = [', + ' {', + ' "action": ["a", "action"],', + ' },', + ']', + 'hooks_os = {', + ' "mac": [', + ' {', + ' "action": ["b", "action"]', + ' },', + ' ]', + '}', + ]), '') + self.assertEqual({ + "hooks": [{"action": ["a", "action"]}, + {"action": ["b", "action"], "condition": "checkout_mac"}], + }, local_scope) diff --git a/tests/gclient_test.py b/tests/gclient_test.py index bd5889a0d..63816944e 100755 --- a/tests/gclient_test.py +++ b/tests/gclient_test.py @@ -1002,7 +1002,6 @@ class GclientTest(trial_dir.TestCase): '}') options, _ = gclient.OptionParser().parse_args([]) options.ignore_dep_type = 'git' - options.validate_syntax = True obj = gclient.GClient.LoadCurrentConfig(options) self.assertEqual(1, len(obj.dependencies)) @@ -1132,7 +1131,7 @@ class GclientTest(trial_dir.TestCase): obj.RunOnDeps('None', []) self.fail() except gclient_utils.Error as e: - self.assertIn('allowed_hosts must be', str(e)) + self.assertIn('Key \'allowed_hosts\' error:', str(e)) finally: self._get_processed() @@ -1160,7 +1159,6 @@ class GclientTest(trial_dir.TestCase): ' }\n' '}') options, _ = gclient.OptionParser().parse_args([]) - options.validate_syntax = True obj = gclient.GClient.LoadCurrentConfig(options) self.assertEqual(1, len(obj.dependencies)) @@ -1202,7 +1200,6 @@ class GclientTest(trial_dir.TestCase): '}') options, _ = gclient.OptionParser().parse_args([]) options.ignore_dep_type = 'cipd' - options.validate_syntax = True obj = gclient.GClient.LoadCurrentConfig(options) self.assertEqual(1, len(obj.dependencies))