gclient: Remove syntax validation flags.

Change-Id: I7215335eb4592cba80d31595b3bac75f55372dee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2083589Reviewed-by: Josip Sokcevic <sokcevic@google.com>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

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:

gclient_eval.py

@@ -372,44 +372,10 @@ def Exec(content, filename='<unknown>', vars_override=None, builtin_vars=None):
     value = _gclient_eval(node, filename, vars_dict)
     local_scope.SetNode(name, value, node)
 
+  try:
     return _GCLIENT_SCHEMA.validate(local_scope)
-
-
-def ExecLegacy(content, filename='<unknown>', 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)
+  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. '<string>', '<unknown>'.
     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)
 
   vars_dict = result.get('vars', {})
   if 'deps' in result:

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, '<unknown>', vars_override)
+    ]), '<unknown>', vars_override)
 
   def test_supports_vars_inside_vars(self):
     deps_file = '\n'.join([
@@ -810,9 +811,7 @@ class ParseTest(unittest.TestCase):
         '  },',
         '}',
     ])
-    for validate_syntax in False, True:
-      local_scope = gclient_eval.Parse(
-          deps_file, validate_syntax, '<unknown>', None)
+    local_scope = gclient_eval.Parse(deps_file, '<unknown>', None)
     self.assertEqual({
         'vars': {'foo': 'bar',
                  'baz': '"bar" == "bar"'},
@@ -828,9 +827,7 @@ 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, '<unknown>', None, builtin_vars)
+    local_scope = gclient_eval.Parse(deps_file, '<unknown>', None, builtin_vars)
     self.assertEqual({
       'deps': {'a_dep': {'url': 'afoob',
                          'dep_type': 'git'}},
@@ -846,9 +843,7 @@ 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, '<unknown>', None, builtin_vars)
+    local_scope = gclient_eval.Parse(deps_file, '<unknown>', None, builtin_vars)
     self.assertEqual({
       'vars': {'builtin_var': 'bar'},
       'deps': {'a_dep': {'url': 'afoob',
@@ -863,17 +858,15 @@ 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, '<unknown>', vars_override, builtin_vars)
+        deps_file, '<unknown>', 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)
+    local_scope = self.callParse()
     self.assertEqual({
         'vars': {'foo': 'bar'},
         'deps': {'a_dep': {'url': 'abarb',
@@ -881,9 +874,7 @@ class ParseTest(unittest.TestCase):
     }, 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'})
+    local_scope = self.callParse(vars_override={'foo': 'baz'})
     self.assertEqual({
         'vars': {'foo': 'bar'},
         'deps': {'a_dep': {'url': 'abazb',
@@ -901,30 +892,22 @@ class ParseTest(unittest.TestCase):
     ])
 
     with self.assertRaises(KeyError) as cm:
-      gclient_eval.Parse(
-          deps_file, True, '<unknown>', {'baz': 'lalala'})
+      gclient_eval.Parse(deps_file, '<unknown>', {'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, '<unknown>', {'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, '<unknown>')
+    ]), '<unknown>')
     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": {',
@@ -932,7 +915,7 @@ class ParseTest(unittest.TestCase):
       '     "condition": "checkout_android",',
       '  },',
       '}',
-      ]), validate_syntax, '<unknown>')
+    ]), '<unknown>')
     self.assertEqual({
         'deps': {'a_dep': {'url': 'a_url@a_rev',
                            'dep_type': 'git',
@@ -940,7 +923,6 @@ class ParseTest(unittest.TestCase):
     }, 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",',
@@ -950,14 +932,13 @@ class ParseTest(unittest.TestCase):
       '     "a_dep": None,',
       '  },',
       '}',
-      ]), validate_syntax, '<unknown>')
+    ]), '<unknown>')
     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",',
@@ -967,7 +948,7 @@ class ParseTest(unittest.TestCase):
       '     "b_dep": "b_url@b_rev"',
       '  },',
       '}',
-      ]), validate_syntax, '<unknown>')
+    ]), '<unknown>')
     self.assertEqual({
         'deps': {'a_dep': {'url': 'a_url@a_rev',
                            'dep_type': 'git'},
@@ -977,7 +958,6 @@ class ParseTest(unittest.TestCase):
     }, 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",',
@@ -987,14 +967,13 @@ class ParseTest(unittest.TestCase):
       '     "a_dep": "a_url@a_rev"',
       '  },',
       '}',
-      ]), validate_syntax, '<unknown>')
+    ]), '<unknown>')
     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([
       'deps = {',
       '  "a_dep": {',
@@ -1007,7 +986,7 @@ class ParseTest(unittest.TestCase):
       '     "a_dep": "a_url@a_rev"',
       '  },',
       '}',
-      ]), validate_syntax, '<unknown>')
+    ]), '<unknown>')
     self.assertEqual({
         'deps': {
             'a_dep': {'url': 'a_url@a_rev',
@@ -1017,7 +996,6 @@ class ParseTest(unittest.TestCase):
     }, 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": {'
@@ -1027,7 +1005,7 @@ class ParseTest(unittest.TestCase):
       '     "a_dep": "a_url@a_rev"',
       '  },',
       '}',
-      ]), validate_syntax, '<unknown>')
+    ]), '<unknown>')
     self.assertEqual({
         'deps': {
             'a_dep': {'url': 'a_url@a_rev',
@@ -1037,7 +1015,6 @@ class ParseTest(unittest.TestCase):
     }, 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 = {',
@@ -1051,11 +1028,10 @@ class ParseTest(unittest.TestCase):
         '     "a_dep": "a_url@b_rev"',
         '  },',
         '}',
-        ]), validate_syntax, '<unknown>')
+      ]), '<unknown>')
     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 = [',
       '  {',
@@ -1069,7 +1045,7 @@ class ParseTest(unittest.TestCase):
       '    },',
       '  ]',
       '}',
-      ]), validate_syntax, '<unknown>')
+    ]), '<unknown>')
     self.assertEqual({
         "hooks": [{"action": ["a", "action"]},
                   {"action": ["b", "action"], "condition": "checkout_mac"}],

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))