Cleanup and reapply uprevs in PublishUprev in CQ.

Currently the CQ reuses old uprevs from the Uprev stage and this is
error-prone because the SHA1s can be out of date. We have robust
logic for fixing this but it only executes when the commit queue
fails. This has the nice side effect of also making the commit queue
more robust to conflicting with other changes, since it resyncs and
reapplies all changes from scratch at the end, rather than trying to
do a complex rebase that could run into conflicts.

This logic is quite well tested and has been in use for a long time,
so it's time we switch to it.

I've also deleted the old UpdateCommitHashesForChanges function. This
logic wasn't working (due to the fact that it was running on the wrong
branch after phobbs' changes) and with my change above, is now
obsolete.

BUG=chromium:549839, chromium:547541
TEST=Unit tests.

Change-Id: I9174504db4f468e04ec7a1c60091c16077be32c1
Reviewed-on: https://chromium-review.googlesource.com/310032
Trybot-Ready: David James <davidjames@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Tested-by: David James <davidjames@chromium.org>

3 files changed, 11 insertions, 69 deletions

diff --git a/cbuildbot/stages/completion_stages.py b/cbuildbot/stages/completion_stages.py
index 2c6b08d34..b77b2f05b 100644
--- a/cbuildbot/stages/completion_stages.py
+++ b/cbuildbot/stages/completion_stages.py
@@ -18,9 +18,7 @@ from chromite.cbuildbot.stages import generic_stages
 from chromite.cbuildbot.stages import sync_stages
 from chromite.lib import clactions
 from chromite.lib import cros_logging as logging
-from chromite.lib import git
 from chromite.lib import patch as cros_patch
-from chromite.lib import portage_util
 
 
 def GetBuilderSuccessMap(builder_run, overall_success):
@@ -485,11 +483,6 @@ class CommitQueueCompletionStage(MasterSlaveSyncCompletionStage):
   def HandleSuccess(self):
     if self._run.config.master:
       self.sync_stage.pool.SubmitPool(reason=constants.STRATEGY_CQ_SUCCESS)
-      # After submitting the pool, update the commit hashes for uprevved
-      # ebuilds.
-      manifest = git.ManifestCheckout.Cached(self._build_root)
-      portage_util.EBuild.UpdateCommitHashesForChanges(
-          self.sync_stage.pool.changes, self._build_root, manifest)
       if config_lib.IsPFQType(self._run.config.build_type):
         super(CommitQueueCompletionStage, self).HandleSuccess()
 
@@ -830,10 +823,17 @@ class PublishUprevChangesStage(generic_stages.BuilderStage):
     overlays, push_overlays = self._ExtractOverlays()
     assert push_overlays, 'push_overlays must be set to run this stage'
 
-    # If the build failed, we don't want to push our local changes, because
-    # they might include some CLs that failed. Instead, clean up our local
-    # changes and do a fresh uprev.
-    if not self.success:
+    # If we're a commit queue, we should clean out our local changes, resync,
+    # and reapply our uprevs. This is necessary so that 1) we are sure to point
+    # at the remote SHA1s, not our local SHA1s; 2) we can avoid doing a
+    # rebase; 3) in the case of failure, we don't submit the changes that were
+    # committed locally.
+    #
+    # If we're not a commit queue and the build succeeded, we can skip the
+    # cleanup here. This is a cheap trick so that the Chrome PFQ pushes its
+    # earlier uprev from the SyncChrome stage (it would be a bit tricky to
+    # replicate the uprev here, so we'll leave it alone).
+    if config_lib.IsCQType(self._run.config.build_type) or not self.success:
       # Clean up our root and sync down the latest changes that were
       # submitted.
       commands.BuildRootGitCleanup(self._build_root)
diff --git a/lib/portage_util.py b/lib/portage_util.py
index 41db7642b..c0fe77fd7 100644
--- a/lib/portage_util.py
+++ b/lib/portage_util.py
@@ -857,36 +857,6 @@ class EBuild(object):
 
     return ebuild_projects
 
