Revert "gclient eval: Expand vars while parsing DEPS files"

This reverts commit 88f9c40e.

Reason for revert: suspected to be causing update scripts failures and prevent gce bots from connecting
https://build.chromium.org/deprecated/tryserver.chromium.win/builders/win7_chromium_rel_ng/builds/128479/steps/update_scripts/logs/stdio

Original change's description:
> gclient eval: Expand vars while parsing DEPS files
> 
> Introduce a Parse function that takes care of expanding vars while parsing
> the DEPS file.
> 
> It wraps Exec and exec calls, and supports deferring the expansion until
> later, so gclient flatten gets access to the unexpanded version.
> 
> Bug: 821199
> Change-Id: I943b021cc4474c9cda67b3816b841dd8ada3f5b2
> Reviewed-on: https://chromium-review.googlesource.com/973749
> Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
> Reviewed-by: Aaron Gable <agable@chromium.org>
> Reviewed-by: Dirk Pranke <dpranke@chromium.org>

TBR=agable@chromium.org,dpranke@chromium.org,ehmaldonado@chromium.org

Change-Id: Ib9b84c13ef9b56735fd8f714b74a6601c61715e5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 821199
Reviewed-on: https://chromium-review.googlesource.com/976721Reviewed-by: Benjamin Pastene <bpastene@chromium.org>
Commit-Queue: Benjamin Pastene <bpastene@chromium.org>

gclient.py

@@ -736,7 +736,7 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
     deps_to_add.sort(key=lambda x: x.name)
     return deps_to_add
 
-  def ParseDepsFile(self, expand_vars=True):
+  def ParseDepsFile(self):
     """Parses the DEPS file for this dependency."""
     assert not self.deps_parsed
     assert not self.dependencies
@@ -765,14 +765,15 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
 
     local_scope = {}
     if deps_content:
+      # Eval the content.
       try:
-        vars_override = self.get_vars()
-        if self.parent:
-          vars_override.update(self.parent.get_vars())
-        local_scope = gclient_eval.Parse(
-            deps_content, expand_vars,
-            self._get_option('validate_syntax', False),
-            filepath, vars_override)
+        if self._get_option('validate_syntax', False):
+          local_scope = gclient_eval.Exec(deps_content, filepath)
+        else:
+          global_scope = {
+            'Var': lambda var_name: '{%s}' % var_name,
+          }
+          exec(deps_content, global_scope, local_scope)
       except SyntaxError as e:
         gclient_utils.SyntaxErrorToError(filepath, e)
 
@@ -987,7 +988,7 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
           file_list[i] = file_list[i][1:]
 
     # Always parse the DEPS file.
-    self.ParseDepsFile(expand_vars=(command != 'flatten'))
+    self.ParseDepsFile()
     self._run_is_done(file_list or [], parsed_url)
     if command in ('update', 'revert') and not options.noprehooks:
       self.RunPreDepsHooks()
@@ -1863,7 +1864,7 @@ it or fix the checkout.
           print('%s: %s' % (x, entries[x]))
     logging.info(str(self))
 
-  def ParseDepsFile(self, expand_vars=None):
+  def ParseDepsFile(self):
     """No DEPS to parse for a .gclient file."""
     raise gclient_utils.Error('Internal error')
 
@@ -1972,7 +1973,7 @@ class CipdDependency(Dependency):
       self._cipd_package = self._cipd_root.add_package(
           self._cipd_subdir, self._package_name, self._package_version)
 
-  def ParseDepsFile(self, expand_vars=None):
+  def ParseDepsFile(self):
     """CIPD dependencies are not currently allowed to have nested deps."""
     self.add_dependencies_and_close([], [])
 
@@ -2929,9 +2930,7 @@ def CMDsetdep(parser, args):
         'DEPS file %s does not exist.' % options.deps_file)
   with open(options.deps_file) as f:
     contents = f.read()
-  local_scope = gclient_eval.Parse(
-      contents, expand_vars=True, validate_syntax=True,
-      filename=options.deps_file)
+  local_scope = gclient_eval.Exec(contents)
 
   for var in options.vars:
     name, _, value = var.partition('=')

