[PATCH v2 00/12] Fix 'make check' errors

[PATCH v2 00/12] Fix 'make check' errors

[Andrew Goodbody]

Various tests can be run using 'make check', 'make qcheck' and 'make
tcheck'. These have become broken and need fixing to be useful again.

Running 'make qcheck' fails on the binman test to check the version
numbers of the tools are found as expected. Update the code for the
tools that have been updated and no longer have their version numbers
found by the code.

There are also failures in buildman. First add code to understand 'gcc
-E' as an alternative to cpp. Second add the code to accept an empty
CROSS_COMPILE variable in the environment when testing that
CROSS_COMPILE is not set by the toolchain selection.

dtoc was not achieving 100% code coverage so add a new test to rectify
that.

Disable event_dump tests that are broken on newer versions of binutils.

Build mkeficapsule in the tree that is used for testing the tools.

'make tcheck' was not achieving 100% code coverage and needs to know the
location of the built tools to resolve.

'make check' tests of the file system had errors due to ext4 file
systems being created with an orphaned file but were marked clean.

Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
Changes in v2:
- Added commit 7/12 to address intermittent failure for binman to get
  version string from lzma_alone.
- Added commit 8/12 to address a test failure due to the build tree not
  containing the built version of mkeficapsule and using the system
  version instead.
- Dropped a space from the beginning of a line in a new file in 5/12 to
  quiet checkpatch warning and remove most of the content as not needed
  for test as suggested by Simon Glass.
- Fix 'make tcheck' binman code coverage test by passing in location of
  tools built from this tree in 9/12.
- Fix typo in tools/Kconfig for TOOLS_MKEFICAPSULE config item in 10/12
- Cope with CPP="gcc -E" in buildman in 11/12
- Ensure fsck can be run and use it to force clean ext4 file systems on
  creation to remove orphaned file. 12/12
- Picked up some rb tags from Simon Glass
- Link to v1: https://patch.msgid.link/20261102-qcheck_fix-v1-0-f1f547c4a731@linaro.org

To: u-boot@lists.u-boot-project.org
To: Kory Maincent <kory.maincent@bootlin.com>
To: Linus Walleij <linusw@kernel.org>
To: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc: Alper Nebi Yasak <alpernebiyasak@gmail.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: Andrew Goodbody <andrew.goodbody@linaro.org>
Cc: Anshul Dalal <anshuld@ti.com>
Cc: Alexey Charkov <alchark@flipper.net>
Cc: Francesco Valla <francesco@valla.it>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Mattijs Korpershoek <mkorpershoek@kernel.org>
Cc: Carlo Caione <ccaione@baylibre.com>
Cc: Yao Zi <me@ziyao.cc>
Cc: Michal Simek <michal.simek@amd.com>
Cc: Quentin Schulz <quentin.schulz@cherry.de>

---
Andrew Goodbody (12):
      binman: btool: lz4: Update version regex
      binman: btool: lzma_alone: Update version function
      test: Do not set DTC environment variable
      buildman: Cope with empty CROSS_COMPILE in tests
      dtoc: test: Fix coverage by adding test
      test: Disable running event_dump test for make check
      binman: bintool: Do not add empty argument
      test: Build mkeficapsule in tree used by tests
      test: Pass location of built tools to binman code coverage test
      tools: Fix typo in name of command mkeficapsule
      buildman: Cope with CPP="gcc -E"
      test: Fix test_basic file system tests

 configs/sandbox_spl_defconfig                          |  8 ++++----
 test/py/tests/fs_helper.py                             |  4 ++++
 test/py/tests/test_fs/fstest_helpers.py                |  5 +++++
 test/run                                               |  8 ++++----
 tools/Kconfig                                          |  2 +-
 tools/binman/bintool.py                                |  5 ++++-
 tools/binman/btool/lz4.py                              |  2 +-
 tools/binman/btool/lzma_alone.py                       |  2 +-
 tools/buildman/boards.py                               |  3 +--
 tools/buildman/func_test.py                            |  5 +++--
 tools/buildman/test.py                                 |  4 ++--
 tools/dtoc/test/dtoc_test_include.dts                  | 13 +++++++++++++
 tools/dtoc/test/newinclude/dtoc_test_include_file.dtsi |  7 +++++++
 tools/dtoc/test_fdt.py                                 |  7 +++++++
 14 files changed, 57 insertions(+), 18 deletions(-)
---
base-commit: 709a97a05df66e394f40d249105d0baade46f107
change-id: 20261029-qcheck_fix-af97ed0d895b

Best regards,
--  
Andrew Goodbody <andrew.goodbody@linaro.org>

[PATCH v2 01/12] binman: btool: lz4: Update version regex

[Andrew Goodbody]

With the update to lz4 v1.10.x the version string has changed format.
Previously the string looked like
"*** LZ4 command line interface 64-bits v1.9.2, by Yann Collet ***"
and now it is
"*** lz4 v1.10.0 64-bit multithread, by Yann Collet ***"
The ',' following the version number is no longer present so remove it
from the regex used to match the version number. The regex will now
match the version number in both of the forms so will continue to work
with older versions of lz4 as well as newer ones.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 tools/binman/btool/lz4.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/binman/btool/lz4.py b/tools/binman/btool/lz4.py
index fd520d13a56..fcc0d91553e 100644
--- a/tools/binman/btool/lz4.py
+++ b/tools/binman/btool/lz4.py
@@ -76,7 +76,7 @@ class Bintoollz4(bintool.Bintool):
         man lz4
     """
     def __init__(self, name):
-        super().__init__(name, 'lz4 compression', r'.* (v[0-9.]*),.*')
+        super().__init__(name, 'lz4 compression', r'.* (v[0-9.]*).*')
 
     def compress(self, indata):
         """Compress data with lz4

-- 
2.47.3

[PATCH v2 02/12] binman: btool: lzma_alone: Update version function

[Andrew Goodbody]

