gclient flatten: preserve variable placeholders (reland #1)

This is an exact reland of https://chromium-review.googlesource.com/583617 .

One of the main use cases is making it clear which revision hashes
need to be changed together. The way it's usually done is one variable
referenced several times. With this CL, we preserve the references
from original DEPS, as opposed to evaluating them and losing some info.

This CL actually makes Var() emit a variable placeholder
instead of its value, and adds support for these placeholders
to gclient.

One of possible next steps might be to deprecate Var().

Bug: 570091, 748486
Change-Id: Id47e3771b7163149a4cd427b84f84ece52772f34
Reviewed-on: https://chromium-review.googlesource.com/586594Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Reviewed-by: William Hesse <whesse@google.com>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>

gclient.py

@@ -194,7 +194,8 @@ class Hook(object):
         not gclient_eval.EvaluateCondition(self._condition, self._variables)):
       return
 
-    cmd = list(self._action)
+    cmd = [arg.format(**self._variables) for arg in self._action]
+
     if cmd[0] == 'python':
       # If the hook specified "python" as the first item, the action is a
       # Python script.  Run it by starting a new copy of the same
@@ -221,32 +222,16 @@ class Hook(object):
             gclient_utils.CommandToStr(cmd), elapsed_time))
 
 
-class GClientKeywords(object):
-  class VarImpl(object):
-    def __init__(self, custom_vars, local_scope):
-      self._custom_vars = custom_vars
-      self._local_scope = local_scope
-
-    def Lookup(self, var_name):
-      """Implements the Var syntax."""
-      if var_name in self._custom_vars:
-        return self._custom_vars[var_name]
-      elif var_name in self._local_scope.get("vars", {}):
-        return self._local_scope["vars"][var_name]
-      raise gclient_utils.Error("Var is not defined: %s" % var_name)
-
-
-class DependencySettings(GClientKeywords):
+class DependencySettings(object):
   """Immutable configuration settings."""
   def __init__(
-      self, parent, url, managed, custom_deps, custom_vars,
+      self, parent, raw_url, url, managed, custom_deps, custom_vars,
       custom_hooks, deps_file, should_process, relative,
       condition, condition_value):
-    GClientKeywords.__init__(self)
-
     # These are not mutable:
     self._parent = parent
     self._deps_file = deps_file
+    self._raw_url = raw_url
     self._url = url
     # The condition as string (or None). Useful to keep e.g. for flatten.
     self._condition = condition
@@ -324,8 +309,14 @@ class DependencySettings(GClientKeywords):
   def custom_hooks(self):
     return self._custom_hooks[:]
 
+  @property
+  def raw_url(self):
+    """URL before variable expansion."""
+    return self._raw_url
+
   @property
   def url(self):
+    """URL after variable expansion."""
     return self._url
 
   @property
@@ -354,12 +345,12 @@ class DependencySettings(GClientKeywords):
 class Dependency(gclient_utils.WorkItem, DependencySettings):
   """Object that represents a dependency checkout."""
 
