From 357415cb11acdf6545158d37bc6fd808d58e3dd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Hajdan=2C=20Jr?= Date: Mon, 24 Jul 2017 14:35:28 +0200 Subject: [PATCH] gclient flatten: fix a bug with some recursedeps not being processed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Added a regression test. Simplified some logic - if we don't add os-specific deps and hooks to |dependencies|, we don't need to keep separate original values. Bug: 570091 Change-Id: I5bdd0b6a66df6b3a2b99d0ad9c6e54ee7114f09b Reviewed-on: https://chromium-review.googlesource.com/581687 Commit-Queue: Paweł Hajdan Jr. Reviewed-by: Dirk Pranke Reviewed-by: Michael Moss --- gclient.py | 57 ++++++++++----------------------- testing_support/fake_repos.py | 24 +++++++++++++- tests/gclient_smoketest.py | 60 +++++++++++++++++++++++++++++++++-- 3 files changed, 98 insertions(+), 43 deletions(-) diff --git a/gclient.py b/gclient.py index 2ded56ba3..e5eba3e9a 100755 --- a/gclient.py +++ b/gclient.py @@ -371,9 +371,6 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): # Calculates properties: self._parsed_url = None self._dependencies = [] - # Keep track of original values, before post-processing (e.g. deps_os). - self._orig_dependencies = [] - self._orig_deps_hooks = [] self._vars = {} self._os_dependencies = {} self._os_deps_hooks = {} @@ -738,7 +735,6 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): rel_prefix = os.path.dirname(self.name) deps = local_scope.get('deps', {}) - orig_deps = gclient_utils.freeze(deps) if 'recursion' in local_scope: self.recursion_override = local_scope.get('recursion') logging.warning( @@ -771,14 +767,13 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): for dep_os, os_deps in local_scope['deps_os'].iteritems(): self._os_dependencies[dep_os] = self._deps_to_objects( self._postprocess_deps(os_deps, rel_prefix), use_relative_paths) - if target_os_list: + if target_os_list and not self._get_option( + 'do_not_merge_os_specific_entries', False): deps = self.MergeWithOsDeps( deps, local_scope['deps_os'], target_os_list) deps_to_add = self._deps_to_objects( self._postprocess_deps(deps, rel_prefix), use_relative_paths) - orig_deps_to_add = self._deps_to_objects( - self._postprocess_deps(orig_deps, rel_prefix), use_relative_paths) # override named sets of hooks by the custom hooks hooks_to_run = [] @@ -786,7 +781,6 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): for hook in local_scope.get('hooks', []): if hook.get('name', '') not in hook_names_to_suppress: hooks_to_run.append(hook) - orig_hooks = gclient_utils.freeze(hooks_to_run) if 'hooks_os' in local_scope and target_os_list: hooks_os = local_scope['hooks_os'] @@ -797,9 +791,10 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): for hook in os_hooks] # Specifically append these to ensure that hooks_os run after hooks. - for the_target_os in target_os_list: - the_target_os_hooks = hooks_os.get(the_target_os, []) - hooks_to_run.extend(the_target_os_hooks) + if not self._get_option('do_not_merge_os_specific_entries', False): + for the_target_os in target_os_list: + the_target_os_hooks = hooks_os.get(the_target_os, []) + hooks_to_run.extend(the_target_os_hooks) # add the replacements and any additions for hook in self.custom_hooks: @@ -811,9 +806,7 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): Hook.from_dict(hook, variables=self.get_vars()) for hook in local_scope.get('pre_deps_hooks', [])] - self.add_dependencies_and_close( - deps_to_add, hooks_to_run, orig_deps_to_add=orig_deps_to_add, - orig_hooks=orig_hooks) + self.add_dependencies_and_close(deps_to_add, hooks_to_run) logging.info('ParseDepsFile(%s) done' % self.name) def _get_option(self, attr, default): @@ -822,19 +815,13 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): obj = obj.parent return getattr(obj._options, attr, default) - def add_dependencies_and_close( - self, deps_to_add, hooks, orig_deps_to_add=None, orig_hooks=None): + def add_dependencies_and_close(self, deps_to_add, hooks): """Adds the dependencies, hooks and mark the parsing as done.""" for dep in deps_to_add: if dep.verify_validity(): self.add_dependency(dep) - for dep in (orig_deps_to_add if orig_deps_to_add is not None - else deps_to_add): - self.add_orig_dependency(dep) self._mark_as_parsed( - [Hook.from_dict(h, variables=self.get_vars()) for h in hooks], - orig_hooks=[Hook.from_dict(h, variables=self.get_vars()) - for h in (orig_hooks if orig_hooks is not None else hooks)]) + [Hook.from_dict(h, variables=self.get_vars()) for h in hooks]) def findDepsFromNotAllowedHosts(self): """Returns a list of depenecies from not allowed hosts. @@ -1041,13 +1028,8 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): self._dependencies.append(new_dep) @gclient_utils.lockedmethod - def add_orig_dependency(self, new_dep): - self._orig_dependencies.append(new_dep) - - @gclient_utils.lockedmethod - def _mark_as_parsed(self, new_hooks, orig_hooks=None): + def _mark_as_parsed(self, new_hooks): self._deps_hooks.extend(new_hooks) - self._orig_deps_hooks.extend(orig_hooks or new_hooks) self._deps_parsed = True @property @@ -1055,11 +1037,6 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): def dependencies(self): return tuple(self._dependencies) - @property - @gclient_utils.lockedmethod - def orig_dependencies(self): - return tuple(self._orig_dependencies) - @property @gclient_utils.lockedmethod def os_dependencies(self): @@ -1070,11 +1047,6 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): def deps_hooks(self): return tuple(self._deps_hooks) - @property - @gclient_utils.lockedmethod - def orig_deps_hooks(self): - return tuple(self._orig_deps_hooks) - @property @gclient_utils.lockedmethod def os_deps_hooks(self): @@ -1806,7 +1778,7 @@ class Flattener(object): assert key not in self._vars self._vars[key] = (dep, value) - self._hooks.extend([(dep, hook) for hook in dep.orig_deps_hooks]) + self._hooks.extend([(dep, hook) for hook in dep.deps_hooks]) self._pre_deps_hooks.extend([(dep, hook) for hook in dep.pre_deps_hooks]) for hook_os, os_hooks in dep.os_deps_hooks.iteritems(): @@ -1827,7 +1799,7 @@ class Flattener(object): """ self._add_deps_os(dep) - for sub_dep in dep.orig_dependencies: + for sub_dep in dep.dependencies: self._flatten_dep(sub_dep) def _add_deps_os(self, dep): @@ -1850,6 +1822,7 @@ def CMDflatten(parser, args): 'for checked out deps, NOT deps_os.')) options, args = parser.parse_args(args) + options.do_not_merge_os_specific_entries = True options.nohooks = True client = GClient.LoadCurrentConfig(options) @@ -2220,6 +2193,10 @@ def CMDsync(parser, args): help='override deps for the specified (comma-separated) ' 'platform(s); \'all\' will process all deps_os ' 'references') + # TODO(phajdan.jr): use argparse.SUPPRESS to hide internal flags. + parser.add_option('--do-not-merge-os-specific-entries', action='store_true', + help='INTERNAL ONLY - disables merging of deps_os and ' + 'hooks_os to dependencies and hooks') parser.add_option('--upstream', action='store_true', help='Make repo state match upstream branch.') parser.add_option('--output-json', diff --git a/testing_support/fake_repos.py b/testing_support/fake_repos.py index 4bc4e15b5..ab4f4c72b 100755 --- a/testing_support/fake_repos.py +++ b/testing_support/fake_repos.py @@ -298,7 +298,7 @@ class FakeReposBase(object): class FakeRepos(FakeReposBase): """Implements populateGit().""" - NB_GIT_REPOS = 8 + NB_GIT_REPOS = 10 def populateGit(self): # Testing: @@ -556,6 +556,28 @@ deps_os ={ 'origin': 'git/repo_8@1\n', }) + self._commit_git('repo_9', { + 'DEPS': """ +deps = { + 'src/repo8': '/repo_8', +} +recursedeps = [ + 'src/repo8', +]""", + 'origin': 'git/repo_9@1\n', + }) + + self._commit_git('repo_10', { + 'DEPS': """ +deps = { + 'src/repo9': '/repo_9', +} +recursedeps = [ + 'src/repo9', +]""", + 'origin': 'git/repo_10@1\n', + }) + class FakeRepoSkiaDEPS(FakeReposBase): """Simulates the Skia DEPS transition in Chrome.""" diff --git a/tests/gclient_smoketest.py b/tests/gclient_smoketest.py index 47ce9476e..aa7e959ca 100755 --- a/tests/gclient_smoketest.py +++ b/tests/gclient_smoketest.py @@ -729,7 +729,8 @@ class GClientSmokeGIT(GClientSmokeBase): 'deps = {', ' # src -> src/repo2 -> foo/bar', ' "foo/bar": {', - ' "url": "/repo_3",', + ' "url": "git://127.0.0.1:20000/git/repo_3@%s",' % ( + self.githash('repo_3', 2)), ' },', '', ' # src', @@ -753,7 +754,8 @@ class GClientSmokeGIT(GClientSmokeBase): '', ' # src -> src/repo8', ' "src/repo8": {', - ' "url": "/repo_8",', + ' "url": "git://127.0.0.1:20000/git/repo_8@%s",' % ( + self.githash('repo_8', 1)), ' },', '', '}', @@ -846,6 +848,60 @@ class GClientSmokeGIT(GClientSmokeBase): '', ], deps_contents.splitlines()) + def testFlattenRecursedeps(self): + if not self.enabled: + return + + output_deps = os.path.join(self.root_dir, 'DEPS.flattened') + self.assertFalse(os.path.exists(output_deps)) + + self.gclient(['config', self.git_base + 'repo_10', '--name', 'src']) + self.gclient(['sync']) + self.gclient(['flatten', '-v', '-v', '-v', '--output-deps', output_deps]) + + with open(output_deps) as f: + deps_contents = f.read() + + self.assertEqual([ + 'deps = {', + ' # src', + ' "src": {', + ' "url": "git://127.0.0.1:20000/git/repo_10",', + ' },', + '', + ' # src -> src/repo9 -> src/repo8', + ' "src/repo8": {', + ' "url": "/repo_8",', + ' },', + '', + ' # src -> src/repo9', + ' "src/repo9": {', + ' "url": "/repo_9",', + ' },', + '', + '}', + '', + 'deps_os = {', + ' "mac": {', + ' # src -> src/repo9 -> src/repo8 -> src/recursed_os_repo', + ' "src/recursed_os_repo": {', + ' "url": "/repo_5",', + ' },', + '', + ' },', + '', + ' "unix": {', + ' # src -> src/repo9 -> src/repo8 -> src/recursed_os_repo', + ' "src/recursed_os_repo": {', + ' "url": "/repo_5",', + ' },', + '', + ' },', + '', + '}', + '', + ], deps_contents.splitlines()) + class GClientSmokeGITMutates(GClientSmokeBase): """testRevertAndStatus mutates the git repo so move it to its own suite."""