gclient_eval.py

@@ -37,7 +37,7 @@ class _NodeDict(collections.MutableMapping):
   def GetNode(self, key):
     return self.data[key][1]
 
-  def SetNode(self, key, value, node):
+  def _SetNode(self, key, value, node):
     self.data[key] = (value, node)
 
 
@@ -183,9 +183,8 @@ _GCLIENT_SCHEMA = schema.Schema(_NodeDictSchema({
 }))
 
 
-def _gclient_eval(node_or_string, vars_dict, expand_vars, filename):
+def _gclient_eval(node_or_string, filename='<unknown>'):
   """Safely evaluates a single expression. Returns the result."""
-  vars_dict = vars_dict or {}
   _allowed_names = {'None': None, 'True': True, 'False': False}
   if isinstance(node_or_string, basestring):
     node_or_string = ast.parse(node_or_string, filename=filename, mode='eval')
@@ -193,15 +192,7 @@ def _gclient_eval(node_or_string, vars_dict, expand_vars, filename):
     node_or_string = node_or_string.body
   def _convert(node):
     if isinstance(node, ast.Str):
-      if not expand_vars:
-        return node.s
-      try:
-        return node.s.format(**vars_dict)
-      except KeyError as e:
-        raise ValueError(
-            '%s was used as a variable, but was not declared in the vars dict '
-            '(file %r, line %s)' % (
-                e.message, filename, getattr(node, 'lineno', '<unknown>')))
+      return node.s
     elif isinstance(node, ast.Num):
       return node.n
     elif isinstance(node, ast.Tuple):
@@ -231,18 +222,7 @@ def _gclient_eval(node_or_string, vars_dict, expand_vars, filename):
         raise ValueError(
             'Var\'s argument must be a variable name (file %r, line %s)' % (
                 filename, getattr(node, 'lineno', '<unknown>')))
-      if not expand_vars:
-        return '{%s}' % arg
-      if vars_dict is None:
-        raise ValueError(
-            'vars must be declared before Var can be used (file %r, line %s)'
-            % (filename, getattr(node, 'lineno', '<unknown>')))
-      if arg not in vars_dict:
-        raise ValueError(
-            '%s was used as a variable, but was not declared in the vars dict '
-            '(file %r, line %s)' % (
-                arg, filename, getattr(node, 'lineno', '<unknown>')))
-      return vars_dict[arg]
+      return '{%s}' % arg
     elif isinstance(node, ast.BinOp) and isinstance(node.op, ast.Add):
       return _convert(node.left) + _convert(node.right)
     elif isinstance(node, ast.BinOp) and isinstance(node.op, ast.Mod):
@@ -255,18 +235,13 @@ def _gclient_eval(node_or_string, vars_dict, expand_vars, filename):
   return _convert(node_or_string)
 
 
-def Exec(content, expand_vars=True, filename='<unknown>'):
-  """Safely execs a set of assignments."""
+def Exec(content, filename='<unknown>'):
+  """Safely execs a set of assignments. Mutates |local_scope|."""
   node_or_string = ast.parse(content, filename=filename, mode='exec')
   if isinstance(node_or_string, ast.Expression):
     node_or_string = node_or_string.body
 
-  tokens = {
-      token[2]: list(token)
-      for token in tokenize.generate_tokens(
-          cStringIO.StringIO(content).readline)
-  }
-  local_scope = _NodeDict({}, tokens)
+  defined_variables = set()
   def _visit_in_module(node):
     if isinstance(node, ast.Assign):
       if len(node.targets) != 1:
@@ -278,15 +253,15 @@ def Exec(content, expand_vars=True, filename='<unknown>'):
         raise ValueError(
             'invalid assignment: target should be a name (file %r, line %s)' % (
                 filename, getattr(node, 'lineno', '<unknown>')))
-      value = _gclient_eval(node.value, local_scope.get('vars', None),
-                            expand_vars, filename)
+      value = _gclient_eval(node.value, filename=filename)
 
