binman: Convert to use ArgumentParser

This class is the new way to handle arguments in Python. Convert binman
over to use it. At the same time, introduce commands so that we can
separate out the different parts of binman functionality.

Signed-off-by: Simon Glass <sjg@chromium.org>

5 files changed, 125 insertions, 129 deletions

diff --git a/tools/binman/README b/tools/binman/README
index ef62d1f1ec..fe734c1e5f 100644
--- a/tools/binman/README
+++ b/tools/binman/README
@@ -187,7 +187,7 @@ First install prerequisites, e.g.
 
 Type:
 
-	binman -b <board_name>
+	binman build -b <board_name>
 
 to build an image for a board. The board name is the same name used when
 configuring U-Boot (e.g. for sandbox_defconfig the board name is 'sandbox').
@@ -195,7 +195,7 @@ Binman assumes that the input files for the build are in ../b/<board_name>.
 
 Or you can specify this explicitly:
 
-	binman -I <build_path>
+	binman build -I <build_path>
 
 where <build_path> is the build directory containing the output of the U-Boot
 build.
@@ -483,7 +483,7 @@ Entry Documentation
 For details on the various entry types supported by binman and how to use them,
 see README.entries. This is generated from the source code using:
 
-	binman -E >tools/binman/README.entries
+	binman entry-docs >tools/binman/README.entries
 
 
 Hashing Entries
@@ -715,7 +715,7 @@ Code coverage
 -------------
 
 Binman is a critical tool and is designed to be very testable. Entry
-implementations target 100% test coverage. Run 'binman -T' to check this.
+implementations target 100% test coverage. Run 'binman test -T' to check this.
 
 To enable Python test coverage on Debian-type distributions (e.g. Ubuntu):
 
diff --git a/tools/binman/binman.py b/tools/binman/binman.py
index 613aad5c45..8bd5868df2 100755
--- a/tools/binman/binman.py
+++ b/tools/binman/binman.py
@@ -20,14 +20,15 @@ import sys
 import traceback
 import unittest
 
-# Bring in the patman and dtoc libraries
+# Bring in the patman and dtoc libraries (but don't override the first path
+# in PYTHONPATH)
 our_path = os.path.dirname(os.path.realpath(__file__))
 for dirname in ['../patman', '../dtoc', '..', '../concurrencytest']:
-    sys.path.insert(0, os.path.join(our_path, dirname))
+    sys.path.insert(2, os.path.join(our_path, dirname))
 
 # Bring in the libfdt module
