Refactoring: Deprecate optparse in push and merge scripts.

- Deprecate optparse with argparse
- The tests include now options parsing by default: each test specifies the command-line args to parse rather than the options directly

This CL is split off from https://codereview.chromium.org/173983002/

TBR=ulan@chromium.org

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

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

diff --git a/tools/push-to-trunk/auto_roll.py b/tools/push-to-trunk/auto_roll.py
index fc50afb..2c71041 100755 (executable)
--- a/tools/push-to-trunk/auto_roll.py
+++ b/tools/push-to-trunk/auto_roll.py
@@ -26,8 +26,8 @@
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+import argparse
 import json
-import optparse
 import os
 import re
 import sys
@@ -180,28 +180,28 @@ def RunAutoRoll(config,
 
 
 def BuildOptions():
-  result = optparse.OptionParser()
-  result.add_option("-a", "--author", dest="a",
-                    help=("Specify the author email used for rietveld."))
-  result.add_option("-c", "--chromium", dest="c",
-                    help=("Specify the path to your Chromium src/ "
-                          "directory to automate the V8 roll."))
-  result.add_option("-p", "--push",
-                    help="Push to trunk if possible. Dry run if unspecified.",
-                    default=False, action="store_true")
-  result.add_option("-r", "--reviewer",
-                    help=("Specify the account name to be used for reviews."))
-  result.add_option("-s", "--step", dest="s",
-                    help="Specify the step where to start work. Default: 0.",
-                    default=0, type="int")
-  result.add_option("--status-password",
-                    help="A file with the password to the status app.")
-  return result
+  parser = argparse.ArgumentParser()
+  parser.add_argument("-a", "--author", dest="a",
+                      help="The author email used for rietveld.")
+  parser.add_argument("-c", "--chromium", dest="c",
+                      help=("The path to your Chromium src/ directory to "
+                            "automate the V8 roll."))
+  parser.add_argument("-p", "--push",
+                      help="Push to trunk if possible. Dry run if unspecified.",
+                      default=False, action="store_true")
+  parser.add_argument("-r", "--reviewer",
+                      help="The account name to be used for reviews.")
+  parser.add_argument("-s", "--step", dest="s",
+                      help="Specify the step where to start work. Default: 0.",
+                      default=0, type=int)
+  parser.add_argument("--status-password",
+                      help="A file with the password to the status app.")
+  return parser
 
 
 def Main():
   parser = BuildOptions()
-  (options, args) = parser.parse_args()
+  options = parser.parse_args()
   if not options.a or not options.c or not options.reviewer:
     print "You need to specify author, chromium src location and reviewer."
     parser.print_help()

diff --git a/tools/push-to-trunk/merge_to_branch.py b/tools/push-to-trunk/merge_to_branch.py
index 5f0b6a6..5f2e573 100755 (executable)
--- a/tools/push-to-trunk/merge_to_branch.py
+++ b/tools/push-to-trunk/merge_to_branch.py
@@ -26,8 +26,8 @@
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+import argparse
 from collections import OrderedDict
-import optparse
 import sys
 
 from common_includes import *
@@ -51,7 +51,7 @@ CONFIG = {
 
 
 class MergeToBranchOptions(CommonOptions):
-  def __init__(self, options, args):
+  def __init__(self, options):
     super(MergeToBranchOptions, self).__init__(options, True)
     self.requires_editor = True
     self.wait_for_lgtm = True
@@ -60,7 +60,8 @@ class MergeToBranchOptions(CommonOptions):
     self.revert = getattr(options, "r", False)
     self.revert_bleeding_edge = getattr(options, "revert_bleeding_edge", False)
     self.patch = getattr(options, "p", "")
-    self.args = args
+    self.branch = options.branch
+    self.revisions = options.revisions
 
 
 class Preparation(Step):
@@ -77,9 +78,8 @@ class Preparation(Step):
     self.InitialEnvironmentChecks()
     if self._options.revert_bleeding_edge:
       self["merge_to_branch"] = "bleeding_edge"
-    elif self._options.args[0]:
-      self["merge_to_branch"] = self._options.args[0]
-      self._options.args = self._options.args[1:]
+    elif self._options.branch:
+      self["merge_to_branch"] = self._options.branch
     else:
       self.Die("Please specify a branch to merge to")
 
@@ -99,7 +99,8 @@ class SearchArchitecturePorts(Step):
   MESSAGE = "Search for corresponding architecture ports."
 
   def RunStep(self):
-    self["full_revision_list"] = list(OrderedDict.fromkeys(self._options.args))
+    self["full_revision_list"] = list(OrderedDict.fromkeys(
+        self._options.revisions))
     port_revision_list = []
     for revision in self["full_revision_list"]:
       # Search for commits which matches the "Port rXXX" pattern.
@@ -310,53 +311,53 @@ def RunMergeToBranch(config,
 
 
 def BuildOptions():
-  result = optparse.OptionParser()
-  result.set_usage("""%prog [OPTIONS]... [BRANCH] [REVISION]...
-
-Performs the necessary steps to merge revisions from bleeding_edge
-to other branches, including trunk.""")
-  result.add_option("-f",
-                    help="Delete sentinel file.",
-                    default=False, action="store_true")
-  result.add_option("-m", "--message",
-                    help="Specify a commit message for the patch.")
-  result.add_option("-r", "--revert",
-                    help="Revert specified patches.",
-                    default=False, action="store_true")
-  result.add_option("-R", "--revert-bleeding-edge",
-                    help="Revert specified patches from bleeding edge.",
-                    default=False, action="store_true")
-  result.add_option("-p", "--patch", dest="p",
-                    help="Specify a patch file to apply as part of the merge.")
-  result.add_option("-s", "--step", dest="s",
-                    help="Specify the step where to start work. Default: 0.",
-                    default=0, type="int")
-  return result
-
-
-def ProcessOptions(options, args):
-  revert_from_bleeding_edge = 1 if options.revert_bleeding_edge else 0
-  min_exp_args = 2 - revert_from_bleeding_edge
-  if len(args) < min_exp_args:
-    if not options.p:
+  parser = argparse.ArgumentParser(
+      description=("Performs the necessary steps to merge revisions from "
+                   "bleeding_edge to other branches, including trunk."))
+  group = parser.add_mutually_exclusive_group(required=True)
+  group.add_argument("--branch", help="The branch to merge to.")
+  group.add_argument("-R", "--revert-bleeding-edge",
+                     help="Revert specified patches from bleeding edge.",
+                     default=False, action="store_true")
+  parser.add_argument("revisions", nargs="*",
+                      help="The revisions to merge.")
+  parser.add_argument("-a", "--author", default="",
+                       help="The author email used for rietveld.")
+  parser.add_argument("-f",
+                      help="Delete sentinel file.",
+                      default=False, action="store_true")
+  parser.add_argument("-m", "--message",
+                      help="A commit message for the patch.")
+  parser.add_argument("-r", "--revert",
+                      help="Revert specified patches.",
+                      default=False, action="store_true")
+  parser.add_argument("-p", "--patch", dest="p",
+                      help="A patch file to apply as part of the merge.")
+  parser.add_argument("-s", "--step", dest="s",
+                      help="The step where to start work. Default: 0.",
+                      default=0, type=int)
+  return parser
+
+
+def ProcessOptions(options):
+  # TODO(machenbach): Add a test that covers revert from bleeding_edge
+  if len(options.revisions) < 1:
+    if not options.patch:
       print "Either a patch file or revision numbers must be specified"
       return False
     if not options.message:
       print "You must specify a merge comment if no patches are specified"
       return False
-  if options.s < 0:
-    print "Bad step number %d" % options.s
-    return False
   return True
 
 
 def Main():
   parser = BuildOptions()
-  (options, args) = parser.parse_args()
-  if not ProcessOptions(options, args):
+  options = parser.parse_args()
+  if not ProcessOptions(options):
     parser.print_help()
     return 1
-  RunMergeToBranch(CONFIG, MergeToBranchOptions(options, args))
+  RunMergeToBranch(CONFIG, MergeToBranchOptions(options))
 
 if __name__ == "__main__":
   sys.exit(Main())

diff --git a/tools/push-to-trunk/push_to_trunk.py b/tools/push-to-trunk/push_to_trunk.py
index 0dc1a07..ffef74d 100755 (executable)
--- a/tools/push-to-trunk/push_to_trunk.py
+++ b/tools/push-to-trunk/push_to_trunk.py
@@ -26,7 +26,7 @@
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-import optparse
+import argparse
 import sys
 import tempfile
 import urllib2
@@ -545,32 +545,31 @@ def RunPushToTrunk(config,
 
 
 def BuildOptions():
-  result = optparse.OptionParser()
-  result.add_option("-a", "--author", dest="a",
-                    help=("Specify the author email used for rietveld."))
-  result.add_option("-b", "--last-bleeding-edge", dest="b",
-                    help=("Manually specify the git commit ID of the last "
-                          "bleeding edge revision that was pushed to trunk. "
-                          "This is used for the auto-generated ChangeLog "
-                          "entry."))
-  result.add_option("-c", "--chromium", dest="c",
-                    help=("Specify the path to your Chromium src/ "
-                          "directory to automate the V8 roll."))
-  result.add_option("-f", "--force", dest="f",
-                    help="Don't prompt the user.",
-                    default=False, action="store_true")
-  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",
-                    help=("Specify the account name to be used for reviews."))
-  result.add_option("-s", "--step", dest="s",
-                    help="Specify the step where to start work. Default: 0.",
-                    default=0, type="int")
-  return result
+  parser = argparse.ArgumentParser()
+  group = parser.add_mutually_exclusive_group()
+  group.add_argument("-f", "--force", dest="f",
+                     help="Don't prompt the user.",
+                     default=False, action="store_true")
+  group.add_argument("-m", "--manual", dest="m",
+                     help="Prompt the user at every important step.",
+                     default=False, action="store_true")
+  parser.add_argument("-a", "--author", dest="a",
+                      help="The author email used for rietveld.")
+  parser.add_argument("-b", "--last-bleeding-edge", dest="b",
+                      help=("The git commit ID of the last bleeding edge "
+                            "revision that was pushed to trunk. This is used "
+                            "for the auto-generated ChangeLog entry."))
+  parser.add_argument("-c", "--chromium", dest="c",
+                      help=("The path to your Chromium src/ directory to "
+                            "automate the V8 roll."))
+  parser.add_argument("-l", "--last-push", dest="l",
+                      help="The git commit ID of the last push to trunk.")
+  parser.add_argument("-r", "--reviewer",
+                      help="The account name to be used for reviews.")
+  parser.add_argument("-s", "--step", dest="s",
+                      help="The step where to start work. Default: 0.",
+                      default=0, type=int)
+  return parser
 
 
 def ProcessOptions(options):
@@ -580,9 +579,6 @@ def ProcessOptions(options):
   if not options.m and not options.reviewer:
     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 not options.m and not options.c:
     print "A chromium checkout (-c) is required in (semi-)automatic mode."
     return False
@@ -594,7 +590,7 @@ def ProcessOptions(options):
 
 def Main():
   parser = BuildOptions()
-  (options, args) = parser.parse_args()
+  options = parser.parse_args()
   if not ProcessOptions(options):
     parser.print_help()
     return 1

diff --git a/tools/push-to-trunk/test_scripts.py b/tools/push-to-trunk/test_scripts.py
index 92aabf6..8e47d46 100644 (file)
--- a/tools/push-to-trunk/test_scripts.py
+++ b/tools/push-to-trunk/test_scripts.py
@@ -64,6 +64,12 @@ TEST_CONFIG = {
   TEMPORARY_PATCH_FILE: "/tmp/test-merge-to-branch-tempfile-temporary-patch",
 }
 
+AUTO_ROLL_ARGS = [
+  "-a", "author@chromium.org",
+  "-c", TEST_CONFIG[CHROMIUM],
+  "-r", "reviewer@chromium.org",
+]
+
 
 def MakeOptions(s=0, l=None, f=False, m=True, r=None, c=None, a=None,
                 status_password=None, revert_bleeding_edge=None, p=None):
@@ -740,9 +746,11 @@ Performance and stability improvements on all platforms.""", commit)
     if force:
       self.ExpectReadline([])
 
-    options = MakeOptions(f=force, m=manual, a="author@chromium.org",
-                          r="reviewer@chromium.org" if not manual else None,
-                          c = TEST_CONFIG[CHROMIUM])
+    args = ["-a", "author@chromium.org", "-c", TEST_CONFIG[CHROMIUM]]
+    if force: args.append("-f")
+    if manual: args.append("-m")
+    else: args += ["-r", "reviewer@chromium.org"]
+    options = push_to_trunk.BuildOptions().parse_args(args)
     RunPushToTrunk(TEST_CONFIG, PushToTrunkOptions(options), self)
 
     deps = FileToText(TEST_CONFIG[DEPS_FILE])
@@ -777,8 +785,8 @@ Performance and stability improvements on all platforms.""", commit)
     self.assertRaises(Exception, self.MakeStep(CheckLastPush, state=state).Run)
 
   def testAutoRoll(self):
-    status_password = self.MakeEmptyTempFile()
-    TextToFile("PW", status_password)
+    password = self.MakeEmptyTempFile()
+    TextToFile("PW", password)
     TEST_CONFIG[DOT_GIT_LOCATION] = self.MakeEmptyTempFile()
     TEST_CONFIG[SETTINGS_LOCATION] = "~/.doesnotexist"
 
@@ -807,8 +815,9 @@ Performance and stability improvements on all platforms.""", commit)
       ["svn find-rev push_hash", "65"],
     ])
 
-    auto_roll.RunAutoRoll(TEST_CONFIG, AutoRollOptions(
-        MakeOptions(status_password=status_password)), self)
+    options = auto_roll.BuildOptions().parse_args(
+        AUTO_ROLL_ARGS + ["--status-password", password])
+    auto_roll.RunAutoRoll(TEST_CONFIG, AutoRollOptions(options), self)
 
     state = json.loads(FileToText("%s-state.json"
                                   % TEST_CONFIG[PERSISTFILE_BASENAME]))
@@ -829,8 +838,9 @@ Performance and stability improvements on all platforms.""", commit)
       ["svn fetch", ""],
     ])
 
