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

Hi Simon,

On 11/8/22 00:28, Simon Glass wrote:

From: Tom Rini trini@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@prevas.dk Cc: Simon Glass sjg@chromium.org Signed-off-by: Tom Rini trini@konsulko.com Signed-off-by: Simon Glass sjg@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