-sys.path.insert(0, 'scripts/dtc/pylibfdt')
-sys.path.insert(0, os.path.join(our_path,
+sys.path.insert(2, 'scripts/dtc/pylibfdt')
+sys.path.insert(2, os.path.join(our_path,
                 '../../build-sandbox_spl/scripts/dtc/pylibfdt'))
 
 # When running under python-coverage on Ubuntu 16.04, the dist-packages
@@ -59,7 +60,7 @@ def RunTests(debug, verbosity, processes, test_preserve_dirs, args, toolpath):
             on the command line.
         processes: Number of processes to use to run tests (None=same as #CPUs)
         args: List of positional args provided to binman. This can hold a test
-            name to execute (as in 'binman -t testSections', for example)
+            name to execute (as in 'binman test testSections', for example)
         toolpath: List of paths to use for tools
     """
     import cbfs_util_test
@@ -98,7 +99,7 @@ def RunTests(debug, verbosity, processes, test_preserve_dirs, args, toolpath):
             setup_test_args = getattr(module, 'setup_test_args')
             setup_test_args(preserve_indir=test_preserve_dirs,
                 preserve_outdirs=test_preserve_dirs and test_name is not None,
-                toolpath=toolpath)
+                toolpath=toolpath, verbosity=verbosity)
         if test_name:
             try:
                 suite.addTests(loader.loadTestsFromName(test_name, module))
@@ -158,37 +159,36 @@ def RunTestCoverage():
                    for item in glob_list if '_testing' not in item])
     test_util.RunTestCoverage('tools/binman/binman.py', None,
             ['*test*', '*binman.py', 'tools/patman/*', 'tools/dtoc/*'],
-            options.build_dir, all_set)
+            args.build_dir, all_set)
 
-def RunBinman(options, args):
+def RunBinman(args):
     """Main entry point to binman once arguments are parsed
 
     Args:
-        options: Command-line options
-        args: Non-option arguments
+        args: Command line arguments Namespace object
     """
     ret_code = 0
 
-    if not options.debug:
+    if not args.debug:
         sys.tracebacklimit = 0
 
-    if options.test:
-        ret_code = RunTests(options.debug, options.verbosity, options.processes,
-                            options.test_preserve_dirs, args[1:],
-                            options.toolpath)
-
-    elif options.test_coverage:
-        RunTestCoverage()
+    if args.cmd == 'test':
+        if args.test_coverage:
+            RunTestCoverage()
+        else:
+            ret_code = RunTests(args.debug, args.verbosity, args.processes,
+                                args.test_preserve_dirs, args.tests,
+                                args.toolpath)
 
-    elif options.entry_docs:
+    elif args.cmd == 'entry-docs':
         control.WriteEntryDocs(GetEntryModules())
 
     else:
         try:
-            ret_code = control.Binman(options, args)
+            ret_code = control.Binman(args)
         except Exception as e:
             print('binman: %s' % e)
-            if options.debug:
+            if args.debug:
                 print()
                 traceback.print_exc()
             ret_code = 1
@@ -196,6 +196,7 @@ def RunBinman(options, args):
 
 
 if __name__ == "__main__":
-    (options, args) = cmdline.ParseArgs(sys.argv)
-    ret_code = RunBinman(options, args)
+    args = cmdline.ParseArgs(sys.argv[1:])
+
+    ret_code = RunBinman(args)
     sys.exit(ret_code)
diff --git a/tools/binman/cmdline.py b/tools/binman/cmdline.py
index 91e007e4e0..a002105fc7 100644
--- a/tools/binman/cmdline.py
+++ b/tools/binman/cmdline.py
@@ -5,7 +5,7 @@
 # Command-line parser for binman
 #
 
-from optparse import OptionParser
+from argparse import ArgumentParser
 
 def ParseArgs(argv):
     """Parse the binman command-line arguments
@@ -17,56 +17,64 @@ def ParseArgs(argv):
             options provides access to the options (e.g. option.debug)
             args is a list of string arguments
     """
-    parser = OptionParser()
-    parser.add_option('-a', '--entry-arg', type='string', action='append',
+    if '-H' in argv:
+        argv.append('build')
+
+    epilog = '''Binman creates and manipulate images for a board from a set of binaries. Binman is
+controlled by a description in the board device tree.'''
+
+    parser = ArgumentParser(epilog=epilog)
+    parser.add_argument('-B', '--build-dir', type=str, default='b',
+        help='Directory containing the build output')
+    parser.add_argument('-D', '--debug', action='store_true',
+        help='Enabling debugging (provides a full traceback on error)')
+    parser.add_argument('-H', '--full-help', action='store_true',
+        default=False, help='Display the README file')
+    parser.add_argument('--toolpath', type=str, action='append',
+        help='Add a path to the directories containing tools')
+    parser.add_argument('-v', '--verbosity', default=1,
+        type=int, help='Control verbosity: 0=silent, 1=warnings, 2=notices, '
+        '3=info, 4=detail, 5=debug')
+
+    subparsers = parser.add_subparsers(dest='cmd')
+
+    build_parser = subparsers.add_parser('build', help='Build firmware image')
+    build_parser.add_argument('-a', '--entry-arg', type=str, action='append',
             help='Set argument value arg=value')
-    parser.add_option('-b', '--board', type='string',
+    build_parser.add_argument('-b', '--board', type=str,
             help='Board name to build')
-    parser.add_option('-B', '--build-dir', type='string', default='b',
-            help='Directory containing the build output')
-    parser.add_option('-d', '--dt', type='string',
+    build_parser.add_argument('-d', '--dt', type=str,
             help='Configuration file (.dtb) to use')
-    parser.add_option('-D', '--debug', action='store_true',
-            help='Enabling debugging (provides a full traceback on error)')
-    parser.add_option('-E', '--entry-docs', action='store_true',
-            help='Write out entry documentation (see README.entries)')
-    parser.add_option('--fake-dtb', action='store_true',
+    build_parser.add_argument('--fake-dtb', action='store_true',
             help='Use fake device tree contents (for testing only)')
-    parser.add_option('-i', '--image', type='string', action='append',
+    build_parser.add_argument('-i', '--image', type=str, action='append',
             help='Image filename to build (if not specified, build all)')
-    parser.add_option('-I', '--indir', action='append',
+    build_parser.add_argument('-I', '--indir', action='append',
             help='Add a path to the list of directories to use for input files')
-    parser.add_option('-H', '--full-help', action='store_true',
-        default=False, help='Display the README file')
-    parser.add_option('-m', '--map', action='store_true',
+    build_parser.add_argument('-m', '--map', action='store_true',
         default=False, help='Output a map file for each image')
-    parser.add_option('-O', '--outdir', type='string',
+    build_parser.add_argument('-O', '--outdir', type=str,
         action='store', help='Path to directory to use for intermediate and '
         'output files')
-    parser.add_option('-p', '--preserve', action='store_true',\
+    build_parser.add_argument('-p', '--preserve', action='store_true',\
         help='Preserve temporary output directory even if option -O is not '
              'given')
-    parser.add_option('-P', '--processes', type=int,
-                      help='set number of processes to use for running tests')
-    parser.add_option('-t', '--test', action='store_true',
-                    default=False, help='run tests')
-    parser.add_option('-T', '--test-coverage', action='store_true',
-                    default=False, help='run tests and check for 100% coverage')
-    parser.add_option('--toolpath', type='string', action='append',
-            help='Add a path to the directories containing tools')
-    parser.add_option('-u', '--update-fdt', action='store_true',
+    build_parser.add_argument('-u', '--update-fdt', action='store_true',
         default=False, help='Update the binman node with offset/size info')
-    parser.add_option('-v', '--verbosity', default=1,
-        type='int', help='Control verbosity: 0=silent, 1=progress, 3=full, '
-        '4=debug')
-    parser.add_option('-X', '--test-preserve-dirs', action='store_true',
+
+    entry_parser = subparsers.add_parser('entry-docs',
+        help='Write out entry documentation (see README.entries)')
+
+    test_parser = subparsers.add_parser('test', help='Run tests')
+    test_parser.add_argument('-P', '--processes', type=int,
+        help='set number of processes to use for running tests')
+    test_parser.add_argument('-T', '--test-coverage', action='store_true',
+        default=False, help='run tests and check for 100%% coverage')
+    test_parser.add_argument('-X', '--test-preserve-dirs', action='store_true',
         help='Preserve and display test-created input directories; also '
              'preserve the output directory if a single test is run (pass test '
              'name at the end of the command line')
-
-    parser.usage += """
-
-Create images for a board from a set of binaries. It is controlled by a
-description in the board device tree."""
+    test_parser.add_argument('tests', nargs='*',
+                             help='Test names to run (omit for all)')
 
     return parser.parse_args(argv)
diff --git a/tools/binman/control.py b/tools/binman/control.py
index 4a94afc864..9022cf76e9 100644
--- a/tools/binman/control.py
+++ b/tools/binman/control.py
@@ -67,19 +67,18 @@ def WriteEntryDocs(modules, test_missing=None):
     from entry import Entry
     Entry.WriteDocs(modules, test_missing)
 
-def Binman(options, args):
+def Binman(args):
     """The main control code for binman
 
     This assumes that help and test options have already been dealt with. It
     deals with the core task of building images.
 
     Args:
-        options: Command line options object
-        args: Command line arguments (list of strings)
+        args: Command line arguments Namespace object
     """
     global images
 
-    if options.full_help:
+    if args.full_help:
         pager = os.getenv('PAGER')
         if not pager:
             pager = 'more'
@@ -89,17 +88,17 @@ def Binman(options, args):
         return 0
 
     # Try to figure out which device tree contains our image description
-    if options.dt:
-        dtb_fname = options.dt
+    if args.dt:
+        dtb_fname = args.dt
     else:
-        board = options.board
+        board = args.board
         if not board:
             raise ValueError('Must provide a board to process (use -b <board>)')
-        board_pathname = os.path.join(options.build_dir, board)
+        board_pathname = os.path.join(args.build_dir, board)
         dtb_fname = os.path.join(board_pathname, 'u-boot.dtb')
-        if not options.indir:
-            options.indir = ['.']
-        options.indir.append(board_pathname)
+        if not args.indir:
+            args.indir = ['.']
+        args.indir.append(board_pathname)
 
     try:
         # Import these here in case libfdt.py is not available, in which case
@@ -107,15 +106,15 @@ def Binman(options, args):
         import fdt
         import fdt_util
 
-        tout.Init(options.verbosity)
-        elf.debug = options.debug
-        cbfs_util.VERBOSE = options.verbosity > 2
-        state.use_fake_dtb = options.fake_dtb
+        tout.Init(args.verbosity)
+        elf.debug = args.debug
+        cbfs_util.VERBOSE = args.verbosity > 2
+        state.use_fake_dtb = args.fake_dtb
         try:
-            tools.SetInputDirs(options.indir)
-            tools.PrepareOutputDir(options.outdir, options.preserve)
-            tools.SetToolPaths(options.toolpath)
-            state.SetEntryArgs(options.entry_arg)
+            tools.SetInputDirs(args.indir)
+            tools.PrepareOutputDir(args.outdir, args.preserve)
+            tools.SetToolPaths(args.toolpath)
+            state.SetEntryArgs(args.entry_arg)
 
             # Get the device tree ready by compiling it and copying the compiled
             # output into a file in our output directly. Then scan it for use
@@ -132,16 +131,16 @@ def Binman(options, args):
 
             images = _ReadImageDesc(node)
 
-            if options.image:
+            if args.image:
                 skip = []
                 new_images = OrderedDict()
                 for name, image in images.items():
-                    if name in options.image:
+                    if name in args.image:
                         new_images[name] = image
                     else:
                         skip.append(name)
                 images = new_images
-                if skip and options.verbosity >= 2:
+                if skip and args.verbosity >= 2:
                     print('Skipping images: %s' % ', '.join(skip))
 
             state.Prepare(images, dtb)
@@ -155,7 +154,7 @@ def Binman(options, args):
             # entry offsets remain the same.
             for image in images.values():
                 image.ExpandEntries()
-                if options.update_fdt:
+                if args.update_fdt:
                     image.AddMissingProperties()
                 image.ProcessFdt(dtb)
 
@@ -176,19 +175,19 @@ def Binman(options, args):
                     image.CheckSize()
                     image.CheckEntries()
                 except Exception as e:
-                    if options.map:
+                    if args.map:
                         fname = image.WriteMap()
                         print("Wrote map file '%s' to show errors"  % fname)
                     raise
                 image.SetImagePos()
-                if options.update_fdt:
+                if args.update_fdt:
                     image.SetCalculatedProperties()
                     for dtb_item in state.GetFdts():
                         dtb_item.Sync()
                 image.ProcessEntryContents()
                 image.WriteSymbols()
                 image.BuildImage()
-                if options.map:
+                if args.map:
                     image.WriteMap()
 
             # Write the updated FDTs to our output files
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 6fdecb2f3b..c675299e1d 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -155,7 +155,7 @@ class TestFunctional(unittest.TestCase):
 
     @classmethod
     def setup_test_args(cls, preserve_indir=False, preserve_outdirs=False,
-                        toolpath=None):
+                        toolpath=None, verbosity=None):
         """Accept arguments controlling test execution
 
         Args:
@@ -169,6 +169,7 @@ class TestFunctional(unittest.TestCase):
         cls.preserve_indir = preserve_indir
         cls.preserve_outdirs = preserve_outdirs
         cls.toolpath = toolpath
+        cls.verbosity = verbosity
 
     def _CheckLz4(self):
         if not self.have_lz4:
@@ -192,19 +193,6 @@ class TestFunctional(unittest.TestCase):
         TestFunctional._MakeInputFile('spl/u-boot-spl.dtb', U_BOOT_SPL_DTB_DATA)
         TestFunctional._MakeInputFile('tpl/u-boot-tpl.dtb', U_BOOT_TPL_DTB_DATA)
 
-    def _GetVerbosity(self):
-        """Check if verbosity should be enabled
-
-        Returns:
-            list containing either:
-                - Verbosity flag (e.g. '-v2') if it is present on the cmd line
-                - nothing if the flag is not present
-        """
-        for arg in sys.argv[1:]:
-            if arg.startswith('-v'):
-                return [arg]
-        return []
-
     def _RunBinman(self, *args, **kwargs):
         """Run binman using the command line
 
@@ -219,7 +207,7 @@ class TestFunctional(unittest.TestCase):
                             result.stdout + result.stderr))
         return result
 
-    def _DoBinman(self, *args):
+    def _DoBinman(self, *argv):
         """Run binman using directly (in the same process)
 
         Args:
@@ -227,16 +215,14 @@ class TestFunctional(unittest.TestCase):
         Returns:
             Return value (0 for success)
         """
-        args = list(args)
-        if '-D' in sys.argv:
-            args = args + ['-D']
-        (options, args) = cmdline.ParseArgs(args)
-        options.pager = 'binman-invalid-pager'
-        options.build_dir = self._indir
+        argv = list(argv)
+        args = cmdline.ParseArgs(argv)
+        args.pager = 'binman-invalid-pager'
+        args.build_dir = self._indir
 
         # For testing, you can force an increase in verbosity here
-        # options.verbosity = tout.DEBUG
-        return control.Binman(options, args)
+        # args.verbosity = tout.DEBUG
+        return control.Binman(args)
 
     def _DoTestFile(self, fname, debug=False, map=False, update_dtb=False,
                     entry_args=None, images=None, use_real_dtb=False,
@@ -254,28 +240,29 @@ class TestFunctional(unittest.TestCase):
                 value: value of that arg
             images: List of image names to build
         """
-        args = ['-p', '-I', self._indir, '-d', self.TestFile(fname)]
+        args = []
         if debug:
             args.append('-D')
+        if verbosity is not None:
+            args.append('-v%d' % verbosity)
+        elif self.verbosity:
+            args.append('-v%d' % self.verbosity)
+        if self.toolpath:
+            for path in self.toolpath:
+                args += ['--toolpath', path]
+        args += ['build', '-p', '-I', self._indir, '-d', self.TestFile(fname)]
         if map:
             args.append('-m')
         if update_dtb:
             args.append('-u')
         if not use_real_dtb:
             args.append('--fake-dtb')
-        if verbosity is not None:
-            args.append('-v%d' % verbosity)
-        else:
-            args += self._GetVerbosity()
         if entry_args:
             for arg, value in entry_args.items():
                 args.append('-a%s=%s' % (arg, value))
         if images:
             for image in images:
                 args += ['-i', image]
-        if self.toolpath:
-            for path in self.toolpath:
-                args += ['--toolpath', path]
         return self._DoBinman(*args)
 
     def _SetupDtb(self, fname, outfile='u-boot.dtb'):
@@ -538,20 +525,20 @@ class TestFunctional(unittest.TestCase):
         """Test that we can run it with a specific board"""
         self._SetupDtb('005_simple.dts', 'sandbox/u-boot.dtb')
         TestFunctional._MakeInputFile('sandbox/u-boot.bin', U_BOOT_DATA)
-        result = self._DoBinman('-b', 'sandbox')
+        result = self._DoBinman('build', '-b', 'sandbox')
         self.assertEqual(0, result)
 
     def testNeedBoard(self):
         """Test that we get an error when no board ius supplied"""
         with self.assertRaises(ValueError) as e:
-            result = self._DoBinman()
+            result = self._DoBinman('build')
         self.assertIn("Must provide a board to process (use -b <board>)",
                 str(e.exception))
 
     def testMissingDt(self):
         """Test that an invalid device-tree file generates an error"""
         with self.assertRaises(Exception) as e:
-            self._RunBinman('-d', 'missing_file')
+            self._RunBinman('build', '-d', 'missing_file')
         # We get one error from libfdt, and a different one from fdtget.
         self.AssertInList(["Couldn't open blob from 'missing_file'",
                            'No such file or directory'], str(e.exception))
@@ -563,26 +550,26 @@ class TestFunctional(unittest.TestCase):
         will come from the device-tree compiler (dtc).
         """
         with self.assertRaises(Exception) as e:
-            self._RunBinman('-d', self.TestFile('001_invalid.dts'))
+            self._RunBinman('build', '-d', self.TestFile('001_invalid.dts'))
         self.assertIn("FATAL ERROR: Unable to parse input tree",
                 str(e.exception))
 
     def testMissingNode(self):
         """Test that a device tree without a 'binman' node generates an error"""
         with self.assertRaises(Exception) as e:
-            self._DoBinman('-d', self.TestFile('002_missing_node.dts'))
+            self._DoBinman('build', '-d', self.TestFile('002_missing_node.dts'))
         self.assertIn("does not have a 'binman' node", str(e.exception))
 
     def testEmpty(self):
         """Test that an empty binman node works OK (i.e. does nothing)"""
-        result = self._RunBinman('-d', self.TestFile('003_empty.dts'))
+        result = self._RunBinman('build', '-d', self.TestFile('003_empty.dts'))
         self.assertEqual(0, len(result.stderr))
         self.assertEqual(0, result.return_code)
 
     def testInvalidEntry(self):
         """Test that an invalid entry is flagged"""
         with self.assertRaises(Exception) as e:
-            result = self._RunBinman('-d',
+            result = self._RunBinman('build', '-d',
                                      self.TestFile('004_invalid_entry.dts'))
         self.assertIn("Unknown entry type 'not-a-valid-type' in node "
                 "'/binman/not-a-valid-type'", str(e.exception))
@@ -1313,7 +1300,8 @@ class TestFunctional(unittest.TestCase):
 
     def testEntryArgsInvalidFormat(self):
         """Test that an invalid entry-argument format is detected"""
-        args = ['-d', self.TestFile('064_entry_args_required.dts'), '-ano-value']
+        args = ['build', '-d', self.TestFile('064_entry_args_required.dts'),
+                '-ano-value']
         with self.assertRaises(ValueError) as e:
             self._DoBinman(*args)
         self.assertIn("Invalid entry arguemnt 'no-value'", str(e.exception))