Add named options to push-to-trunk script.

Also make sure that on exceptions from the test infrastructure there is no retry.

BUG=
R=ulan@chromium.org

Review URL: https://codereview.chromium.org/114203002

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@18313 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

tools/push-to-trunk/auto_roll.py

@@ -38,6 +38,12 @@ CONFIG = {
 }
 
 
+class AutoRollOptions(CommonOptions):
+  def __init__(self, options):
+    super(AutoRollOptions, self).__init__(options)
+    self.requires_editor = False
+
+
 class Preparation(Step):
   MESSAGE = "Preparation."
 
@@ -137,7 +143,7 @@ def Main():
     print "You need to specify the chromium src location and a reviewer."
     parser.print_help()
     return 1
-  RunAutoRoll(CONFIG, options)
+  RunAutoRoll(CONFIG, AutoRollOptions(options))
 
 if __name__ == "__main__":
   sys.exit(Main())

tools/push-to-trunk/common_includes.py

@@ -212,6 +212,19 @@ class SideEffectHandler(object):
 DEFAULT_SIDE_EFFECT_HANDLER = SideEffectHandler()
 
 
+class NoRetryException(Exception):
+  pass
+
+class CommonOptions(object):
+  def __init__(self, options, manual=True):
+    self.requires_editor = True
+    self.wait_for_lgtm = True
+    self.s = options.s
+    self.force_readline_defaults = not manual
+    self.force_upload = not manual
+    self.manual = manual
+
+
 class Step(object):
   def __init__(self, text, requires, number, config, state, options, handler):
     self._text = text
@@ -225,16 +238,11 @@ class Step(object):
     assert self._config is not None
     assert self._state is not None
     assert self._side_effect_handler is not None
+    assert isinstance(options, CommonOptions)
 
   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)
@@ -263,6 +271,8 @@ class Step(object):
       got_exception = False
       try:
         result = cb()
+      except NoRetryException, e:
+        raise e
       except Exception:
         got_exception = True
       if got_exception or retry_on(result):
@@ -277,7 +287,7 @@ class Step(object):
 
   def ReadLine(self, default=None):
     # Don't prompt in forced mode.
-    if not self.IsManual() and default is not None:
+    if self._options.force_readline_defaults and default is not None:
       print "%s (forced)" % default
       return default
     else:
@@ -288,7 +298,7 @@ class Step(object):
     return self.Retry(cmd, retry_on, [5, 30])
 
   def Editor(self, args):
-    if not self.IsForced():
+    if self._options.requires_editor:
       return self._side_effect_handler.Command(os.environ["EDITOR"], args,
                                                pipe=False)
 
@@ -307,7 +317,7 @@ class Step(object):
     raise Exception(msg)
 
   def DieNoManualMode(self, msg=""):
-    if not self.IsManual():
+    if not self._options.manual:
       msg = msg or "Only available in manual mode."
       self.Die(msg)
 
@@ -348,7 +358,7 @@ class Step(object):
       self.Die("This is not a git checkout, this script won't work for you.")
 
     # Cancel if EDITOR is unset or not executable.
