From 4373d97c21ece5ef4d76d056565bfe53e273063f Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Tue, 1 Apr 2025 20:40:52 -0700 Subject: [PATCH] roll_dep: Update README.chromium when rolling submodules Attempt to update the README.chromium file with the new submodule revision after rolling it in a DEPS update. This update is flag-guarded and will only run when explicitly specified. Currently, only README.chromium files with a single "Revision:" line are supported. Multiple lines and files with the divider are not handled. These are left as future TODOs. Bug: 390067679 Change-Id: Ib776564ae94360cc72dd633fc7ed7b3f84b5b9d2 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6173767 Reviewed-by: Gavin Mak Commit-Queue: Jordan Brown Reviewed-by: Jiewei Qian --- roll_dep.py | 70 ++++++++++++- tests/roll_dep_test.py | 228 +++++++++++++++++++++++++++++------------ 2 files changed, 228 insertions(+), 70 deletions(-) diff --git a/roll_dep.py b/roll_dep.py index fa39e6897..bc81ea25f 100755 --- a/roll_dep.py +++ b/roll_dep.py @@ -21,6 +21,8 @@ import gclient_utils NEED_SHELL = sys.platform.startswith('win') GCLIENT_PATH = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'gclient.py') +_DEPENDENCY_DIVIDER_PATTERN = re.compile(r'^-{20} DEPENDENCY DIVIDER -{20}$', re.MULTILINE) +_REVISION_LINE_PATTERN = re.compile(r'^Revision: ([a-f0-9]+|DEPS)$', re.MULTILINE) # Commit subject that will be considered a roll. In the format generated by the # git log used, so it's "-- " @@ -218,17 +220,21 @@ def gen_commit_msg(logs, cmdline, reviewers, bug): return commit_msg -def finalize(commit_msg, current_dir, rolls): +def finalize(args, commit_msg, current_dir, rolls): """Commits changes to the DEPS file, then uploads a CL.""" print('Commit message:') print('\n'.join(' ' + i for i in commit_msg.splitlines())) # Pull the dependency to the right revision. This is surprising to users - # otherwise. The revision update is done before commiting to update + # otherwise. The revision update is done before committing to update # submodule revision if present. for dependency, (_head, roll_to, full_dir) in sorted(rolls.items()): check_call(['git', 'checkout', '--quiet', roll_to], cwd=full_dir) + # Attempt to update README.chromium. + if args.update_readme: + update_readme_chromium(dependency, roll_to, current_dir) + # This adds the submodule revision update to the commit. if is_submoduled(): check_call([ @@ -247,6 +253,59 @@ def finalize(commit_msg, current_dir, rolls): cwd=current_dir) os.remove(commit_filename) +def update_readme_chromium(dependency, roll_to, current_dir): + """Attempts to update the README.chromium file with the new revision. + + TODO(b/390067679): Handle README.chromium files with multiple dependencies. + + TODO(b/390067679): Add flag to provide custom location for README.chromium. + + Args: + dependency: Path to the dependency being rolled. + roll_to: New revision hash to roll to. + current_dir: Current working directory. + """ + + # README.chromium is typically one directory up from the dependency. + gclient_root = gclient(['root']) + readme_path = os.path.normpath( + os.path.join(gclient_root, dependency, os.path.pardir, + 'README.chromium')) + + if not os.path.isfile(readme_path): + print(f'No README.chromium found at {readme_path}') + return + + with open(readme_path, 'r') as f: + content = f.read() + + # TODO(b/390067679): Handle README.chromium files with multiple dependencies. + if _DEPENDENCY_DIVIDER_PATTERN.match(content): + print('README.chromium contains "- DEPENDENCY DIVIDER -"\n' + 'Files with multiple dependencies are not supported') + return + + # Only update when there is exactly one `Revision: line`. + revision_count = len(_REVISION_LINE_PATTERN.findall(content)) + if revision_count != 1: + print(f'README.chromium contains {revision_count} Revision: lines, skipping update.\n' + 'Files with multiple dependencies are not supported') + return + + # Update the revision line. + new_content = _REVISION_LINE_PATTERN.sub( + f'Revision: {roll_to}', + content) + + if new_content == content: + print(f'README.chromium already has revision {roll_to}, \ncontent:{new_content}') + return + + with open(readme_path, 'w') as f: + f.write(new_content) + check_call(['git', 'add', readme_path], cwd=current_dir) + print(f'Updated revision in README.chromium for {dependency} to {roll_to}') + def main(): if gclient_utils.IsEnvCog(): @@ -290,6 +349,9 @@ def main(): default=[], help='Regex(es) for dependency in DEPS file') parser.add_argument('dep_path', nargs='+', help='Path(s) to dependency') + parser.add_argument('--update-readme', + action='store_true', + help='Update Revision in README.chromium if it exists') args = parser.parse_args() if len(args.dep_path) > 1: @@ -318,6 +380,8 @@ def main(): d.replace('\\', '/').rstrip('/') for d in args.dep_path) cmdline = 'roll-dep ' + ' '.join(dependencies) + ''.join(' --key ' + k for k in args.key) + if args.update_readme: + cmdline += ' --update-readme' try: if not args.ignore_dirty_tree and not is_pristine(current_dir): raise Error('Ensure %s is clean first (no non-merged commits).' % @@ -367,7 +431,7 @@ def main(): gclient(['setdep'] + setdep_args) commit_msg = gen_commit_msg(logs, cmdline, reviewers, args.bug) - finalize(commit_msg, current_dir, rolls) + finalize(args, commit_msg, current_dir, rolls) except Error as e: sys.stderr.write('error: %s\n' % e) return 2 if isinstance(e, AlreadyRolledError) else 1 diff --git a/tests/roll_dep_test.py b/tests/roll_dep_test.py index 462474de5..98e8af945 100755 --- a/tests/roll_dep_test.py +++ b/tests/roll_dep_test.py @@ -22,34 +22,103 @@ GCLIENT = os.path.join(ROOT_DIR, 'gclient') # TODO: Should fix these warnings. # pylint: disable=line-too-long +def create_deps_content(git_base, path_to_revision_map): + """ + Create a DEPS file content string with the given dependency mappings. + + Args: + git_base: The base URL for git repositories + path_to_revision_map: Dictionary mapping dependency paths to their revisions + + Returns: + String with the complete DEPS file content including standard hooks + """ + dep_lines = [] + git_base = git_base.replace('\\', '\\\\') + for path, revision in path_to_revision_map.items(): + dep_lines.append(f' "{path}": "file://{git_base}repo_2@{revision}",') + + # Combine all parts with standard hooks. + deps_content = [ + 'deps = {', + '\n'.join(dep_lines), + '}', + 'hooks = [', + ' {"action": ["foo", "--android", "{checkout_android}"]}', + ']', + ] + return '\n'.join(deps_content) + class FakeRepos(fake_repos.FakeReposBase): NB_GIT_REPOS = 2 def populateGit(self): - self._commit_git('repo_2', { - 'origin': 'git/repo_2@1', - }) - self._commit_git('repo_2', { - 'origin': 'git/repo_2@2', - }) - self._commit_git('repo_2', { - 'origin': 'git/repo_2@3', - }) + for x in range(1,4): + self._commit_git('repo_2', {'origin': f'git/repo_2@{x}'}) + # repo_2@1 is the default revision. + # Anything under 'third_party/not_supported' tests handling unsupported + # cases. + repo2_revision = self.git_hashes['repo_2'][1][0] self._commit_git( 'repo_1', { - 'DEPS': '\n'.join([ - 'deps = {', - ' "src/foo": "file://%(git_base)srepo_2@%(repo_2_revision)s",', - '}', - 'hooks = [', - ' {"action": ["foo", "--android", "{checkout_android}"]}', - ']', - ]) % { - 'git_base': self.git_base.replace('\\', '\\\\'), - 'repo_2_revision': self.git_hashes['repo_2'][1][0], - }, + 'DEPS': create_deps_content(self.git_base, { + 'src/foo': repo2_revision, + 'src/third_party/repo_2/src': repo2_revision, + 'src/third_party/repo_2B/src': repo2_revision, + 'src/third_party/not_supported/with_divider/src': repo2_revision, + 'src/third_party/not_supported/multiple_revisions/src': repo2_revision, + 'src/third_party/not_supported/no_revision/src': repo2_revision + }), + 'README.chromium': '\n'.join([ + 'Name: test repo', + 'URL: https://example.com', + 'Version: 1.0', + 'Revision: abcabc123123', + 'License: MIT', + ]), + 'third_party/repo_2/README.chromium': '\n'.join([ + 'Name: test repo 2', + 'URL: https://example.com', + 'Version: 1.0', + 'Revision: abc1234', + 'License: MIT', + ]), + 'third_party/repo_2B/README.chromium': '\n'.join([ + 'Name: Override DEPS value for revision', + 'URL: https://example.com', + 'Version: 1.0', + 'Revision: DEPS', + 'License: MIT', + ]), + 'third_party/not_supported/with_divider/README.chromium': '\n'.join([ + 'Name: Deps divider not supported', + 'URL: https://example.com', + 'Version: 1.0', + 'Revision: abc1234', + 'License: MIT', + '-------------------- DEPENDENCY DIVIDER --------------------', + 'Name: So nothing here should change', + 'URL: https://example.com', + 'Version: 1.0', + 'Revision: abc1234', + 'License: MIT', + ]), + 'third_party/not_supported/multiple_revisions/README.chromium': '\n'.join([ + 'Name: Multiple revisions', + 'URL: https://example.com', + 'Version: 1.0', + 'Revision: abc1234', + 'License: MIT', + 'Revision: abc1235', # This should not happen. + ]), + 'third_party/not_supported/no_revision/README.chromium': '\n'.join([ + 'Name: No revision', + 'URL: https://example.com', + 'Version: 1.0', + 'License: MIT', + ]), }) @@ -70,6 +139,14 @@ class RollDepTest(fake_repos.FakeReposTestBase): self.enabled = self.FAKE_REPOS.set_up_git() self.src_dir = os.path.join(self.root_dir, 'src') self.foo_dir = os.path.join(self.src_dir, 'foo') + self.all_repos = [ + 'src/foo', + 'src/third_party/repo_2/src', + 'src/third_party/repo_2B/src', + 'src/third_party/not_supported/with_divider/src', + 'src/third_party/not_supported/multiple_revisions/src', + 'src/third_party/not_supported/no_revision/src', + ] if self.enabled: self.call([ GCLIENT, 'config', 'file://' + self.git_base + 'repo_1', @@ -95,37 +172,76 @@ class RollDepTest(fake_repos.FakeReposTestBase): '\n'), stderr.replace('\r\n', '\n'), process.returncode) + def assert_deps_match(self, expected_path_to_revision_map): + # Assume everything is at the default revision and only update the + # provided paths. + default_revision = self.githash('repo_2', 1) + expected_map = {path: default_revision for path in self.all_repos} + expected_map.update(expected_path_to_revision_map) + + for path, revision in expected_map.items(): + with self.subTest(path=path): + path_dir = os.path.join(self.root_dir, path) + self.assertEqual(self.gitrevparse(path_dir), revision) + + with open(os.path.join(self.src_dir, 'DEPS')) as f: + actual_content = f.read() + with self.subTest(path='DEPS'): + expected_content = create_deps_content(self.git_base,expected_map) + self.assertEqual(expected_content, actual_content) + + def testRollsDep(self): if not self.enabled: return - stdout, stderr, returncode = self.call([ROLL_DEP, 'src/foo']) - expected_revision = self.githash('repo_2', 3) + stdout, stderr, returncode = self.call([ROLL_DEP]+self.all_repos) + latest_revision = self.githash('repo_2', 3) self.assertEqual(stderr, '') self.assertEqual(returncode, 0) - with open(os.path.join(self.src_dir, 'DEPS')) as f: - contents = f.read() - - self.assertEqual(self.gitrevparse(self.foo_dir), expected_revision) - self.assertEqual([ - 'deps = {', - ' "src/foo": "file://' + self.git_base.replace('\\', '\\\\') + - 'repo_2@' + expected_revision + '",', - '}', - 'hooks = [', - ' {"action": ["foo", "--android", "{checkout_android}"]}', - ']', - ], contents.splitlines()) + # All deps should be rolled to the latest revision. + self.assert_deps_match({p: latest_revision for p in self.all_repos}) commit_message = self.call(['git', 'log', '-n', '1'])[0] expected_message = 'Roll src/foo/ %s..%s (2 commits)' % (self.githash( - 'repo_2', 1)[:9], self.githash('repo_2', 3)[:9]) + 'repo_2', 1)[:9], latest_revision[:9]) self.assertIn(expected_message, stdout) self.assertIn(expected_message, commit_message) + + def testRollsDepWithReadme(self): + """Tests roll-dep when updating README.chromium files.""" + if not self.enabled: + return + stdout, stderr, returncode = self.call( + [ROLL_DEP, '--update-readme']+self.all_repos + ) + latest_revision = self.githash('repo_2', 3) + + # All deps should be rolled to the latest revision (3). + self.assert_deps_match({p: latest_revision for p in self.all_repos}) + self.assertEqual(stderr, '') + self.assertEqual(returncode, 0) + for path in self.all_repos: + with self.subTest(path=path): + contents = '' + readme_path = os.path.join(self.root_dir, path, os.path.pardir, 'README.chromium') + if os.path.exists(readme_path): + with open(readme_path, 'r') as f: + contents = f.read() + if path == 'src/third_party/not_supported/no_revision/src': + self.assertIn('README.chromium contains 0 Revision: lines', stdout) + if 'not_supported' in path: + self.assertNotIn(latest_revision, contents) + continue + # Check that the revision was updated. + self.assertIn(f'Revision: {latest_revision}', contents) + self.assertNotIn('Revision: abcabc123123', contents) + self.assertNotIn('No README.chromium found', stdout) + def testRollsDepReviewers(self): if not self.enabled: return @@ -145,27 +261,16 @@ class RollDepTest(fake_repos.FakeReposTestBase): def testRollsDepToSpecificRevision(self): if not self.enabled: return + specified_revision = self.githash('repo_2', 2) stdout, stderr, returncode = self.call( - [ROLL_DEP, 'src/foo', '--roll-to', - self.githash('repo_2', 2)]) - expected_revision = self.githash('repo_2', 2) + [ROLL_DEP, 'src/foo', '--roll-to', specified_revision]) self.assertEqual(stderr, '') self.assertEqual(returncode, 0) - with open(os.path.join(self.src_dir, 'DEPS')) as f: - contents = f.read() - - self.assertEqual(self.gitrevparse(self.foo_dir), expected_revision) - self.assertEqual([ - 'deps = {', - ' "src/foo": "file://' + self.git_base.replace('\\', '\\\\') + - 'repo_2@' + expected_revision + '",', - '}', - 'hooks = [', - ' {"action": ["foo", "--android", "{checkout_android}"]}', - ']', - ], contents.splitlines()) + self.assert_deps_match({ + 'src/foo': specified_revision, + }) commit_message = self.call(['git', 'log', '-n', '1'])[0] @@ -180,24 +285,13 @@ class RollDepTest(fake_repos.FakeReposTestBase): return stdout, stderr, returncode = self.call( [ROLL_DEP, 'src/foo', '--log-limit', '1']) - expected_revision = self.githash('repo_2', 3) + latest_revision = self.githash('repo_2', 3) self.assertEqual(stderr, '') self.assertEqual(returncode, 0) - - with open(os.path.join(self.src_dir, 'DEPS')) as f: - contents = f.read() - - self.assertEqual(self.gitrevparse(self.foo_dir), expected_revision) - self.assertEqual([ - 'deps = {', - ' "src/foo": "file://' + self.git_base.replace('\\', '\\\\') + - 'repo_2@' + expected_revision + '",', - '}', - 'hooks = [', - ' {"action": ["foo", "--android", "{checkout_android}"]}', - ']', - ], contents.splitlines()) + self.assert_deps_match({ + 'src/foo':latest_revision, + }) commit_message = self.call(['git', 'log', '-n', '1'])[0]