+    options = auto_roll.BuildOptions().parse_args(AUTO_ROLL_ARGS)
     def RunAutoRoll():
-      auto_roll.RunAutoRoll(TEST_CONFIG, AutoRollOptions(MakeOptions()), self)
+      auto_roll.RunAutoRoll(TEST_CONFIG, AutoRollOptions(options), self)
     self.assertRaises(Exception, RunAutoRoll)
 
   def testAutoRollStoppedByTreeStatus(self):
@@ -848,8 +858,9 @@ Performance and stability improvements on all platforms.""", commit)
       ["svn fetch", ""],
     ])
 
+    options = auto_roll.BuildOptions().parse_args(AUTO_ROLL_ARGS)
     def RunAutoRoll():
-      auto_roll.RunAutoRoll(TEST_CONFIG, AutoRollOptions(MakeOptions()), self)
+      auto_roll.RunAutoRoll(TEST_CONFIG, AutoRollOptions(options), self)
     self.assertRaises(Exception, RunAutoRoll)
 
   def testMergeToBranch(self):
@@ -966,21 +977,22 @@ LOG=N
       "LGTM",  # Enter LGTM for V8 CL.
     ])
 
-    options = MakeOptions(p=extra_patch, f=True)
     # r12345 and r34567 are patches. r23456 (included) and r45678 are the MIPS
     # ports of r12345. r56789 is the MIPS port of r34567.
-    args = ["trunk", "12345", "23456", "34567"]
-    self.assertTrue(merge_to_branch.ProcessOptions(options, args))
+    args = ["-f", "-p", extra_patch, "--branch", "trunk", "12345", "23456",
+            "34567"]
+    options = merge_to_branch.BuildOptions().parse_args(args)
+    self.assertTrue(merge_to_branch.ProcessOptions(options))
 
     # The first run of the script stops because of the svn being down.
     self.assertRaises(GitFailedException,
         lambda: RunMergeToBranch(TEST_CONFIG,
-                                 MergeToBranchOptions(options, args),
+                                 MergeToBranchOptions(options),
                                  self))
 
     # Test that state recovery after restarting the script works.
     options.s = 3
-    RunMergeToBranch(TEST_CONFIG, MergeToBranchOptions(options, args), self)
+    RunMergeToBranch(TEST_CONFIG, MergeToBranchOptions(options), self)
 
 
 class SystemTest(unittest.TestCase):