-  @classmethod
-  def UpdateCommitHashesForChanges(cls, changes, buildroot, manifest):
-    """Updates the commit hashes for the EBuilds uprevved in changes.
-
-    Args:
-      changes: Changes from Gerrit that are being pushed.
-      buildroot: Path to root of build directory.
-      manifest: git.ManifestCheckout object.
-    """
-    path_sha1s = {}
-    overlay_list = FindOverlays(constants.BOTH_OVERLAYS, buildroot=buildroot)
-    ebuild_paths = cls._GetEBuildPaths(buildroot, manifest, overlay_list,
-                                       changes)
-    for ebuild, paths in ebuild_paths.iteritems():
-      # Calculate any SHA1s that are not already in path_sha1s.
-      for path in set(paths).difference(path_sha1s):
-        path_sha1s[path] = cls._GetSHA1ForPath(manifest, path)
-
-      sha1s = [path_sha1s[path] for path in paths]
-      logging.info('Updating ebuild for package %s with commit hashes %r',
-                   ebuild.package, sha1s)
-      updates = dict(CROS_WORKON_COMMIT=cls.FormatBashArray(sha1s))
-      EBuild.UpdateEBuild(ebuild.ebuild_path, updates)
-
-    # Commit any changes to all overlays.
-    for overlay in overlay_list:
-      if EBuild.GitRepoHasChanges(overlay):
-        EBuild.CommitChange('Updating commit hashes in ebuilds '
-                            'to match remote repository.', overlay=overlay)
-
 
 class PortageDBException(Exception):
   """Generic PortageDB error."""
diff --git a/lib/portage_util_unittest.py b/lib/portage_util_unittest.py
index 216c7f734..31833032b 100644
--- a/lib/portage_util_unittest.py
+++ b/lib/portage_util_unittest.py
@@ -7,7 +7,6 @@
 from __future__ import print_function
 
 import cStringIO
-import mock
 import os
 
 from chromite.cbuildbot import constants
@@ -454,33 +453,6 @@ class EBuildRevWorkonTest(cros_test_lib.MockTempDirTestCase):
     self.m_ebuild.CommitChange(mock_message, '.')
     m.assert_called_once_with('.', ['commit', '-a', '-m', 'Commitme'])
 
-  def testUpdateCommitHashesForChanges(self):
-    """Tests that we can update the commit hashes for changes correctly."""
-    build_root = 'fakebuildroot'
-    overlays = ['public_overlay']
-    changes = ['fake change']
-    paths = ['fake_path1', 'fake_path2']
-    sha1s = ['sha1', 'shaaaaaaaaaaaaaaaa2']
-    path_ebuilds = {self.m_ebuild: paths}
-
-    self.PatchObject(portage_util, 'FindOverlays', return_value=overlays)
-    self.PatchObject(portage_util.EBuild, '_GetEBuildPaths',
-                     return_value=path_ebuilds)
-    self.PatchObject(portage_util.EBuild, '_GetSHA1ForPath',
-                     side_effect=reversed(sha1s))
-    update_mock = self.PatchObject(portage_util.EBuild, 'UpdateEBuild')
-    self.PatchObject(portage_util.EBuild, 'GitRepoHasChanges',
-                     return_value=True)
-    commit_mock = self.PatchObject(portage_util.EBuild, 'CommitChange')
-
-    portage_util.EBuild.UpdateCommitHashesForChanges(changes, build_root,
-                                                     MANIFEST)
-
-    update_mock.assert_called_once_with(
-        self.m_ebuild.ebuild_path,
-        {'CROS_WORKON_COMMIT': '(%s)' % ' '.join('"%s"' % x for x in sha1s)})
-    commit_mock.assert_called_once_with(mock.ANY, overlay=overlays[0])
-
   def testGitRepoHasChanges(self):
     """Tests that GitRepoHasChanges works correctly."""
     git.RunGit(self.tempdir,