[PATCH v4 11/11] buildman: Add --allow-missing flag to allow missing blobs

Quentin Schulz quentin.schulz at theobroma-systems.com
Wed Nov 9 17:03:25 CET 2022


Hi Simon,

On 11/8/22 00:28, Simon Glass wrote:
> From: Tom Rini <trini at konsulko.com>
> 
> Add a new flag to buildman so that we will in turn pass
> BINMAN_ALLOW_MISSING=1 to 'make'. Make use of this flag in CI.
> 
> Allow the settings file to control this.
> 
> Cc: Rasmus Villemoes <rasmus.villemoes at prevas.dk>
> Cc: Simon Glass <sjg at chromium.org>
> Signed-off-by: Tom Rini <trini at konsulko.com>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
> 
> (no changes since v3)
> 

There were actually substantial changes between v3 and v4.

> Changes in v3:
> - Add tests docs and a settings-file option
> 
>   .azure-pipelines.yml            |  2 +-
>   .gitlab-ci.yml                  |  6 +--
>   tools/buildman/bsettings.py     | 16 ++++++
>   tools/buildman/builder.py       |  5 +-
>   tools/buildman/builderthread.py |  2 +
>   tools/buildman/buildman.rst     | 43 +++++++++++++++
>   tools/buildman/cmdline.py       |  6 +++
>   tools/buildman/control.py       | 29 +++++++++-
>   tools/buildman/func_test.py     | 96 +++++++++++++++++++++++++++++++--
>   9 files changed, 196 insertions(+), 9 deletions(-)
> 
> diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
> index bda762451fd..665b5d2026f 100644
> --- a/.azure-pipelines.yml
> +++ b/.azure-pipelines.yml
> @@ -553,7 +553,7 @@ stages:
>             cat << "EOF" >> build.sh
>             if [[ "${BUILDMAN}" != "" ]]; then
>                 ret=0;
> -              tools/buildman/buildman -o /tmp -P -E -W ${BUILDMAN} ${OVERRIDE} || ret=$?;
> +              tools/buildman/buildman -o /tmp -PEWM ${BUILDMAN} ${OVERRIDE} || ret=$?;
>                 if [[ $ret -ne 0 ]]; then
>                     tools/buildman/buildman -o /tmp -seP ${BUILDMAN};
>                     exit $ret;
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 6f4c34fc4a3..3deaeca1cdd 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -81,7 +81,7 @@ build all 32bit ARM platforms:
>     stage: world build
>     script:
>       - ret=0;
> -      ./tools/buildman/buildman -o /tmp -P -E -W arm -x aarch64 || ret=$?;
> +      ./tools/buildman/buildman -o /tmp -PEWM arm -x aarch64 || ret=$?;
>         if [[ $ret -ne 0 ]]; then
>           ./tools/buildman/buildman -o /tmp -seP;
>           exit $ret;
> @@ -93,7 +93,7 @@ build all 64bit ARM platforms:
>       - virtualenv -p /usr/bin/python3 /tmp/venv
>       - . /tmp/venv/bin/activate
>       - ret=0;
> -      ./tools/buildman/buildman -o /tmp -P -E -W aarch64 || ret=$?;
> +      ./tools/buildman/buildman -o /tmp -PEWM aarch64 || ret=$?;
>         if [[ $ret -ne 0 ]]; then
>           ./tools/buildman/buildman -o /tmp -seP;
>           exit $ret;
> @@ -113,7 +113,7 @@ build all other platforms:
>     stage: world build
>     script:
>       - ret=0;
> -      ./tools/buildman/buildman -o /tmp -P -E -W -x arm,powerpc || ret=$?;
> +      ./tools/buildman/buildman -o /tmp -PEWM -x arm,powerpc || ret=$?;
>         if [[ $ret -ne 0 ]]; then
>           ./tools/buildman/buildman -o /tmp -seP;
>           exit $ret;
> diff --git a/tools/buildman/bsettings.py b/tools/buildman/bsettings.py
> index dcc200ea79d..9df87f53e49 100644
> --- a/tools/buildman/bsettings.py
> +++ b/tools/buildman/bsettings.py
> @@ -5,6 +5,7 @@ import configparser
>   import os
>   import io
>   
> +config_fname = None
>   

This seems unrelated?

