Add a Str() function to gclient for use in DEPS files.

gclient's existing functionality for handling variables is
ambiguous: the value of a variable can either be a string literal
or an expression fragment. The implementation is required to
parse a value as an expression, and, if it is legal, treat it
as an expression instead of a literal. This means that

  gclient_gn_args_file = 'src/build/args.gni'
  gclient_gn_args = ['xcode_version']
  vars = {
    'xcode_version': 'xcode-12'
  }

would cause a problem because gclient would try to parse the
variable as an expression, and 'xcode' would not be defined.

This patch adds a workaround for this, where you can instead
use the Str() function to explicitly tell gclient to treat the
value as a string and not a potential expression.

The above example would be changed to:

  gclient_gn_args_file = 'src/build/args.gni'
  gclient_gn_args = ['xcode_version']
  vars = {
    'xcode_version': Str('xcode-12')
  }

The variable may still be used in every context where it was legal
to be used before.

Bug: 1099242

Change-Id: Ic2a17eea5f7098113bdba0557fe29e1a931a74b8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2268406
Reviewed-by: Ben Pastene <bpastene@chromium.org>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Commit-Queue: Dirk Pranke <dpranke@google.com>

gclient.py

@@ -1043,7 +1043,9 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
     variables = self.get_vars()
     for arg in self._gn_args:
       value = variables[arg]
-      if isinstance(value, basestring):
+      if isinstance(value, gclient_eval.ConstantString):
+        value = value.value
+      elif 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), 'wb') as f:
@@ -1265,11 +1267,11 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
     result = {}
     result.update(self._vars)
     if self.parent:
-      parent_vars = self.parent.get_vars()
+      merge_vars(result, self.parent.get_vars())
-      result.update(parent_vars)
     # Provide some built-in variables.
     result.update(self.get_builtin_vars())
-    result.update(self.custom_vars or {})
+    merge_vars(result, self.custom_vars)
+
     return result
 
 
@@ -1283,6 +1285,18 @@ _PLATFORM_MAPPING = {
 }
 
 
+def merge_vars(result, new_vars):
+  for k, v in new_vars.items():
+    if (k in result and
+        isinstance(result[k], gclient_eval.ConstantString)):
+      if isinstance(v, gclient_eval.ConstantString):
+        result[k].value = v.value
+      else:
+        result[k].value = v
+    else:
+      result.setdefault(k, v)
+
+
 def _detect_host_os():
   return _PLATFORM_MAPPING[sys.platform]
 

gclient_eval.py

@@ -24,6 +24,27 @@ else:
   basestring = str
 
 
+class ConstantString(object):
+  def __init__(self, value):
+    self.value = value
+
+  def __format__(self, format_spec):
+    del format_spec
+    return self.value
+
+  def __repr__(self):
+    return "Str('" + self.value + "')"
+
+  def __eq__(self, other):
+    if isinstance(other, ConstantString):
+      return self.value == other.value
+    else:
+      return self.value == other
+
+  def __hash__(self):
+      return self.value.__hash__()
+
+
 class _NodeDict(collections_abc.MutableMapping):
   """Dict-like type that also stores information on AST nodes and tokens."""
   def __init__(self, data=None, tokens=None):
