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

This is a reland of 88f9c40e

We no longer need the vars dict to be declared before vars can be used.
This was causing problems because gclient flatten printed vars after deps,
breaking the above assumption.

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>

Bug: 821199
Change-Id: I583df23558f91871e1a2aa2574c20d35e54635f6
Reviewed-on: https://chromium-review.googlesource.com/981086
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Michael Moss <mmoss@chromium.org>
Reviewed-by: Aaron Gable <agable@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):
+  def ParseDepsFile(self, expand_vars=True):
     """Parses the DEPS file for this dependency."""
     assert not self.deps_parsed
     assert not self.dependencies
@@ -765,15 +765,15 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
 
     local_scope = {}
     if deps_content:
-      # Eval the content.
       try:
-        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)
+        vars_override = {}
+        if self.parent:
+          vars_override = self.parent.get_vars()
+        vars_override.update(self.get_vars())
+        local_scope = gclient_eval.Parse(
+            deps_content, expand_vars,
+            self._get_option('validate_syntax', False),
+            filepath, vars_override)
       except SyntaxError as e:
         gclient_utils.SyntaxErrorToError(filepath, e)
 
@@ -988,7 +988,7 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
           file_list[i] = file_list[i][1:]
 
     # Always parse the DEPS file.
-    self.ParseDepsFile()
+    self.ParseDepsFile(expand_vars=(command != 'flatten'))
     self._run_is_done(file_list or [], parsed_url)
     if command in ('update', 'revert') and not options.noprehooks:
       self.RunPreDepsHooks()
@@ -1873,7 +1873,7 @@ it or fix the checkout.
           print('%s: %s' % (x, entries[x]))
     logging.info(str(self))
 
-  def ParseDepsFile(self):
+  def ParseDepsFile(self, expand_vars=None):
     """No DEPS to parse for a .gclient file."""
     raise gclient_utils.Error('Internal error')
 
@@ -1982,7 +1982,7 @@ class CipdDependency(Dependency):
       self._cipd_package = self._cipd_root.add_package(
           self._cipd_subdir, self._package_name, self._package_version)
 
-  def ParseDepsFile(self):
+  def ParseDepsFile(self, expand_vars=None):
     """CIPD dependencies are not currently allowed to have nested deps."""
     self.add_dependencies_and_close([], [])
 
@@ -2939,7 +2939,9 @@ 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.Exec(contents)
+  local_scope = gclient_eval.Parse(
+      contents, expand_vars=True, validate_syntax=True,
+      filename=options.deps_file)
 
   for var in options.vars:
     name, _, value = var.partition('=')

tests/gclient_eval_unittest.py

@@ -20,6 +20,15 @@ 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 with bad indentation
+   # and trailing whitespaces  """ + """
+   'dep_3_rev': '5p1e5',
+}
+
 deps = {
     'src/dep': Var('git_repo') + '/dep' + '@' + 'deadbeef',
     # Some comment
@@ -45,17 +54,7 @@ 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):
@@ -79,10 +78,29 @@ class GClientEvalTest(unittest.TestCase):
       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")')
+    self.assertIn('Var is the only allowed function', str(cm.exception))
+
   def test_call(self):
+    self.assertEqual('{bar}', gclient_eval._gclient_eval('Var("bar")'))
+
+  def test_expands_vars(self):
+    self.assertEqual(
+        'foo',
+        gclient_eval._gclient_eval('Var("bar")', {'bar': 'foo'}, True))
+
+  def test_expands_vars_with_braces(self):
     self.assertEqual(
-        '{bar}',
-        gclient_eval._gclient_eval('Var("bar")'))
+        'foo',
+        gclient_eval._gclient_eval('"{bar}"', {'bar': 'foo'}, True))
+
+  def test_invalid_var(self):
+    with self.assertRaises(ValueError) as cm:
+      gclient_eval._gclient_eval('"{bar}"', {}, True)
+    self.assertIn('bar was used as a variable, but was not declared',
+                  str(cm.exception))
 
   def test_plus(self):
     self.assertEqual('foo', gclient_eval._gclient_eval('"f" + "o" + "o"'))
