From 34943ca0c813c9869486e4fdc020d3c81104c659 Mon Sep 17 00:00:00 2001 From: "machenbach@chromium.org" Date: Wed, 4 Dec 2013 08:47:18 +0000 Subject: [PATCH] Add fully automated mode to push-to-trunk script. Now there are three modes to run the script: (1) default: semi-automated (2) manual (-m option), like in the old script (3) forced (-f option), no user input required no editor check R=ulan@chromium.org Review URL: https://codereview.chromium.org/102253002 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@18247 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- tools/push-to-trunk/auto_roll.py | 3 ++ tools/push-to-trunk/common_includes.py | 35 +++++++------- tools/push-to-trunk/push_to_trunk.py | 31 +++++++----- tools/push-to-trunk/test_scripts.py | 88 +++++++++++++++++++--------------- 4 files changed, 91 insertions(+), 66 deletions(-) diff --git a/tools/push-to-trunk/auto_roll.py b/tools/push-to-trunk/auto_roll.py index 895ae54..fa65ee2 100755 --- a/tools/push-to-trunk/auto_roll.py +++ b/tools/push-to-trunk/auto_roll.py @@ -96,6 +96,9 @@ def RunAutoRoll(config, def BuildOptions(): result = optparse.OptionParser() + result.add_option("-f", "--force", dest="f", + help="Don't prompt the user.", + default=True, action="store_true") result.add_option("-s", "--step", dest="s", help="Specify the step where to start work. Default: 0.", default=0, type="int") diff --git a/tools/push-to-trunk/common_includes.py b/tools/push-to-trunk/common_includes.py index ba23314..8d08f30 100644 --- a/tools/push-to-trunk/common_includes.py +++ b/tools/push-to-trunk/common_includes.py @@ -229,6 +229,12 @@ class Step(object): def Config(self, key): return self._config[key] + def IsForced(self): + return self._options and self._options.f + + def IsManual(self): + return self._options and self._options.m + def Run(self): if self._requires: self.RestoreIfUnset(self._requires) @@ -271,7 +277,7 @@ class Step(object): def ReadLine(self, default=None): # Don't prompt in forced mode. - if self._options and self._options.f and default is not None: + if not self.IsManual() and default is not None: print "%s (forced)" % default return default else: @@ -282,8 +288,9 @@ class Step(object): return self.Retry(cmd, retry_on, [5, 30]) def Editor(self, args): - return self._side_effect_handler.Command(os.environ["EDITOR"], args, - pipe=False) + if not self.IsForced(): + return self._side_effect_handler.Command(os.environ["EDITOR"], args, + pipe=False) def ReadURL(self, url, retry_on=None, wait_plan=None): wait_plan = wait_plan or [3, 60, 600] @@ -299,9 +306,9 @@ class Step(object): print "Exiting" raise Exception(msg) - def DieInForcedMode(self, msg=""): - if self._options and self._options.f: - msg = msg or "Not implemented in forced mode." + def DieNoManualMode(self, msg=""): + if not self.IsManual(): + msg = msg or "Only available in manual mode." self.Die(msg) def Confirm(self, msg): @@ -340,11 +347,9 @@ class Step(object): if not os.path.exists(self._config[DOT_GIT_LOCATION]): self.Die("This is not a git checkout, this script won't work for you.") - # TODO(machenbach): Don't use EDITOR in forced mode as soon as script is - # well tested. # Cancel if EDITOR is unset or not executable. - if (not os.environ.get("EDITOR") or - Command("which", os.environ["EDITOR"]) is None): + if (not self.IsForced() and (not os.environ.get("EDITOR") or + Command("which", os.environ["EDITOR"]) is None)): self.Die("Please set your EDITOR environment variable, you'll need it.") def CommonPrepare(self): @@ -413,9 +418,7 @@ class Step(object): answer = "" while answer != "LGTM": print "> ", - # TODO(machenbach): Add default="LGTM" to avoid prompt when script is - # well tested and when prepare push cl has TBR flag. - answer = self.ReadLine() + answer = self.ReadLine("LGTM" if self.IsForced() else None) if answer != "LGTM": print "That was not 'LGTM'." @@ -423,7 +426,7 @@ class Step(object): print("Applying the patch \"%s\" failed. Either type \"ABORT\", " "or resolve the conflicts, stage *all* touched files with " "'git add', and type \"RESOLVED\"") - self.DieInForcedMode() + self.DieNoManualMode() answer = "" while answer != "RESOLVED": if answer == "ABORT": @@ -449,9 +452,9 @@ class UploadStep(Step): reviewer = self._options.r else: print "Please enter the email address of a V8 reviewer for your patch: ", - self.DieInForcedMode("A reviewer must be specified in forced mode.") + self.DieNoManualMode("A reviewer must be specified in forced mode.") reviewer = self.ReadLine() - force_flag = " -f" if self._options.f else "" + force_flag = " -f" if not self.IsManual() else "" args = "cl upload -r \"%s\" --send-mail%s" % (reviewer, force_flag) # TODO(machenbach): Check output in forced mode. Verify that all required # base files were uploaded, if not retry. diff --git a/tools/push-to-trunk/push_to_trunk.py b/tools/push-to-trunk/push_to_trunk.py index 9c9c75f..58e2cb9 100755 --- a/tools/push-to-trunk/push_to_trunk.py +++ b/tools/push-to-trunk/push_to_trunk.py @@ -157,9 +157,6 @@ class EditChangeLog(Step): "entry, then edit its contents to your liking. When you're done, " "save the file and exit your EDITOR. ") self.ReadLine(default="") - - # TODO(machenbach): Don't use EDITOR in forced mode as soon as script is - # well tested. self.Editor(self.Config(CHANGELOG_ENTRY_FILE)) handle, new_changelog = tempfile.mkstemp() os.close(handle) @@ -214,7 +211,11 @@ class CommitLocal(Step): self._state["new_minor"], self._state["new_build"])) self.Persist("prep_commit_msg", prep_commit_msg) - if self.Git("commit -a -m \"%s\"" % prep_commit_msg) is None: + + # Include optional TBR only in the git command. The persisted commit + # message is used for finding the commit again later. + review = "\n\nTBR=%s" % self._options.r if not self.IsManual() else "" + if self.Git("commit -a -m \"%s%s\"" % (prep_commit_msg, review)) is None: self.Die("'git commit -a' failed.") @@ -364,7 +365,7 @@ class CommitSVN(Step): print("Sorry, grepping for the SVN revision failed. Please look for it " "in the last command's output above and provide it manually (just " "the number, without the leading \"r\").") - self.DieInForcedMode("Can't prompt in forced mode.") + self.DieNoManualMode("Can't prompt in forced mode.") while not trunk_revision: print "> ", trunk_revision = self.ReadLine() @@ -389,7 +390,7 @@ class CheckChromium(Step): def Run(self): chrome_path = self._options.c if not chrome_path: - self.DieInForcedMode("Please specify the path to a Chromium checkout in " + self.DieNoManualMode("Please specify the path to a Chromium checkout in " "forced mode.") print ("Do you have a \"NewGit\" Chromium checkout and want " "this script to automate creation of the roll CL? If yes, enter the " @@ -457,12 +458,12 @@ class UploadCL(Step): rev = self._options.r else: print "Please enter the email address of a reviewer for the roll CL: ", - self.DieInForcedMode("A reviewer must be specified in forced mode.") + self.DieNoManualMode("A reviewer must be specified in forced mode.") rev = self.ReadLine() args = "commit -am \"Update V8 to version %s.\n\nTBR=%s\"" % (ver, rev) if self.Git(args) is None: self.Die("'git commit' failed.") - force_flag = " -f" if self._options.f else "" + force_flag = " -f" if not self.IsManual() else "" if self.Git("cl upload --send-mail%s" % force_flag, pipe=False) is None: self.Die("'git cl upload' failed, please try again.") print "CL uploaded." @@ -547,6 +548,9 @@ def BuildOptions(): result.add_option("-l", "--last-push", dest="l", help=("Manually specify the git commit ID " "of the last push to trunk.")) + result.add_option("-m", "--manual", dest="m", + help="Prompt the user at every important step.", + default=False, action="store_true") result.add_option("-r", "--reviewer", dest="r", help=("Specify the account name to be used for reviews.")) result.add_option("-s", "--step", dest="s", @@ -559,11 +563,14 @@ def ProcessOptions(options): if options.s < 0: print "Bad step number %d" % options.s return False - if options.f and not options.r: - print "A reviewer (-r) is required in forced mode." + if not options.m and not options.r: + print "A reviewer (-r) is required in (semi-)automatic mode." + return False + if options.f and options.m: + print "Manual and forced mode cannot be combined." return False - if options.f and not options.c: - print "A chromium checkout (-c) is required in forced mode." + if not options.m and not options.c: + print "A chromium checkout (-c) is required in (semi-)automatic mode." return False return True diff --git a/tools/push-to-trunk/test_scripts.py b/tools/push-to-trunk/test_scripts.py index 0262fc0..ce90192 100644 --- a/tools/push-to-trunk/test_scripts.py +++ b/tools/push-to-trunk/test_scripts.py @@ -53,6 +53,20 @@ TEST_CONFIG = { } +def MakeOptions(s=0, l=None, f=False, m=True, r=None, c=None): + """Convenience wrapper.""" + class Options(object): + pass + options = Options() + options.s = s + options.l = l + options.f = f + options.m = m + options.r = r + options.c = c + return options + + class ToplevelTest(unittest.TestCase): def testMakeComment(self): self.assertEquals("# Line 1\n# Line 2\n#", @@ -260,12 +274,15 @@ class ScriptTest(unittest.TestCase): f.write("#define IS_CANDIDATE_VERSION 0\n") return name - def MakeStep(self, step_class=Step, state=None): + def MakeStep(self, step_class=Step, state=None, options=None): """Convenience wrapper.""" + options = options or MakeOptions() return MakeStep(step_class=step_class, number=0, state=state, - config=TEST_CONFIG, options=None, side_effect_handler=self) + config=TEST_CONFIG, options=options, + side_effect_handler=self) def GitMock(self, cmd, args="", pipe=True): + print "%s %s" % (cmd, args) return self._git_mock.Call(args) def LogMock(self, cmd, args=""): @@ -555,7 +572,7 @@ class ScriptTest(unittest.TestCase): patch = FileToText(TEST_CONFIG[ PATCH_FILE]) self.assertTrue(re.search(r"patch content", patch)) - def _PushToTrunk(self, force=False): + def _PushToTrunk(self, force=False, manual=False): TEST_CONFIG[DOT_GIT_LOCATION] = self.MakeEmptyTempFile() TEST_CONFIG[VERSION_FILE] = self.MakeTempVersionFile() TEST_CONFIG[CHANGELOG_ENTRY_FILE] = self.MakeEmptyTempFile() @@ -593,7 +610,8 @@ class ScriptTest(unittest.TestCase): self.assertTrue(re.search(r"#define PATCH_LEVEL\s+0", version)) self.assertTrue(re.search(r"#define IS_CANDIDATE_VERSION\s+0", version)) - force_flag = " -f" if force else "" + force_flag = " -f" if not manual else "" + review_suffix = "\n\nTBR=reviewer@chromium.org" if not manual else "" self.ExpectGit([ ["status -s -uno", ""], ["status -s -b -uno", "## some_branch\n"], @@ -610,7 +628,7 @@ class ScriptTest(unittest.TestCase): ["log -1 rev1 --format=\"%B\"", "Text\nLOG=YES\nBUG=v8:321\nText\n"], ["log -1 rev1 --format=\"%an\"", "author1@chromium.org\n"], [("commit -a -m \"Prepare push to trunk. " - "Now working on version 3.22.6.\""), + "Now working on version 3.22.6.%s\"" % review_suffix), " 2 files changed\n", CheckPreparePush], ["cl upload -r \"reviewer@chromium.org\" --send-mail%s" % force_flag, @@ -641,32 +659,33 @@ class ScriptTest(unittest.TestCase): ["branch -D %s" % TEST_CONFIG[BRANCHNAME], ""], ["branch -D %s" % TEST_CONFIG[TRUNKBRANCH], ""], ]) - self.ExpectReadline([ - "Y", # Confirm last push. - "", # Open editor. - "Y", # Increment build number. - "reviewer@chromium.org", # V8 reviewer. - "LGTX", # Enter LGTM for V8 CL (wrong). - "LGTM", # Enter LGTM for V8 CL. - "Y", # Sanity check. - "reviewer@chromium.org", # Chromium reviewer. - ]) - if force: - # TODO(machenbach): The lgtm for the prepare push is just temporary. - # There should be no user input in "force" mode. + + # Expected keyboard input in manual mode: + if manual: self.ExpectReadline([ + "Y", # Confirm last push. + "", # Open editor. + "Y", # Increment build number. + "reviewer@chromium.org", # V8 reviewer. + "LGTX", # Enter LGTM for V8 CL (wrong). "LGTM", # Enter LGTM for V8 CL. + "Y", # Sanity check. + "reviewer@chromium.org", # Chromium reviewer. ]) - class Options( object ): - pass + # Expected keyboard input in semi-automatic mode: + if not manual and not force: + self.ExpectReadline([ + "LGTM", # Enter LGTM for V8 CL. + ]) - options = Options() - options.s = 0 - options.l = None - options.f = force - options.r = "reviewer@chromium.org" if force else None - options.c = TEST_CONFIG[CHROMIUM] + # No keyboard input in forced mode: + if force: + self.ExpectReadline([]) + + options = MakeOptions(f=force, m=manual, + r="reviewer@chromium.org" if not manual else None, + c = TEST_CONFIG[CHROMIUM]) RunPushToTrunk(TEST_CONFIG, options, self) deps = FileToText(TEST_CONFIG[DEPS_FILE]) @@ -681,7 +700,10 @@ class ScriptTest(unittest.TestCase): # since the git command that merges to the bleeding edge branch is mocked # out. - def testPushToTrunk(self): + def testPushToTrunkManual(self): + self._PushToTrunk(manual=True) + + def testPushToTrunkSemiAutomatic(self): self._PushToTrunk() def testPushToTrunkForced(self): @@ -690,9 +712,6 @@ class ScriptTest(unittest.TestCase): def testAutoRoll(self): TEST_CONFIG[DOT_GIT_LOCATION] = self.MakeEmptyTempFile() - # TODO(machenbach): Get rid of the editor check in automatic mode. - os.environ["EDITOR"] = "vi" - self.ExpectReadURL([ ["https://v8-status.appspot.com/lkgr", Exception("Network problem")], ["https://v8-status.appspot.com/lkgr", "100"], @@ -705,14 +724,7 @@ class ScriptTest(unittest.TestCase): ["svn log -1 --oneline", "r101 | Text"], ]) - # TODO(machenbach): Make a convenience wrapper for this. - class Options( object ): - pass - - options = Options() - options.s = 0 - - auto_roll.RunAutoRoll(TEST_CONFIG, options, self) + auto_roll.RunAutoRoll(TEST_CONFIG, MakeOptions(m=False, f=True), self) self.assertEquals("100", self.MakeStep().Restore("lkgr")) self.assertEquals("101", self.MakeStep().Restore("latest")) -- 2.7.4