-      if target.id in local_scope:
+      if target.id in defined_variables:
         raise ValueError(
             'invalid assignment: overrides var %r (file %r, line %s)' % (
                 target.id, filename, getattr(node, 'lineno', '<unknown>')))
 
-      local_scope.SetNode(target.id, value, node.value)
+      defined_variables.add(target.id)
+      return target.id, (value, node.value)
     else:
       raise ValueError(
           'unexpected AST node: %s %s (file %r, line %s)' % (
@@ -294,8 +269,15 @@ def Exec(content, expand_vars=True, filename='<unknown>'):
               getattr(node, 'lineno', '<unknown>')))
 
   if isinstance(node_or_string, ast.Module):
+    data = []
     for stmt in node_or_string.body:
-      _visit_in_module(stmt)
+      data.append(_visit_in_module(stmt))
+    tokens = {
+        token[2]: list(token)
+        for token in tokenize.generate_tokens(
+            cStringIO.StringIO(content).readline)
+    }
+    local_scope = _NodeDict(data, tokens)
   else:
     raise ValueError(
         'unexpected AST node: %s %s (file %r, line %s)' % (
@@ -307,69 +289,6 @@ def Exec(content, expand_vars=True, filename='<unknown>'):
   return _GCLIENT_SCHEMA.validate(local_scope)
 
 
-def Parse(content, expand_vars, validate_syntax, filename, vars_override=None):
-  """Parses DEPS strings.
-
-  Executes the Python-like string stored in content, resulting in a Python
-  dictionary specifyied by the schema above. Supports syntax validation and
-  variable expansion.
-
-  Args:
-    content: str. DEPS file stored as a string.
-    expand_vars: bool. Whether variables should be expanded to their values.
-    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
-      defined by the DEPS file.
-
-  Returns:
-    A Python dict with the parsed contents of the DEPS file, as specified by the
-    schema above.
-  """
-  # TODO(ehmaldonado): Make validate_syntax = True the only case
-  if validate_syntax:
-    return Exec(content, expand_vars, filename)
-
-  vars_dict = {}
-  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.iteritems()
-      }
-    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
-
-  local_scope = {}
-  global_scope = {'Var': lambda var_name: '{%s}' % var_name}
-  exec(content, global_scope, local_scope)
-
-  if 'vars' not in local_scope or not expand_vars:
-    return local_scope
-
-  vars_dict.update(local_scope['vars'])
-  vars_override = vars_override or {}
-  for var, value in vars_override.iteritems():
-    if var in vars_dict:
-      vars_dict[var] = value
-
-  return _DeepFormat(local_scope)
-
-
 def EvaluateCondition(condition, variables, referenced_variables=None):
   """Safely evaluates a boolean condition. Returns the result."""
   if not referenced_variables:
@@ -497,7 +416,7 @@ def SetVar(gclient_dict, var_name, value):
         "The vars entry for %s has no formatting information." % var_name)
 
   _UpdateAstString(tokens, node, value)
-  gclient_dict['vars'].SetNode(var_name, value, node)
+  gclient_dict['vars']._SetNode(var_name, value, node)
 
 
 def SetCIPD(gclient_dict, dep_name, package_name, new_version):
@@ -531,7 +450,7 @@ def SetCIPD(gclient_dict, dep_name, package_name, new_version):
 
   new_version = 'version:' + new_version
   _UpdateAstString(tokens, node, new_version)
-  packages[0].SetNode('version', new_version, node)
+  packages[0]._SetNode('version', new_version, node)
 
 
 def SetRevision(gclient_dict, dep_name, new_revision):
@@ -558,9 +477,8 @@ def SetRevision(gclient_dict, dep_name, new_revision):
       SetVar(gclient_dict, node.args[0].s, new_revision)
     else:
       _UpdateAstString(tokens, node, new_revision)
