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 <gavinmak@google.com>
Commit-Queue: Jordan Brown <rop@google.com>
Reviewed-by: Jiewei Qian <qjw@chromium.org>

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 "<year>-<month>-<day> <author> <subject>"
@@ -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

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]