@@ -124,12 +142,11 @@ class ExecTest(unittest.TestCase):
 
   def test_schema_wrong_type(self):
     with self.assertRaises(schema.SchemaError):
-      gclient_eval.Exec('include_rules = {}', '<string>')
+      gclient_eval.Exec('include_rules = {}')
 
   def test_recursedeps_list(self):
     local_scope = gclient_eval.Exec(
-        'recursedeps = [["src/third_party/angle", "DEPS.chromium"]]',
-        '<string>')
+        'recursedeps = [["src/third_party/angle", "DEPS.chromium"]]')
     self.assertEqual(
         {'recursedeps': [['src/third_party/angle', 'DEPS.chromium']]},
         local_scope)
@@ -142,16 +159,71 @@ class ExecTest(unittest.TestCase):
         'deps = {',
         '  "a_dep": "a" + Var("foo") + "b",',
         '}',
-    ]), '<string>')
+    ]))
+    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",',
+        '}',
+    ]), False)
     self.assertEqual({
         'vars': collections.OrderedDict([('foo', 'bar')]),
         'deps': collections.OrderedDict([('a_dep', 'a{foo}b')]),
     }, local_scope)
 
   def test_empty_deps(self):
-    local_scope = gclient_eval.Exec('deps = {}', '<string>')
+    local_scope = gclient_eval.Exec('deps = {}')
     self.assertEqual({'deps': {}}, local_scope)
 
+  def test_overrides_vars(self):
+    local_scope = gclient_eval.Exec('\n'.join([
+        'vars = {',
+        '  "foo": "bar",',
+        '}',
+        'deps = {',
+        '  "a_dep": "a{foo}b",',
+        '}',
+    ]), True, vars_override={'foo': 'baz'})
+    self.assertEqual({
+        'vars': collections.OrderedDict([('foo', 'bar')]),
+        'deps': collections.OrderedDict([('a_dep', 'abazb')]),
+    }, local_scope)
+
+  def test_doesnt_override_undeclared_vars(self):
+    with self.assertRaises(ValueError) as cm:
+      gclient_eval.Exec('\n'.join([
+          'vars = {',
+          '  "foo": "bar",',
+          '}',
+          'deps = {',
+          '  "a_dep": "a{baz}b",',
+          '}',
+      ]), True, vars_override={'baz': 'lalala'})
+    self.assertIn('baz was used as a variable, but was not declared',
+                  str(cm.exception))
+
 
 class EvaluateConditionTest(unittest.TestCase):
   def test_true(self):
@@ -204,8 +276,8 @@ class EvaluateConditionTest(unittest.TestCase):
 
 
 class SetVarTest(unittest.TestCase):
-  def testSetVar(self):
-    local_scope = gclient_eval.Exec(_SAMPLE_DEPS_FILE)
+  def test_sets_var(self):
+    local_scope = gclient_eval.Exec(_SAMPLE_DEPS_FILE, True)
 
     gclient_eval.SetVar(local_scope, 'dep_2_rev', 'c0ffee')
     result = gclient_eval.RenderDEPSFile(local_scope)
@@ -216,8 +288,8 @@ class SetVarTest(unittest.TestCase):
 
 
 class SetCipdTest(unittest.TestCase):
-  def testSetCIPD(self):
-    local_scope = gclient_eval.Exec(_SAMPLE_DEPS_FILE)
+  def test_sets_cipd(self):
+    local_scope = gclient_eval.Exec(_SAMPLE_DEPS_FILE, True)
 
     gclient_eval.SetCIPD(
         local_scope, 'src/cipd/package', 'another/cipd/package', '6.789')
@@ -228,23 +300,23 @@ class SetCipdTest(unittest.TestCase):
 
 class SetRevisionTest(unittest.TestCase):
   def setUp(self):
-    self.local_scope = gclient_eval.Exec(_SAMPLE_DEPS_FILE)
+    self.local_scope = gclient_eval.Exec(_SAMPLE_DEPS_FILE, True)
 
-  def testSetRevision(self):
+  def test_sets_revision(self):
     gclient_eval.SetRevision(
         self.local_scope, 'src/dep', 'deadfeed')
     result = gclient_eval.RenderDEPSFile(self.local_scope)
 
     self.assertEqual(result, _SAMPLE_DEPS_FILE.replace('deadbeef', 'deadfeed'))
 