>   def Setup(fname=''):
>       """Set up the buildman settings module by reading config files
> @@ -46,6 +47,21 @@ def GetItems(section):
>       except:
>           raise
>   
> +def GetGlobalItem(name):

I would rename this to GetGlobalItemValue or something more explicit, 
it's not clear that you're returning the value of a key from its name.

> +    """Get an items from the 'global' section of the config.
> +
s/items/item/

> +    Args:
> +        name: name of item to retrieve
> +
> +    Returns:
> +        str: Value of item, or None if not present
> +    """
> +    items = GetItems('global')
> +    for item in items:
> +        if item[0] == name:
> +            return item[1]

I had to run an example myself to see why this was done this way. 
configparser returns tuples with (key, value).

You could simply do:
return settings.get('global', name, fallback=None)

> +    return None
> +
>   def SetItem(section, tag, value):
>       """Set an item and write it back to the settings file"""
>       global settings
> diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
> index 76252b90792..c2a69027f88 100644
> --- a/tools/buildman/builder.py
> +++ b/tools/buildman/builder.py
> @@ -252,7 +252,8 @@ class Builder:
>                    mrproper=False, per_board_out_dir=False,
>                    config_only=False, squash_config_y=False,
>                    warnings_as_errors=False, work_in_output=False,
> -                 test_thread_exceptions=False, adjust_cfg=None):
> +                 test_thread_exceptions=False, adjust_cfg=None,
> +                 allow_missing=False):
>           """Create a new Builder object
>   
>           Args:
> @@ -290,6 +291,7 @@ class Builder:
>                       ~C to disable C
>                       C=val to set the value of C (val must have quotes if C is
>                           a string Kconfig
> +            allow_missing: Run build with BINMAN_ALLOW_MISSING=1
>   
>           """
>           self.toolchains = toolchains
> @@ -327,6 +329,7 @@ class Builder:
>           self.config_filenames = BASE_CONFIG_FILENAMES
>           self.work_in_output = work_in_output
>           self.adjust_cfg = adjust_cfg
> +        self.allow_missing = allow_missing
>           self._ide = False
>   
>           if not self.squash_config_y:
> diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
> index 065d836d68c..680efae02d7 100644
> --- a/tools/buildman/builderthread.py
> +++ b/tools/buildman/builderthread.py
> @@ -253,6 +253,8 @@ class BuilderThread(threading.Thread):
>                       args.extend(['-j', str(self.builder.num_jobs)])
>                   if self.builder.warnings_as_errors:
>                       args.append('KCFLAGS=-Werror')
> +                if self.builder.allow_missing:
> +                    args.append('BINMAN_ALLOW_MISSING=1')
>                   config_args = ['%s_defconfig' % brd.target]
>                   config_out = ''
>                   args.extend(self.builder.toolchains.GetMakeArguments(brd))
> diff --git a/tools/buildman/buildman.rst b/tools/buildman/buildman.rst
> index 33ad6d9e2c9..dd1a2be27b1 100644
> --- a/tools/buildman/buildman.rst
> +++ b/tools/buildman/buildman.rst
> @@ -913,6 +913,25 @@ also allows build flags to be passed to 'make'. It consists of several
>   sections, with the section name in square brackets. Within each section are
>   a set of (tag, value) pairs.
>   
> +global section

'[global]' section
to match how the other sections are shown? I don't think it is specific?

> +    allow-missing
> +        Indicates the policy to use for missing blobs. Note that the flags
> +        --allow-missing (-M) and --no-allow-missing (--no-a) override these

Double-tick quote the arguments to highlight them (I also remember 
(maybe incorrectly) that two dashes are modified into something else 
than just two dashes when not highlighted?).

> +        setting.
> +
> +        always
> +           Run with -M by default.
> +
> +        multiple
> +           Run with -M if more than one board is being built.
> +
> +        branch
> +           Run with -M if a branch is being built.
> +
> +        Note that the last two can be given together::
> +
> +           allow-missing = multiple branch
> +
>   '[toolchain]' section
>       This lists the available toolchains. The tag here doesn't matter, but
>       make sure it is unique. The value is the path to the toolchain. Buildman
> @@ -1140,6 +1159,30 @@ not cause the build to fail:
>      buildman -o /tmp/build --board sandbox -wWI
>   
>   
> +Support for binary blobs
> +------------------------
> +
> +U-Boot is moving to using Binman (see :doc:`../develop/package/binman`) for
> +dealing with the complexities of packaging U-Boot along with binary files from
> +other projects. These are called 'external blobs' by Binman.
> +
> +Typically a missing external blob causes a build failure. For build testing of
> +a lot of boards, or boards for which you do not have the blobs, you can use the
> +-M flag to allow missing blobs. This marks the build as if it succeeded,
> +although with warnings shown, including 'Some images are invalid'. If any boards
> +fail in this way, buildman exits with status 101.
> +
> +To convert warnings to errors, use -E. To make buildman return success with
> +these warnings, use -W.
> +
> +It is generally safe to default to enabling -M for all runs of buildman, so long
> +as you check the exit code. To do this, add::
> +
> +   allow-missing = "always"
> +
> +to the top of the buildman_settings_ file.
> +
> +
>   Changing the configuration
>   --------------------------
>   
> diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py
> index b29c1eb5ee7..c485994e9fe 100644
> --- a/tools/buildman/cmdline.py
> +++ b/tools/buildman/cmdline.py
> @@ -75,6 +75,12 @@ def ParseArgs():
>             help='List available tool chains (use -v to see probing detail)')
>       parser.add_option('-m', '--mrproper', action='store_true',
>             default=False, help="Run 'make mrproper before reconfiguring")
> +    parser.add_option(
> +          '-M', '--allow-missing', action='store_true', default=False,
> +          help='Tell binman to allow missing blobs and generate fake ones as needed'),
> +    parser.add_option(
> +          '--no-allow-missing', action='store_true', default=False,
> +          help='Disable telling binman to allow missing blobs'),
>       parser.add_option('-n', '--dry-run', action='store_true', dest='dry_run',
>             default=False, help="Do a dry run (describe actions, but do nothing)")
>       parser.add_option('-N', '--no-subdirs', action='store_true', dest='no_subdirs',
> diff --git a/tools/buildman/control.py b/tools/buildman/control.py
> index 377b580b253..b1a22cdfc14 100644
> --- a/tools/buildman/control.py
> +++ b/tools/buildman/control.py
> @@ -111,6 +111,23 @@ def ShowToolchainPrefix(brds, toolchains):
>       print(tc.GetEnvArgs(toolchain.VAR_CROSS_COMPILE))
>       return None
>   
> +def get_allow_missing(opt_allow, opt_no_allow, num_selected, has_branch):
> +    allow_missing = False
> +    am_setting = bsettings.GetGlobalItem('allow-missing')
> +    if am_setting:
> +        if am_setting == 'always':
> +            allow_missing = True
> +        if 'multiple' in am_setting and num_selected > 1:
> +            allow_missing = True
> +        if 'branch' in am_setting and has_branch:
> +            allow_missing = True
> +
> +    if opt_allow:
> +        allow_missing = True
> +    if opt_no_allow:
> +        allow_missing = False
> +    return allow_missing
> +
>   def DoBuildman(options, args, toolchains=None, make_func=None, brds=None,
>                  clean_dir=False, test_thread_exceptions=False):
>       """The main control code for buildman
> @@ -305,6 +322,15 @@ def DoBuildman(options, args, toolchains=None, make_func=None, brds=None,
>       if not gnu_make:
>           sys.exit('GNU Make not found')
>   
> +    allow_missing = get_allow_missing(options.allow_missing,
> +                                      options.no_allow_missing, len(selected),
> +                                      options.branch)
> +
> +    if options.allow_missing:
> +        allow_missing = True
> +    if options.no_allow_missing:
> +        allow_missing = False
> +

This is already done in get_allow_missing so no need to be rundundant I 
think?

>       # Create a new builder with the selected options.
>       output_dir = options.output_dir
>       if options.branch:
> @@ -329,7 +355,8 @@ def DoBuildman(options, args, toolchains=None, make_func=None, brds=None,
>               warnings_as_errors=options.warnings_as_errors,
>               work_in_output=options.work_in_output,
>               test_thread_exceptions=test_thread_exceptions,
> -            adjust_cfg=adjust_cfg)
> +            adjust_cfg=adjust_cfg,
> +            allow_missing=allow_missing)
>       builder.force_config_on_failure = not options.quick
>       if make_func:
>           builder.do_make = make_func
> diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py
> index b4f3b46fcb1..32e305b8c59 100644
> --- a/tools/buildman/func_test.py
> +++ b/tools/buildman/func_test.py
> @@ -22,6 +22,7 @@ from patman import tools
>   
>   settings_data = '''
>   # Buildman settings file
> +[global]
>   
>   [toolchain]
>   
> @@ -205,13 +206,16 @@ class TestFunctional(unittest.TestCase):
>   
>           self._test_branch = TEST_BRANCH
>   
> +        # Set to True to report missing blobs
> +        self._missing = False
> +
>           # Avoid sending any output and clear all terminal output
>           terminal.set_print_test_mode()
>           terminal.get_print_test_lines()
>   
>       def tearDown(self):
>           shutil.rmtree(self._base_dir)
> -        #shutil.rmtree(self._output_dir)
> +        shutil.rmtree(self._output_dir)
>   

Is this actually related?

>       def setupToolchains(self):
>           self._toolchains = toolchain.Toolchains()
> @@ -424,10 +428,21 @@ class TestFunctional(unittest.TestCase):
>                       out_dir = arg[2:]
>               fname = os.path.join(cwd or '', out_dir, 'u-boot')
>               tools.write_file(fname, b'U-Boot')
> -            if type(commit) is not str:
> +
> +            # Handle missing blobs
> +            if self._missing:
> +                if 'BINMAN_ALLOW_MISSING=1' in args:
> +                    stderr = '''+Image 'main-section' is missing external blobs and is non-functional: intel-descriptor intel-ifwi intel-fsp-m intel-fsp-s intel-vbt
> +Image 'main-section' has faked external blobs and is non-functional: descriptor.bin fsp_m.bin fsp_s.bin vbt.bin
> +
> +Some images are invalid'''
> +                else:
> +                    stderr = "binman: Filename 'fsp.bin' not found in input path"
> +            elif type(commit) is not str:
>                   stderr = self._error.get((brd.target, commit.sequence))
> +
>               if stderr:
> -                return command.CommandResult(return_code=1, stderr=stderr)
> +                return command.CommandResult(return_code=2, stderr=stderr)
>               return command.CommandResult(return_code=0)
>   
>           # Not handled, so abort
> @@ -621,3 +636,78 @@ class TestFunctional(unittest.TestCase):
>           self.assertIn(
>               'Thread exception (use -T0 to run without threads): test exception',
>               stdout.getvalue())
> +
> +    def testBlobs(self):
> +        """Test handling of missing blobs"""
> +        self._missing = True
> +
> +        board0_dir = os.path.join(self._output_dir, 'current', 'board0')
> +        errfile = os.path.join(board0_dir, 'err')
> +        logfile = os.path.join(board0_dir, 'log')
> +
> +        # We expect failure when there are missing blobs
> +        result = self._RunControl('board0', '-o', self._output_dir)
> +        self.assertEqual(100, result)
> +        self.assertTrue(os.path.exists(os.path.join(board0_dir, 'done')))
> +        self.assertTrue(os.path.exists(errfile))
> +        self.assertIn(b"Filename 'fsp.bin' not found in input path",
> +                      tools.read_file(errfile))
> +


> +        # Allow missing blobs - still failure but a different exit code
> +        result = self._RunControl('board0', '-o', self._output_dir, '-M',
> +                                  clean_dir=True)
> +        self.assertEqual(101, result)
> +        self.assertTrue(os.path.exists(errfile))
> +        self.assertIn(b'Some images are invalid', tools.read_file(errfile))
> +

I would have a separate test for this one.

> +        # Allow missing blobs and ignore warnings
> +        result = self._RunControl('board0', '-o', self._output_dir, '-MW')
> +        self.assertEqual(0, result)
> +        self.assertIn(b'Some images are invalid', tools.read_file(errfile))
> +

Ditto.

Otherwise if there's a regression you don't know which one exactly easily.

> +    def testBlobSettings(self):
> +        # Test with no settings
> +        self.assertEqual(False,
> +                         control.get_allow_missing(False, False, 1, False))
> +        self.assertEqual(True,
> +                         control.get_allow_missing(True, False, 1, False))
> +        self.assertEqual(False,
> +                         control.get_allow_missing(True, True, 1, False))
> +

Same here and the tests below.

Cheers,
Quentin


More information about the U-Boot mailing list