@@ -114,7 +135,7 @@ _GCLIENT_DEPS_SCHEMA = _NodeDictSchema({
 _GCLIENT_HOOKS_SCHEMA = [
     _NodeDictSchema({
         # Hook action: list of command-line arguments to invoke.
-        'action': [basestring],
+        'action': [schema.Or(basestring)],
 
         # Name of the hook. Doesn't affect operation.
         schema.Optional('name'): basestring,
@@ -220,7 +241,9 @@ _GCLIENT_SCHEMA = schema.Schema(
 
         # Variables that can be referenced using Var() - see 'deps'.
         schema.Optional('vars'): _NodeDictSchema({
-            schema.Optional(basestring): schema.Or(basestring, bool),
+            schema.Optional(basestring): schema.Or(ConstantString,
+                                                   basestring,
+                                                   bool),
         }),
     }))
 
@@ -228,6 +251,8 @@ _GCLIENT_SCHEMA = schema.Schema(
 def _gclient_eval(node_or_string, filename='<unknown>', vars_dict=None):
   """Safely evaluates a single expression. Returns the result."""
   _allowed_names = {'None': None, 'True': True, 'False': False}
+  if isinstance(node_or_string, ConstantString):
+    return node_or_string.value
   if isinstance(node_or_string, basestring):
     node_or_string = ast.parse(node_or_string, filename=filename, mode='eval')
   if isinstance(node_or_string, ast.Expression):
@@ -269,16 +294,23 @@ def _gclient_eval(node_or_string, filename='<unknown>', vars_dict=None):
         node, ast.NameConstant):  # Since Python 3.4
       return node.value
     elif isinstance(node, ast.Call):
-      if not isinstance(node.func, ast.Name) or node.func.id != 'Var':
+      if (not isinstance(node.func, ast.Name) or
+          (node.func.id not in ('Str', 'Var'))):
         raise ValueError(
-            'Var is the only allowed function (file %r, line %s)' % (
+            'Str and Var are the only allowed functions (file %r, line %s)' % (
                 filename, getattr(node, 'lineno', '<unknown>')))
       if node.keywords or getattr(node, 'starargs', None) or getattr(
           node, 'kwargs', None) or len(node.args) != 1:
         raise ValueError(
-            'Var takes exactly one argument (file %r, line %s)' % (
+            '%s takes exactly one argument (file %r, line %s)' % (
-                filename, getattr(node, 'lineno', '<unknown>')))
+                node.func.id, filename, getattr(node, 'lineno', '<unknown>')))
-      arg = _convert(node.args[0])
+      if node.func.id == 'Str':
+        if isinstance(node.args[0], ast.Str):
+          return ConstantString(node.args[0].s)
+        raise ValueError('Passed a non-string to Str() (file %r, line%s)' % (
+            filename, getattr(node, 'lineno', '<unknown>')))
+      else:
+        arg = _convert(node.args[0])
       if not isinstance(arg, basestring):
         raise ValueError(
             'Var\'s argument must be a variable name (file %r, line %s)' % (
@@ -290,7 +322,10 @@ def _gclient_eval(node_or_string, filename='<unknown>', vars_dict=None):
             '%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]
+      val = vars_dict[arg]
+      if isinstance(val, ConstantString):
+        val = val.value
+      return val
     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):
@@ -601,6 +636,8 @@ def RenderDEPSFile(gclient_dict):
 
 
 def _UpdateAstString(tokens, node, value):
+  if isinstance(node, ast.Call):
+    node = node.args[0]
   position = node.lineno, node.col_offset
   quote_char = ''
   if isinstance(node, ast.Str):
@@ -810,7 +847,10 @@ def GetVar(gclient_dict, var_name):
     raise KeyError(
         "Could not find any variable called %s." % var_name)
 
-  return gclient_dict['vars'][var_name]
+  val = gclient_dict['vars'][var_name]
+  if isinstance(val, ConstantString):
+    return val.value
+  return val
 
 
 def GetCIPD(gclient_dict, dep_name, package_name):

tests/gclient_eval_unittest.py

@@ -47,17 +47,28 @@ class GClientEvalTest(unittest.TestCase):
   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))
+    self.assertIn('Str and Var are the only allowed functions',
+                  str(cm.exception))
 
   def test_expands_vars(self):
     self.assertEqual(
         'foo',
         gclient_eval._gclient_eval('Var("bar")', vars_dict={'bar': 'foo'}))
+    self.assertEqual(
+        'baz',
+        gclient_eval._gclient_eval(
+            'Var("bar")',
+            vars_dict={'bar': gclient_eval.ConstantString('baz')}))
 
   def test_expands_vars_with_braces(self):
     self.assertEqual(
         'foo',
         gclient_eval._gclient_eval('"{bar}"', vars_dict={'bar': 'foo'}))
+    self.assertEqual(
+        'baz',
+        gclient_eval._gclient_eval(
+            '"{bar}"',
+            vars_dict={'bar': gclient_eval.ConstantString('baz')}))
 
   def test_invalid_var(self):
     with self.assertRaises(KeyError) as cm:
@@ -118,28 +129,33 @@ class ExecTest(unittest.TestCase):
     local_scope = gclient_eval.Exec('\n'.join([
         'vars = {',
         '  "foo": "bar",',
+        '  "baz": Str("quux")',
         '}',
         'deps = {',
-        '  "a_dep": "a" + Var("foo") + "b",',
+        '  "a_dep": "a" + Var("foo") + "b" + Var("baz"),',
         '}',
     ]))
+    Str = gclient_eval.ConstantString
     self.assertEqual({
-        'vars': collections.OrderedDict([('foo', 'bar')]),
+        'vars': {'foo': 'bar', 'baz': Str('quux')},
-        'deps': collections.OrderedDict([('a_dep', 'abarb')]),
+        'deps': {'a_dep': 'abarbquux'},
     }, local_scope)
 
   def test_braces_var(self):
     local_scope = gclient_eval.Exec('\n'.join([
         'vars = {',
         '  "foo": "bar",',
+        '  "baz": Str("quux")',
         '}',
         'deps = {',
-        '  "a_dep": "a{foo}b",',
+        '  "a_dep": "a{foo}b{baz}",',
         '}',
     ]))
+    Str = gclient_eval.ConstantString
     self.assertEqual({
-        'vars': collections.OrderedDict([('foo', 'bar')]),
+        'vars': {'foo': 'bar',
-        'deps': collections.OrderedDict([('a_dep', 'abarb')]),
+                 'baz': Str('quux')},
+        'deps': {'a_dep': 'abarbquux'},
     }, local_scope)
 
   def test_empty_deps(self):
@@ -150,14 +166,17 @@ class ExecTest(unittest.TestCase):
     local_scope = gclient_eval.Exec('\n'.join([
         'vars = {',
         '  "foo": "bar",',
+        '  "quux": Str("quuz")',
         '}',
         'deps = {',
         '  "a_dep": "a{foo}b",',
+        '  "b_dep": "c{quux}d",',
         '}',
-    ]), vars_override={'foo': 'baz'})
+    ]), vars_override={'foo': 'baz', 'quux': 'corge'})
+    Str = gclient_eval.ConstantString
     self.assertEqual({
-        'vars': collections.OrderedDict([('foo', 'bar')]),
+        'vars': {'foo': 'bar', 'quux': Str('quuz')},
-        'deps': collections.OrderedDict([('a_dep', 'abazb')]),
+        'deps': {'a_dep': 'abazb', 'b_dep': 'ccorged'},
     }, local_scope)
 
   def test_doesnt_override_undeclared_vars(self):
@@ -337,6 +356,15 @@ class EvaluateConditionTest(unittest.TestCase):
       gclient_eval.EvaluateCondition('(foo,) == "bar"', {'foo': 'bar'})
     self.assertIn('unexpected AST node', str(cm.exception))
 
+  def test_str_in_condition(self):
+    Str = gclient_eval.ConstantString
+    self.assertTrue(gclient_eval.EvaluateCondition(
+        's_var == "foo"',
+        {'s_var': Str("foo")}))
+
+    self.assertFalse(gclient_eval.EvaluateCondition(
+        's_var in ("baz", "quux")',
+        {'s_var': Str("foo")}))
 
 class VarTest(unittest.TestCase):
   def assert_adds_var(self, before, after):
@@ -382,18 +410,23 @@ class VarTest(unittest.TestCase):
     local_scope = gclient_eval.Exec('\n'.join([
         'vars = {',
         '  "foo": "bar",',
+        '  "quux": Str("quuz")',
         '}',
     ]))
 
-    result = gclient_eval.GetVar(local_scope, 'foo')
+    self.assertEqual(gclient_eval.GetVar(local_scope, 'foo'),
-    self.assertEqual(result, "bar")
+                     "bar")
+    self.assertEqual(gclient_eval.GetVar(local_scope, 'quux'),
+                     "quuz")
 
     gclient_eval.SetVar(local_scope, 'foo', 'baz')
+    gclient_eval.SetVar(local_scope, 'quux', 'corge')
     result = gclient_eval.RenderDEPSFile(local_scope)
 
     self.assertEqual(result, '\n'.join([
         'vars = {',
         '  "foo": "baz",',
+        '  "quux": Str("corge")',
         '}',
     ]))
 

tests/gclient_test.py

@@ -667,6 +667,42 @@ class GclientTest(trial_dir.TestCase):
         ('foo/skip2', None),
     ], [(dep.name, dep.url) for dep in sol.dependencies])
 
+  def testVarOverrides(self):
+    """Verifies expected behavior of variable overrides."""
+    write(
+        '.gclient',
+        'solutions = [\n'
+        '  { "name": "foo",\n'
+        '    "url": "svn://example.com/foo",\n'
+        '    "custom_vars": {\n'
+        '      "path": "c-d",\n'
+        '    },\n'
+        '  },]\n')
+    write(
+        os.path.join('foo', 'DEPS'),
+        'vars = {\n'
+        '  "path": Str("a-b"),\n'
+        '}\n'
+        'deps = {\n'
+        '  "foo/bar": "svn://example.com/foo/" + Var("path"),\n'
+        '}')
+    parser = gclient.OptionParser()
+    options, _ = parser.parse_args(['--jobs', '1'])
+
+    obj = gclient.GClient.LoadCurrentConfig(options)
+    obj.RunOnDeps('None', [])
+
+    sol = obj.dependencies[0]
+    self.assertEqual([
+        ('foo', 'svn://example.com/foo'),
+        ('foo/bar', 'svn://example.com/foo/c-d'),
+    ], self._get_processed())
+
+    self.assertEqual(1, len(sol.dependencies))
+    self.assertEqual([
+        ('foo/bar', 'svn://example.com/foo/c-d'),
+    ], [(dep.name, dep.url) for dep in sol.dependencies])
+
   def testDepsOsOverrideDepsInDepsFile(self):
     """Verifies that a 'deps_os' path cannot override a 'deps' path. Also
     see testUpdateWithOsDeps above.