gclient flatten: preserve variable placeholders

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
Change-Id: I9b13a691b5203cc284c33a59438720e31c9ebf7a
Reviewed-on: https://chromium-review.googlesource.com/583617
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>

gclient.py

@@ -221,32 +221,16 @@ class Hook(object):
             gclient_utils.CommandToStr(cmd), elapsed_time))
 
 
-class GClientKeywords(object):
+class DependencySettings(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):
   """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 +308,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 +344,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 +626,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 +678,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.
@@ -1203,7 +1194,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 +1277,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 +1729,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 +1866,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,7 @@ pre_deps_hooks = [
       'DEPS': """
 vars = {
   'DummyVariable': 'repo',
+  'git_base': '%(git_base)s',
 }
 gclient_gn_args_file = 'src/gclient.args'
 gclient_gn_args = ['DummyVariable']
@@ -466,7 +467,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': {

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

@@ -602,7 +602,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",',
         '  },',
@@ -704,6 +704,9 @@ class GClientSmokeGIT(GClientSmokeBase):
         '  # src',
         '  "DummyVariable": \'repo\',',
         '',
+        '  # src',
+        '  "git_base": \'git://127.0.0.1:20000/git/\',',
+        '',
         '}',
         '',
     ], deps_contents.splitlines())
@@ -745,7 +748,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",',
         '  },',
@@ -848,6 +851,9 @@ class GClientSmokeGIT(GClientSmokeBase):
         '  # src',
         '  "DummyVariable": \'repo\',',
         '',
+        '  # src',
+        '  "git_base": \'git://127.0.0.1:20000/git/\',',
+        '',
         '}',
         '',
     ], 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,
+          obj, 'foo', 'raw_url', 'url', None, None, None, None, 'DEPS', True,
-          None, True),
+          False, None, True),
         gclient.Dependency(
-          obj, 'bar', 'url', None, None, None, None, 'DEPS', True, False,
+          obj, 'bar', 'raw_url', 'url', None, None, None, None, 'DEPS', True,
-          None, 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)