-      value = _gclient_eval(dep_node, gclient_dict.get('vars', None),
-                            expand_vars=True, filename='<unknown>')
-      dep_dict.SetNode(dep_key, value, dep_node)
+      value = _gclient_eval(dep_node)
+      dep_dict._SetNode(dep_key, value, dep_node)
 
   if isinstance(gclient_dict['deps'][dep_name], _NodeDict):
     _UpdateRevision(gclient_dict['deps'][dep_name], 'url')

tests/gclient_eval_unittest.py

@@ -20,17 +20,6 @@ import gclient_eval
 
 
 _SAMPLE_DEPS_FILE = textwrap.dedent("""\
-vars = {
-    'git_repo': 'https://example.com/repo.git',
-     # Some comment with bad indentation
-     'dep_2_rev': '1ced',
-     # Some more comments
-     # 1 
-     # 2
-     # 3
-    'dep_3_rev': '5p1e5',
-}
-
 deps = {
     'src/dep': Var('git_repo') + '/dep' + '@' + 'deadbeef',
     # Some comment
@@ -56,82 +45,59 @@ deps = {
         'dep_type': 'cipd',
     },
 }
-""")
+
+vars = {
+    'git_repo': 'https://example.com/repo.git',
+     # Some comment with bad indentation
+     'dep_2_rev': '1ced',
+     # Some more comments
+     # 1 
+     # 2
+     # 3
+    'dep_3_rev': '5p1e5',
+}""")
 
 
 class GClientEvalTest(unittest.TestCase):
   def test_str(self):
-    self.assertEqual(
-        'foo',
-        gclient_eval._gclient_eval('"foo"', vars_dict=None, expand_vars=False,
-                                   filename='<unknown>'))
+    self.assertEqual('foo', gclient_eval._gclient_eval('"foo"'))
 
   def test_tuple(self):
-    self.assertEqual(
-        ('a', 'b'),
-        gclient_eval._gclient_eval('("a", "b")', vars_dict=None,
-                                   expand_vars=False, filename='<unknown>'))
+    self.assertEqual(('a', 'b'), gclient_eval._gclient_eval('("a", "b")'))
 
   def test_list(self):
-    self.assertEqual(
-        ['a', 'b'],
-        gclient_eval._gclient_eval('["a", "b"]', vars_dict=None,
-                                   expand_vars=False, filename='<unknown>'))
+    self.assertEqual(['a', 'b'], gclient_eval._gclient_eval('["a", "b"]'))
 
   def test_dict(self):
-    self.assertEqual(
-        {'a': 'b'},
-        gclient_eval._gclient_eval('{"a": "b"}', vars_dict=None,
-                                   expand_vars=False, filename='<unknown>'))
+    self.assertEqual({'a': 'b'}, gclient_eval._gclient_eval('{"a": "b"}'))
 
   def test_name_safe(self):
-    self.assertEqual(
-        True,
-        gclient_eval._gclient_eval('True', vars_dict=None,
-                                   expand_vars=False, filename='<unknown>'))
+    self.assertEqual(True, gclient_eval._gclient_eval('True'))
 
   def test_name_unsafe(self):
     with self.assertRaises(ValueError) as cm:
-      gclient_eval._gclient_eval('UnsafeName', vars_dict=None,
-                                 expand_vars=False, filename='<unknown>')
+      gclient_eval._gclient_eval('UnsafeName')
     self.assertIn('invalid name \'UnsafeName\'', str(cm.exception))
 
-  def test_invalid_call(self):
-    with self.assertRaises(ValueError) as cm:
-      gclient_eval._gclient_eval('Foo("bar")', vars_dict=None,
-                                 expand_vars=False, filename='<unknown>')
-    self.assertIn('Var is the only allowed function', str(cm.exception))
-
   def test_call(self):
     self.assertEqual(
         '{bar}',
-        gclient_eval._gclient_eval('Var("bar")', vars_dict=None,
-                                   expand_vars=False, filename='<unknown>'))
+        gclient_eval._gclient_eval('Var("bar")'))
 
   def test_plus(self):
-    self.assertEqual(
-        'foo',
-        gclient_eval._gclient_eval('"f" + "o" + "o"', vars_dict=None,
-                                   expand_vars=False, filename='<unknown>'))
+    self.assertEqual('foo', gclient_eval._gclient_eval('"f" + "o" + "o"'))
 
   def test_format(self):
-    self.assertEqual(
-        'foo',
-        gclient_eval._gclient_eval('"%s" % "foo"', vars_dict=None,
-                                   expand_vars=False, filename='<unknown>'))
+    self.assertEqual('foo', gclient_eval._gclient_eval('"%s" % "foo"'))
 
   def test_not_expression(self):
     with self.assertRaises(SyntaxError) as cm:
-      gclient_eval._gclient_eval(
-          'def foo():\n  pass', vars_dict=None, expand_vars=False,
-          filename='<unknown>')
+      gclient_eval._gclient_eval('def foo():\n  pass')
     self.assertIn('invalid syntax', str(cm.exception))
 
   def test_not_whitelisted(self):
     with self.assertRaises(ValueError) as cm:
-      gclient_eval._gclient_eval(
-          '[x for x in [1, 2, 3]]', vars_dict=None, expand_vars=False,
-          filename='<unknown>')
+      gclient_eval._gclient_eval('[x for x in [1, 2, 3]]')
     self.assertIn(
         'unexpected AST node: <_ast.ListComp object', str(cm.exception))
 
@@ -139,9 +105,7 @@ class GClientEvalTest(unittest.TestCase):
     for test_case in itertools.permutations(range(4)):
       input_data = ['{'] + ['"%s": "%s",' % (n, n) for n in test_case] + ['}']
       expected = [(str(n), str(n)) for n in test_case]
-      result = gclient_eval._gclient_eval(
-          ''.join(input_data), vars_dict=None, expand_vars=False,
-          filename='<unknown>')
+      result = gclient_eval._gclient_eval(''.join(input_data))
       self.assertEqual(expected, result.items())
 
 
@@ -160,11 +124,12 @@ class ExecTest(unittest.TestCase):
 
   def test_schema_wrong_type(self):
     with self.assertRaises(schema.SchemaError):
-      gclient_eval.Exec('include_rules = {}')
+      gclient_eval.Exec('include_rules = {}', '<string>')
 
   def test_recursedeps_list(self):
     local_scope = gclient_eval.Exec(
-        'recursedeps = [["src/third_party/angle", "DEPS.chromium"]]')
+        'recursedeps = [["src/third_party/angle", "DEPS.chromium"]]',
+        '<string>')
     self.assertEqual(
         {'recursedeps': [['src/third_party/angle', 'DEPS.chromium']]},
         local_scope)
@@ -177,35 +142,7 @@ class ExecTest(unittest.TestCase):
         'deps = {',
         '  "a_dep": "a" + Var("foo") + "b",',
         '}',
-    ]))
-    self.assertEqual({
-        'vars': collections.OrderedDict([('foo', 'bar')]),
-        'deps': collections.OrderedDict([('a_dep', 'abarb')]),
-    }, local_scope)
-
-  def test_braces_var(self):
-    local_scope = gclient_eval.Exec('\n'.join([
-        'vars = {',
-        '  "foo": "bar",',
-        '}',
-        'deps = {',
-        '  "a_dep": "a{foo}b",',
-        '}',
-    ]))
-    self.assertEqual({
-        'vars': collections.OrderedDict([('foo', 'bar')]),
-        'deps': collections.OrderedDict([('a_dep', 'abarb')]),
-    }, local_scope)
-
-  def test_var_unexpanded(self):
-    local_scope = gclient_eval.Exec('\n'.join([
-        'vars = {',
-        '  "foo": "bar",',
-        '}',
-        'deps = {',
-        '  "a_dep": "a" + Var("foo") + "b",',
-        '}',
-    ]), expand_vars=False)
+    ]), '<string>')
     self.assertEqual({
         'vars': collections.OrderedDict([('foo', 'bar')]),
         'deps': collections.OrderedDict([('a_dep', 'a{foo}b')]),