-  def __init__(self, parent, name, url, managed, custom_deps,
+  def __init__(self, parent, name, raw_url, url, managed, custom_deps,
                custom_vars, custom_hooks, deps_file, should_process,
                relative, condition, condition_value):
     gclient_utils.WorkItem.__init__(self, name)
     DependencySettings.__init__(
-        self, parent, url, managed, custom_deps, custom_vars,
+        self, parent, raw_url, url, managed, custom_deps, custom_vars,
         custom_hooks, deps_file, should_process, relative,
         condition, condition_value)
 
@@ -636,21 +627,24 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
       condition = None
       condition_value = True
       if isinstance(dep_value, basestring):
-        url = dep_value
+        raw_url = dep_value
       else:
         # This should be guaranteed by schema checking in gclient_eval.
         assert isinstance(dep_value, collections.Mapping)
-        url = dep_value['url']
+        raw_url = dep_value['url']
         # Take into account should_process metadata set by MergeWithOsDeps.
         should_process = (should_process and
                           dep_value.get('should_process', True))
         condition = dep_value.get('condition')
+
+      url = raw_url.format(**self.get_vars())
+
       if condition:
         condition_value = gclient_eval.EvaluateCondition(
             condition, self.get_vars())
         should_process = should_process and condition_value
       deps_to_add.append(Dependency(
-          self, name, url, None, None, self.custom_vars, None,
+          self, name, raw_url, url, None, None, self.custom_vars, None,
           deps_file, should_process, use_relative_paths, condition,
           condition_value))
     deps_to_add.sort(key=lambda x: x.name)
@@ -685,10 +679,8 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
 
     local_scope = {}
     if deps_content:
-      # One thing is unintuitive, vars = {} must happen before Var() use.
-      var = self.VarImpl(self.custom_vars, local_scope)
       global_scope = {
-        'Var': var.Lookup,
+        'Var': lambda var_name: '{%s}' % var_name,
         'deps_os': {},
       }
       # Eval the content.
@@ -734,7 +726,10 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
     elif self._relative:
       rel_prefix = os.path.dirname(self.name)
 
-    deps = local_scope.get('deps', {})
+    deps = {}
+    for key, value in local_scope.get('deps', {}).iteritems():
+      deps[key.format(**self.get_vars())] = value
+
     if 'recursion' in local_scope:
       self.recursion_override = local_scope.get('recursion')
       logging.warning(
@@ -1203,7 +1198,7 @@ solutions = [
     # Do not change previous behavior. Only solution level and immediate DEPS
     # are processed.
     self._recursion_limit = 2
-    Dependency.__init__(self, None, None, None, True, None, None, None,
+    Dependency.__init__(self, None, None, None, None, True, None, None, None,
                         'unused', True, None, None, True)
     self._options = options
     if options.deps_os:
@@ -1286,7 +1281,7 @@ it or fix the checkout.
     for s in config_dict.get('solutions', []):
       try:
         deps_to_add.append(Dependency(
-            self, s['name'], s['url'],
+            self, s['name'], s['url'], s['url'],
             s.get('managed', True),
             s.get('custom_deps', {}),
             s.get('custom_vars', {}),
@@ -1738,7 +1733,7 @@ class Flattener(object):
           continue
         scm = gclient_scm.CreateSCM(
             dep.parsed_url, self._client.root_dir, dep.name, dep.outbuf)
-        dep._parsed_url = dep._url = '%s@%s' % (
+        dep._parsed_url = dep._raw_url = dep._url = '%s@%s' % (
             url, scm.revinfo(self._client._options, [], None))
 
     self._deps_string = '\n'.join(
@@ -1875,7 +1870,7 @@ def _DepsToLines(deps):
     s.extend([
         '  # %s' % dep.hierarchy(include_url=False),
         '  "%s": {' % (name,),
-        '    "url": "%s",' % (dep.url,),
+        '    "url": "%s",' % (dep.raw_url,),
     ] + condition_part + [
         '  },',
         '',

testing_support/fake_repos.py

@@ -458,6 +458,8 @@ pre_deps_hooks = [
       'DEPS': """
 vars = {
   'DummyVariable': 'repo',
+  'git_base': '%(git_base)s',
+  'hook1_contents': 'git_hooked1',
 }
 gclient_gn_args_file = 'src/gclient.args'
 gclient_gn_args = ['DummyVariable']
@@ -466,7 +468,7 @@ allowed_hosts = [
 ]
 deps = {
   'src/repo2': {
-    'url': '%(git_base)srepo_2@%(hash)s',
+    'url': Var('git_base') + 'repo_2@%(hash)s',
     'condition': 'True',
   },
   'src/repo4': {
@@ -493,7 +495,7 @@ hooks = [
   {
     'pattern': '.',
     'action': ['python', '-c',
-               'open(\\'src/git_hooked1\\', \\'w\\').write(\\'git_hooked1\\')'],
+               'open(\\'src/git_hooked1\\', \\'w\\').write(\\'{hook1_contents}\\')'],
   },
   {
     # Should not be run.

tests/gclient_eval_unittest.py

@@ -103,7 +103,7 @@ class ExecTest(unittest.TestCase):
   def test_var(self):
     local_scope = {}
     global_scope = {
-        'Var': gclient.GClientKeywords.VarImpl({}, local_scope).Lookup,
+        'Var': lambda var_name: '{%s}' % var_name,
     }
     gclient_eval.Exec('\n'.join([
         'vars = {',
@@ -115,7 +115,7 @@ class ExecTest(unittest.TestCase):
     ]), global_scope, local_scope, '<string>')
     self.assertEqual({
         'vars': collections.OrderedDict([('foo', 'bar')]),
-        'deps': collections.OrderedDict([('a_dep', 'abarb')]),
+        'deps': collections.OrderedDict([('a_dep', 'a{foo}b')]),
     }, local_scope)
 
 

tests/gclient_smoketest.py

@@ -578,6 +578,7 @@ class GClientSmokeGIT(GClientSmokeBase):
     with open(output_deps) as f:
       deps_contents = f.read()
 
+    self.maxDiff = None
     self.assertEqual([
         'gclient_gn_args_file = "src/gclient.args"',
         'gclient_gn_args = [\'DummyVariable\']',
@@ -598,7 +599,7 @@ class GClientSmokeGIT(GClientSmokeBase):
         '',
         '  # src -> src/repo2',
         '  "src/repo2": {',
-        '    "url": "git://127.0.0.1:20000/git/repo_2@%s",' % (
+        '    "url": "{git_base}repo_2@%s",' % (
                  self.githash('repo_2', 1)[:7]),
         '    "condition": "True",',
         '  },',
@@ -661,7 +662,8 @@ class GClientSmokeGIT(GClientSmokeBase):
         '    "action": [',
         '        "python",',
         '        "-c",',
-        '        "open(\'src/git_hooked1\', \'w\').write(\'git_hooked1\')",',
+        '        "open(\'src/git_hooked1\', \'w\')'
+            '.write(\'{hook1_contents}\')",',
         '    ]',
         '  },',
         '',
@@ -700,6 +702,12 @@ class GClientSmokeGIT(GClientSmokeBase):
         '  # src',
         '  "DummyVariable": \'repo\',',
         '',
+        '  # src',
+        '  "git_base": \'git://127.0.0.1:20000/git/\',',
+        '',
+        '  # src',
+        '  "hook1_contents": \'git_hooked1\',',
+        '',
         '}',
         '',
     ], deps_contents.splitlines())
@@ -741,7 +749,7 @@ class GClientSmokeGIT(GClientSmokeBase):
         '',
         '  # src -> src/repo2',
         '  "src/repo2": {',
-        '    "url": "git://127.0.0.1:20000/git/repo_2@%s",' % (
+        '    "url": "{git_base}repo_2@%s",' % (
                  self.githash('repo_2', 1)[:7]),
         '    "condition": "True",',
         '  },',
@@ -805,7 +813,8 @@ class GClientSmokeGIT(GClientSmokeBase):
         '    "action": [',
         '        "python",',
         '        "-c",',
-        '        "open(\'src/git_hooked1\', \'w\').write(\'git_hooked1\')",',
+        '        "open(\'src/git_hooked1\', \'w\')'
+            '.write(\'{hook1_contents}\')",',
         '    ]',
         '  },',
         '',
@@ -844,6 +853,12 @@ class GClientSmokeGIT(GClientSmokeBase):
         '  # src',
         '  "DummyVariable": \'repo\',',
         '',
+        '  # src',
+        '  "git_base": \'git://127.0.0.1:20000/git/\',',
+        '',
+        '  # src',
+        '  "hook1_contents": \'git_hooked1\',',
+        '',
         '}',
         '',
     ], deps_contents.splitlines())

tests/gclient_test.py

@@ -200,8 +200,9 @@ class GclientTest(trial_dir.TestCase):
   def testAutofix(self):
     # Invalid urls causes pain when specifying requirements. Make sure it's
     # auto-fixed.
+    url = 'proto://host/path/@revision'
     d = gclient.Dependency(
-        None, 'name', 'proto://host/path/@revision', None, None, None,
+        None, 'name', url, url, None, None, None,
         None, '', True, False, None, True)
     self.assertEquals('proto://host/path@revision', d.url)
 
@@ -212,17 +213,17 @@ class GclientTest(trial_dir.TestCase):
     obj.add_dependencies_and_close(
       [
         gclient.Dependency(
-          obj, 'foo', 'url', None, None, None, None, 'DEPS', True, False,
-          None, True),
+          obj, 'foo', 'raw_url', 'url', None, None, None, None, 'DEPS', True,
+          False, None, True),
         gclient.Dependency(
-          obj, 'bar', 'url', None, None, None, None, 'DEPS', True, False,
-          None, True),
+          obj, 'bar', 'raw_url', 'url', None, None, None, None, 'DEPS', True,
+          False, None, True),
       ],
       [])
     obj.dependencies[0].add_dependencies_and_close(
       [
         gclient.Dependency(
-          obj.dependencies[0], 'foo/dir1', 'url', None, None, None,
+          obj.dependencies[0], 'foo/dir1', 'raw_url', 'url', None, None, None,
           None, 'DEPS', True, False, None, True),
       ],
       [])
@@ -620,7 +621,7 @@ class GclientTest(trial_dir.TestCase):
   def testLateOverride(self):
     """Verifies expected behavior of LateOverride."""
     url = "git@github.com:dart-lang/spark.git"
-    d = gclient.Dependency(None, 'name', 'url',
+    d = gclient.Dependency(None, 'name', 'raw_url', 'url',
                            None, None, None, None, '', True, False, None, True)
     late_url = d.LateOverride(url)
     self.assertEquals(url, late_url)