Commit ac93e6d5 authored by Dirk Pranke's avatar Dirk Pranke Committed by LUCI CQ

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

This reverts commit c7eed83f.

Reason for revert: I'm getting reports of internal iOS checkouts being broken. Reverting while I reproduce / debug it.

Original change's description:
> 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>

TBR=thakis@chromium.org,dpranke@google.com,ehmaldonado@chromium.org,bpastene@chromium.org,apolito@google.com,infra-scoped@luci-project-accounts.iam.gserviceaccount.com

Change-Id: Iac2b003f32acdbca15a19f821b61423e34b3466c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1099242
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2273978Reviewed-by: 's avatarDirk Pranke <dpranke@google.com>
Commit-Queue: Dirk Pranke <dpranke@google.com>
parent c7eed83f
......@@ -1043,9 +1043,7 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
variables = self.get_vars()
for arg in self._gn_args:
value = variables[arg]
if isinstance(value, gclient_eval.ConstantString):
value = value.value
elif isinstance(value, basestring):
if 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:
......@@ -1267,11 +1265,11 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
result = {}
result.update(self._vars)
if self.parent:
merge_vars(result, self.parent.get_vars())
parent_vars = self.parent.get_vars()
result.update(parent_vars)
# Provide some built-in variables.
result.update(self.get_builtin_vars())
merge_vars(result, self.custom_vars)
result.update(self.custom_vars or {})
return result
......@@ -1285,18 +1283,6 @@ _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]
......
......@@ -24,27 +24,6 @@ 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):
......@@ -135,7 +114,7 @@ _GCLIENT_DEPS_SCHEMA = _NodeDictSchema({
_GCLIENT_HOOKS_SCHEMA = [
_NodeDictSchema({
# Hook action: list of command-line arguments to invoke.
'action': [schema.Or(basestring)],
'action': [basestring],
# Name of the hook. Doesn't affect operation.
schema.Optional('name'): basestring,
......@@ -241,9 +220,7 @@ _GCLIENT_SCHEMA = schema.Schema(
# Variables that can be referenced using Var() - see 'deps'.
schema.Optional('vars'): _NodeDictSchema({
schema.Optional(basestring): schema.Or(ConstantString,
basestring,
bool),
schema.Optional(basestring): schema.Or(basestring, bool),
}),
}))
......@@ -251,8 +228,6 @@ _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):
......@@ -294,23 +269,16 @@ 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 not in ('Str', 'Var'))):
if not isinstance(node.func, ast.Name) or node.func.id != 'Var':
raise ValueError(
'Str and Var are the only allowed functions (file %r, line %s)' % (
'Var is the only allowed function (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(
'%s takes exactly one argument (file %r, line %s)' % (
node.func.id, filename, getattr(node, 'lineno', '<unknown>')))
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])
'Var takes exactly one argument (file %r, line %s)' % (
filename, getattr(node, 'lineno', '<unknown>')))
arg = _convert(node.args[0])
if not isinstance(arg, basestring):
raise ValueError(
'Var\'s argument must be a variable name (file %r, line %s)' % (
......@@ -322,10 +290,7 @@ 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>')))
val = vars_dict[arg]
if isinstance(val, ConstantString):
val = val.value
return val
return vars_dict[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):
......@@ -636,8 +601,6 @@ 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):
......@@ -847,10 +810,7 @@ def GetVar(gclient_dict, var_name):
raise KeyError(
"Could not find any variable called %s." % var_name)
val = gclient_dict['vars'][var_name]
if isinstance(val, ConstantString):
return val.value
return val
return gclient_dict['vars'][var_name]
def GetCIPD(gclient_dict, dep_name, package_name):
......
......@@ -47,28 +47,17 @@ class GClientEvalTest(unittest.TestCase):
def test_invalid_call(self):
with self.assertRaises(ValueError) as cm:
gclient_eval._gclient_eval('Foo("bar")')
self.assertIn('Str and Var are the only allowed functions',
str(cm.exception))
self.assertIn('Var is the only allowed function', 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:
......@@ -129,33 +118,28 @@ class ExecTest(unittest.TestCase):
local_scope = gclient_eval.Exec('\n'.join([
'vars = {',
' "foo": "bar",',
' "baz": Str("quux")',
'}',
'deps = {',
' "a_dep": "a" + Var("foo") + "b" + Var("baz"),',
' "a_dep": "a" + Var("foo") + "b",',
'}',
]))
Str = gclient_eval.ConstantString
self.assertEqual({
'vars': {'foo': 'bar', 'baz': Str('quux')},
'deps': {'a_dep': 'abarbquux'},
'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",',
' "baz": Str("quux")',
'}',
'deps = {',
' "a_dep": "a{foo}b{baz}",',
' "a_dep": "a{foo}b",',
'}',
]))
Str = gclient_eval.ConstantString
self.assertEqual({
'vars': {'foo': 'bar',
'baz': Str('quux')},
'deps': {'a_dep': 'abarbquux'},
'vars': collections.OrderedDict([('foo', 'bar')]),
'deps': collections.OrderedDict([('a_dep', 'abarb')]),
}, local_scope)
def test_empty_deps(self):
......@@ -166,17 +150,14 @@ 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', 'quux': 'corge'})
Str = gclient_eval.ConstantString
]), vars_override={'foo': 'baz'})
self.assertEqual({
'vars': {'foo': 'bar', 'quux': Str('quuz')},
'deps': {'a_dep': 'abazb', 'b_dep': 'ccorged'},
'vars': collections.OrderedDict([('foo', 'bar')]),
'deps': collections.OrderedDict([('a_dep', 'abazb')]),
}, local_scope)
def test_doesnt_override_undeclared_vars(self):
......@@ -356,15 +337,6 @@ 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):
......@@ -410,23 +382,18 @@ class VarTest(unittest.TestCase):
local_scope = gclient_eval.Exec('\n'.join([
'vars = {',
' "foo": "bar",',
' "quux": Str("quuz")',
'}',
]))
self.assertEqual(gclient_eval.GetVar(local_scope, 'foo'),
"bar")
self.assertEqual(gclient_eval.GetVar(local_scope, 'quux'),
"quuz")
result = gclient_eval.GetVar(local_scope, 'foo')
self.assertEqual(result, "bar")
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")',
'}',
]))
......
......@@ -667,42 +667,6 @@ 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.
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment