From 9f20d020855c8ee87ba64d53ef94051c06aab860 Mon Sep 17 00:00:00 2001 From: Dirk Pranke Date: Wed, 11 Oct 2017 18:36:54 -0700 Subject: [PATCH] Do not write gclient_gn_args_file too early. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, gclient would attempt to write an args file after a dependency was checked out, but before any sub-dependencies had been checked out. If the args file path pointed at something inside a sub-dependency, this wouldn't work, because the directory might not yet exist. This most obviously happened for buildspec clobber builds. The fix is to wait until after the sub-dependencies have been checked out to write the file. R=phajdan.jr@chromium.org, mmoss@chromium.org BUG=773933 Change-Id: I0cf4564204f7dabd9f843dc7904db7050fcc0d23 Reviewed-on: https://chromium-review.googlesource.com/714644 Reviewed-by: Paweł Hajdan Jr. Commit-Queue: Dirk Pranke --- gclient.py | 18 ++++++++++++++---- testing_support/fake_repos.py | 6 ++++-- tests/gclient_smoketest.py | 14 +++++++------- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/gclient.py b/gclient.py index d80f99d60..35e059738 100755 --- a/gclient.py +++ b/gclient.py @@ -893,8 +893,6 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): # Always parse the DEPS file. self.ParseDepsFile() - if self._gn_args_file and command == 'update': - self.WriteGNArgsFile() self._run_is_done(file_list or [], parsed_url) if command in ('update', 'revert') and not options.noprehooks: self.RunPreDepsHooks() @@ -961,6 +959,9 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): else: print('Skipped missing %s' % cwd, file=sys.stderr) + def HasGNArgsFile(self): + return self._gn_args_file is not None + def WriteGNArgsFile(self): lines = ['# Generated from %r' % self.deps_file] variables = self.get_vars() @@ -1006,6 +1007,12 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): result.extend(s.GetHooks(options)) return result + def WriteGNArgsFilesRecursively(self, dependencies): + for dep in dependencies: + if dep.HasGNArgsFile(): + dep.WriteGNArgsFile() + self.WriteGNArgsFilesRecursively(dep.dependencies) + def RunHooksRecursively(self, options): assert self.hooks_ran == False self._hooks_ran = True @@ -1478,8 +1485,11 @@ it or fix the checkout. print('Please fix your script, having invalid --revision flags will soon ' 'considered an error.', file=sys.stderr) - # Once all the dependencies have been processed, it's now safe to run the - # hooks. + # Once all the dependencies have been processed, it's now safe to write + # out any gn_args_files and run the hooks. + if command == 'update': + self.WriteGNArgsFilesRecursively(self.dependencies) + if not self._options.nohooks: self.RunHooksRecursively(self._options) diff --git a/testing_support/fake_repos.py b/testing_support/fake_repos.py index ebaccc680..0826f0ab2 100755 --- a/testing_support/fake_repos.py +++ b/testing_support/fake_repos.py @@ -336,7 +336,9 @@ vars = { 'str_var': 'abc', 'cond_var': 'false_str_var and true_var', } -gclient_gn_args_file = 'src/gclient.args' +# Nest the args file in a sub-repo, to make sure we don't try to +# write it before we've cloned everything. +gclient_gn_args_file = 'src/repo2/gclient.args' gclient_gn_args = [ 'false_var', 'false_str_var', @@ -489,7 +491,7 @@ vars = { 'cond_var': 'false_str_var and true_var', } -gclient_gn_args_file = 'src/gclient.args' +gclient_gn_args_file = 'src/repo2/gclient.args' gclient_gn_args = [ 'false_var', 'false_str_var', diff --git a/tests/gclient_smoketest.py b/tests/gclient_smoketest.py index 999b9487b..40efe0244 100755 --- a/tests/gclient_smoketest.py +++ b/tests/gclient_smoketest.py @@ -343,7 +343,7 @@ class GClientSmokeGIT(GClientSmokeBase): ('repo_3@1', 'src/repo2/repo3'), ('repo_4@2', 'src/repo4')) tree['src/git_hooked2'] = 'git_hooked2' - tree['src/gclient.args'] = '\n'.join([ + tree['src/repo2/gclient.args'] = '\n'.join([ '# Generated from \'DEPS\'', 'false_var = false', 'false_str_var = false', @@ -364,7 +364,7 @@ class GClientSmokeGIT(GClientSmokeBase): ('repo_4@2', 'src/repo4')) tree['src/git_hooked1'] = 'git_hooked1' tree['src/git_hooked2'] = 'git_hooked2' - tree['src/gclient.args'] = '\n'.join([ + tree['src/repo2/gclient.args'] = '\n'.join([ '# Generated from \'DEPS\'', 'false_var = false', 'false_str_var = false', @@ -406,7 +406,7 @@ class GClientSmokeGIT(GClientSmokeBase): ('repo_2@2', 'src/repo2'), ('repo_3@1', 'src/repo2/repo3'), ('repo_4@2', 'src/repo4')) - tree['src/gclient.args'] = '\n'.join([ + tree['src/repo2/gclient.args'] = '\n'.join([ '# Generated from \'DEPS\'', 'false_var = false', 'false_str_var = false', @@ -451,7 +451,7 @@ class GClientSmokeGIT(GClientSmokeBase): ('repo_3@1', 'src/repo2/repo3'), ('repo_4@2', 'src/repo4')) tree['src/git_hooked2'] = 'git_hooked2' - tree['src/gclient.args'] = '\n'.join([ + tree['src/repo2/gclient.args'] = '\n'.join([ '# Generated from \'DEPS\'', 'false_var = false', 'false_str_var = false', @@ -473,7 +473,7 @@ class GClientSmokeGIT(GClientSmokeBase): ('repo_4@2', 'src/repo4')) tree['src/git_hooked1'] = 'git_hooked1' tree['src/git_hooked2'] = 'git_hooked2' - tree['src/gclient.args'] = '\n'.join([ + tree['src/repo2/gclient.args'] = '\n'.join([ '# Generated from \'DEPS\'', 'false_var = false', 'false_str_var = false', @@ -659,7 +659,7 @@ class GClientSmokeGIT(GClientSmokeBase): self.maxDiff = None self.assertEqual([ - 'gclient_gn_args_file = "src/gclient.args"', + 'gclient_gn_args_file = "src/repo2/gclient.args"', 'gclient_gn_args = [\'false_var\', \'false_str_var\', \'true_var\', ' '\'true_str_var\', \'str_var\', \'cond_var\']', 'allowed_hosts = [', @@ -833,7 +833,7 @@ class GClientSmokeGIT(GClientSmokeBase): deps_contents = f.read() self.assertEqual([ - 'gclient_gn_args_file = "src/gclient.args"', + 'gclient_gn_args_file = "src/repo2/gclient.args"', 'gclient_gn_args = [\'false_var\', \'false_str_var\', \'true_var\', ' '\'true_str_var\', \'str_var\', \'cond_var\']', 'allowed_hosts = [',