From e79ddeaabf246769937282f952897bcec3e9de05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Hajdan=2C=20Jr?= Date: Mon, 24 Jul 2017 22:23:49 +0200 Subject: [PATCH] gclient flatten: preserve variable placeholders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Reviewed-by: Dirk Pranke --- gclient.py | 53 ++++++++++++++-------------------- testing_support/fake_repos.py | 3 +- tests/gclient_eval_unittest.py | 4 +-- tests/gclient_smoketest.py | 10 +++++-- tests/gclient_test.py | 15 +++++----- 5 files changed, 42 insertions(+), 43 deletions(-) diff --git a/gclient.py b/gclient.py index e5eba3e9a..4eb04bd66 100755 --- a/gclient.py +++ b/gclient.py @@ -221,32 +221,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 +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 + [ ' },', '', diff --git a/testing_support/fake_repos.py b/testing_support/fake_repos.py index ab4f4c72b..87eb6389b 100755 --- a/testing_support/fake_repos.py +++ b/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': { diff --git a/tests/gclient_eval_unittest.py b/tests/gclient_eval_unittest.py index 7ef6b38ae..d8579b8ef 100755 --- a/tests/gclient_eval_unittest.py +++ b/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, '') self.assertEqual({ 'vars': collections.OrderedDict([('foo', 'bar')]), - 'deps': collections.OrderedDict([('a_dep', 'abarb')]), + 'deps': collections.OrderedDict([('a_dep', 'a{foo}b')]), }, local_scope) diff --git a/tests/gclient_smoketest.py b/tests/gclient_smoketest.py index 683d2b97f..0249a91d3 100755 --- a/tests/gclient_smoketest.py +++ b/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()) diff --git a/tests/gclient_test.py b/tests/gclient_test.py index c185cb6ac..ae8c3cc35 100755 --- a/tests/gclient_test.py +++ b/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)