[PATCH v3 0/6] Add Binman code-coverage test to CI

[PATCH v3 0/6] Add Binman code-coverage test to CI

[Simon Glass]

Binman has 100% code coverage to ensure that future changes and
refactors do not break existing entry types. This is a critical feature,
given that it is relied on to produce images for all sorts of different
SoCs and vendors.

With the NXP additions the 'binman test -T' step was missed, so the
Binman coverage test is currently failing.

This series provides a means to close the testing gap. It cannot be
applied until the tests are added, which should happen before -next is
applied to -master

Changes in v3:
- Drop the PATH modification altogether

Changes in v2:
- Add new patch to fix a few typos in toolchain code
- Add to azure also (oops)
- Add to buildman requirements instead

Simon Glass (6):
  buildman: Add python3-coverage
  buildman: Add python3-pycryptodome
  buildman: Fix a few typos in toolchain code
  buildman: Always use the full path in CROSS_COMPILE
  u_boot_pylib: Use correct coverage tool within venv
  CI: Run code-coverage test for Binman

 .azure-pipelines.yml            |  5 ++-
 .gitlab-ci.yml                  |  4 +-
 tools/buildman/bsettings.py     |  3 ++
 tools/buildman/builder.py       |  5 +--
 tools/buildman/builderthread.py |  4 +-
 tools/buildman/cmdline.py       |  2 -
 tools/buildman/control.py       |  6 +--
 tools/buildman/requirements.txt |  2 +
 tools/buildman/test.py          | 75 +++++++++++++++++++++++++++++++++
 tools/buildman/toolchain.py     | 24 +++++------
 tools/u_boot_pylib/test_util.py | 11 +++--
 11 files changed, 111 insertions(+), 30 deletions(-)

-- 
2.34.1

[PATCH v3 1/6] buildman: Add python3-coverage

[Simon Glass]

Add this package so we can run code-coverage tests for Binman.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Tom Rini <trini@konsulko.com>
---

(no changes since v2)

Changes in v2:
- Add to buildman requirements instead

 tools/buildman/requirements.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/buildman/requirements.txt b/tools/buildman/requirements.txt
index 4a31e69e4cb..564e54898a4 100644
--- a/tools/buildman/requirements.txt
+++ b/tools/buildman/requirements.txt
@@ -1,3 +1,4 @@
+coverage==6.2
 jsonschema==4.17.3
 pyyaml==6.0
 yamllint==1.26.3
-- 
2.34.1

Re: [PATCH v3 1/6] buildman: Add python3-coverage

[Simon Glass]

Add this package so we can run code-coverage tests for Binman.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Tom Rini <trini@konsulko.com>
---

(no changes since v2)

Changes in v2:
- Add to buildman requirements instead

 tools/buildman/requirements.txt | 1 +
 1 file changed, 1 insertion(+)

Applied to u-boot-dm, thanks!

[PATCH v3 2/6] buildman: Add python3-pycryptodome

[Simon Glass]

This is used by some Binman entry types, so add it to allow more tests
to pass.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Tom Rini <trini@konsulko.com>
---

(no changes since v2)

Changes in v2:
- Add to buildman requirements instead

 tools/buildman/requirements.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/buildman/requirements.txt b/tools/buildman/requirements.txt
index 564e54898a4..052d0ed5c6f 100644
--- a/tools/buildman/requirements.txt
+++ b/tools/buildman/requirements.txt
@@ -1,4 +1,5 @@
 coverage==6.2
 jsonschema==4.17.3
+pycryptodome==3.20
 pyyaml==6.0
 yamllint==1.26.3
-- 
2.34.1

Re: [PATCH v3 2/6] buildman: Add python3-pycryptodome

[Simon Glass]

This is used by some Binman entry types, so add it to allow more tests
to pass.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Tom Rini <trini@konsulko.com>
---

(no changes since v2)

Changes in v2:
- Add to buildman requirements instead

 tools/buildman/requirements.txt | 1 +
 1 file changed, 1 insertion(+)

Applied to u-boot-dm, thanks!

[PATCH v3 3/6] buildman: Fix a few typos in toolchain code

[Simon Glass]

Fix 'Thie' and capitalise 'unicode'.

Signed-off-by: Simon Glass <sjg@chromium.org>
Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---

(no changes since v2)

Changes in v2:
- Add new patch to fix a few typos in toolchain code

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

diff --git a/tools/buildman/toolchain.py b/tools/buildman/toolchain.py
index 79c7c11a110..324ad0e0821 100644
--- a/tools/buildman/toolchain.py
+++ b/tools/buildman/toolchain.py
@@ -175,9 +175,9 @@ class Toolchain:
     def MakeEnvironment(self, full_path):
         """Returns an environment for using the toolchain.
 
-        Thie takes the current environment and adds CROSS_COMPILE so that
+        This takes the current environment and adds CROSS_COMPILE so that
         the tool chain will operate correctly. This also disables localized
-        output and possibly unicode encoded output of all build tools by
+        output and possibly Unicode encoded output of all build tools by
         adding LC_ALL=C.
 
         Note that os.environb is used to obtain the environment, since in some
-- 
2.34.1

Re: [PATCH v3 3/6] buildman: Fix a few typos in toolchain code

[Simon Glass]

Fix 'Thie' and capitalise 'unicode'.

Signed-off-by: Simon Glass <sjg@chromium.org>
Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---

(no changes since v2)

Changes in v2:
- Add new patch to fix a few typos in toolchain code

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

Applied to u-boot-dm, thanks!

[PATCH v3 4/6] buildman: Always use the full path in CROSS_COMPILE

[Simon Glass]

The feature to set the toolchain path does not seem to be needed. It
causes problems with venv (see [1]). Let's remove it.

Add some tests while we are here.

It does not look like any docs changes are needed for this.

[1] https://patchwork.ozlabs.org/project/uboot/patch/20240621131423.2363294-6-sjg@chromium.org/

Signed-off-by: Simon Glass <sjg@chromium.org>
Suggested-by: Tom Rini <trini@konsulko.com>
---

Changes in v3:
- Drop the PATH modification altogether

 tools/buildman/bsettings.py     |  3 ++
 tools/buildman/builder.py       |  5 +--
 tools/buildman/builderthread.py |  4 +-
 tools/buildman/cmdline.py       |  2 -
 tools/buildman/control.py       |  6 +--
 tools/buildman/test.py          | 75 +++++++++++++++++++++++++++++++++
 tools/buildman/toolchain.py     | 20 ++++-----
 7 files changed, 92 insertions(+), 23 deletions(-)

diff --git a/tools/buildman/bsettings.py b/tools/buildman/bsettings.py
index e225ac2ca0f..1be1d45e0fa 100644
--- a/tools/buildman/bsettings.py
+++ b/tools/buildman/bsettings.py
@@ -31,6 +31,9 @@ def setup(fname=''):
 def add_file(data):
     settings.readfp(io.StringIO(data))
 
+def add_section(name):
+    settings.add_section(name)
+
 def get_items(section):
     """Get the items from a section of the config.
 
diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
index f35175b4598..7c563cddada 100644
--- a/tools/buildman/builder.py
+++ b/tools/buildman/builder.py
@@ -255,7 +255,7 @@ class Builder:
 
     def __init__(self, toolchains, base_dir, git_dir, num_threads, num_jobs,
                  gnu_make='make', checkout=True, show_unknown=True, step=1,
-                 no_subdirs=False, full_path=False, verbose_build=False,
+                 no_subdirs=False, verbose_build=False,
                  mrproper=False, per_board_out_dir=False,
                  config_only=False, squash_config_y=False,
                  warnings_as_errors=False, work_in_output=False,
@@ -279,8 +279,6 @@ class Builder:
             step: 1 to process every commit, n to process every nth commit
             no_subdirs: Don't create subdirectories when building current
                 source for a single board
-            full_path: Return the full path in CROSS_COMPILE and don't set
-                PATH
             verbose_build: Run build with V=1 and don't use 'make -s'
             mrproper: Always run 'make mrproper' when configuring
             per_board_out_dir: Build in a separate persistent directory per
@@ -336,7 +334,6 @@ class Builder:
         self._step = step
         self._error_lines = 0
         self.no_subdirs = no_subdirs
-        self.full_path = full_path
         self.verbose_build = verbose_build
         self.config_only = config_only
         self.squash_config_y = squash_config_y
diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index a8599c0bb2a..c23c3254d2d 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -404,7 +404,7 @@ class BuilderThread(threading.Thread):
                     the next incremental build
         """
         # Set up the environment and command line
-        env = self.toolchain.MakeEnvironment(self.builder.full_path)
+        env = self.toolchain.MakeEnvironment()
         mkdir(out_dir)
 
         args, cwd, src_dir = self._build_args(brd, out_dir, out_rel_dir,
@@ -569,7 +569,7 @@ class BuilderThread(threading.Thread):
                 outf.write(f'{result.return_code}')
 
             # Write out the image and function size information and an objdump
-            env = result.toolchain.MakeEnvironment(self.builder.full_path)
+            env = result.toolchain.MakeEnvironment()
             with open(os.path.join(build_dir, 'out-env'), 'wb') as outf:
                 for var in sorted(env.keys()):
                     outf.write(b'%s="%s"' % (var, env[var]))
diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py
index 03211bd5aa5..5fda90508f2 100644
--- a/tools/buildman/cmdline.py
+++ b/tools/buildman/cmdline.py
@@ -121,8 +121,6 @@ def add_after_m(parser):
           help="Override host toochain to use for sandbox (e.g. 'clang-7')")
     parser.add_argument('-Q', '--quick', action='store_true',
           default=False, help='Do a rough build, with limited warning resolution')
-    parser.add_argument('-p', '--full-path', action='store_true',
-          default=False, help="Use full toolchain path in CROSS_COMPILE")
     parser.add_argument('-P', '--per-board-out-dir', action='store_true',
           default=False, help="Use an O= (output) directory per board rather than per thread")
     parser.add_argument('--print-arch', action='store_true',
diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index 8f6850c5211..3ca9e2e8761 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -653,10 +653,8 @@ def do_buildman(args, toolchains=None, make_func=None, brds=None,
     builder = Builder(toolchains, output_dir, git_dir,
             args.threads, args.jobs, checkout=True,
             show_unknown=args.show_unknown, step=args.step,
-            no_subdirs=args.no_subdirs, full_path=args.full_path,
-            verbose_build=args.verbose_build,
-            mrproper=args.mrproper,
-            per_board_out_dir=args.per_board_out_dir,
+            no_subdirs=args.no_subdirs, verbose_build=args.verbose_build,
+            mrproper=args.mrproper, per_board_out_dir=args.per_board_out_dir,
             config_only=args.config_only,
             squash_config_y=not args.preserve_config_y,
             warnings_as_errors=args.warnings_as_errors,
diff --git a/tools/buildman/test.py b/tools/buildman/test.py
index f92add7a7c5..ae9963eed4f 100644
--- a/tools/buildman/test.py
+++ b/tools/buildman/test.py
@@ -146,6 +146,7 @@ class TestBuild(unittest.TestCase):
         self.toolchains.Add('arm-linux-gcc', test=False)
         self.toolchains.Add('sparc-linux-gcc', test=False)
         self.toolchains.Add('powerpc-linux-gcc', test=False)
+        self.toolchains.Add('/path/to/aarch64-linux-gcc', test=False)
         self.toolchains.Add('gcc', test=False)
 
         # Avoid sending any output
@@ -747,6 +748,80 @@ class TestBuild(unittest.TestCase):
         self.assertEqual([
             ['MARY="mary"', 'Missing expected line: CONFIG_MARY="mary"']], result)
 
+    def call_make_environment(self, tchn, in_env=None):
+        """Call Toolchain.MakeEnvironment() and process the result
+
+        Args:
+            tchn (Toolchain): Toolchain to use
+            in_env (dict): Input environment to use, None to use current env
+
+        Returns:
+            tuple:
+                dict: Changes that MakeEnvironment has made to the environment
+                    key: Environment variable that was changed
+                    value: New value (for PATH this only includes components
+                        which were added)
+                str: Full value of the new PATH variable
+        """
+        env = tchn.MakeEnvironment(env=in_env)
+
+        # Get the original environment
+        orig_env = dict(os.environb if in_env is None else in_env)
+        orig_path = orig_env[b'PATH'].split(b':')
+
+        # Find new variables
+        diff = dict((k, env[k]) for k in env if orig_env.get(k) != env[k])
+
+        # Find new / different path components
+        diff_path = None
+        new_path = None
+        if b'PATH' in diff:
+            new_path = diff[b'PATH'].split(b':')
+            diff_paths = [p for p in new_path if p not in orig_path]
+            diff_path = b':'.join(p for p in new_path if p not in orig_path)
+            if diff_path:
+                diff[b'PATH'] = diff_path
+            else:
+                del diff[b'PATH']
+        return diff, new_path
+
+    def test_toolchain_env(self):
+        """Test PATH and other environment settings for toolchains"""
+        # Use a toolchain which has a path
+        tchn = self.toolchains.Select('aarch64')
+
+        # Normal case
+        diff = self.call_make_environment(tchn)[0]
+        self.assertEqual(
+            {b'CROSS_COMPILE': b'/path/to/aarch64-linux-', b'LC_ALL': b'C'},
+            diff)
+
+        # When overriding the toolchain, only LC_ALL should be set
+        tchn.override_toolchain = True
+        diff = self.call_make_environment(tchn)[0]
+        self.assertEqual({b'LC_ALL': b'C'}, diff)
+
+        # Test that virtualenv is handled correctly
+        tchn.override_toolchain = False
+        sys.prefix = '/some/venv'
+        env = dict(os.environb)
+        env[b'PATH'] = b'/some/venv/bin:other/things'
+        tchn.path = '/my/path'
+        diff, diff_path = self.call_make_environment(tchn, env)
+
+        self.assertNotIn(b'PATH', diff)
+        self.assertEqual(None, diff_path)
+        self.assertEqual(
+            {b'CROSS_COMPILE': b'/my/path/aarch64-linux-', b'LC_ALL': b'C'},
+            diff)
+
+        # Handle a toolchain wrapper
+        tchn.path = ''
+        bsettings.add_section('toolchain-wrapper')
+        bsettings.set_item('toolchain-wrapper', 'my-wrapper', 'fred')
+        diff = self.call_make_environment(tchn)[0]
+        self.assertEqual(
+            {b'CROSS_COMPILE': b'fred aarch64-linux-', b'LC_ALL': b'C'}, diff)
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/buildman/toolchain.py b/tools/buildman/toolchain.py
index 324ad0e0821..739acf3ec53 100644
--- a/tools/buildman/toolchain.py
+++ b/tools/buildman/toolchain.py
@@ -90,7 +90,7 @@ class Toolchain:
         if self.arch == 'sandbox' and override_toolchain:
             self.gcc = override_toolchain
 
-        env = self.MakeEnvironment(False)
+        env = self.MakeEnvironment()
 
         # As a basic sanity check, run the C compiler with --version
         cmd = [fname, '--version']
@@ -172,7 +172,7 @@ class Toolchain:
         else:
             raise ValueError('Unknown arg to GetEnvArgs (%d)' % which)
 
-    def MakeEnvironment(self, full_path):
+    def MakeEnvironment(self, env=None):
         """Returns an environment for using the toolchain.
 
         This takes the current environment and adds CROSS_COMPILE so that
@@ -188,25 +188,23 @@ class Toolchain:
              569-570: surrogates not allowed
 
         Args:
-            full_path: Return the full path in CROSS_COMPILE and don't set
-                PATH
+            env (dict of bytes): Original environment, used for testing
+
         Returns:
             Dict containing the (bytes) environment to use. This is based on the
-            current environment, with changes as needed to CROSS_COMPILE, PATH
-            and LC_ALL.
+            current environment, with changes as needed to CROSS_COMPILE and
+            LC_ALL.
         """
-        env = dict(os.environb)
+        env = dict(env or os.environb)
+
         wrapper = self.GetWrapper()
 
         if self.override_toolchain:
             # We'll use MakeArgs() to provide this
             pass
-        elif full_path:
+        else:
             env[b'CROSS_COMPILE'] = tools.to_bytes(
                 wrapper + os.path.join(self.path, self.cross))
-        else:
-            env[b'CROSS_COMPILE'] = tools.to_bytes(wrapper + self.cross)
-            env[b'PATH'] = tools.to_bytes(self.path) + b':' + env[b'PATH']
 
         env[b'LC_ALL'] = b'C'
 
-- 
2.34.1

Re: [PATCH v3 4/6] buildman: Always use the full path in CROSS_COMPILE

[Tom Rini]

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

On Sun, Jun 23, 2024 at 11:56:20AM -0600, Simon Glass wrote:

> The feature to set the toolchain path does not seem to be needed. It
> causes problems with venv (see [1]). Let's remove it.
> 
> Add some tests while we are here.
> 
> It does not look like any docs changes are needed for this.
> 
> [1] https://patchwork.ozlabs.org/project/uboot/patch/20240621131423.2363294-6-sjg@chromium.org/
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Suggested-by: Tom Rini <trini@konsulko.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

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

Re: [PATCH v3 4/6] buildman: Always use the full path in CROSS_COMPILE

[Andrejs Cainikovs]

On Sun, Jun 23, 2024 at 11:56:20AM -0600, Simon Glass wrote:
> The feature to set the toolchain path does not seem to be needed. It
> causes problems with venv (see [1]). Let's remove it.
> 
> Add some tests while we are here.
> 
> It does not look like any docs changes are needed for this.
> 
> [1] https://patchwork.ozlabs.org/project/uboot/patch/20240621131423.2363294-6-sjg@chromium.org/
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Suggested-by: Tom Rini <trini@konsulko.com>
> ---
> 
> Changes in v3:
> - Drop the PATH modification altogether
> 
>  tools/buildman/bsettings.py     |  3 ++
>  tools/buildman/builder.py       |  5 +--
>  tools/buildman/builderthread.py |  4 +-
>  tools/buildman/cmdline.py       |  2 -
>  tools/buildman/control.py       |  6 +--
>  tools/buildman/test.py          | 75 +++++++++++++++++++++++++++++++++
>  tools/buildman/toolchain.py     | 20 ++++-----
>  7 files changed, 92 insertions(+), 23 deletions(-)
> 
> diff --git a/tools/buildman/bsettings.py b/tools/buildman/bsettings.py
> index e225ac2ca0f..1be1d45e0fa 100644
> --- a/tools/buildman/bsettings.py
> +++ b/tools/buildman/bsettings.py
> @@ -31,6 +31,9 @@ def setup(fname=''):
>  def add_file(data):
>      settings.readfp(io.StringIO(data))
>  
> +def add_section(name):
> +    settings.add_section(name)
> +
>  def get_items(section):
>      """Get the items from a section of the config.
>  
> diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
> index f35175b4598..7c563cddada 100644
> --- a/tools/buildman/builder.py
> +++ b/tools/buildman/builder.py
> @@ -255,7 +255,7 @@ class Builder:
>  
>      def __init__(self, toolchains, base_dir, git_dir, num_threads, num_jobs,
>                   gnu_make='make', checkout=True, show_unknown=True, step=1,
> -                 no_subdirs=False, full_path=False, verbose_build=False,
> +                 no_subdirs=False, verbose_build=False,
>                   mrproper=False, per_board_out_dir=False,
>                   config_only=False, squash_config_y=False,
>                   warnings_as_errors=False, work_in_output=False,
> @@ -279,8 +279,6 @@ class Builder:
>              step: 1 to process every commit, n to process every nth commit
>              no_subdirs: Don't create subdirectories when building current
>                  source for a single board
> -            full_path: Return the full path in CROSS_COMPILE and don't set
> -                PATH
>              verbose_build: Run build with V=1 and don't use 'make -s'
>              mrproper: Always run 'make mrproper' when configuring
>              per_board_out_dir: Build in a separate persistent directory per
> @@ -336,7 +334,6 @@ class Builder:
>          self._step = step
>          self._error_lines = 0
>          self.no_subdirs = no_subdirs
> -        self.full_path = full_path
>          self.verbose_build = verbose_build
>          self.config_only = config_only
>          self.squash_config_y = squash_config_y
> diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
> index a8599c0bb2a..c23c3254d2d 100644
> --- a/tools/buildman/builderthread.py
> +++ b/tools/buildman/builderthread.py
> @@ -404,7 +404,7 @@ class BuilderThread(threading.Thread):
>                      the next incremental build
>          """
>          # Set up the environment and command line
> -        env = self.toolchain.MakeEnvironment(self.builder.full_path)
> +        env = self.toolchain.MakeEnvironment()
>          mkdir(out_dir)
>  
>          args, cwd, src_dir = self._build_args(brd, out_dir, out_rel_dir,
> @@ -569,7 +569,7 @@ class BuilderThread(threading.Thread):
>                  outf.write(f'{result.return_code}')
>  
>              # Write out the image and function size information and an objdump
> -            env = result.toolchain.MakeEnvironment(self.builder.full_path)
> +            env = result.toolchain.MakeEnvironment()
>              with open(os.path.join(build_dir, 'out-env'), 'wb') as outf:
>                  for var in sorted(env.keys()):
>                      outf.write(b'%s="%s"' % (var, env[var]))
> diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py
> index 03211bd5aa5..5fda90508f2 100644
> --- a/tools/buildman/cmdline.py
> +++ b/tools/buildman/cmdline.py
> @@ -121,8 +121,6 @@ def add_after_m(parser):
>            help="Override host toochain to use for sandbox (e.g. 'clang-7')")
>      parser.add_argument('-Q', '--quick', action='store_true',
>            default=False, help='Do a rough build, with limited warning resolution')
> -    parser.add_argument('-p', '--full-path', action='store_true',
> -          default=False, help="Use full toolchain path in CROSS_COMPILE")
>      parser.add_argument('-P', '--per-board-out-dir', action='store_true',
>            default=False, help="Use an O= (output) directory per board rather than per thread")
>      parser.add_argument('--print-arch', action='store_true',
> diff --git a/tools/buildman/control.py b/tools/buildman/control.py
> index 8f6850c5211..3ca9e2e8761 100644
> --- a/tools/buildman/control.py
> +++ b/tools/buildman/control.py
> @@ -653,10 +653,8 @@ def do_buildman(args, toolchains=None, make_func=None, brds=None,
>      builder = Builder(toolchains, output_dir, git_dir,
>              args.threads, args.jobs, checkout=True,
>              show_unknown=args.show_unknown, step=args.step,
> -            no_subdirs=args.no_subdirs, full_path=args.full_path,
> -            verbose_build=args.verbose_build,
> -            mrproper=args.mrproper,
> -            per_board_out_dir=args.per_board_out_dir,
> +            no_subdirs=args.no_subdirs, verbose_build=args.verbose_build,
> +            mrproper=args.mrproper, per_board_out_dir=args.per_board_out_dir,
>              config_only=args.config_only,
>              squash_config_y=not args.preserve_config_y,
>              warnings_as_errors=args.warnings_as_errors,
> diff --git a/tools/buildman/test.py b/tools/buildman/test.py
> index f92add7a7c5..ae9963eed4f 100644
> --- a/tools/buildman/test.py
> +++ b/tools/buildman/test.py
> @@ -146,6 +146,7 @@ class TestBuild(unittest.TestCase):
>          self.toolchains.Add('arm-linux-gcc', test=False)
>          self.toolchains.Add('sparc-linux-gcc', test=False)
>          self.toolchains.Add('powerpc-linux-gcc', test=False)
> +        self.toolchains.Add('/path/to/aarch64-linux-gcc', test=False)

Sorry Simon, but me and others love to be consistent.

Best regards,
Andrejs.

>          self.toolchains.Add('gcc', test=False)
>  
>          # Avoid sending any output
> @@ -747,6 +748,80 @@ class TestBuild(unittest.TestCase):
>          self.assertEqual([
>              ['MARY="mary"', 'Missing expected line: CONFIG_MARY="mary"']], result)
>  
> +    def call_make_environment(self, tchn, in_env=None):
> +        """Call Toolchain.MakeEnvironment() and process the result
> +
> +        Args:
> +            tchn (Toolchain): Toolchain to use
> +            in_env (dict): Input environment to use, None to use current env
> +
> +        Returns:
> +            tuple:
> +                dict: Changes that MakeEnvironment has made to the environment
> +                    key: Environment variable that was changed
> +                    value: New value (for PATH this only includes components
> +                        which were added)
> +                str: Full value of the new PATH variable
> +        """
> +        env = tchn.MakeEnvironment(env=in_env)
> +
> +        # Get the original environment
> +        orig_env = dict(os.environb if in_env is None else in_env)
> +        orig_path = orig_env[b'PATH'].split(b':')
> +
> +        # Find new variables
> +        diff = dict((k, env[k]) for k in env if orig_env.get(k) != env[k])
> +
> +        # Find new / different path components
> +        diff_path = None
> +        new_path = None
> +        if b'PATH' in diff:
> +            new_path = diff[b'PATH'].split(b':')
> +            diff_paths = [p for p in new_path if p not in orig_path]
> +            diff_path = b':'.join(p for p in new_path if p not in orig_path)
> +            if diff_path:
> +                diff[b'PATH'] = diff_path
> +            else:
> +                del diff[b'PATH']
> +        return diff, new_path
> +
> +    def test_toolchain_env(self):
> +        """Test PATH and other environment settings for toolchains"""
> +        # Use a toolchain which has a path
> +        tchn = self.toolchains.Select('aarch64')
> +
> +        # Normal case
> +        diff = self.call_make_environment(tchn)[0]
> +        self.assertEqual(
> +            {b'CROSS_COMPILE': b'/path/to/aarch64-linux-', b'LC_ALL': b'C'},
> +            diff)
> +
> +        # When overriding the toolchain, only LC_ALL should be set
> +        tchn.override_toolchain = True
> +        diff = self.call_make_environment(tchn)[0]
> +        self.assertEqual({b'LC_ALL': b'C'}, diff)
> +
> +        # Test that virtualenv is handled correctly
> +        tchn.override_toolchain = False
> +        sys.prefix = '/some/venv'
> +        env = dict(os.environb)
> +        env[b'PATH'] = b'/some/venv/bin:other/things'
> +        tchn.path = '/my/path'
> +        diff, diff_path = self.call_make_environment(tchn, env)
> +
> +        self.assertNotIn(b'PATH', diff)
> +        self.assertEqual(None, diff_path)
> +        self.assertEqual(
> +            {b'CROSS_COMPILE': b'/my/path/aarch64-linux-', b'LC_ALL': b'C'},
> +            diff)
> +
> +        # Handle a toolchain wrapper
> +        tchn.path = ''
> +        bsettings.add_section('toolchain-wrapper')
> +        bsettings.set_item('toolchain-wrapper', 'my-wrapper', 'fred')
> +        diff = self.call_make_environment(tchn)[0]
> +        self.assertEqual(
> +            {b'CROSS_COMPILE': b'fred aarch64-linux-', b'LC_ALL': b'C'}, diff)
>  
>  if __name__ == "__main__":
>      unittest.main()
> diff --git a/tools/buildman/toolchain.py b/tools/buildman/toolchain.py
> index 324ad0e0821..739acf3ec53 100644
> --- a/tools/buildman/toolchain.py
> +++ b/tools/buildman/toolchain.py
> @@ -90,7 +90,7 @@ class Toolchain:
>          if self.arch == 'sandbox' and override_toolchain:
>              self.gcc = override_toolchain
>  
> -        env = self.MakeEnvironment(False)
> +        env = self.MakeEnvironment()
>  
>          # As a basic sanity check, run the C compiler with --version
>          cmd = [fname, '--version']
> @@ -172,7 +172,7 @@ class Toolchain:
>          else:
>              raise ValueError('Unknown arg to GetEnvArgs (%d)' % which)
>  
> -    def MakeEnvironment(self, full_path):
> +    def MakeEnvironment(self, env=None):
>          """Returns an environment for using the toolchain.
>  
>          This takes the current environment and adds CROSS_COMPILE so that
> @@ -188,25 +188,23 @@ class Toolchain:
>               569-570: surrogates not allowed
>  
>          Args:
> -            full_path: Return the full path in CROSS_COMPILE and don't set
> -                PATH
> +            env (dict of bytes): Original environment, used for testing
> +
>          Returns:
>              Dict containing the (bytes) environment to use. This is based on the
> -            current environment, with changes as needed to CROSS_COMPILE, PATH
> -            and LC_ALL.
> +            current environment, with changes as needed to CROSS_COMPILE and
> +            LC_ALL.
>          """
> -        env = dict(os.environb)
> +        env = dict(env or os.environb)
> +
>          wrapper = self.GetWrapper()
>  
>          if self.override_toolchain:
>              # We'll use MakeArgs() to provide this
>              pass
> -        elif full_path:
> +        else:
>              env[b'CROSS_COMPILE'] = tools.to_bytes(
>                  wrapper + os.path.join(self.path, self.cross))
> -        else:
> -            env[b'CROSS_COMPILE'] = tools.to_bytes(wrapper + self.cross)
> -            env[b'PATH'] = tools.to_bytes(self.path) + b':' + env[b'PATH']
>  
>          env[b'LC_ALL'] = b'C'
>  
> -- 
> 2.34.1
> 

Re: [PATCH v3 4/6] buildman: Always use the full path in CROSS_COMPILE

[Simon Glass]

Hi Andrejs,

On Tue, 25 Jun 2024 at 01:06, Andrejs Cainikovs
<andrejs.cainikovs@toradex.com> wrote:
>
> On Sun, Jun 23, 2024 at 11:56:20AM -0600, Simon Glass wrote:
> > The feature to set the toolchain path does not seem to be needed. It
> > causes problems with venv (see [1]). Let's remove it.
> >
> > Add some tests while we are here.
> >
> > It does not look like any docs changes are needed for this.
> >
> > [1] https://patchwork.ozlabs.org/project/uboot/patch/20240621131423.2363294-6-sjg@chromium.org/
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > Suggested-by: Tom Rini <trini@konsulko.com>
> > ---
> >
> > Changes in v3:
> > - Drop the PATH modification altogether
> >
> >  tools/buildman/bsettings.py     |  3 ++
> >  tools/buildman/builder.py       |  5 +--
> >  tools/buildman/builderthread.py |  4 +-
> >  tools/buildman/cmdline.py       |  2 -
> >  tools/buildman/control.py       |  6 +--
> >  tools/buildman/test.py          | 75 +++++++++++++++++++++++++++++++++
> >  tools/buildman/toolchain.py     | 20 ++++-----
> >  7 files changed, 92 insertions(+), 23 deletions(-)
> >
> > diff --git a/tools/buildman/bsettings.py b/tools/buildman/bsettings.py
> > index e225ac2ca0f..1be1d45e0fa 100644
> > --- a/tools/buildman/bsettings.py
> > +++ b/tools/buildman/bsettings.py
> > @@ -31,6 +31,9 @@ def setup(fname=''):
> >  def add_file(data):
> >      settings.readfp(io.StringIO(data))
> >
> > +def add_section(name):
> > +    settings.add_section(name)
> > +
> >  def get_items(section):
> >      """Get the items from a section of the config.
> >
> > diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
> > index f35175b4598..7c563cddada 100644
> > --- a/tools/buildman/builder.py
> > +++ b/tools/buildman/builder.py
> > @@ -255,7 +255,7 @@ class Builder:
> >
> >      def __init__(self, toolchains, base_dir, git_dir, num_threads, num_jobs,
> >                   gnu_make='make', checkout=True, show_unknown=True, step=1,
> > -                 no_subdirs=False, full_path=False, verbose_build=False,
> > +                 no_subdirs=False, verbose_build=False,
> >                   mrproper=False, per_board_out_dir=False,
> >                   config_only=False, squash_config_y=False,
> >                   warnings_as_errors=False, work_in_output=False,
> > @@ -279,8 +279,6 @@ class Builder:
> >              step: 1 to process every commit, n to process every nth commit
> >              no_subdirs: Don't create subdirectories when building current
> >                  source for a single board
> > -            full_path: Return the full path in CROSS_COMPILE and don't set
> > -                PATH
> >              verbose_build: Run build with V=1 and don't use 'make -s'
> >              mrproper: Always run 'make mrproper' when configuring
> >              per_board_out_dir: Build in a separate persistent directory per
> > @@ -336,7 +334,6 @@ class Builder:
> >          self._step = step
> >          self._error_lines = 0
> >          self.no_subdirs = no_subdirs
> > -        self.full_path = full_path
> >          self.verbose_build = verbose_build
> >          self.config_only = config_only
> >          self.squash_config_y = squash_config_y
> > diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
> > index a8599c0bb2a..c23c3254d2d 100644
> > --- a/tools/buildman/builderthread.py
> > +++ b/tools/buildman/builderthread.py
> > @@ -404,7 +404,7 @@ class BuilderThread(threading.Thread):
> >                      the next incremental build
> >          """
> >          # Set up the environment and command line
> > -        env = self.toolchain.MakeEnvironment(self.builder.full_path)
> > +        env = self.toolchain.MakeEnvironment()
> >          mkdir(out_dir)
> >
> >          args, cwd, src_dir = self._build_args(brd, out_dir, out_rel_dir,
> > @@ -569,7 +569,7 @@ class BuilderThread(threading.Thread):
> >                  outf.write(f'{result.return_code}')
> >
> >              # Write out the image and function size information and an objdump
> > -            env = result.toolchain.MakeEnvironment(self.builder.full_path)
> > +            env = result.toolchain.MakeEnvironment()
> >              with open(os.path.join(build_dir, 'out-env'), 'wb') as outf:
> >                  for var in sorted(env.keys()):
> >                      outf.write(b'%s="%s"' % (var, env[var]))
> > diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py
> > index 03211bd5aa5..5fda90508f2 100644
> > --- a/tools/buildman/cmdline.py
> > +++ b/tools/buildman/cmdline.py
> > @@ -121,8 +121,6 @@ def add_after_m(parser):
> >            help="Override host toochain to use for sandbox (e.g. 'clang-7')")
> >      parser.add_argument('-Q', '--quick', action='store_true',
> >            default=False, help='Do a rough build, with limited warning resolution')
> > -    parser.add_argument('-p', '--full-path', action='store_true',
> > -          default=False, help="Use full toolchain path in CROSS_COMPILE")
> >      parser.add_argument('-P', '--per-board-out-dir', action='store_true',
> >            default=False, help="Use an O= (output) directory per board rather than per thread")
> >      parser.add_argument('--print-arch', action='store_true',
> > diff --git a/tools/buildman/control.py b/tools/buildman/control.py
> > index 8f6850c5211..3ca9e2e8761 100644
> > --- a/tools/buildman/control.py
> > +++ b/tools/buildman/control.py
> > @@ -653,10 +653,8 @@ def do_buildman(args, toolchains=None, make_func=None, brds=None,
> >      builder = Builder(toolchains, output_dir, git_dir,
> >              args.threads, args.jobs, checkout=True,
> >              show_unknown=args.show_unknown, step=args.step,
> > -            no_subdirs=args.no_subdirs, full_path=args.full_path,
> > -            verbose_build=args.verbose_build,
> > -            mrproper=args.mrproper,
> > -            per_board_out_dir=args.per_board_out_dir,
> > +            no_subdirs=args.no_subdirs, verbose_build=args.verbose_build,
> > +            mrproper=args.mrproper, per_board_out_dir=args.per_board_out_dir,
> >              config_only=args.config_only,
> >              squash_config_y=not args.preserve_config_y,
> >              warnings_as_errors=args.warnings_as_errors,
> > diff --git a/tools/buildman/test.py b/tools/buildman/test.py
> > index f92add7a7c5..ae9963eed4f 100644
> > --- a/tools/buildman/test.py
> > +++ b/tools/buildman/test.py
> > @@ -146,6 +146,7 @@ class TestBuild(unittest.TestCase):
> >          self.toolchains.Add('arm-linux-gcc', test=False)
> >          self.toolchains.Add('sparc-linux-gcc', test=False)
> >          self.toolchains.Add('powerpc-linux-gcc', test=False)
> > +        self.toolchains.Add('/path/to/aarch64-linux-gcc', test=False)
>
> Sorry Simon, but me and others love to be consistent.

That's fine, but do you have a comment on this patch?

Regards,
Simon

Re: [PATCH v3 4/6] buildman: Always use the full path in CROSS_COMPILE

[Andrejs Cainikovs]

On Tue, Jun 25, 2024 at 01:38:07PM +0100, Simon Glass wrote:
> Hi Andrejs,
> 
> On Tue, 25 Jun 2024 at 01:06, Andrejs Cainikovs
> <andrejs.cainikovs@toradex.com> wrote:
> >
> > On Sun, Jun 23, 2024 at 11:56:20AM -0600, Simon Glass wrote:
> > > The feature to set the toolchain path does not seem to be needed. It
> > > causes problems with venv (see [1]). Let's remove it.
> > >
> > > Add some tests while we are here.
> > >
> > > It does not look like any docs changes are needed for this.
> > >
> > > [1] https://patchwork.ozlabs.org/project/uboot/patch/20240621131423.2363294-6-sjg@chromium.org/
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > Suggested-by: Tom Rini <trini@konsulko.com>
> > > ---
> > >
> > > Changes in v3:
> > > - Drop the PATH modification altogether
> > >
> > >  tools/buildman/bsettings.py     |  3 ++
> > >  tools/buildman/builder.py       |  5 +--
> > >  tools/buildman/builderthread.py |  4 +-
> > >  tools/buildman/cmdline.py       |  2 -
> > >  tools/buildman/control.py       |  6 +--
> > >  tools/buildman/test.py          | 75 +++++++++++++++++++++++++++++++++
> > >  tools/buildman/toolchain.py     | 20 ++++-----
> > >  7 files changed, 92 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/tools/buildman/bsettings.py b/tools/buildman/bsettings.py
> > > index e225ac2ca0f..1be1d45e0fa 100644
> > > --- a/tools/buildman/bsettings.py
> > > +++ b/tools/buildman/bsettings.py
> > > @@ -31,6 +31,9 @@ def setup(fname=''):
> > >  def add_file(data):
> > >      settings.readfp(io.StringIO(data))
> > >
> > > +def add_section(name):
> > > +    settings.add_section(name)
> > > +
> > >  def get_items(section):
> > >      """Get the items from a section of the config.
> > >
> > > diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
> > > index f35175b4598..7c563cddada 100644
> > > --- a/tools/buildman/builder.py
> > > +++ b/tools/buildman/builder.py
> > > @@ -255,7 +255,7 @@ class Builder:
> > >
> > >      def __init__(self, toolchains, base_dir, git_dir, num_threads, num_jobs,
> > >                   gnu_make='make', checkout=True, show_unknown=True, step=1,
> > > -                 no_subdirs=False, full_path=False, verbose_build=False,
> > > +                 no_subdirs=False, verbose_build=False,
> > >                   mrproper=False, per_board_out_dir=False,
> > >                   config_only=False, squash_config_y=False,
> > >                   warnings_as_errors=False, work_in_output=False,
> > > @@ -279,8 +279,6 @@ class Builder:
> > >              step: 1 to process every commit, n to process every nth commit
> > >              no_subdirs: Don't create subdirectories when building current
> > >                  source for a single board
> > > -            full_path: Return the full path in CROSS_COMPILE and don't set
> > > -                PATH
> > >              verbose_build: Run build with V=1 and don't use 'make -s'
> > >              mrproper: Always run 'make mrproper' when configuring
> > >              per_board_out_dir: Build in a separate persistent directory per
> > > @@ -336,7 +334,6 @@ class Builder:
> > >          self._step = step
> > >          self._error_lines = 0
> > >          self.no_subdirs = no_subdirs
> > > -        self.full_path = full_path
> > >          self.verbose_build = verbose_build
> > >          self.config_only = config_only
> > >          self.squash_config_y = squash_config_y
> > > diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
> > > index a8599c0bb2a..c23c3254d2d 100644
> > > --- a/tools/buildman/builderthread.py
> > > +++ b/tools/buildman/builderthread.py
> > > @@ -404,7 +404,7 @@ class BuilderThread(threading.Thread):
> > >                      the next incremental build
> > >          """
> > >          # Set up the environment and command line
> > > -        env = self.toolchain.MakeEnvironment(self.builder.full_path)
> > > +        env = self.toolchain.MakeEnvironment()
> > >          mkdir(out_dir)
> > >
> > >          args, cwd, src_dir = self._build_args(brd, out_dir, out_rel_dir,
> > > @@ -569,7 +569,7 @@ class BuilderThread(threading.Thread):
> > >                  outf.write(f'{result.return_code}')
> > >
> > >              # Write out the image and function size information and an objdump
> > > -            env = result.toolchain.MakeEnvironment(self.builder.full_path)
> > > +            env = result.toolchain.MakeEnvironment()
> > >              with open(os.path.join(build_dir, 'out-env'), 'wb') as outf:
> > >                  for var in sorted(env.keys()):
> > >                      outf.write(b'%s="%s"' % (var, env[var]))
> > > diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py
> > > index 03211bd5aa5..5fda90508f2 100644
> > > --- a/tools/buildman/cmdline.py
> > > +++ b/tools/buildman/cmdline.py
> > > @@ -121,8 +121,6 @@ def add_after_m(parser):
> > >            help="Override host toochain to use for sandbox (e.g. 'clang-7')")
> > >      parser.add_argument('-Q', '--quick', action='store_true',
> > >            default=False, help='Do a rough build, with limited warning resolution')
> > > -    parser.add_argument('-p', '--full-path', action='store_true',
> > > -          default=False, help="Use full toolchain path in CROSS_COMPILE")
> > >      parser.add_argument('-P', '--per-board-out-dir', action='store_true',
> > >            default=False, help="Use an O= (output) directory per board rather than per thread")
> > >      parser.add_argument('--print-arch', action='store_true',
> > > diff --git a/tools/buildman/control.py b/tools/buildman/control.py
> > > index 8f6850c5211..3ca9e2e8761 100644
> > > --- a/tools/buildman/control.py
> > > +++ b/tools/buildman/control.py
> > > @@ -653,10 +653,8 @@ def do_buildman(args, toolchains=None, make_func=None, brds=None,
> > >      builder = Builder(toolchains, output_dir, git_dir,
> > >              args.threads, args.jobs, checkout=True,
> > >              show_unknown=args.show_unknown, step=args.step,
> > > -            no_subdirs=args.no_subdirs, full_path=args.full_path,
> > > -            verbose_build=args.verbose_build,
> > > -            mrproper=args.mrproper,
> > > -            per_board_out_dir=args.per_board_out_dir,
> > > +            no_subdirs=args.no_subdirs, verbose_build=args.verbose_build,
> > > +            mrproper=args.mrproper, per_board_out_dir=args.per_board_out_dir,
> > >              config_only=args.config_only,
> > >              squash_config_y=not args.preserve_config_y,
> > >              warnings_as_errors=args.warnings_as_errors,
> > > diff --git a/tools/buildman/test.py b/tools/buildman/test.py
> > > index f92add7a7c5..ae9963eed4f 100644
> > > --- a/tools/buildman/test.py
> > > +++ b/tools/buildman/test.py
> > > @@ -146,6 +146,7 @@ class TestBuild(unittest.TestCase):
> > >          self.toolchains.Add('arm-linux-gcc', test=False)
> > >          self.toolchains.Add('sparc-linux-gcc', test=False)
> > >          self.toolchains.Add('powerpc-linux-gcc', test=False)
> > > +        self.toolchains.Add('/path/to/aarch64-linux-gcc', test=False)
> >
> > Sorry Simon, but me and others love to be consistent.
> 
> That's fine, but do you have a comment on this patch?
> 
> Regards,
> Simon

I was just thinking about aarch64-linux-gcc (should it be
aarch64-linux-gnu-gcc, btw?)  without path for consistency.
But this of course is very minor - feel free to disregard my
comment.

Best regards,
Andrejs.

Re: [PATCH v3 4/6] buildman: Always use the full path in CROSS_COMPILE

[Simon Glass]

Hi Andrejs,

On Tue, 25 Jun 2024 at 16:16, Andrejs Cainikovs
<andrejs.cainikovs@toradex.com> wrote:
>
> On Tue, Jun 25, 2024 at 01:38:07PM +0100, Simon Glass wrote:
> > Hi Andrejs,
> >
> > On Tue, 25 Jun 2024 at 01:06, Andrejs Cainikovs
> > <andrejs.cainikovs@toradex.com> wrote:
> > >
> > > On Sun, Jun 23, 2024 at 11:56:20AM -0600, Simon Glass wrote:
> > > > The feature to set the toolchain path does not seem to be needed. It
> > > > causes problems with venv (see [1]). Let's remove it.
> > > >
> > > > Add some tests while we are here.
> > > >
> > > > It does not look like any docs changes are needed for this.
> > > >
> > > > [1] https://patchwork.ozlabs.org/project/uboot/patch/20240621131423.2363294-6-sjg@chromium.org/
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > Suggested-by: Tom Rini <trini@konsulko.com>
> > > > ---
> > > >
> > > > Changes in v3:
> > > > - Drop the PATH modification altogether
> > > >
> > > >  tools/buildman/bsettings.py     |  3 ++
> > > >  tools/buildman/builder.py       |  5 +--
> > > >  tools/buildman/builderthread.py |  4 +-
> > > >  tools/buildman/cmdline.py       |  2 -
> > > >  tools/buildman/control.py       |  6 +--
> > > >  tools/buildman/test.py          | 75 +++++++++++++++++++++++++++++++++
> > > >  tools/buildman/toolchain.py     | 20 ++++-----
> > > >  7 files changed, 92 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/tools/buildman/bsettings.py b/tools/buildman/bsettings.py
> > > > index e225ac2ca0f..1be1d45e0fa 100644
> > > > --- a/tools/buildman/bsettings.py
> > > > +++ b/tools/buildman/bsettings.py
> > > > @@ -31,6 +31,9 @@ def setup(fname=''):
> > > >  def add_file(data):
> > > >      settings.readfp(io.StringIO(data))
> > > >
> > > > +def add_section(name):
> > > > +    settings.add_section(name)
> > > > +
> > > >  def get_items(section):
> > > >      """Get the items from a section of the config.
> > > >
> > > > diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
> > > > index f35175b4598..7c563cddada 100644
> > > > --- a/tools/buildman/builder.py
> > > > +++ b/tools/buildman/builder.py
> > > > @@ -255,7 +255,7 @@ class Builder:
> > > >
> > > >      def __init__(self, toolchains, base_dir, git_dir, num_threads, num_jobs,
> > > >                   gnu_make='make', checkout=True, show_unknown=True, step=1,
> > > > -                 no_subdirs=False, full_path=False, verbose_build=False,
> > > > +                 no_subdirs=False, verbose_build=False,
> > > >                   mrproper=False, per_board_out_dir=False,
> > > >                   config_only=False, squash_config_y=False,
> > > >                   warnings_as_errors=False, work_in_output=False,
> > > > @@ -279,8 +279,6 @@ class Builder:
> > > >              step: 1 to process every commit, n to process every nth commit
> > > >              no_subdirs: Don't create subdirectories when building current
> > > >                  source for a single board
> > > > -            full_path: Return the full path in CROSS_COMPILE and don't set
> > > > -                PATH
> > > >              verbose_build: Run build with V=1 and don't use 'make -s'
> > > >              mrproper: Always run 'make mrproper' when configuring
> > > >              per_board_out_dir: Build in a separate persistent directory per
> > > > @@ -336,7 +334,6 @@ class Builder:
> > > >          self._step = step
> > > >          self._error_lines = 0
> > > >          self.no_subdirs = no_subdirs
> > > > -        self.full_path = full_path
> > > >          self.verbose_build = verbose_build
> > > >          self.config_only = config_only
> > > >          self.squash_config_y = squash_config_y
> > > > diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
> > > > index a8599c0bb2a..c23c3254d2d 100644
> > > > --- a/tools/buildman/builderthread.py
> > > > +++ b/tools/buildman/builderthread.py
> > > > @@ -404,7 +404,7 @@ class BuilderThread(threading.Thread):
> > > >                      the next incremental build
> > > >          """
> > > >          # Set up the environment and command line
> > > > -        env = self.toolchain.MakeEnvironment(self.builder.full_path)
> > > > +        env = self.toolchain.MakeEnvironment()
> > > >          mkdir(out_dir)
> > > >
> > > >          args, cwd, src_dir = self._build_args(brd, out_dir, out_rel_dir,
> > > > @@ -569,7 +569,7 @@ class BuilderThread(threading.Thread):
> > > >                  outf.write(f'{result.return_code}')
> > > >
> > > >              # Write out the image and function size information and an objdump
> > > > -            env = result.toolchain.MakeEnvironment(self.builder.full_path)
> > > > +            env = result.toolchain.MakeEnvironment()
> > > >              with open(os.path.join(build_dir, 'out-env'), 'wb') as outf:
> > > >                  for var in sorted(env.keys()):
> > > >                      outf.write(b'%s="%s"' % (var, env[var]))
> > > > diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py
> > > > index 03211bd5aa5..5fda90508f2 100644
> > > > --- a/tools/buildman/cmdline.py
> > > > +++ b/tools/buildman/cmdline.py
> > > > @@ -121,8 +121,6 @@ def add_after_m(parser):
> > > >            help="Override host toochain to use for sandbox (e.g. 'clang-7')")
> > > >      parser.add_argument('-Q', '--quick', action='store_true',
> > > >            default=False, help='Do a rough build, with limited warning resolution')
> > > > -    parser.add_argument('-p', '--full-path', action='store_true',
> > > > -          default=False, help="Use full toolchain path in CROSS_COMPILE")
> > > >      parser.add_argument('-P', '--per-board-out-dir', action='store_true',
> > > >            default=False, help="Use an O= (output) directory per board rather than per thread")
> > > >      parser.add_argument('--print-arch', action='store_true',
> > > > diff --git a/tools/buildman/control.py b/tools/buildman/control.py
> > > > index 8f6850c5211..3ca9e2e8761 100644
> > > > --- a/tools/buildman/control.py
> > > > +++ b/tools/buildman/control.py
> > > > @@ -653,10 +653,8 @@ def do_buildman(args, toolchains=None, make_func=None, brds=None,
> > > >      builder = Builder(toolchains, output_dir, git_dir,
> > > >              args.threads, args.jobs, checkout=True,
> > > >              show_unknown=args.show_unknown, step=args.step,
> > > > -            no_subdirs=args.no_subdirs, full_path=args.full_path,
> > > > -            verbose_build=args.verbose_build,
> > > > -            mrproper=args.mrproper,
> > > > -            per_board_out_dir=args.per_board_out_dir,
> > > > +            no_subdirs=args.no_subdirs, verbose_build=args.verbose_build,
> > > > +            mrproper=args.mrproper, per_board_out_dir=args.per_board_out_dir,
> > > >              config_only=args.config_only,
> > > >              squash_config_y=not args.preserve_config_y,
> > > >              warnings_as_errors=args.warnings_as_errors,
> > > > diff --git a/tools/buildman/test.py b/tools/buildman/test.py
> > > > index f92add7a7c5..ae9963eed4f 100644
> > > > --- a/tools/buildman/test.py
> > > > +++ b/tools/buildman/test.py
> > > > @@ -146,6 +146,7 @@ class TestBuild(unittest.TestCase):
> > > >          self.toolchains.Add('arm-linux-gcc', test=False)
> > > >          self.toolchains.Add('sparc-linux-gcc', test=False)
> > > >          self.toolchains.Add('powerpc-linux-gcc', test=False)
> > > > +        self.toolchains.Add('/path/to/aarch64-linux-gcc', test=False)
> > >
> > > Sorry Simon, but me and others love to be consistent.
> >
> > That's fine, but do you have a comment on this patch?
> >
> > Regards,
> > Simon
>
> I was just thinking about aarch64-linux-gcc (should it be
> aarch64-linux-gnu-gcc, btw?)  without path for consistency.
> But this of course is very minor - feel free to disregard my
> comment.

Oh I see...the point here is to test a toolchain which has a path
prepended to it, That is why this test case is not consistent with the
others.

Regards,
Simon

Re: [PATCH v3 4/6] buildman: Always use the full path in CROSS_COMPILE

[Andrejs Cainikovs]

On Wed, Jun 26, 2024 at 09:00:43AM +0100, Simon Glass wrote:
> Hi Andrejs,
> 
> On Tue, 25 Jun 2024 at 16:16, Andrejs Cainikovs
> <andrejs.cainikovs@toradex.com> wrote:
> >
> > On Tue, Jun 25, 2024 at 01:38:07PM +0100, Simon Glass wrote:
> > > Hi Andrejs,
> > >
> > > On Tue, 25 Jun 2024 at 01:06, Andrejs Cainikovs
> > > <andrejs.cainikovs@toradex.com> wrote:
> > > >
> > > > On Sun, Jun 23, 2024 at 11:56:20AM -0600, Simon Glass wrote:
> > > > > The feature to set the toolchain path does not seem to be needed. It
> > > > > causes problems with venv (see [1]). Let's remove it.
> > > > >
> > > > > Add some tests while we are here.
> > > > >
> > > > > It does not look like any docs changes are needed for this.
> > > > >
> > > > > [1] https://patchwork.ozlabs.org/project/uboot/patch/20240621131423.2363294-6-sjg@chromium.org/
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > Suggested-by: Tom Rini <trini@konsulko.com>
> > > > > ---
> > > > >
> > > > > Changes in v3:
> > > > > - Drop the PATH modification altogether
> > > > >
> > > > >  tools/buildman/bsettings.py     |  3 ++
> > > > >  tools/buildman/builder.py       |  5 +--
> > > > >  tools/buildman/builderthread.py |  4 +-
> > > > >  tools/buildman/cmdline.py       |  2 -
> > > > >  tools/buildman/control.py       |  6 +--
> > > > >  tools/buildman/test.py          | 75 +++++++++++++++++++++++++++++++++
> > > > >  tools/buildman/toolchain.py     | 20 ++++-----
> > > > >  7 files changed, 92 insertions(+), 23 deletions(-)
> > > > >
> > > > > diff --git a/tools/buildman/bsettings.py b/tools/buildman/bsettings.py
> > > > > index e225ac2ca0f..1be1d45e0fa 100644
> > > > > --- a/tools/buildman/bsettings.py
> > > > > +++ b/tools/buildman/bsettings.py
> > > > > @@ -31,6 +31,9 @@ def setup(fname=''):
> > > > >  def add_file(data):
> > > > >      settings.readfp(io.StringIO(data))
> > > > >
> > > > > +def add_section(name):
> > > > > +    settings.add_section(name)
> > > > > +
> > > > >  def get_items(section):
> > > > >      """Get the items from a section of the config.
> > > > >
> > > > > diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
> > > > > index f35175b4598..7c563cddada 100644
> > > > > --- a/tools/buildman/builder.py
> > > > > +++ b/tools/buildman/builder.py
> > > > > @@ -255,7 +255,7 @@ class Builder:
> > > > >
> > > > >      def __init__(self, toolchains, base_dir, git_dir, num_threads, num_jobs,
> > > > >                   gnu_make='make', checkout=True, show_unknown=True, step=1,
> > > > > -                 no_subdirs=False, full_path=False, verbose_build=False,
> > > > > +                 no_subdirs=False, verbose_build=False,
> > > > >                   mrproper=False, per_board_out_dir=False,
> > > > >                   config_only=False, squash_config_y=False,
> > > > >                   warnings_as_errors=False, work_in_output=False,
> > > > > @@ -279,8 +279,6 @@ class Builder:
> > > > >              step: 1 to process every commit, n to process every nth commit
> > > > >              no_subdirs: Don't create subdirectories when building current
> > > > >                  source for a single board
> > > > > -            full_path: Return the full path in CROSS_COMPILE and don't set
> > > > > -                PATH
> > > > >              verbose_build: Run build with V=1 and don't use 'make -s'
> > > > >              mrproper: Always run 'make mrproper' when configuring
> > > > >              per_board_out_dir: Build in a separate persistent directory per
> > > > > @@ -336,7 +334,6 @@ class Builder:
> > > > >          self._step = step
> > > > >          self._error_lines = 0
> > > > >          self.no_subdirs = no_subdirs
> > > > > -        self.full_path = full_path
> > > > >          self.verbose_build = verbose_build
> > > > >          self.config_only = config_only
> > > > >          self.squash_config_y = squash_config_y
> > > > > diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
> > > > > index a8599c0bb2a..c23c3254d2d 100644
> > > > > --- a/tools/buildman/builderthread.py
> > > > > +++ b/tools/buildman/builderthread.py
> > > > > @@ -404,7 +404,7 @@ class BuilderThread(threading.Thread):
> > > > >                      the next incremental build
> > > > >          """
> > > > >          # Set up the environment and command line
> > > > > -        env = self.toolchain.MakeEnvironment(self.builder.full_path)
> > > > > +        env = self.toolchain.MakeEnvironment()
> > > > >          mkdir(out_dir)
> > > > >
> > > > >          args, cwd, src_dir = self._build_args(brd, out_dir, out_rel_dir,
> > > > > @@ -569,7 +569,7 @@ class BuilderThread(threading.Thread):
> > > > >                  outf.write(f'{result.return_code}')
> > > > >
> > > > >              # Write out the image and function size information and an objdump
> > > > > -            env = result.toolchain.MakeEnvironment(self.builder.full_path)
> > > > > +            env = result.toolchain.MakeEnvironment()
> > > > >              with open(os.path.join(build_dir, 'out-env'), 'wb') as outf:
> > > > >                  for var in sorted(env.keys()):
> > > > >                      outf.write(b'%s="%s"' % (var, env[var]))
> > > > > diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py
> > > > > index 03211bd5aa5..5fda90508f2 100644
> > > > > --- a/tools/buildman/cmdline.py
> > > > > +++ b/tools/buildman/cmdline.py
> > > > > @@ -121,8 +121,6 @@ def add_after_m(parser):
> > > > >            help="Override host toochain to use for sandbox (e.g. 'clang-7')")
> > > > >      parser.add_argument('-Q', '--quick', action='store_true',
> > > > >            default=False, help='Do a rough build, with limited warning resolution')
> > > > > -    parser.add_argument('-p', '--full-path', action='store_true',
> > > > > -          default=False, help="Use full toolchain path in CROSS_COMPILE")
> > > > >      parser.add_argument('-P', '--per-board-out-dir', action='store_true',
> > > > >            default=False, help="Use an O= (output) directory per board rather than per thread")
> > > > >      parser.add_argument('--print-arch', action='store_true',
> > > > > diff --git a/tools/buildman/control.py b/tools/buildman/control.py
> > > > > index 8f6850c5211..3ca9e2e8761 100644
> > > > > --- a/tools/buildman/control.py
> > > > > +++ b/tools/buildman/control.py
> > > > > @@ -653,10 +653,8 @@ def do_buildman(args, toolchains=None, make_func=None, brds=None,
> > > > >      builder = Builder(toolchains, output_dir, git_dir,
> > > > >              args.threads, args.jobs, checkout=True,
> > > > >              show_unknown=args.show_unknown, step=args.step,
> > > > > -            no_subdirs=args.no_subdirs, full_path=args.full_path,
> > > > > -            verbose_build=args.verbose_build,
> > > > > -            mrproper=args.mrproper,
> > > > > -            per_board_out_dir=args.per_board_out_dir,
> > > > > +            no_subdirs=args.no_subdirs, verbose_build=args.verbose_build,
> > > > > +            mrproper=args.mrproper, per_board_out_dir=args.per_board_out_dir,
> > > > >              config_only=args.config_only,
> > > > >              squash_config_y=not args.preserve_config_y,
> > > > >              warnings_as_errors=args.warnings_as_errors,
> > > > > diff --git a/tools/buildman/test.py b/tools/buildman/test.py
> > > > > index f92add7a7c5..ae9963eed4f 100644
> > > > > --- a/tools/buildman/test.py
> > > > > +++ b/tools/buildman/test.py
> > > > > @@ -146,6 +146,7 @@ class TestBuild(unittest.TestCase):
> > > > >          self.toolchains.Add('arm-linux-gcc', test=False)
> > > > >          self.toolchains.Add('sparc-linux-gcc', test=False)
> > > > >          self.toolchains.Add('powerpc-linux-gcc', test=False)
> > > > > +        self.toolchains.Add('/path/to/aarch64-linux-gcc', test=False)
> > > >
> > > > Sorry Simon, but me and others love to be consistent.
> > >
> > > That's fine, but do you have a comment on this patch?
> > >
> > > Regards,
> > > Simon
> >
> > I was just thinking about aarch64-linux-gcc (should it be
> > aarch64-linux-gnu-gcc, btw?)  without path for consistency.
> > But this of course is very minor - feel free to disregard my
> > comment.
> 
> Oh I see...the point here is to test a toolchain which has a path
> prepended to it, That is why this test case is not consistent with the
> others.
> 
> Regards,
> Simon

In this case:

Reviewed-by: Andrejs Cainikovs <andrejs.cainikovs@toradex.com>

Regards,
Andrejs.

Re: [PATCH v3 4/6] buildman: Always use the full path in CROSS_COMPILE

[Simon Glass]

On Wed, Jun 26, 2024 at 09:00:43AM +0100, Simon Glass wrote:
> Hi Andrejs,
>
> On Tue, 25 Jun 2024 at 16:16, Andrejs Cainikovs
> <andrejs.cainikovs@toradex.com> wrote:
> >
> > On Tue, Jun 25, 2024 at 01:38:07PM +0100, Simon Glass wrote:
> > > Hi Andrejs,
> > >
> > > On Tue, 25 Jun 2024 at 01:06, Andrejs Cainikovs
> > > <andrejs.cainikovs@toradex.com> wrote:
> > > >
> > > > On Sun, Jun 23, 2024 at 11:56:20AM -0600, Simon Glass wrote:
> > > > > The feature to set the toolchain path does not seem to be needed. It
> > > > > causes problems with venv (see [1]). Let's remove it.
> > > > >
> > > > > Add some tests while we are here.
> > > > >
> > > > > It does not look like any docs changes are needed for this.
> > > > >
> > > > > [1] https://patchwork.ozlabs.org/project/uboot/patch/20240621131423.2363294-6-sjg@chromium.org/
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > Suggested-by: Tom Rini <trini@konsulko.com>
> > > > > ---
> > > > >
> > > > > Changes in v3:
> > > > > - Drop the PATH modification altogether
> > > > >
> > > > >  tools/buildman/bsettings.py     |  3 ++
> > > > >  tools/buildman/builder.py       |  5 +--
> > > > >  tools/buildman/builderthread.py |  4 +-
> > > > >  tools/buildman/cmdline.py       |  2 -
> > > > >  tools/buildman/control.py       |  6 +--
> > > > >  tools/buildman/test.py          | 75 +++++++++++++++++++++++++++++++++
> > > > >  tools/buildman/toolchain.py     | 20 ++++-----
> > > > >  7 files changed, 92 insertions(+), 23 deletions(-)
> > > > >
Applied to u-boot-dm, thanks!

[PATCH v3 5/6] u_boot_pylib: Use correct coverage tool within venv

[Simon Glass]

When running within a Python venv we must use the 'coverage' tool (which
is within the venv) so that the venv packages are used in preference to
system packages. Otherwise the coverage tests run in a different
environment from the normal tests and may fail due to missing packages.

Handle this by detecting the venv and changing the tool name.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 tools/u_boot_pylib/test_util.py | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/u_boot_pylib/test_util.py b/tools/u_boot_pylib/test_util.py
index f18d385d995..857ce58c98c 100644
--- a/tools/u_boot_pylib/test_util.py
+++ b/tools/u_boot_pylib/test_util.py
@@ -60,12 +60,17 @@ def run_test_coverage(prog, filter_fname, exclude_list, build_dir, required=None
     prefix = ''
     if build_dir:
         prefix = 'PYTHONPATH=$PYTHONPATH:%s/sandbox_spl/tools ' % build_dir
-    cmd = ('%spython3-coverage run '
-           '--omit "%s" %s %s %s %s' % (prefix, ','.join(glob_list),
+
+    # Detect a Python virtualenv and use 'coverage' instead
+    covtool = ('python3-coverage' if sys.prefix == sys.base_prefix else
+               'coverage')
+
+    cmd = ('%s%s run '
+           '--omit "%s" %s %s %s %s' % (prefix, covtool, ','.join(glob_list),
                                         prog, extra_args or '', test_cmd,
                                         single_thread or '-P1'))
     os.system(cmd)
-    stdout = command.output('python3-coverage', 'report')
+    stdout = command.output(covtool, 'report')
     lines = stdout.splitlines()
     if required:
         # Convert '/path/to/name.py' just the module name 'name'
-- 
2.34.1

Re: [PATCH v3 5/6] u_boot_pylib: Use correct coverage tool within venv

[Simon Glass]

When running within a Python venv we must use the 'coverage' tool (which
is within the venv) so that the venv packages are used in preference to
system packages. Otherwise the coverage tests run in a different
environment from the normal tests and may fail due to missing packages.

Handle this by detecting the venv and changing the tool name.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 tools/u_boot_pylib/test_util.py | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Applied to u-boot-dm, thanks!

[PATCH v3 6/6] CI: Run code-coverage test for Binman

[Simon Glass]

Binman includes a good set of tests covering all of its functionality.
This includes a code-coverage test.

However to date the code-coverage test has not been checked
automatically by CI, relying on people to run 'binman test -T'
themselves.

Plug the gap to avoid bugs creeping in future.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v2)

Changes in v2:
- Add to azure also (oops)

 .azure-pipelines.yml | 5 ++++-
 .gitlab-ci.yml       | 4 +++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
index 27f69583c65..65d1d639e49 100644
--- a/.azure-pipelines.yml
+++ b/.azure-pipelines.yml
@@ -128,7 +128,10 @@ stages:
           export PATH=${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc:${PATH}
           ./tools/buildman/buildman -T0 -o ${UBOOT_TRAVIS_BUILD_DIR} -w --board tools-only
           set -ex
-          ./tools/binman/binman --toolpath ${UBOOT_TRAVIS_BUILD_DIR}/tools test
+          export TOOLPATH="--toolpath ${UBOOT_TRAVIS_BUILD_DIR}/tools --toolpath /opt/coreboot"
+          ./tools/binman/binman ${TOOLPATH} test
+          # Avoid "Permission denied: 'cov'" error by using a temporary file
+          COVERAGE_FILE=/tmp/.coverage ./tools/binman/binman ${TOOLPATH} test -T
           ./tools/buildman/buildman -t
           ./tools/dtoc/dtoc -t
           ./tools/patman/patman test
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 165f765a833..eb01fa4868d 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -201,7 +201,9 @@ Run binman, buildman, dtoc, Kconfig and patman testsuites:
       ./tools/buildman/buildman -T0 -o ${UBOOT_TRAVIS_BUILD_DIR} -w
         --board tools-only;
       set -e;
-      ./tools/binman/binman --toolpath ${UBOOT_TRAVIS_BUILD_DIR}/tools test;
+      export TOOLPATH="--toolpath ${UBOOT_TRAVIS_BUILD_DIR}/tools --toolpath /opt/coreboot";
+      ./tools/binman/binman ${TOOLPATH} test;
+      ./tools/binman/binman ${TOOLPATH} test -T;
       ./tools/buildman/buildman -t;
       ./tools/dtoc/dtoc -t;
       ./tools/patman/patman test;
-- 
2.34.1

Re: [PATCH v3 6/6] CI: Run code-coverage test for Binman

[Tom Rini]

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

On Sun, Jun 23, 2024 at 11:56:22AM -0600, Simon Glass wrote:

> Binman includes a good set of tests covering all of its functionality.
> This includes a code-coverage test.
> 
> However to date the code-coverage test has not been checked
> automatically by CI, relying on people to run 'binman test -T'
> themselves.
> 
> Plug the gap to avoid bugs creeping in future.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

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