-    if (not self.IsForced() and (not os.environ.get("EDITOR") or
+    if (self._options.requires_editor 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.")
 
@@ -418,7 +428,7 @@ class Step(object):
     answer = ""
     while answer != "LGTM":
       print "> ",
-      answer = self.ReadLine("LGTM" if self.IsForced() else None)
+      answer = self.ReadLine(None if self._options.wait_for_lgtm else "LGTM")
       if answer != "LGTM":
         print "That was not 'LGTM'."
 
@@ -454,7 +464,7 @@ class UploadStep(Step):
       print "Please enter the email address of a V8 reviewer for your patch: ",
       self.DieNoManualMode("A reviewer must be specified in forced mode.")
       reviewer = self.ReadLine()
-    force_flag = " -f" if not self.IsManual() else ""
+    force_flag = " -f" if self._options.force_upload 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.

tools/push-to-trunk/push_to_trunk.py

@@ -52,6 +52,16 @@ CONFIG = {
 }
 
 
+class PushToTrunkOptions(CommonOptions):
+  def __init__(self, options):
+    super(PushToTrunkOptions, self).__init__(options, options.m)
+    self.requires_editor = not options.f
+    self.wait_for_lgtm = not options.f
+    self.tbr_commit = not options.m
+    self.l = options.l
+    self.r = options.r
+    self.c = options.c
+
 class Preparation(Step):
   MESSAGE = "Preparation."
 
@@ -214,7 +224,7 @@ class CommitLocal(Step):
 
     # 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 ""
+    review = "\n\nTBR=%s" % self._options.r if self._options.tbr_commit else ""
     if self.Git("commit -a -m \"%s%s\"" % (prep_commit_msg, review)) is None:
       self.Die("'git commit -a' failed.")
 
@@ -441,7 +451,7 @@ class UploadCL(Step):
     ver = "%s.%s.%s" % (self._state["major"],
                         self._state["minor"],
                         self._state["build"])
-    if self._options and self._options.r:
+    if self._options.r:
       print "Using account %s for review." % self._options.r
       rev = self._options.r
     else:
@@ -451,7 +461,7 @@ class UploadCL(Step):
     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 not self.IsManual() else ""
+    force_flag = " -f" if self._options.force_upload 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."
@@ -569,7 +579,7 @@ def Main():
   if not ProcessOptions(options):
     parser.print_help()
     return 1
-  RunPushToTrunk(CONFIG, options)
+  RunPushToTrunk(CONFIG, PushToTrunkOptions(options))
 
 if __name__ == "__main__":
   sys.exit(Main())

tools/push-to-trunk/test_scripts.py

@@ -35,8 +35,9 @@ from common_includes import *
 import push_to_trunk
 from push_to_trunk import *
 import auto_roll
-from auto_roll import FetchLatestRevision
+from auto_roll import AutoRollOptions
 from auto_roll import CheckLastPush
+from auto_roll import FetchLatestRevision
 
 
 TEST_CONFIG = {
@@ -230,14 +231,14 @@ class SimpleMock(object):
     # The number of arguments in the expectation must match the actual
     # arguments.
     if len(args) > len(expected_call):
-      raise Exception("When calling %s with arguments, the expectations "
-                      "must consist of at least as many arguments.")
+      raise NoRetryException("When calling %s with arguments, the "
+          "expectations must consist of at least as many arguments.")
 
     # Compare expected and actual arguments.
     for (expected_arg, actual_arg) in zip(expected_call, args):
       if expected_arg != actual_arg:
-        raise Exception("Expected: %s - Actual: %s"
-                        % (expected_arg, actual_arg))
+        raise NoRetryException("Expected: %s - Actual: %s"
+                               % (expected_arg, actual_arg))
 
     # The expectation list contains a mandatory return value and an optional
     # callback for checking the context at the time of the call.
@@ -252,8 +253,8 @@ class SimpleMock(object):
 
   def AssertFinished(self):
     if self._index < len(self._recipe) -1:
-      raise Exception("Called %s too seldom: %d vs. %d"
-                      % (self._name, self._index, len(self._recipe)))
+      raise NoRetryException("Called %s too seldom: %d vs. %d"
+                             % (self._name, self._index, len(self._recipe)))
 
 
 class ScriptTest(unittest.TestCase):
@@ -278,7 +279,7 @@ class ScriptTest(unittest.TestCase):
 
   def MakeStep(self, step_class=Step, state=None, options=None):
     """Convenience wrapper."""
-    options = options or MakeOptions()
+    options = options or CommonOptions(MakeOptions())
     return MakeStep(step_class=step_class, number=0, state=state,
                     config=TEST_CONFIG, options=options,
                     side_effect_handler=self)
@@ -710,7 +711,7 @@ Performance and stability improvements on all platforms."""
     options = MakeOptions(f=force, m=manual,
                           r="reviewer@chromium.org" if not manual else None,
                           c = TEST_CONFIG[CHROMIUM])
-    RunPushToTrunk(TEST_CONFIG, options, self)
+    RunPushToTrunk(TEST_CONFIG, PushToTrunkOptions(options), self)
 
     deps = FileToText(TEST_CONFIG[DEPS_FILE])
     self.assertTrue(re.search("\"v8_revision\": \"123456\"", deps))
@@ -759,7 +760,9 @@ Performance and stability improvements on all platforms."""
       ["svn log -1 --oneline ChangeLog", "r65 | Prepare push to trunk..."],
     ])
 
-    auto_roll.RunAutoRoll(TEST_CONFIG, MakeOptions(m=False, f=True), self)
+    auto_roll.RunAutoRoll(TEST_CONFIG,
+                          AutoRollOptions(MakeOptions(m=False, f=True)),
+                          self)
 
     self.assertEquals("100", self.MakeStep().Restore("lkgr"))
     self.assertEquals("101", self.MakeStep().Restore("latest"))
@@ -768,7 +771,7 @@ Performance and stability improvements on all platforms."""
 class SystemTest(unittest.TestCase):
   def testReload(self):
     step = MakeStep(step_class=PrepareChangeLog, number=0, state={}, config={},
-                    options=None,
+                    options=CommonOptions(MakeOptions()),
                     side_effect_handler=DEFAULT_SIDE_EFFECT_HANDLER)
     body = step.Reload(
 """------------------------------------------------------------------------