Merge patch series "binman: Check code-coverage requirements"

Simon Glass <sjg@chromium.org> says:

This series adds a cover-coverage check to CI for Binman. The iMX8 tests
are still not completed, so a work-around is included for those.

A few fixes are included for some other problems.

Link: https://lore.kernel.org/r/20250410124333.843527-1-sjg@chromium.org

.azure-pipelines.yml

@@ -144,8 +144,11 @@ stages:
           export PATH=${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc:${PATH}
           ./tools/buildman/buildman -T0 -o ${UBOOT_TRAVIS_BUILD_DIR} -w --board tools-only
           set -ex
-          ./tools/binman/binman --toolpath ${UBOOT_TRAVIS_BUILD_DIR}/tools tool -f missing
+          export TOOLPATH="--toolpath ${UBOOT_TRAVIS_BUILD_DIR}/tools --toolpath /opt/coreboot"
-          ./tools/binman/binman --toolpath ${UBOOT_TRAVIS_BUILD_DIR}/tools test
+          ./tools/binman/binman ${TOOLPATH} tool -f missing
+          ./tools/binman/binman ${TOOLPATH} test
+          # Avoid "Permission denied: 'cov'" error by using a temporary file
+          COVERAGE_FILE=/tmp/.coverage ./tools/binman/binman ${TOOLPATH} test -T
           ./tools/buildman/buildman -t
           ./tools/dtoc/dtoc -t
           ./tools/patman/patman test

.gitlab-ci.yml

@@ -196,8 +196,10 @@ Run binman, buildman, dtoc, Kconfig and patman testsuites:
       ./tools/buildman/buildman -T0 -o ${UBOOT_TRAVIS_BUILD_DIR} -w
         --board tools-only;
       set -e;
-      ./tools/binman/binman --toolpath ${UBOOT_TRAVIS_BUILD_DIR}/tools tool -f missing;
+      export TOOLPATH="--toolpath ${UBOOT_TRAVIS_BUILD_DIR}/tools --toolpath /opt/coreboot";
-      ./tools/binman/binman --toolpath ${UBOOT_TRAVIS_BUILD_DIR}/tools test;
+      ./tools/binman/binman ${TOOLPATH} tool -f missing;
+      ./tools/binman/binman ${TOOLPATH} test;
+      ./tools/binman/binman ${TOOLPATH} test -T;
       ./tools/buildman/buildman -t;
       ./tools/dtoc/dtoc -t;
       ./tools/patman/patman test;

tools/binman/etype/fit.py

@@ -562,8 +562,6 @@ class Entry_fit(Entry_section):
             for subnode in node.subnodes:
                 if (subnode.name.startswith('signature') or
                     subnode.name.startswith('cipher')):
-                    if subnode.props.get('key-name-hint') is None:
-                        continue
                     hint = subnode.props['key-name-hint'].value
                     name = tools.get_input_filename(
                         f"{hint}.key" if subnode.name.startswith('signature')

tools/binman/etype/section.py

@@ -662,23 +662,6 @@ class Entry_section(Entry):
         else:
             raise ValueError("%s: No such property '%s'" % (msg, prop_name))
 
-    def GetRootSkipAtStart(self):
-        """Get the skip-at-start value for the top-level section
-
-        This is used to find out the starting offset for root section that
-        contains this section. If this is a top-level section then it returns
-        the skip-at-start offset for this section.
-
-        This is used to get the absolute position of section within the image.
-
-        Returns:
-            Integer skip-at-start value for the root section containing this
-                section
-        """
-        if self.section:
-            return self.section.GetRootSkipAtStart()
-        return self._skip_at_start
-
     def GetStartOffset(self):
         """Get the start offset for this section
 

tools/binman/ftest.py

@@ -4613,6 +4613,8 @@ class TestFunctional(unittest.TestCase):
         dtb.Scan()
         props = self._GetPropTree(dtb, ['offset', 'image-pos', 'size',
                                         'uncomp-size'])
+        data = data[:0x30]
+        data = data.rstrip(b'\xff')
         orig = self._decompress(data)
         self.assertEqual(COMPRESS_DATA + U_BOOT_DATA, orig)
         expected = {
@@ -6218,8 +6220,9 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
 
     def testCompUtilPadding(self):
         """Test padding of compression algorithms"""
-        # Skip zstd because it doesn't support padding
+        # Skip zstd and lz4 because they doesn't support padding
-        for bintool in [v for k,v in self.comp_bintools.items() if k != 'zstd']:
+        for bintool in [v for k,v in self.comp_bintools.items()
+                        if not k in ['zstd', 'lz4']]:
             self._CheckBintool(bintool)
             data = bintool.compress(COMPRESS_DATA)
             self.assertNotEqual(COMPRESS_DATA, data)

tools/binman/main.py

@@ -94,10 +94,16 @@ def RunTestCoverage(toolpath, build_dir, args):
     if toolpath:
         for path in toolpath:
             extra_args += ' --toolpath %s' % path
+
+    # Some files unfortunately don't thave the required test coverage. This will
+    # eventually be fixed, but exclude them for now
     test_util.run_test_coverage('tools/binman/binman', None,
             ['*test*', '*main.py', 'tools/patman/*', 'tools/dtoc/*',
              'tools/u_boot_pylib/*'],
-            build_dir, all_set, extra_args or None, args=args)
+            build_dir, all_set, extra_args or None, args=args,
+            allow_failures=['tools/binman/btool/cst.py',
+                            'tools/binman/etype/nxp_imx8mcst.py',
+                            'tools/binman/etype/nxp_imx8mimage.py'])
 
 def RunBinman(args):
     """Main entry point to binman once arguments are parsed

tools/binman/requirements.txt

@@ -1,3 +1,4 @@
+coverage==7.8.0
 importlib_resources==6.5.2
 jsonschema==4.23.0
 pycryptodomex==3.21.0

tools/binman/state.py

@@ -411,8 +411,7 @@ def CheckSetHashValue(node, get_data_func):
             m = hashlib.sha256()
             m.update(get_data_func())
             data = m.digest()
-        if data is None:
+        assert data
-            raise ValueError(f"Node '{node.path}': Unknown hash algorithm '{algo}'")
         for n in GetUpdateNodes(hash_node):
             n.SetData('value', data)
 

tools/binman/test/184_compress_section_size.dts

@@ -6,6 +6,7 @@
 		section {
 			size = <0x30>;
 			compress = "lz4";
+			pad-byte = <0xff>;
 			blob {
 				filename = "compress";
 			};

tools/u_boot_pylib/test_util.py

@@ -8,6 +8,7 @@ import doctest
 import glob
 import multiprocessing
 import os
+import re
 import sys
 import unittest
 
@@ -25,7 +26,7 @@ except:
 
 def run_test_coverage(prog, filter_fname, exclude_list, build_dir,
                       required=None, extra_args=None, single_thread='-P1',
-                      args=None):
+                      args=None, allow_failures=None):
     """Run tests and check that we get 100% coverage
 
     Args:
@@ -56,7 +57,7 @@ def run_test_coverage(prog, filter_fname, exclude_list, build_dir,
     else:
         glob_list = []
     glob_list += exclude_list
-    glob_list += ['*libfdt.py', '*site-packages*', '*dist-packages*']
+    glob_list += ['*libfdt.py', '*/site-packages/*', '*/dist-packages/*']
     glob_list += ['*concurrencytest*']
     test_cmd = 'test' if 'binman' in prog or 'patman' in prog else '-t'
     prefix = ''
@@ -96,6 +97,19 @@ def run_test_coverage(prog, filter_fname, exclude_list, build_dir,
         print('Coverage error: %s, but should be 100%%' % coverage)
         ok = False
     if not ok:
+        if allow_failures:
+            # for line in lines:
+                # print('.', line, re.match(r'^(tools/.*py) *\d+ *(\d+) *(\d+)%$', line))
+            lines = [re.match(r'^(tools/.*py) *\d+ *(\d+) *\d+%$', line)
+                     for line in stdout.splitlines()]
+            bad = []
+            for mat in lines:
+                if mat and mat.group(2) != '0':
+                    fname = mat.group(1)
+                    if fname not in allow_failures:
+                        bad.append(fname)
+            if not bad:
+                return
         raise ValueError('Test coverage failure')