-  def testSetRevisionInUrl(self):
+  def test_sets_revision_inside_dict(self):
     gclient_eval.SetRevision(
         self.local_scope, 'src/dep_3', '0ff1ce')
     result = gclient_eval.RenderDEPSFile(self.local_scope)
 
     self.assertEqual(result, _SAMPLE_DEPS_FILE.replace('5p1e5', '0ff1ce'))
 
-  def testSetRevisionInVars(self):
+  def test_sets_revision_in_vars(self):
     gclient_eval.SetRevision(
         self.local_scope, 'src/android/dep_2', 'c0ffee')
     result = gclient_eval.RenderDEPSFile(self.local_scope)
@@ -252,6 +324,68 @@ class SetRevisionTest(unittest.TestCase):
     self.assertEqual(result, _SAMPLE_DEPS_FILE.replace('1ced', 'c0ffee'))
 
 
+class ParseTest(unittest.TestCase):
+  def callParse(self, expand_vars=True, validate_syntax=True,
+                vars_override=None):
+    return gclient_eval.Parse('\n'.join([
+        'vars = {',
+        '  "foo": "bar",',
+        '}',
+        'deps = {',
+        '  "a_dep": "a{foo}b",',
+        '}',
+    ]), expand_vars, validate_syntax, '<unknown>', vars_override)
+
+  def test_expands_vars(self):
+    for validate_syntax in True, False:
+      local_scope = self.callParse(validate_syntax=validate_syntax)
+      self.assertEqual({
+          'vars': collections.OrderedDict([('foo', 'bar')]),
+          'deps': collections.OrderedDict([('a_dep', 'abarb')]),
+      }, local_scope)
+
+  def test_no_expands_vars(self):
+    for validate_syntax in True, False:
+      local_scope = self.callParse(False,
+                                   validate_syntax=validate_syntax)
+      self.assertEqual({
+          'vars': collections.OrderedDict([('foo', 'bar')]),
+          'deps': collections.OrderedDict([('a_dep', 'a{foo}b')]),
+      }, 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': collections.OrderedDict([('foo', 'bar')]),
+          'deps': collections.OrderedDict([('a_dep', 'abazb')]),
+      }, local_scope)
+
+  def test_no_extra_vars(self):
+    deps_file = '\n'.join([
+        'vars = {',
+        '  "foo": "bar",',
+        '}',
+        'deps = {',
+        '  "a_dep": "a{baz}b",',
+        '}',
+    ])
+
+    with self.assertRaises(ValueError) as cm:
+      gclient_eval.Parse(
+          deps_file, True, True,
+          '<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, True, False,
+          '<unknown>', {'baz': 'lalala'})
+    self.assertIn('baz', str(cm.exception))
+
+
 if __name__ == '__main__':
   level = logging.DEBUG if '-v' in sys.argv else logging.FATAL
   logging.basicConfig(

tests/gclient_smoketest.py

@@ -802,10 +802,22 @@ class GClientSmokeGIT(GClientSmokeBase):
                   # defined in the DEPS.
                   '--custom-var', 'custom_true_var=True',
                   # This should override 'true_var=True' from the DEPS.
-                  '--custom-var', 'true_var="0"'])
+                  '--custom-var', 'true_var="False"'])
     self.gclient(['sync'])
     self.gclient(['flatten', '-v', '-v', '-v', '--output-deps', output_deps])
 
+    # Assert we can sync to the flattened DEPS we just wrote.
+    solutions = [{
+        "url": self.git_base + 'repo_6',
+        'name': 'src',
+        'deps_file': output_deps
+    }]
+    results = self.gclient([
+        'sync',
+        '--spec=solutions=%s' % solutions
+    ])
+    self.assertEqual(results[2], 0)
+
     with open(output_deps) as f:
       deps_contents = f.read()
 
@@ -961,7 +973,7 @@ class GClientSmokeGIT(GClientSmokeBase):
         '  "true_str_var": \'True\',',
         '',
         '  # src [custom_var override]',
-        '  "true_var": \'0\',',
+        '  "true_var": \'False\',',
         '',
         '}',
         '',