buildman: Always use the full path in CROSS_COMPILE

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>
Reviewed-by: Andrejs Cainikovs <andrejs.cainikovs@toradex.com>

7 files changed, 92 insertions, 23 deletions

diff --git a/tools/buildman/bsettings.py b/tools/buildman/bsettings.py
index aea724fc559..a7358cfc08a 100644
--- a/tools/buildman/bsettings.py
+++ b/tools/buildman/bsettings.py
@@ -31,6 +31,9 @@ def setup(fname=''):
 def add_file(data):
     settings.read_file(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 c4384f53e8d..c794177f2ba 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, fallback_mrproper=False,
                  per_board_out_dir=False, config_only=False,
                  squash_config_y=False, warnings_as_errors=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
             fallback_mrproper: Run 'make mrproper' and retry on build failure
@@ -337,7 +335,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 bbe2f6f0d24..4e085d6d675 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -406,7 +406,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,
@@ -574,7 +574,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 544a391a464..2673e749d58 100644
--- a/tools/buildman/cmdline.py
+++ b/tools/buildman/cmdline.py
@@ -123,8 +123,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 464835c5be5..037854dfc07 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -788,10 +788,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,
-            fallback_mrproper=args.fallback_mrproper,
+            no_subdirs=args.no_subdirs, verbose_build=args.verbose_build,
+            mrproper=args.mrproper, fallback_mrproper=args.fallback_mrproper,
             per_board_out_dir=args.per_board_out_dir,
             config_only=args.config_only,
             squash_config_y=not args.preserve_config_y,
diff --git a/tools/buildman/test.py b/tools/buildman/test.py
index bfad3093030..ac0a7ab06d6 100644
--- a/tools/buildman/test.py
+++ b/tools/buildman/test.py
@@ -148,6 +148,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
@@ -868,6 +869,80 @@ class TestBuild(unittest.TestCase):
             self.assertEqual([4, 5], control.read_procs(tmpdir))
             self.assertEqual(self.finish_time, self.cur_time)
 
+    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'