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 ---
Changes in v2: - Don't allow global arguments after the command - Fix up arguments in main Makefile - Pass verbosity to tests via the setup_test_args() method - Put --toolpath in the right place in functional tests
.travis.yml | 2 +- Makefile | 4 +- test/run | 4 +- tools/binman/README | 8 ++-- tools/binman/binman.py | 47 +++++++++++----------- tools/binman/cmdline.py | 82 +++++++++++++++++++++------------------ tools/binman/control.py | 51 ++++++++++++------------ tools/binman/ftest.py | 66 +++++++++++++------------------ tools/patman/test_util.py | 5 ++- 9 files changed, 133 insertions(+), 136 deletions(-)
diff --git a/.travis.yml b/.travis.yml index 70d89d3e233..2f604152090 100644 --- a/.travis.yml +++ b/.travis.yml @@ -147,7 +147,7 @@ script: if [[ -n "${TEST_PY_TOOLS}" ]]; then PYTHONPATH="${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc/pylibfdt" PATH="${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc:${PATH}" - ./tools/binman/binman --toolpath ${UBOOT_TRAVIS_BUILD_DIR}/tools -t && + ./tools/binman/binman --toolpath ${UBOOT_TRAVIS_BUILD_DIR}/tools test && ./tools/patman/patman --test && ./tools/buildman/buildman -t && PYTHONPATH="${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc/pylibfdt" diff --git a/Makefile b/Makefile index f3857ab6a9b..ee0d3c90029 100644 --- a/Makefile +++ b/Makefile @@ -1196,9 +1196,9 @@ u-boot.ldr: u-boot # --------------------------------------------------------------------------- # Use 'make BINMAN_DEBUG=1' to enable debugging quiet_cmd_binman = BINMAN $@ -cmd_binman = $(srctree)/tools/binman/binman -u -d u-boot.dtb -O . -m \ +cmd_binman = $(srctree)/tools/binman/binman build -u -d u-boot.dtb -O . -m \ -I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \ - $(if $(BINMAN_DEBUG),-D) $(BINMAN_$(@F)) $< + $(if $(BINMAN_DEBUG),-D) $(BINMAN_$(@F))
OBJCOPYFLAGS_u-boot.ldr.hex := -I binary -O ihex
diff --git a/test/run b/test/run index b97647eba6f..d635622c106 100755 --- a/test/run +++ b/test/run @@ -40,7 +40,7 @@ export PYTHONPATH=${DTC_DIR}/pylibfdt export DTC=${DTC_DIR}/dtc TOOLS_DIR=build-sandbox_spl/tools
-run_test "binman" ./tools/binman/binman -t --toolpath ${TOOLS_DIR} +run_test "binman" ./tools/binman/binman --toolpath ${TOOLS_DIR} test run_test "patman" ./tools/patman/patman --test
[ "$1" == "quick" ] && skip=--skip-net-tests @@ -52,7 +52,7 @@ run_test "dtoc" ./tools/dtoc/dtoc -t # To enable Python test coverage on Debian-type distributions (e.g. Ubuntu): # $ sudo apt-get install python-pytest python-coverage export PATH=$PATH:${TOOLS_DIR} -run_test "binman code coverage" ./tools/binman/binman -T --toolpath ${TOOLS_DIR} +run_test "binman code coverage" ./tools/binman/binman test -T run_test "dtoc code coverage" ./tools/dtoc/dtoc -T run_test "fdt code coverage" ./tools/dtoc/test_fdt -T
diff --git a/tools/binman/README b/tools/binman/README index ef62d1f1ec7..fe734c1e5f7 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 613aad5c451..8bd5868df26 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 91e007e4e03..a002105fc77 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 4a94afc8640..9022cf76e99 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 6fdecb2f3bf..c675299e1da 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)) diff --git a/tools/patman/test_util.py b/tools/patman/test_util.py index ea36cd16339..40098159c08 100644 --- a/tools/patman/test_util.py +++ b/tools/patman/test_util.py @@ -46,9 +46,10 @@ def RunTestCoverage(prog, filter_fname, exclude_list, build_dir, required=None): glob_list = [] glob_list += exclude_list glob_list += ['*libfdt.py', '*site-packages*', '*dist-packages*'] + test_cmd = 'test' if 'binman.py' in prog else '-t' cmd = ('PYTHONPATH=$PYTHONPATH:%s/sandbox_spl/tools %s-coverage run ' - '--omit "%s" %s -P1 -t' % (build_dir, PYTHON, ','.join(glob_list), - prog)) + '--omit "%s" %s %s -P1' % (build_dir, PYTHON, ','.join(glob_list), + prog, test_cmd)) os.system(cmd) stdout = command.Output('%s-coverage' % PYTHON, 'report') lines = stdout.splitlines()