lzma_alone 24.08 now prints its version string to stdout instead of
stderr which means the current version function will not find it as it
only looks in stderr. Instead look in 'combined' which contains both
stdout and stderr and so will work with older and newer versions of
lzma_alone.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 tools/binman/btool/lzma_alone.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/binman/btool/lzma_alone.py b/tools/binman/btool/lzma_alone.py
index 1fda2f68c7b..aa44c897223 100644
--- a/tools/binman/btool/lzma_alone.py
+++ b/tools/binman/btool/lzma_alone.py
@@ -116,7 +116,7 @@ class Bintoollzma_alone(bintool.Bintool):
         Returns:
             str: Version number of lzma_alone
         """
-        out = self.run_cmd_result('', raise_on_error=False).stderr.strip()
+        out = self.run_cmd_result('', raise_on_error=False).combined.strip()
         lines = out.splitlines()
         if not lines:
             return super().version()

-- 
2.47.3

[PATCH v2 03/12] test: Do not set DTC environment variable

[Andrew Goodbody]

The presence of the DTC environment variable breaks a binman test that
attempts to execute dtc from a temporary directory using the value of
DTC but that is a relative directory path based on the source tree not
this temporary directory and so it fails.
It also breaks a buildman test that checks the environment and asserts
that DTC is not present.
Remove the setting of DTC as it is not needed.

Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 test/run | 1 -
 1 file changed, 1 deletion(-)

diff --git a/test/run b/test/run
index 7c0263713c6..17e78815fa7 100755
--- a/test/run
+++ b/test/run
@@ -76,7 +76,6 @@ fi
 # to tools build by the build.
 DTC_DIR=build-sandbox_spl/scripts/dtc
 export PYTHONPATH=${DTC_DIR}/pylibfdt
-export DTC=${DTC_DIR}/dtc
 TOOLS_DIR=build-sandbox_spl/tools
 
 run_test "binman" ./tools/binman/binman --toolpath ${TOOLS_DIR} test

-- 
2.47.3

[PATCH v2 04/12] buildman: Cope with empty CROSS_COMPILE in tests

[Andrew Goodbody]

When the buildman tests are run through the Makefile with eg 'make
qcheck' the Makefile will export an empty CROSS_COMPILE variable into
the environment. Add the code to the test that will accept this as OK.

Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 tools/buildman/test.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/buildman/test.py b/tools/buildman/test.py
index 7ee9496ffb3..e620a92126f 100644
--- a/tools/buildman/test.py
+++ b/tools/buildman/test.py
@@ -656,7 +656,7 @@ class TestBuild(unittest.TestCase):
 
         tc = self.toolchains.Select('sandbox')
         env = tc.MakeEnvironment(False)
-        self.assertTrue(b'CROSS_COMPILE' not in env)
+        self.assertTrue(b'CROSS_COMPILE' not in env or env[b'CROSS_COMPILE'] == b'')
 
         # Test config with ccache wrapper
         bsettings.setup(None)
@@ -668,7 +668,7 @@ class TestBuild(unittest.TestCase):
 
         tc = self.toolchains.Select('sandbox')
         env = tc.MakeEnvironment(False)
-        self.assertTrue(b'CROSS_COMPILE' not in env)
+        self.assertTrue(b'CROSS_COMPILE' not in env or env[b'CROSS_COMPILE'] == b'')
 
     def testPrepareOutputSpace(self):
         def _Touch(fname):

-- 
2.47.3

[PATCH v2 05/12] dtoc: test: Fix coverage by adding test

[Andrew Goodbody]

Use of 'indir' in EnsureCompiled() was not being tested and resulted in
less than 100% code coverage. Add a test and supporting files that
depends on the use of 'indir' by placing an included file in a new
directory. The dts file will fail to compile if the new directory is not
added to the search path with the use of 'indir'.

Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 tools/dtoc/test/dtoc_test_include.dts                  | 13 +++++++++++++
 tools/dtoc/test/newinclude/dtoc_test_include_file.dtsi |  7 +++++++
 tools/dtoc/test_fdt.py                                 |  7 +++++++
 3 files changed, 27 insertions(+)

diff --git a/tools/dtoc/test/dtoc_test_include.dts b/tools/dtoc/test/dtoc_test_include.dts
new file mode 100644
index 00000000000..b5ef647c1b6
--- /dev/null
+++ b/tools/dtoc/test/dtoc_test_include.dts
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Test device tree file for dtoc
+ *
+ * Copyright 2017 Google, Inc
+ */
+
+/dts-v1/;
+
+/ {
+};
+
+#include "dtoc_test_include_file.dtsi"
diff --git a/tools/dtoc/test/newinclude/dtoc_test_include_file.dtsi b/tools/dtoc/test/newinclude/dtoc_test_include_file.dtsi
new file mode 100644
index 00000000000..f975005d023
--- /dev/null
+++ b/tools/dtoc/test/newinclude/dtoc_test_include_file.dtsi
@@ -0,0 +1,7 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/ {
+	new_node {
+		intval = <2>;
+	};
+};
diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py
index 66d346b1272..a63c860eae1 100755
--- a/tools/dtoc/test_fdt.py
+++ b/tools/dtoc/test_fdt.py
@@ -1105,6 +1105,13 @@ class TestFdtUtil(unittest.TestCase):
         finally:
             tools.outdir = old_outdir
 
+    def test_ensure_compiled_indir(self):
+        """Test providing directory as indir"""
+        indir = os.path.dirname(find_dtb_file('dtoc_test_include.dts'))
+        indir = os.path.join(indir, 'newinclude')
+        dtb = fdt_util.EnsureCompiled(find_dtb_file('dtoc_test_include.dts'), indir=[indir])
+        self.assertEqual(dtb, fdt_util.EnsureCompiled(dtb))
+
     def test_get_phandle_name_offset(self):
         val = fdt_util.GetPhandleNameOffset(self.node, 'missing')
         self.assertIsNone(val)

-- 
2.47.3

[PATCH v2 06/12] test: Disable running event_dump test for make check

[Andrew Goodbody]

The event dump script does not work on more recent versions of binutils
so disable it from running for the moment.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 test/run | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/test/run b/test/run
index 17e78815fa7..d1654dffb5f 100755
--- a/test/run
+++ b/test/run
@@ -15,8 +15,9 @@ run_test() {
 
 # Select test attributes
 ut_mark_expr=test_ut
+mark_expr="not event_dump"
 if [ "$1" = "quick" ]; then
-	mark_expr="not slow"
+	mark_expr="not slow and not event_dump"
 	ut_mark_expr="test_ut and not slow"
 	skip=--skip-net-tests
 fi
@@ -32,7 +33,7 @@ if [ "$1" = "parallel" ]; then
 	para="-n${jobs} -q"
 	prompt="Building and..."
 	skip=--skip-net-tests
-	mark_expr="not slow and not bootstd and not spi_flash"
+	mark_expr="not slow and not bootstd and not spi_flash and not event_dump"
 	ut_mark_expr="test_ut and not slow and not bootstd and not spi_flash"
 	echo "Note: test log is garbled with parallel tests"
 fi

-- 
2.47.3

[PATCH v2 07/12] binman: bintool: Do not add empty argument

[Andrew Goodbody]

If lzma_alone is invoked with an empty argument it will emit a message
to stderr. When 'combined' is used to examine the output of this command
the content is not deterministic in that stdout and stderr may appear
out of order on occasions. To prevent this situation do not add an
argument list to the command when it is empty. In this situation no
error message is emitted only the usage and this is always sent to only
stdout or only to stderr depending on the version of lzma_alone.

Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 tools/binman/bintool.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/binman/bintool.py b/tools/binman/bintool.py
index 4cf8d5b165b..15e2df2fd80 100644
--- a/tools/binman/bintool.py
+++ b/tools/binman/bintool.py
@@ -290,7 +290,10 @@ class Bintool:
         if self.name in self.missing_list:
             return None
         name = os.path.expanduser(self.name)  # Expand paths containing ~
-        all_args = (name,) + args
+        if args[0] == '':
+            all_args = (name,)
+        else:
+            all_args = (name,) + args
         env = tools.get_env_with_path()
         tout.debug(f"bintool: {' '.join(all_args)}")
         result = command.run_pipe(

-- 
2.47.3

[PATCH v2 08/12] test: Build mkeficapsule in tree used by tests

[Andrew Goodbody]

Since the commit to only build mkeficapsule when needed rather than by
default, the build tree build-sandbox_spl no longer contains the built
version of mkeficapsule and this can lead a test failure. Add the
config line to the defconfig so that mkeficapsule is built and the tests
will now work. 'make savedefconfig' was used to generate the new
defconfig file.

Fixes: 2f736e8e3f4b ("tools: build mkeficapsule for CONFIG_EFI_HAVE_CAPSULE_SUPPORT=y")
Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 configs/sandbox_spl_defconfig | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig
index b1112fd45e7..34ad509a85e 100644
--- a/configs/sandbox_spl_defconfig
+++ b/configs/sandbox_spl_defconfig
@@ -1,6 +1,5 @@
 CONFIG_TEXT_BASE=0x400000
 CONFIG_SPL_GPIO=y
-CONFIG_SPL_LIBCOMMON_SUPPORT=y
 CONFIG_NR_DRAM_BANKS=1
 CONFIG_ENV_SIZE=0x2000
 CONFIG_SPL_SYS_MALLOC_F_LEN=0x8000
@@ -123,6 +122,7 @@ CONFIG_CPU=y
 CONFIG_DM_DEMO=y
 CONFIG_DM_DEMO_SIMPLE=y
 CONFIG_DM_DEMO_SHAPE=y
+CONFIG_SPL_FASTBOOT=y
 CONFIG_SPL_FIRMWARE=y
 CONFIG_DM_FPGA=y
 CONFIG_SANDBOX_FPGA=y
@@ -214,11 +214,10 @@ CONFIG_SANDBOX_TIMER=y
 CONFIG_USB=y
 CONFIG_DM_USB_GADGET=y
 CONFIG_SPL_DM_USB_GADGET=y
-CONFIG_USB_GADGET=y
-CONFIG_SPL_USB_GADGET=y
-CONFIG_SPL_FASTBOOT=y
 CONFIG_USB_EMUL=y
 CONFIG_USB_KEYBOARD=y
+CONFIG_USB_GADGET=y
+CONFIG_SPL_USB_GADGET=y
 CONFIG_VIDEO=y
 CONFIG_CONSOLE_ROTATION=y
 CONFIG_CONSOLE_TRUETYPE=y
@@ -242,3 +241,4 @@ CONFIG_UNIT_TEST=y
 CONFIG_SPL_UNIT_TEST=y
 CONFIG_UT_TIME=y
 CONFIG_UT_DM=y
+CONFIG_TOOLS_MKEFICAPSULE=y

-- 
2.47.3

[PATCH v2 09/12] test: Pass location of built tools to binman code coverage test

[Andrew Goodbody]

When running binman for code coverage tests and invoked by 'make tcheck'
some tests can fail if they use system installed versions of tools
instead of ones built from the tree under test. Ensure that only built
versions of tools are used by passing the location of these tools to the
binman test being run.

Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 test/run | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/run b/test/run
index d1654dffb5f..0857fc14c9d 100755
--- a/test/run
+++ b/test/run
@@ -93,7 +93,7 @@ run_test "dtoc" ./tools/dtoc/dtoc -t
 # Code-coverage tests cannot run in parallel, so skip them in that case
 if [ -z "${para}" ]; then
 	export PATH=$PATH:${TOOLS_DIR}
-	run_test "binman code coverage" ./tools/binman/binman test -T
+	run_test "binman code coverage" ./tools/binman/binman --toolpath ${TOOLS_DIR} test -T
 	run_test "dtoc code coverage" ./tools/dtoc/dtoc -T
 	run_test "fdt code coverage" ./tools/dtoc/test_fdt -T
 fi

-- 
2.47.3

[PATCH v2 10/12] tools: Fix typo in name of command mkeficapsule

[Andrew Goodbody]

The description for the config item TOOLS_MKEFICAPSULE has a typo, fix
it. efimkcapsule -> mkeficapsule

Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 tools/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/Kconfig b/tools/Kconfig
index e151b30217e..78172e8f9a2 100644
--- a/tools/Kconfig
+++ b/tools/Kconfig
@@ -107,7 +107,7 @@ config TOOLS_SHA512
 	  Enable SHA512 support in the tools builds
 
 config TOOLS_MKEFICAPSULE
-	bool "Build efimkcapsule command"
+	bool "Build mkeficapsule command"
 	default y if EFI_HAVE_CAPSULE_SUPPORT
 	help
 	  This command allows users to create a UEFI capsule file and,

-- 
2.47.3

[PATCH v2 11/12] buildman: Cope with CPP="gcc -E"

[Andrew Goodbody]

Allow the use of "gcc -E" as an alternative to "cpp" when set in the
environment as the value of CPP. Accept it as a valid command for
running the preprocessor and adjust the test to not depend on a fixed
number of arguments. Also split it into parts for constructing the
command list so that the validation for running a command does not fail
when it tries to find an executable named "gcc -E".

Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 tools/buildman/boards.py    | 3 +--
 tools/buildman/func_test.py | 5 +++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/buildman/boards.py b/tools/buildman/boards.py
index 5d5d3a3822b..111789f1bd4 100644
--- a/tools/buildman/boards.py
+++ b/tools/buildman/boards.py
@@ -244,8 +244,7 @@ class KconfigScanner:
 
         temp = None
         if b'#include' in tools.read_file(defconfig):
-            cmd = [
-                os.getenv('CPP', 'cpp'),
+            cmd = os.getenv('CPP', 'cpp').split() + [
                 '-nostdinc', '-P',
                 '-I', self._srctree,
                 '-undef',
diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py
index fce615ba8b8..eb13589ba17 100644
--- a/tools/buildman/func_test.py
+++ b/tools/buildman/func_test.py
@@ -378,7 +378,7 @@ class TestFunctional(unittest.TestCase):
     def _HandleCommandCpp(self, args):
         # args ['-nostdinc', '-P', '-I', '/tmp/tmp7f17xk_o/src', '-undef',
         # '-x', 'assembler-with-cpp', fname]
-        fname = args[7]
+        fname = args[-1]
         buf = io.StringIO()
         for line in tools.read_file(fname, False).splitlines():
             if line.startswith('#include'):
@@ -424,7 +424,8 @@ class TestFunctional(unittest.TestCase):
             return self._HandleCommandObjcopy(args)
         elif cmd.endswith( 'size'):
             return self._HandleCommandSize(args)
-        elif cmd.endswith( 'cpp'):
+        elif cmd.endswith('cpp') \
+                or (cmd.endswith('gcc') and pipe_list[0][1] == '-E'):
             return self._HandleCommandCpp(args)
 
         if not result:

-- 
2.47.3

[PATCH v2 12/12] test: Fix test_basic file system tests

[Andrew Goodbody]

Ensure that /sbin is on the path so that distributions that do not have
it will still be able to successfully run these tests that need fsck.
Add a forced clean of the newly created file system to cope with the
fact that there is an orphaned file on ext4 that will cause the
integrity test to fail later on if not removed now.

Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 test/py/tests/fs_helper.py              | 4 ++++
 test/py/tests/test_fs/fstest_helpers.py | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/test/py/tests/fs_helper.py b/test/py/tests/fs_helper.py
index e3824b2c1fd..48b047aeefd 100644
--- a/test/py/tests/fs_helper.py
+++ b/test/py/tests/fs_helper.py
@@ -250,6 +250,10 @@ def mk_fs(config, fs_type, size, prefix, src_dir=None, fs_img=None, quiet=False)
                                       shell=True).decode()
             if 'metadata_csum' in sb_content:
                 check_call(f'tune2fs -O ^metadata_csum {fs_img}', shell=True)
+            # Cannot use check_call here as it will return an exit code of 1
+            # as it modifies the file system. The -f is needed as although the
+            # fs is marked clean, it is not really.
+            call(f'fsck.ext4 -f -y {fs_img}', shell=True)
         elif fs_lnxtype == 'vfat' and src_dir:
             flags = f"-smpQ{'' if quiet else 'v'}"
             check_call(f'mcopy -i {fs_img} {flags} {src_dir}/* ::/',
diff --git a/test/py/tests/test_fs/fstest_helpers.py b/test/py/tests/test_fs/fstest_helpers.py
index d25326ee993..6164eca476a 100644
--- a/test/py/tests/test_fs/fstest_helpers.py
+++ b/test/py/tests/test_fs/fstest_helpers.py
@@ -3,9 +3,14 @@
 # Author: JJ Hiblot <jjhiblot@ti.com>
 #
 
+import os
 from subprocess import check_call, CalledProcessError
 
 def assert_fs_integrity(fs_type, fs_img):
+    # Some distributions do not add /sbin to the default PATH, where fsck lives
+    if '/sbin' not in os.environ["PATH"].split(os.pathsep):
+        os.environ["PATH"] += os.pathsep + '/sbin'
+
     try:
         if fs_type == 'ext4':
             check_call('fsck.ext4 -n -f %s' % fs_img, shell=True)

-- 
2.47.3

Re: [PATCH v2 03/12] test: Do not set DTC environment variable

[Quentin Schulz]

Hi Andrew,

On 11/10/26 1:24 PM, Andrew Goodbody wrote:
> The presence of the DTC environment variable breaks a binman test that
> attempts to execute dtc from a temporary directory using the value of
> DTC but that is a relative directory path based on the source tree not
> this temporary directory and so it fails.
> It also breaks a buildman test that checks the environment and asserts
> that DTC is not present.
> Remove the setting of DTC as it is not needed.
> 

What happens if you have DTC exported in your current environment before 
the tests are run? It'd be nice to mention which tests exactly fail for 
future reference.

I'm thinking this is not enough and we should also fix the binman and 
buildman tests to run with an environment where the DTC variable is 
removed/absent/set to the correct path so we don't depend on a "correct" 
external environment.

Cheers,
Quentin

Re: [PATCH v2 04/12] buildman: Cope with empty CROSS_COMPILE in tests

[Quentin Schulz]

Hi Andrew,

On 11/10/26 1:24 PM, Andrew Goodbody wrote:
> When the buildman tests are run through the Makefile with eg 'make
> qcheck' the Makefile will export an empty CROSS_COMPILE variable into
> the environment. Add the code to the test that will accept this as OK.
> 
> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
> ---
>   tools/buildman/test.py | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/buildman/test.py b/tools/buildman/test.py
> index 7ee9496ffb3..e620a92126f 100644
> --- a/tools/buildman/test.py
> +++ b/tools/buildman/test.py
> @@ -656,7 +656,7 @@ class TestBuild(unittest.TestCase):
>   
>           tc = self.toolchains.Select('sandbox')
>           env = tc.MakeEnvironment(False)
> -        self.assertTrue(b'CROSS_COMPILE' not in env)
> +        self.assertTrue(b'CROSS_COMPILE' not in env or env[b'CROSS_COMPILE'] == b'')

suggestion: merge both checks:

self.assertEqual(env.get(b'CROSS_COMPILE', ''), '')

should do the same.

In any case:

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH v2 05/12] dtoc: test: Fix coverage by adding test

[Quentin Schulz]

Hi Andrew,

On 11/10/26 1:24 PM, Andrew Goodbody wrote:
> Use of 'indir' in EnsureCompiled() was not being tested and resulted in
> less than 100% code coverage. Add a test and supporting files that
> depends on the use of 'indir' by placing an included file in a new
> directory. The dts file will fail to compile if the new directory is not
> added to the search path with the use of 'indir'.
> 
> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
> ---
>   tools/dtoc/test/dtoc_test_include.dts                  | 13 +++++++++++++
>   tools/dtoc/test/newinclude/dtoc_test_include_file.dtsi |  7 +++++++
>   tools/dtoc/test_fdt.py                                 |  7 +++++++
>   3 files changed, 27 insertions(+)
> 
> diff --git a/tools/dtoc/test/dtoc_test_include.dts b/tools/dtoc/test/dtoc_test_include.dts
> new file mode 100644
> index 00000000000..b5ef647c1b6
> --- /dev/null
> +++ b/tools/dtoc/test/dtoc_test_include.dts
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Test device tree file for dtoc
> + *
> + * Copyright 2017 Google, Inc
> + */
> +
> +/dts-v1/;
> +
> +/ {
> +};
> +
> +#include "dtoc_test_include_file.dtsi"

suggestion: move this line close to /dts-v1/

I was about to dig deep into fdt_util.EnsureCompiled() to see what kind 
of magic it does to automatically include a file because I had 
completely missed this line. In DTS, we usually have the #include follow 
the /dts-v1/; line, can you please move this up so it more closely matches?

> diff --git a/tools/dtoc/test/newinclude/dtoc_test_include_file.dtsi b/tools/dtoc/test/newinclude/dtoc_test_include_file.dtsi
> new file mode 100644
> index 00000000000..f975005d023
> --- /dev/null
> +++ b/tools/dtoc/test/newinclude/dtoc_test_include_file.dtsi
> @@ -0,0 +1,7 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/ {
> +	new_node {
> +		intval = <2>;
> +	};
> +};
> diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py
> index 66d346b1272..a63c860eae1 100755
> --- a/tools/dtoc/test_fdt.py
> +++ b/tools/dtoc/test_fdt.py
> @@ -1105,6 +1105,13 @@ class TestFdtUtil(unittest.TestCase):
>           finally:
>               tools.outdir = old_outdir
>   
> +    def test_ensure_compiled_indir(self):
> +        """Test providing directory as indir"""
> +        indir = os.path.dirname(find_dtb_file('dtoc_test_include.dts'))
> +        indir = os.path.join(indir, 'newinclude')
> +        dtb = fdt_util.EnsureCompiled(find_dtb_file('dtoc_test_include.dts'), indir=[indir])
> +        self.assertEqual(dtb, fdt_util.EnsureCompiled(dtb))
> +

question: do we want to check the content of the DTB?

Checking for the node that comes from the included file, to make sure 
the included directive actually works? At the same time, that would 
essentially mean checking cc+dtc works as expected, so maybe checking 
too much in the wrong place?

Looks ok to me:

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH v2 06/12] test: Disable running event_dump test for make check

[Quentin Schulz]

Hi Andrew,

On 11/10/26 1:24 PM, Andrew Goodbody wrote:
> The event dump script does not work on more recent versions of binutils

Please provide the error log or something so that we get a chance to fix 
this in the future.

Cheers,
Quentin

Re: [PATCH v2 03/12] test: Do not set DTC environment variable

[Andrew Goodbody]

On 10/11/2026 13:30, Quentin Schulz wrote:
> Hi Andrew,
> 
> On 11/10/26 1:24 PM, Andrew Goodbody wrote:
>> The presence of the DTC environment variable breaks a binman test that
>> attempts to execute dtc from a temporary directory using the value of
>> DTC but that is a relative directory path based on the source tree not
>> this temporary directory and so it fails.
>> It also breaks a buildman test that checks the environment and asserts
>> that DTC is not present.
>> Remove the setting of DTC as it is not needed.
>>
> 
> What happens if you have DTC exported in your current environment before 
> the tests are run? It'd be nice to mention which tests exactly fail for 
> future reference.

It will fail if DTC is already set in the environment. I will add the 
failing test names to the commit message.

> I'm thinking this is not enough and we should also fix the binman and 
> buildman tests to run with an environment where the DTC variable is 
> removed/absent/set to the correct path so we don't depend on a "correct" 
> external environment.

I was hoping that assuming a 'clean' environment would be acceptable but 
I can look at what needs to be done to make the test work no matter what 
the DTC variable is set to on entry.

Andrew

> Cheers,
> Quentin

Re: [PATCH v2 07/12] binman: bintool: Do not add empty argument

[Quentin Schulz]

Hi Andrew,

On 11/10/26 1:24 PM, Andrew Goodbody wrote:
> If lzma_alone is invoked with an empty argument it will emit a message
> to stderr. When 'combined' is used to examine the output of this command
> the content is not deterministic in that stdout and stderr may appear
> out of order on occasions. To prevent this situation do not add an
> argument list to the command when it is empty. In this situation no
> error message is emitted only the usage and this is always sent to only
> stdout or only to stderr depending on the version of lzma_alone.
> 

Then this patch should be before patch 2 otherwise we use broken code.

> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
> ---
>   tools/binman/bintool.py | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/binman/bintool.py b/tools/binman/bintool.py
> index 4cf8d5b165b..15e2df2fd80 100644
> --- a/tools/binman/bintool.py
> +++ b/tools/binman/bintool.py
> @@ -290,7 +290,10 @@ class Bintool:
>           if self.name in self.missing_list:
>               return None
>           name = os.path.expanduser(self.name)  # Expand paths containing ~
> -        all_args = (name,) + args
> +        if args[0] == '':

question: is this really correct?

What if we really want an empty argument?

Say

echo '' 'test'

for a simple example, or even

echo ''

which will print a newline.

I'm wondering if you cannot simply remove the '' argument in the caller 
of run_cmd_result?

That is:

out = self.run_cmd_result(raise_on_error=False).combined.strip()

in tools/binman/btool/lzma_alone.py?

Cheers,
Quentin

Re: [PATCH v2 03/12] test: Do not set DTC environment variable

[Quentin Schulz]

On 11/10/26 3:30 PM, Andrew Goodbody wrote:
> On 10/11/2026 13:30, Quentin Schulz wrote:
>> Hi Andrew,
>>
>> On 11/10/26 1:24 PM, Andrew Goodbody wrote:
>>> The presence of the DTC environment variable breaks a binman test that
>>> attempts to execute dtc from a temporary directory using the value of
>>> DTC but that is a relative directory path based on the source tree not
>>> this temporary directory and so it fails.
>>> It also breaks a buildman test that checks the environment and asserts
>>> that DTC is not present.
>>> Remove the setting of DTC as it is not needed.
>>>
>>
>> What happens if you have DTC exported in your current environment 
>> before the tests are run? It'd be nice to mention which tests exactly 
>> fail for future reference.
> 
> It will fail if DTC is already set in the environment. I will add the 
> failing test names to the commit message.
> 
>> I'm thinking this is not enough and we should also fix the binman and 
>> buildman tests to run with an environment where the DTC variable is 
>> removed/absent/set to the correct path so we don't depend on a 
>> "correct" external environment.
> 
> I was hoping that assuming a 'clean' environment would be acceptable but 
> I can look at what needs to be done to make the test work no matter what 
> the DTC variable is set to on entry.
> 

Mock the environment with with unittest.mock.patch.dict('os.environ', 
environ) for example, see tools/binman/ftest.py for examples. If you 
want to only remove some variables, you can duplicate the environment, 
delete the key from the dict and use that as mock value.

Cheers,
Quentin

Re: [PATCH v2 08/12] test: Build mkeficapsule in tree used by tests

[Quentin Schulz]

Hi Andrew,

On 11/10/26 1:24 PM, Andrew Goodbody wrote:
> Since the commit to only build mkeficapsule when needed rather than by
> default, the build tree build-sandbox_spl no longer contains the built
> version of mkeficapsule and this can lead a test failure. Add the
> config line to the defconfig so that mkeficapsule is built and the tests
> will now work. 'make savedefconfig' was used to generate the new
> defconfig file.
> 

This sounds like this is the incorrect way to fix the issue. The proper 
fix is probably to gate the test on the presence of 
CONFIG_TOOLS_MKEFICAPSULE in the config.

@pytest.mark.buildconfigspec('tools_mkeficapsule')

is likely the required fix before the test?

At the same time, we want to make sure this gets tested, so if nothing 
actually runs this test anymore, we should indeed add it to sandbox_spl 
(or any other defconfig that is used for testing), but in a separate 
patch, that justifies this addition not by saying we unbreak a test, but 
that we actually run a test.

Cheers,
Quentin

Re: [PATCH v2 06/12] test: Disable running event_dump test for make check

[Andrew Goodbody]

On 10/11/2026 14:25, Quentin Schulz wrote:
> Hi Andrew,
> 
> On 11/10/26 1:24 PM, Andrew Goodbody wrote:
>> The event dump script does not work on more recent versions of binutils
> 
> Please provide the error log or something so that we get a chance to fix 
> this in the future.
> 
> Cheers,
> Quentin

This is a known issue and was already addressed in CI last year.

9420160a0d14 ("CI: Move to Ubuntu 24.04 'Noble' as the base")

The error is that in recent versions of binutils addr2line cannot get 
the source reference for static functions. Whether the information is 
missing from the debug data or is just a failure of addr2line is not known.

Andrew

__________________________________________________________ 
test_event_dump ___________________________________________________________
test/py/tests/test_event_dump.py:26: in test_event_dump
     assert re.match(expect, out, re.MULTILINE) is not None
E   AssertionError: assert None is not None
E    +  where None = <function match at 0x7f0798574e00>('.*Event type 
         Id                              Source 
location\n--------------------  ----------------------...check 
.*arch/sandbox/cpu/start.c:.*\nEVT_TEST              h_adder_simple 
             .*test/common/event.c:', 'Event type            Id 
                     Source location\n-------------------- 
------------------------...pt_check 
build-sandbox/../arch/sandbox/cpu/start.c:61\nEVT_TEST 
h_adder_simple                  :?\n', re.MULTILINE)
E    +    where <function match at 0x7f0798574e00> = re.match
E    +    and   re.MULTILINE = re.MULTILINE
-------------------------------------------------------- Captured stdout 
call --------------------------------------------------------
+scripts/event_dump.py /home/andrew/src/work/u-boot/build-sandbox/u-boot
Event type            Id                              Source location
--------------------  ------------------------------ 
------------------------------
EVT_DM_PRE_REMOVE     dm_mdio_mux_parent_pre_remove   :?
EVT_FT_FIXUP          bootmeth_vbe_ft_fixup           :?
EVT_FT_FIXUP          bootmeth_vbe_simple_ft_fixup    :?
EVT_LAST_STAGE_INIT   alloc_write_acpi_tables         :?
EVT_LAST_STAGE_INIT   efi_block_device_create         :?
EVT_LAST_STAGE_INIT   install_smbios_table            :?
EVT_LAST_STAGE_INIT   tpm_evt_autostart               :?
EVT_MISC_INIT_F       sandbox_early_getopt_check 
build-sandbox/../arch/sandbox/cpu/start.c:61
EVT_TEST              h_adder_simple                  :?
====================================================== short test 
summary info =======================================================
FAILED test/py/tests/test_event_dump.py::test_event_dump - 
AssertionError: assert None is not None
========================= 1 failed, 1151 passed, 170 skipped, 345 
deselected, 1 xfailed in 147.30s (0:02:27) =========================

Re: [PATCH v2 06/12] test: Disable running event_dump test for make check

[Quentin Schulz]

On 11/10/26 3:51 PM, Andrew Goodbody wrote:
> On 10/11/2026 14:25, Quentin Schulz wrote:
>> Hi Andrew,
>>
>> On 11/10/26 1:24 PM, Andrew Goodbody wrote:
>>> The event dump script does not work on more recent versions of binutils
>>
>> Please provide the error log or something so that we get a chance to 
>> fix this in the future.
>>
>> Cheers,
>> Quentin
> 
> This is a known issue and was already addressed in CI last year.
> 
> 9420160a0d14 ("CI: Move to Ubuntu 24.04 'Noble' as the base")
> 
> The error is that in recent versions of binutils addr2line cannot get 
> the source reference for static functions. Whether the information is 
> missing from the debug data or is just a failure of addr2line is not known.
> 

Thanks, just have the output below in the commit log + your explanation 
above and we're good :)

Cheers,
Quentin

Re: [PATCH v2 00/12] Fix 'make check' errors

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1635 bytes --]

On Tue, Nov 10, 2026 at 12:23:57PM +0000, Andrew Goodbody wrote:

> Various tests can be run using 'make check', 'make qcheck' and 'make
> tcheck'. These have become broken and need fixing to be useful again.
> 
> Running 'make qcheck' fails on the binman test to check the version
> numbers of the tools are found as expected. Update the code for the
> tools that have been updated and no longer have their version numbers
> found by the code.
> 
> There are also failures in buildman. First add code to understand 'gcc
> -E' as an alternative to cpp. Second add the code to accept an empty
> CROSS_COMPILE variable in the environment when testing that
> CROSS_COMPILE is not set by the toolchain selection.
> 
> dtoc was not achieving 100% code coverage so add a new test to rectify
> that.
> 
> Disable event_dump tests that are broken on newer versions of binutils.
> 
> Build mkeficapsule in the tree that is used for testing the tools.
> 
> 'make tcheck' was not achieving 100% code coverage and needs to know the
> location of the built tools to resolve.
> 
> 'make check' tests of the file system had errors due to ext4 file
> systems being created with an orphaned file but were marked clean.

The biggest problem I see is that we need to step back and see what of
make check/qcheck/tcheck make sense to support at this point. As you run
in to, and was my concern last I tried any of these targets, things
quickly become very host dependent. Which is why we have CI abstracting
all of this away. Personally, I found it best to just fire off a
one-shot container to run pytest in.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 09/12] test: Pass location of built tools to binman code coverage test

[Quentin Schulz]

Hi Andrew,

On 11/10/26 1:24 PM, Andrew Goodbody wrote:
> When running binman for code coverage tests and invoked by 'make tcheck'
> some tests can fail if they use system installed versions of tools
> instead of ones built from the tree under test. Ensure that only built
> versions of tools are used by passing the location of these tools to the
> binman test being run.
> 
> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
> ---
>   test/run | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/test/run b/test/run
> index d1654dffb5f..0857fc14c9d 100755
> --- a/test/run
> +++ b/test/run
> @@ -93,7 +93,7 @@ run_test "dtoc" ./tools/dtoc/dtoc -t
>   # Code-coverage tests cannot run in parallel, so skip them in that case
>   if [ -z "${para}" ]; then
>   	export PATH=$PATH:${TOOLS_DIR}
> -	run_test "binman code coverage" ./tools/binman/binman test -T
> +	run_test "binman code coverage" ./tools/binman/binman --toolpath ${TOOLS_DIR} test -T

question: Quickly reading the code, I'm not sure this "ensure"s that 
only built versions of tools are used? binman's code is very convoluted 
so it's not easy to figure out what --toolpath actually does without 
digging deep into it. But if you say so, then fine with me.

question: Don't we want to (also?) start PATH with TOOLS_DIR instead of 
end with it?

Cheers,
Quentin

Re: [PATCH v2 10/12] tools: Fix typo in name of command mkeficapsule

[Quentin Schulz]

Hi Andrew,

On 11/10/26 1:24 PM, Andrew Goodbody wrote:
> The description for the config item TOOLS_MKEFICAPSULE has a typo, fix
> it. efimkcapsule -> mkeficapsule
> 

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH v2 11/12] buildman: Cope with CPP="gcc -E"

[Quentin Schulz]

Hi Andrew,

On 11/10/26 1:24 PM, Andrew Goodbody wrote:
> Allow the use of "gcc -E" as an alternative to "cpp" when set in the
> environment as the value of CPP. Accept it as a valid command for
> running the preprocessor and adjust the test to not depend on a fixed
> number of arguments. Also split it into parts for constructing the
> command list so that the validation for running a command does not fail
> when it tries to find an executable named "gcc -E".
> 
> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
> ---
>   tools/buildman/boards.py    | 3 +--
>   tools/buildman/func_test.py | 5 +++--
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/buildman/boards.py b/tools/buildman/boards.py
> index 5d5d3a3822b..111789f1bd4 100644
> --- a/tools/buildman/boards.py
> +++ b/tools/buildman/boards.py
> @@ -244,8 +244,7 @@ class KconfigScanner:
>   
>           temp = None
>           if b'#include' in tools.read_file(defconfig):
> -            cmd = [
> -                os.getenv('CPP', 'cpp'),
> +            cmd = os.getenv('CPP', 'cpp').split() + [

issue: str.split() is not safe for shell commands.

I think the proper way is to run shlex.split() on it instead of a simple 
str.split()

>                   '-nostdinc', '-P',
>                   '-I', self._srctree,
>                   '-undef',
> diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py
> index fce615ba8b8..eb13589ba17 100644
> --- a/tools/buildman/func_test.py
> +++ b/tools/buildman/func_test.py
> @@ -378,7 +378,7 @@ class TestFunctional(unittest.TestCase):
>       def _HandleCommandCpp(self, args):
>           # args ['-nostdinc', '-P', '-I', '/tmp/tmp7f17xk_o/src', '-undef',
>           # '-x', 'assembler-with-cpp', fname]
> -        fname = args[7]
> +        fname = args[-1]

todo: add a comment with the new possible list of arguments

This should explain why we are using -1 instead of an offset from the 
beginning.

>           buf = io.StringIO()
>           for line in tools.read_file(fname, False).splitlines():
>               if line.startswith('#include'):
> @@ -424,7 +424,8 @@ class TestFunctional(unittest.TestCase):
>               return self._HandleCommandObjcopy(args)
>           elif cmd.endswith( 'size'):
>               return self._HandleCommandSize(args)
> -        elif cmd.endswith( 'cpp'):
> +        elif cmd.endswith('cpp') \
> +                or (cmd.endswith('gcc') and pipe_list[0][1] == '-E'):

question: can't -E be anywhere in the list?

Should we use "'-E' in pipe_list[0]" instead maybe?

question: what about "gcc --preprocess"?

This is the long-format version of -E according to gcc(1) manpage.

question: what if we want to use clang instead?

It seems like clang also supports -E/--preprocess.

Cheers,
Quentin

Re: [PATCH v2 12/12] test: Fix test_basic file system tests

[Quentin Schulz]

Hi Andrew,

On 11/10/26 1:24 PM, Andrew Goodbody wrote:
> Ensure that /sbin is on the path so that distributions that do not have
> it will still be able to successfully run these tests that need fsck.
> Add a forced clean of the newly created file system to cope with the
> fact that there is an orphaned file on ext4 that will cause the
> integrity test to fail later on if not removed now.
> 

question: are those really related?

It feels to me that those should be fixed in separate commits?

> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
> ---
>   test/py/tests/fs_helper.py              | 4 ++++
>   test/py/tests/test_fs/fstest_helpers.py | 5 +++++
>   2 files changed, 9 insertions(+)
> 
> diff --git a/test/py/tests/fs_helper.py b/test/py/tests/fs_helper.py
> index e3824b2c1fd..48b047aeefd 100644
> --- a/test/py/tests/fs_helper.py
> +++ b/test/py/tests/fs_helper.py
> @@ -250,6 +250,10 @@ def mk_fs(config, fs_type, size, prefix, src_dir=None, fs_img=None, quiet=False)
>                                         shell=True).decode()
>               if 'metadata_csum' in sb_content:
>                   check_call(f'tune2fs -O ^metadata_csum {fs_img}', shell=True)
> +            # Cannot use check_call here as it will return an exit code of 1
> +            # as it modifies the file system. The -f is needed as although the
> +            # fs is marked clean, it is not really.
> +            call(f'fsck.ext4 -f -y {fs_img}', shell=True)

nitpick: just use an array to avoid shell=True

Considering the rest of the file still uses shell=True with a single 
string, I guess it's fine for now but we should really think about 
getting rid of those.

>           elif fs_lnxtype == 'vfat' and src_dir:
>               flags = f"-smpQ{'' if quiet else 'v'}"
>               check_call(f'mcopy -i {fs_img} {flags} {src_dir}/* ::/',
> diff --git a/test/py/tests/test_fs/fstest_helpers.py b/test/py/tests/test_fs/fstest_helpers.py
> index d25326ee993..6164eca476a 100644
> --- a/test/py/tests/test_fs/fstest_helpers.py
> +++ b/test/py/tests/test_fs/fstest_helpers.py
> @@ -3,9 +3,14 @@
>   # Author: JJ Hiblot <jjhiblot@ti.com>
>   #
>   
> +import os
>   from subprocess import check_call, CalledProcessError
>   
>   def assert_fs_integrity(fs_type, fs_img):
> +    # Some distributions do not add /sbin to the default PATH, where fsck lives
> +    if '/sbin' not in os.environ["PATH"].split(os.pathsep):
> +        os.environ["PATH"] += os.pathsep + '/sbin'
> +

issue: modifying the environment like this is a big no-no.

Modifying the environment like this should really be last resort as it 
breaks things in various sneaky and random ways.

check_call() takes an env parameter, so copy os.environ and edit the 
PATH key, then pass this copy as env parameter to all check_call calls 
in this function.

Cheers,
Quentin

Re: [PATCH v2 00/12] Fix 'make check' errors

[Andrew Goodbody]

On 10/11/2026 14:55, Tom Rini wrote:
> On Tue, Nov 10, 2026 at 12:23:57PM +0000, Andrew Goodbody wrote:
> 
>> Various tests can be run using 'make check', 'make qcheck' and 'make
>> tcheck'. These have become broken and need fixing to be useful again.
>>
>> Running 'make qcheck' fails on the binman test to check the version
>> numbers of the tools are found as expected. Update the code for the
>> tools that have been updated and no longer have their version numbers
>> found by the code.
>>
>> There are also failures in buildman. First add code to understand 'gcc
>> -E' as an alternative to cpp. Second add the code to accept an empty
>> CROSS_COMPILE variable in the environment when testing that
>> CROSS_COMPILE is not set by the toolchain selection.
>>
>> dtoc was not achieving 100% code coverage so add a new test to rectify
>> that.
>>
>> Disable event_dump tests that are broken on newer versions of binutils.
>>
>> Build mkeficapsule in the tree that is used for testing the tools.
>>
>> 'make tcheck' was not achieving 100% code coverage and needs to know the
>> location of the built tools to resolve.
>>
>> 'make check' tests of the file system had errors due to ext4 file
>> systems being created with an orphaned file but were marked clean.
> 
> The biggest problem I see is that we need to step back and see what of
> make check/qcheck/tcheck make sense to support at this point. As you run
> in to, and was my concern last I tried any of these targets, things
> quickly become very host dependent. Which is why we have CI abstracting
> all of this away. Personally, I found it best to just fire off a
> one-shot container to run pytest in.
This all started when I found a suggestion to run 'make qcheck' on the 
U-Boot source tree to confirm all was good. Of course I found that it 
was not good and started to fix things.

I do not mind if we just remove 'make check' and friends or if we 
resolve the issues but the status quo at the moment is a bad place to be 
in. I have tried to make my fixes be less host dependent than they were.

CI is all well and good but having a local smoke test seems to me to be 
of some value. If the request is going to be 'please rewrite this whole 
mechanism to use containers' then I am not sure that I will have the 
time for that.

I would appreciate some indication of how to proceed here because while 
I understand and agree with your concern I am not sure what is being 
asked of me.

Andrew

Re: [PATCH v2 05/12] dtoc: test: Fix coverage by adding test

[Andrew Goodbody]

On 10/11/2026 14:25, Quentin Schulz wrote:
> Hi Andrew,
> 
> On 11/10/26 1:24 PM, Andrew Goodbody wrote:
>> Use of 'indir' in EnsureCompiled() was not being tested and resulted in
>> less than 100% code coverage. Add a test and supporting files that
>> depends on the use of 'indir' by placing an included file in a new
>> directory. The dts file will fail to compile if the new directory is not
>> added to the search path with the use of 'indir'.
>>
>> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
>> ---
>>   tools/dtoc/test/dtoc_test_include.dts                  | 13 ++++++++ 
>> +++++
>>   tools/dtoc/test/newinclude/dtoc_test_include_file.dtsi |  7 +++++++
>>   tools/dtoc/test_fdt.py                                 |  7 +++++++
>>   3 files changed, 27 insertions(+)
>>
>> diff --git a/tools/dtoc/test/dtoc_test_include.dts b/tools/dtoc/test/ 
>> dtoc_test_include.dts
>> new file mode 100644
>> index 00000000000..b5ef647c1b6
>> --- /dev/null
>> +++ b/tools/dtoc/test/dtoc_test_include.dts
>> @@ -0,0 +1,13 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Test device tree file for dtoc
>> + *
>> + * Copyright 2017 Google, Inc
>> + */
>> +
>> +/dts-v1/;
>> +
>> +/ {
>> +};
>> +
>> +#include "dtoc_test_include_file.dtsi"
> 
> suggestion: move this line close to /dts-v1/

Ack.

> I was about to dig deep into fdt_util.EnsureCompiled() to see what kind 
> of magic it does to automatically include a file because I had 
> completely missed this line. In DTS, we usually have the #include follow 
> the /dts-v1/; line, can you please move this up so it more closely matches?
> 
>> diff --git a/tools/dtoc/test/newinclude/dtoc_test_include_file.dtsi b/ 
>> tools/dtoc/test/newinclude/dtoc_test_include_file.dtsi
>> new file mode 100644
>> index 00000000000..f975005d023
>> --- /dev/null
>> +++ b/tools/dtoc/test/newinclude/dtoc_test_include_file.dtsi
>> @@ -0,0 +1,7 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +/ {
>> +    new_node {
>> +        intval = <2>;
>> +    };
>> +};
>> diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py
>> index 66d346b1272..a63c860eae1 100755
>> --- a/tools/dtoc/test_fdt.py
>> +++ b/tools/dtoc/test_fdt.py
>> @@ -1105,6 +1105,13 @@ class TestFdtUtil(unittest.TestCase):
>>           finally:
>>               tools.outdir = old_outdir
>> +    def test_ensure_compiled_indir(self):
>> +        """Test providing directory as indir"""
>> +        indir = os.path.dirname(find_dtb_file('dtoc_test_include.dts'))
>> +        indir = os.path.join(indir, 'newinclude')
>> +        dtb = 
>> fdt_util.EnsureCompiled(find_dtb_file('dtoc_test_include.dts'), 
>> indir=[indir])
>> +        self.assertEqual(dtb, fdt_util.EnsureCompiled(dtb))
>> +
> 
> question: do we want to check the content of the DTB?

Answer: No.

> Checking for the node that comes from the included file, to make sure 
> the included directive actually works? At the same time, that would 
> essentially mean checking cc+dtc works as expected, so maybe checking 
> too much in the wrong place?

This is about checking the use of 'indir' in EnsureCompiled. It is not 
about checking the functioning of cc+dtc. If it compiles without 
complaint then that is good enough.

> Looks ok to me:

Thanks,
Andrew

> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
> 
> Thanks!
> Quentin

Re: [PATCH v2 08/12] test: Build mkeficapsule in tree used by tests

[Andrew Goodbody]

On 10/11/2026 14:49, Quentin Schulz wrote:
> Hi Andrew,
> 
> On 11/10/26 1:24 PM, Andrew Goodbody wrote:
>> Since the commit to only build mkeficapsule when needed rather than by
>> default, the build tree build-sandbox_spl no longer contains the built
>> version of mkeficapsule and this can lead a test failure. Add the
>> config line to the defconfig so that mkeficapsule is built and the tests
>> will now work. 'make savedefconfig' was used to generate the new
>> defconfig file.
>>
> 
> This sounds like this is the incorrect way to fix the issue. The proper 
> fix is probably to gate the test on the presence of 
> CONFIG_TOOLS_MKEFICAPSULE in the config.
> 
> @pytest.mark.buildconfigspec('tools_mkeficapsule')
> 
> is likely the required fix before the test?
> 
> At the same time, we want to make sure this gets tested, so if nothing 
> actually runs this test anymore, we should indeed add it to sandbox_spl 
> (or any other defconfig that is used for testing), but in a separate 
> patch, that justifies this addition not by saying we unbreak a test, but 
> that we actually run a test.
> 
> Cheers,
> Quentin

The test is run from the script test/run at line 82 [1]. As this is not 
a pytest your suggestion is not possible. I should probably have 
mentioned it was a binman test in the commit message.

The test is run for both 'make qcheck' and also 'make tcheck'. The 
sandbox_defconfig board is the only sandbox board that currently builds 
mkeficapsule but this board is not built for 'make tcheck'.

The binman tests are run pointing at the sandbox_spl_defconfig build 
tree as that is built for both 'make qcheck' and also 'make tcheck'.

So the simple, easy fixes available are either 1. Switch the binman test 
to use sandbox_defconfig build tree which means adding that build into 
the run of 'make tcheck' and so taking longer to run. 2. The fix I 
propose which is to just add the build of mkeficapsule to 
sandbox_spl_defconfig.

At the moment 'make tcheck' runs the test but it fails and it fails with 
an obscure message if it picks up the system mkeficapsule that does not 
support the right options.

Andrew

1. 
https://git.u-boot-project.org/u-boot/u-boot/-/blob/main/test/run?ref_type=heads#L82

Re: [PATCH v2 00/12] Fix 'make check' errors

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3784 bytes --]

On Tue, Nov 10, 2026 at 03:55:55PM +0000, Andrew Goodbody wrote:
> On 10/11/2026 14:55, Tom Rini wrote:
> > On Tue, Nov 10, 2026 at 12:23:57PM +0000, Andrew Goodbody wrote:
> > 
> > > Various tests can be run using 'make check', 'make qcheck' and 'make
> > > tcheck'. These have become broken and need fixing to be useful again.
> > > 
> > > Running 'make qcheck' fails on the binman test to check the version
> > > numbers of the tools are found as expected. Update the code for the
> > > tools that have been updated and no longer have their version numbers
> > > found by the code.
> > > 
> > > There are also failures in buildman. First add code to understand 'gcc
> > > -E' as an alternative to cpp. Second add the code to accept an empty
> > > CROSS_COMPILE variable in the environment when testing that
> > > CROSS_COMPILE is not set by the toolchain selection.
> > > 
> > > dtoc was not achieving 100% code coverage so add a new test to rectify
> > > that.
> > > 
> > > Disable event_dump tests that are broken on newer versions of binutils.
> > > 
> > > Build mkeficapsule in the tree that is used for testing the tools.
> > > 
> > > 'make tcheck' was not achieving 100% code coverage and needs to know the
> > > location of the built tools to resolve.
> > > 
> > > 'make check' tests of the file system had errors due to ext4 file
> > > systems being created with an orphaned file but were marked clean.
> > 
> > The biggest problem I see is that we need to step back and see what of
> > make check/qcheck/tcheck make sense to support at this point. As you run
> > in to, and was my concern last I tried any of these targets, things
> > quickly become very host dependent. Which is why we have CI abstracting
> > all of this away. Personally, I found it best to just fire off a
> > one-shot container to run pytest in.
> This all started when I found a suggestion to run 'make qcheck' on the
> U-Boot source tree to confirm all was good. Of course I found that it was
> not good and started to fix things.
> 
> I do not mind if we just remove 'make check' and friends or if we resolve
> the issues but the status quo at the moment is a bad place to be in. I have
> tried to make my fixes be less host dependent than they were.
> 
> CI is all well and good but having a local smoke test seems to me to be of
> some value. If the request is going to be 'please rewrite this whole
> mechanism to use containers' then I am not sure that I will have the time
> for that.
> 
> I would appreciate some indication of how to proceed here because while I
> understand and agree with your concern I am not sure what is being asked of
> me.

The problem I see (and have talked about before) is that having a shell
script to run $X means that it's expected to work everywhere. This was
difficult before and even harder currently.  Personally, I run mine in
containers because the last time I tried "make check" it was failing on
things passing in the container, and I had everything documented
installed.  What I would like to see is:
- The fixes this series exposes, for running tests on more modern
  distributions than CI is[1] using.
- Improving documentation how how to run tests manually. And I'm not
- See if there's things make check/etc are testing that CI is not
  testing.
  saying "and in the CI containers". Just, run them manually.
- RFC dropping make check/qcheck/tcheck in favor of pointing people at
  the docs.

And FWIW, with this series applied on current main 'make check' still
fails on my dev box, which is just Ubuntu 24.04.

[1]: Updating our containers is on my TODO list. The first step is
moving to having CI build containers. This is blocked on in-progress
work at OSU OSL.
-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 12/12] test: Fix test_basic file system tests

[Andrew Goodbody]

On 10/11/2026 15:39, Quentin Schulz wrote:
> Hi Andrew,
> 
> On 11/10/26 1:24 PM, Andrew Goodbody wrote:
>> Ensure that /sbin is on the path so that distributions that do not have
>> it will still be able to successfully run these tests that need fsck.
>> Add a forced clean of the newly created file system to cope with the
>> fact that there is an orphaned file on ext4 that will cause the
>> integrity test to fail later on if not removed now.
>>
> 
> question: are those really related?
> 
> It feels to me that those should be fixed in separate commits?

OK.

>> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
>> ---
>>   test/py/tests/fs_helper.py              | 4 ++++
>>   test/py/tests/test_fs/fstest_helpers.py | 5 +++++
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/test/py/tests/fs_helper.py b/test/py/tests/fs_helper.py
>> index e3824b2c1fd..48b047aeefd 100644
>> --- a/test/py/tests/fs_helper.py
>> +++ b/test/py/tests/fs_helper.py
>> @@ -250,6 +250,10 @@ def mk_fs(config, fs_type, size, prefix, 
>> src_dir=None, fs_img=None, quiet=False)
>>                                         shell=True).decode()
>>               if 'metadata_csum' in sb_content:
>>                   check_call(f'tune2fs -O ^metadata_csum {fs_img}', 
>> shell=True)
>> +            # Cannot use check_call here as it will return an exit 
>> code of 1
>> +            # as it modifies the file system. The -f is needed as 
>> although the
>> +            # fs is marked clean, it is not really.
>> +            call(f'fsck.ext4 -f -y {fs_img}', shell=True)
> 
> nitpick: just use an array to avoid shell=True
> 
> Considering the rest of the file still uses shell=True with a single 
> string, I guess it's fine for now but we should really think about 
> getting rid of those.
> 
>>           elif fs_lnxtype == 'vfat' and src_dir:
>>               flags = f"-smpQ{'' if quiet else 'v'}"
>>               check_call(f'mcopy -i {fs_img} {flags} {src_dir}/* ::/',
>> diff --git a/test/py/tests/test_fs/fstest_helpers.py b/test/py/tests/ 
>> test_fs/fstest_helpers.py
>> index d25326ee993..6164eca476a 100644
>> --- a/test/py/tests/test_fs/fstest_helpers.py
>> +++ b/test/py/tests/test_fs/fstest_helpers.py
>> @@ -3,9 +3,14 @@
>>   # Author: JJ Hiblot <jjhiblot@ti.com>
>>   #
>> +import os
>>   from subprocess import check_call, CalledProcessError
>>   def assert_fs_integrity(fs_type, fs_img):
>> +    # Some distributions do not add /sbin to the default PATH, where 
>> fsck lives
>> +    if '/sbin' not in os.environ["PATH"].split(os.pathsep):
>> +        os.environ["PATH"] += os.pathsep + '/sbin'
>> +
> 
> issue: modifying the environment like this is a big no-no.

I copied the code from fs_helper.py but OK.

> Modifying the environment like this should really be last resort as it 
> breaks things in various sneaky and random ways.
> 
> check_call() takes an env parameter, so copy os.environ and edit the 
> PATH key, then pass this copy as env parameter to all check_call calls 
> in this function.

Ack.

Thanks,
Andrew

> Cheers,
> Quentin

Re: [PATCH v2 07/12] binman: bintool: Do not add empty argument

[Andrew Goodbody]

On 10/11/2026 14:41, Quentin Schulz wrote:
> Hi Andrew,
> 
> On 11/10/26 1:24 PM, Andrew Goodbody wrote:
>> If lzma_alone is invoked with an empty argument it will emit a message
>> to stderr. When 'combined' is used to examine the output of this command
>> the content is not deterministic in that stdout and stderr may appear
>> out of order on occasions. To prevent this situation do not add an
>> argument list to the command when it is empty. In this situation no
>> error message is emitted only the usage and this is always sent to only
>> stdout or only to stderr depending on the version of lzma_alone.
>>
> 
> Then this patch should be before patch 2 otherwise we use broken code.

OK.

>> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
>> ---
>>   tools/binman/bintool.py | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/binman/bintool.py b/tools/binman/bintool.py
>> index 4cf8d5b165b..15e2df2fd80 100644
>> --- a/tools/binman/bintool.py
>> +++ b/tools/binman/bintool.py
>> @@ -290,7 +290,10 @@ class Bintool:
>>           if self.name in self.missing_list:
>>               return None
>>           name = os.path.expanduser(self.name)  # Expand paths 
>> containing ~
>> -        all_args = (name,) + args
>> +        if args[0] == '':
> 
> question: is this really correct?
> 
> What if we really want an empty argument?
> 
> Say
> 
> echo '' 'test'
> 
> for a simple example, or even
> 
> echo ''
> 
> which will print a newline.
> 
> I'm wondering if you cannot simply remove the '' argument in the caller 
> of run_cmd_result?
> 
> That is:
> 
> out = self.run_cmd_result(raise_on_error=False).combined.strip()
> 
> in tools/binman/btool/lzma_alone.py?

Yes, that works. Thank you.

Andrew

> Cheers,
> Quentin

Re: [PATCH v2 11/12] buildman: Cope with CPP="gcc -E"

[Andrew Goodbody]

On 10/11/2026 15:29, Quentin Schulz wrote:
> Hi Andrew,
> 
> On 11/10/26 1:24 PM, Andrew Goodbody wrote:
>> Allow the use of "gcc -E" as an alternative to "cpp" when set in the
>> environment as the value of CPP. Accept it as a valid command for
>> running the preprocessor and adjust the test to not depend on a fixed
>> number of arguments. Also split it into parts for constructing the
>> command list so that the validation for running a command does not fail
>> when it tries to find an executable named "gcc -E".
>>
>> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
>> ---
>>   tools/buildman/boards.py    | 3 +--
>>   tools/buildman/func_test.py | 5 +++--
>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/buildman/boards.py b/tools/buildman/boards.py
>> index 5d5d3a3822b..111789f1bd4 100644
>> --- a/tools/buildman/boards.py
>> +++ b/tools/buildman/boards.py
>> @@ -244,8 +244,7 @@ class KconfigScanner:
>>           temp = None
>>           if b'#include' in tools.read_file(defconfig):
>> -            cmd = [
>> -                os.getenv('CPP', 'cpp'),
>> +            cmd = os.getenv('CPP', 'cpp').split() + [
> 
> issue: str.split() is not safe for shell commands.
> 
> I think the proper way is to run shlex.split() on it instead of a simple 
> str.split()

OK.

>>                   '-nostdinc', '-P',
>>                   '-I', self._srctree,
>>                   '-undef',
>> diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py
>> index fce615ba8b8..eb13589ba17 100644
>> --- a/tools/buildman/func_test.py
>> +++ b/tools/buildman/func_test.py
>> @@ -378,7 +378,7 @@ class TestFunctional(unittest.TestCase):
>>       def _HandleCommandCpp(self, args):
>>           # args ['-nostdinc', '-P', '-I', '/tmp/tmp7f17xk_o/src', '- 
>> undef',
>>           # '-x', 'assembler-with-cpp', fname]
>> -        fname = args[7]
>> +        fname = args[-1]
> 
> todo: add a comment with the new possible list of arguments
> 
> This should explain why we are using -1 instead of an offset from the 
> beginning.

OK.

>>           buf = io.StringIO()
>>           for line in tools.read_file(fname, False).splitlines():
>>               if line.startswith('#include'):
>> @@ -424,7 +424,8 @@ class TestFunctional(unittest.TestCase):
>>               return self._HandleCommandObjcopy(args)
>>           elif cmd.endswith( 'size'):
>>               return self._HandleCommandSize(args)
>> -        elif cmd.endswith( 'cpp'):
>> +        elif cmd.endswith('cpp') \
>> +                or (cmd.endswith('gcc') and pipe_list[0][1] == '-E'):
> 
> question: can't -E be anywhere in the list?
> Should we use "'-E' in pipe_list[0]" instead maybe?
> 
> question: what about "gcc --preprocess"?
> 
> This is the long-format version of -E according to gcc(1) manpage.
> 
> question: what if we want to use clang instead?
> 
> It seems like clang also supports -E/--preprocess.

OK. Updated to allow gcc or clang and -E or --preprocess anywhere in the 
args list.

Andrew

> Cheers,
> Quentin

Re: [PATCH v2 09/12] test: Pass location of built tools to binman code coverage test

[Andrew Goodbody]

On 10/11/2026 15:06, Quentin Schulz wrote:
> Hi Andrew,
> 
> On 11/10/26 1:24 PM, Andrew Goodbody wrote:
>> When running binman for code coverage tests and invoked by 'make tcheck'
>> some tests can fail if they use system installed versions of tools
>> instead of ones built from the tree under test. Ensure that only built
>> versions of tools are used by passing the location of these tools to the
>> binman test being run.
>>
>> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
>> ---
>>   test/run | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/test/run b/test/run
>> index d1654dffb5f..0857fc14c9d 100755
>> --- a/test/run
>> +++ b/test/run
>> @@ -93,7 +93,7 @@ run_test "dtoc" ./tools/dtoc/dtoc -t
>>   # Code-coverage tests cannot run in parallel, so skip them in that case
>>   if [ -z "${para}" ]; then
>>       export PATH=$PATH:${TOOLS_DIR}
>> -    run_test "binman code coverage" ./tools/binman/binman test -T
>> +    run_test "binman code coverage" ./tools/binman/binman --toolpath 
>> ${TOOLS_DIR} test -T
> 
> question: Quickly reading the code, I'm not sure this "ensure"s that 
> only built versions of tools are used? binman's code is very convoluted 
> so it's not easy to figure out what --toolpath actually does without 
> digging deep into it. But if you say so, then fine with me.

I can reword the commit message along the lines of "Use the toolpath 
option to point at the locally built tools so that these built versions 
are chosen before system ones.

> question: Don't we want to (also?) start PATH with TOOLS_DIR instead of 
> end with it?

That is actually an alternative fix. I chose to add the toolpath option 
to make it match the invocation of binman a few lines above. With that I 
can drop the change to PATH and make it all a bit more consistent. There 
seems no need for both the change to PATH and the use of toolpath.

Andrew

> Cheers,
> Quentin

Re: [PATCH v2 09/12] test: Pass location of built tools to binman code coverage test

[Quentin Schulz]

On 11/11/26 3:34 PM, Andrew Goodbody wrote:
> On 10/11/2026 15:06, Quentin Schulz wrote:
>> Hi Andrew,
>>
>> On 11/10/26 1:24 PM, Andrew Goodbody wrote:
>>> When running binman for code coverage tests and invoked by 'make tcheck'
>>> some tests can fail if they use system installed versions of tools
>>> instead of ones built from the tree under test. Ensure that only built
>>> versions of tools are used by passing the location of these tools to the
>>> binman test being run.
>>>
>>> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
>>> ---
>>>   test/run | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/test/run b/test/run
>>> index d1654dffb5f..0857fc14c9d 100755
>>> --- a/test/run
>>> +++ b/test/run
>>> @@ -93,7 +93,7 @@ run_test "dtoc" ./tools/dtoc/dtoc -t
>>>   # Code-coverage tests cannot run in parallel, so skip them in that 
>>> case
>>>   if [ -z "${para}" ]; then
>>>       export PATH=$PATH:${TOOLS_DIR}
>>> -    run_test "binman code coverage" ./tools/binman/binman test -T
>>> +    run_test "binman code coverage" ./tools/binman/binman --toolpath 
>>> ${TOOLS_DIR} test -T
>>
>> question: Quickly reading the code, I'm not sure this "ensure"s that 
>> only built versions of tools are used? binman's code is very 
>> convoluted so it's not easy to figure out what --toolpath actually 
>> does without digging deep into it. But if you say so, then fine with me.
> 
> I can reword the commit message along the lines of "Use the toolpath 
> option to point at the locally built tools so that these built versions 
> are chosen before system ones.
> 

Yes please. Otherwise this implies that a tool we somehow forgot to 
build but is present on the host, would not be found, but as far as I 
understood, it will be found and thus this is misleading.

>> question: Don't we want to (also?) start PATH with TOOLS_DIR instead 
>> of end with it?
> 
> That is actually an alternative fix. I chose to add the toolpath option 
> to make it match the invocation of binman a few lines above. With that I 
> can drop the change to PATH and make it all a bit more consistent. There 
> seems no need for both the change to PATH and the use of toolpath.
> 

ACK.

Cheers,
Quentin

Re: [PATCH v2 03/12] test: Do not set DTC environment variable

[Andrew Goodbody]

On 10/11/2026 14:43, Quentin Schulz wrote:
> 
> 
> On 11/10/26 3:30 PM, Andrew Goodbody wrote:
>> On 10/11/2026 13:30, Quentin Schulz wrote:
>>> Hi Andrew,
>>>
>>> On 11/10/26 1:24 PM, Andrew Goodbody wrote:
>>>> The presence of the DTC environment variable breaks a binman test that
>>>> attempts to execute dtc from a temporary directory using the value of
>>>> DTC but that is a relative directory path based on the source tree not
>>>> this temporary directory and so it fails.
>>>> It also breaks a buildman test that checks the environment and asserts
>>>> that DTC is not present.
>>>> Remove the setting of DTC as it is not needed.
>>>>
>>>
>>> What happens if you have DTC exported in your current environment 
>>> before the tests are run? It'd be nice to mention which tests exactly 
>>> fail for future reference.
>>
>> It will fail if DTC is already set in the environment. I will add the 
>> failing test names to the commit message.
>>
>>> I'm thinking this is not enough and we should also fix the binman and 
>>> buildman tests to run with an environment where the DTC variable is 
>>> removed/absent/set to the correct path so we don't depend on a 
>>> "correct" external environment.

OK and will split into 3 commits.

>> I was hoping that assuming a 'clean' environment would be acceptable 
>> but I can look at what needs to be done to make the test work no 
>> matter what the DTC variable is set to on entry.
>>
> 
> Mock the environment with with unittest.mock.patch.dict('os.environ', 
> environ) for example, see tools/binman/ftest.py for examples. If you 
> want to only remove some variables, you can duplicate the environment, 
> delete the key from the dict and use that as mock value.

Thanks. Will do.

Andrew

> Cheers,
> Quentin

Re: [PATCH v2 08/12] test: Build mkeficapsule in tree used by tests

[Quentin Schulz]

Hi Andrew,

On 11/10/26 5:35 PM, Andrew Goodbody wrote:
> On 10/11/2026 14:49, Quentin Schulz wrote:
>> Hi Andrew,
>>
>> On 11/10/26 1:24 PM, Andrew Goodbody wrote:
>>> Since the commit to only build mkeficapsule when needed rather than by
>>> default, the build tree build-sandbox_spl no longer contains the built
>>> version of mkeficapsule and this can lead a test failure. Add the
>>> config line to the defconfig so that mkeficapsule is built and the tests
>>> will now work. 'make savedefconfig' was used to generate the new
>>> defconfig file.
>>>
>>
>> This sounds like this is the incorrect way to fix the issue. The 
>> proper fix is probably to gate the test on the presence of 
>> CONFIG_TOOLS_MKEFICAPSULE in the config.
>>
>> @pytest.mark.buildconfigspec('tools_mkeficapsule')
>>
>> is likely the required fix before the test?
>>
>> At the same time, we want to make sure this gets tested, so if nothing 
>> actually runs this test anymore, we should indeed add it to 
>> sandbox_spl (or any other defconfig that is used for testing), but in 
>> a separate patch, that justifies this addition not by saying we 
>> unbreak a test, but that we actually run a test.
>>
>> Cheers,
>> Quentin
> 
> The test is run from the script test/run at line 82 [1]. As this is not 
> a pytest your suggestion is not possible. I should probably have 
> mentioned it was a binman test in the commit message.
> 

Ah, I see, sorry for the noise. Expand a bit on the commit log for the 
v3 and I think we're good thanks.

Cheers,
Quentin