Fix 'make check' errors

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.

Hi Andrew,

Quoted 8 lines · 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.

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

Quoted 12 lines · Quentin Schulz

Hi Andrew,

Quoted 8 lines · 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.

Quoted 1 line

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.

Quoted 1 line

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.

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 <[email protected]>

---
  tools/buildman/test.py | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

tools/buildman/test.py +0 -0

index 7ee9496ffb3..e620a92126f 100644

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 <[email protected]>

---
  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(+)

tools/dtoc/test/dtoc_test_include.dts new file +3 -0

new file mode 100644
index 00000000000..b5ef647c1b6
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Test device tree file for dtoc

tools/dtoc/test/newinclude/dtoc_test_include_file.dtsi new file +7 -0

new file mode 100644
index 00000000000..f975005d023
@@ -0,0 +1,7 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/ {
+	new_node {
+		intval = <2>;
+	};
+};

tools/dtoc/test_fdt.py +0 -0

index 66d346b1272..a63c860eae1 100755

Hi Andrew,

Quoted 27 lines · 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 <[email protected]>

---
  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(+)

tools/dtoc/test/dtoc_test_include.dts new file +7 -0

new file mode 100644
index 00000000000..b5ef647c1b6
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Test device tree file for dtoc
+ *
+ * Copyright 2017 Google, Inc
+ */
+

tools/dtoc/test/dtoc_test_include.dts line 13

Quoted 6 lines

tools/dtoc/test/dtoc_test_include.dts new file +6 -0

+/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?

Quoted 22 lines

tools/dtoc/test/newinclude/dtoc_test_include_file.dtsi new file +7 -0

new file mode 100644
index 00000000000..f975005d023
@@ -0,0 +1,7 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/ {
+    new_node {
+        intval = <2>;
+    };
+};

tools/dtoc/test_fdt.py +0 -0

index 66d346b1272..a63c860eae1 100755
@@ -1105,6 +1105,13 @@ class TestFdtUtil(unittest.TestCase):

          finally:
              tools.outdir = old_outdir
+    def test_ensure_compiled_indir(self):
+        """Test providing directory as indir"""

Quoted 6 lines

+        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 <[email protected]>

Thanks!
Quentin

The event dump script does not work on more recent versions of binutils

Hi Andrew,

Quoted 1 line · Andrew Goodbody

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

Quoted 7 lines · Quentin Schulz

Hi Andrew,

Quoted 1 line · Andrew Goodbody

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.

Quoted 2 lines

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.

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 <[email protected]>

---
  tools/binman/bintool.py | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

tools/binman/bintool.py +0 -0

index 4cf8d5b165b..15e2df2fd80 100644

Hi Andrew,

Quoted 8 lines · 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.

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

Quoted 10 lines

Signed-off-by Andrew Goodbody <[email protected]>

---
  tools/binman/bintool.py | 5 ++++-

  1 file changed, 4 insertions(+), 1 deletion(-)

tools/binman/bintool.py +0 -0

index 4cf8d5b165b..15e2df2fd80 100644
@@ -290,7 +290,10 @@ class Bintool:

Quoted 5 lines

          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

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.

Hi Andrew,

Quoted 7 lines · 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.

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

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 <[email protected]>

---
  test/run | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

test/run +0 -0

index d1654dffb5f..0857fc14c9d 100755

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

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 <[email protected]>

---
  tools/buildman/boards.py    | 3 +--
  tools/buildman/func_test.py | 5 +++--
  2 files changed, 4 insertions(+), 4 deletions(-)

tools/buildman/boards.py +0 -0

index 5d5d3a3822b..111789f1bd4 100644

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 <[email protected]>

---
  test/py/tests/fs_helper.py              | 4 ++++
  test/py/tests/test_fs/fstest_helpers.py | 5 +++++
  2 files changed, 9 insertions(+)

test/py/tests/fs_helper.py +0 -0

index e3824b2c1fd..48b047aeefd 100644

test/py/tests/fs_helper.py +0 -0

          elif fs_lnxtype == 'vfat' and src_dir:
              flags = f"-smpQ{'' if quiet else 'v'}"
              check_call(f'mcopy -i {fs_img} {flags} {src_dir}/* ::/',

test/py/tests/test_fs/fstest_helpers.py +0 -0

index d25326ee993..6164eca476a 100644
@@ -3,9 +3,14 @@
  # Author: JJ Hiblot <[email protected]>

Hi Andrew,

Quoted 6 lines · 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.

question: are those really related?

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

Quoted 13 lines

Signed-off-by Andrew Goodbody <[email protected]>

---
  test/py/tests/fs_helper.py              | 4 ++++ test/py/tests/test_fs/fstest_helpers.py | 5 +++++

  2 files changed, 9 insertions(+)

test/py/tests/fs_helper.py +0 -0

index e3824b2c1fd..48b047aeefd 100644
@@ -250,6 +250,10 @@ def mk_fs(config, fs_type, size, prefix, 
 src_dir=None, fs_img=None, quiet=False)
                                         shell=True).decode()

Quoted 8 lines

              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.

Quoted 12 lines

          elif fs_lnxtype == 'vfat' and src_dir:
              flags = f"-smpQ{'' if quiet else 'v'}" check_call(f'mcopy -i {fs_img} {flags} {src_dir}/* ::/',

test/py/tests/test_fs/fstest_helpers.py +1 -0

index d25326ee993..6164eca476a 100644
@@ -3,9 +3,14 @@
   # Author: JJ Hiblot <[email protected]>
   #
+import os

  from subprocess import check_call, CalledProcessError

Quoted 6 lines

  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

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.

Quoted 18 lines · 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.

Quoted 7 lines

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.

Quoted 28 lines · Tom Rini

Quoted 18 lines · 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.

Quoted 7 lines

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.

Quoted 6 lines

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.