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

[PATCH 0/5] 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


Simon Glass (5):
  Dockerfile: Add python3-coverage
  Dockerfile: Add python3-pycryptodome
  buildman: Support building within a Python venv
  u_boot_pylib: Use correct coverage tool within venv
  CI: Run code-coverage test for Binman

 .gitlab-ci.yml                  |  4 +-
 tools/buildman/bsettings.py     |  3 ++
 tools/buildman/test.py          | 83 +++++++++++++++++++++++++++++++++
 tools/buildman/toolchain.py     | 31 ++++++++++--
 tools/docker/Dockerfile         |  2 +
 tools/u_boot_pylib/test_util.py | 11 +++--
 6 files changed, 126 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH 1/5] Dockerfile: 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>
---

 tools/docker/Dockerfile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/docker/Dockerfile b/tools/docker/Dockerfile
index cda87354566..0941b0f6952 100644
--- a/tools/docker/Dockerfile
+++ b/tools/docker/Dockerfile
@@ -97,6 +97,7 @@ RUN apt-get update && apt-get install -y \
 	python-is-python3 \
 	python2.7 \
 	python3 \
+	python3-coverage \
 	python3-dev \
 	python3-pip \
 	python3-pyelftools \
-- 
2.34.1

[PATCH 2/5] Dockerfile: 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>
---

 tools/docker/Dockerfile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/docker/Dockerfile b/tools/docker/Dockerfile
index 0941b0f6952..a23504179c2 100644
--- a/tools/docker/Dockerfile
+++ b/tools/docker/Dockerfile
@@ -98,6 +98,7 @@ RUN apt-get update && apt-get install -y \
 	python2.7 \
 	python3 \
 	python3-coverage \
+	python3-pycryptodome \
 	python3-dev \
 	python3-pip \
 	python3-pyelftools \
-- 
2.34.1

[PATCH 3/5] buildman: Support building within a Python venv

[Simon Glass]

The Python virtualenv tool sets up a few things in the envronment,
putting its path first in the PATH environment variable and setting up
a sys.prefix different from the sys.base_prefix value.

At present buildman puts the toolchain path first in PATH so that it can
be found easily during the build. For sandbox this causes problems since
/usr/bin/gcc (for example) results in '/usr/bin' being prepended to the
PATH variable. As a result, the venv is partially disabled.

The result is that sandbox builds within a venv ignore the venv, e.g.
when looking for packages.

Correct this by detecting the venv and adding the toolchain path after
the venv path.

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

 tools/buildman/bsettings.py |  3 ++
 tools/buildman/test.py      | 83 +++++++++++++++++++++++++++++++++++++
 tools/buildman/toolchain.py | 31 ++++++++++++--
 3 files changed, 113 insertions(+), 4 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/test.py b/tools/buildman/test.py
index f92add7a7c5..83219182fe0 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,88 @@ class TestBuild(unittest.TestCase):
         self.assertEqual([
             ['MARY="mary"', 'Missing expected line: CONFIG_MARY="mary"']], result)
 
+    def call_make_environment(self, tchn, full_path, in_env=None):
+        """Call Toolchain.MakeEnvironment() and process the result
+
+        Args:
+            tchn (Toolchain): Toolchain to use
+            full_path (bool): True to return the full path in CROSS_COMPILE
+                rather than adding it to the PATH variable
+            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(full_path, 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, so that full_path makes a difference
+        tchn = self.toolchains.Select('aarch64')
+
+        # Normal cases
+        diff = self.call_make_environment(tchn, full_path=False)[0]
+        self.assertEqual(
+            {b'CROSS_COMPILE': b'aarch64-linux-', b'LC_ALL': b'C',
+             b'PATH': b'/path/to'}, diff)
+
+        diff = self.call_make_environment(tchn, full_path=True)[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, full_path=True)[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, False, env)
+
+        self.assertIn(b'PATH', diff)
+        self.assertEqual([b'/some/venv/bin', b'/my/path', b'other/things'],
+                         diff_path)
+        self.assertEqual(
+            {b'CROSS_COMPILE': b'aarch64-linux-', b'LC_ALL': b'C',
+             b'PATH': b'/my/path'}, 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, full_path=True)[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 79c7c11a110..cd917d8b7eb 100644
--- a/tools/buildman/toolchain.py
+++ b/tools/buildman/toolchain.py
@@ -172,13 +172,14 @@ class Toolchain:
         else:
             raise ValueError('Unknown arg to GetEnvArgs (%d)' % which)
 
-    def MakeEnvironment(self, full_path):
+    def MakeEnvironment(self, full_path, env=None):
         """Returns an environment for using the toolchain.
 
         Thie 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
-        adding LC_ALL=C.
+        adding LC_ALL=C. For the case where full_path is False, it prepends
+        the toolchain to PATH
 
         Note that os.environb is used to obtain the environment, since in some
         cases the environment many contain non-ASCII characters and we see
@@ -187,15 +188,21 @@ class Toolchain:
           UnicodeEncodeError: 'utf-8' codec can't encode characters in position
              569-570: surrogates not allowed
 
+        When running inside a Python venv, care is taken not to put the
+        toolchain path before the venv path, so that builds initiated by
+        buildman will still respect the venv.
+
         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.
         """
-        env = dict(os.environb)
+        env = dict(env or os.environb)
+
         wrapper = self.GetWrapper()
 
         if self.override_toolchain:
@@ -206,7 +213,23 @@ class Toolchain:
                 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']
+
+            # Detect a Python virtualenv and avoid defeating it
+            if sys.prefix != sys.base_prefix:
+                paths = env[b'PATH'].split(b':')
+                new_paths = []
+                to_insert = tools.to_bytes(self.path)
+                insert_after = tools.to_bytes(sys.prefix)
+                for path in paths:
+                    new_paths.append(path)
+                    if to_insert and path.startswith(insert_after):
+                        new_paths.append(to_insert)
+                        to_insert = None
+                if to_insert:
+                    new_paths.append(to_insert)
+                env[b'PATH'] = b':'.join(new_paths)
+            else:
+                env[b'PATH'] = tools.to_bytes(self.path) + b':' + env[b'PATH']
 
         env[b'LC_ALL'] = b'C'
 
-- 
2.34.1

[PATCH 4/5] 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>
---

 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

[PATCH 5/5] 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>
---

 .gitlab-ci.yml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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 3/5] buildman: Support building within a Python venv

[Heinrich Schuchardt]

On 20.06.24 15:19, Simon Glass wrote:
> The Python virtualenv tool sets up a few things in the envronment,

Nits

%s/envronment/environment/

> putting its path first in the PATH environment variable and setting up
> a sys.prefix different from the sys.base_prefix value.
>
> At present buildman puts the toolchain path first in PATH so that it can
> be found easily during the build. For sandbox this causes problems since
> /usr/bin/gcc (for example) results in '/usr/bin' being prepended to the
> PATH variable. As a result, the venv is partially disabled.

If you want to remember a PATH, why don't you use a differnet variable
like U_BOOT_TOOLPATH to convey this information instead of manipulating
the PATH variable?

>
> The result is that sandbox builds within a venv ignore the venv, e.g.
> when looking for packages.
>
> Correct this by detecting the venv and adding the toolchain path after
> the venv path.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>   tools/buildman/bsettings.py |  3 ++
>   tools/buildman/test.py      | 83 +++++++++++++++++++++++++++++++++++++
>   tools/buildman/toolchain.py | 31 ++++++++++++--
>   3 files changed, 113 insertions(+), 4 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/test.py b/tools/buildman/test.py
> index f92add7a7c5..83219182fe0 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,88 @@ class TestBuild(unittest.TestCase):
>           self.assertEqual([
>               ['MARY="mary"', 'Missing expected line: CONFIG_MARY="mary"']], result)
>
> +    def call_make_environment(self, tchn, full_path, in_env=None):
> +        """Call Toolchain.MakeEnvironment() and process the result
> +
> +        Args:
> +            tchn (Toolchain): Toolchain to use
> +            full_path (bool): True to return the full path in CROSS_COMPILE
> +                rather than adding it to the PATH variable
> +            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(full_path, 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, so that full_path makes a difference
> +        tchn = self.toolchains.Select('aarch64')
> +
> +        # Normal cases
> +        diff = self.call_make_environment(tchn, full_path=False)[0]
> +        self.assertEqual(
> +            {b'CROSS_COMPILE': b'aarch64-linux-', b'LC_ALL': b'C',
> +             b'PATH': b'/path/to'}, diff)
> +
> +        diff = self.call_make_environment(tchn, full_path=True)[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, full_path=True)[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, False, env)
> +
> +        self.assertIn(b'PATH', diff)
> +        self.assertEqual([b'/some/venv/bin', b'/my/path', b'other/things'],
> +                         diff_path)
> +        self.assertEqual(
> +            {b'CROSS_COMPILE': b'aarch64-linux-', b'LC_ALL': b'C',
> +             b'PATH': b'/my/path'}, 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, full_path=True)[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 79c7c11a110..cd917d8b7eb 100644
> --- a/tools/buildman/toolchain.py
> +++ b/tools/buildman/toolchain.py
> @@ -172,13 +172,14 @@ class Toolchain:
>           else:
>               raise ValueError('Unknown arg to GetEnvArgs (%d)' % which)
>
> -    def MakeEnvironment(self, full_path):
> +    def MakeEnvironment(self, full_path, env=None):
>           """Returns an environment for using the toolchain.
>
>           Thie takes the current environment and adds CROSS_COMPILE so that

Maybe you could resolve these nits too:

%s/Thie/This/

>           the tool chain will operate correctly. This also disables localized
>           output and possibly unicode encoded output of all build tools by

%s/unicode/Unicode/

Best regards

Heinrich

> -        adding LC_ALL=C.
> +        adding LC_ALL=C. For the case where full_path is False, it prepends
> +        the toolchain to PATH
>
>           Note that os.environb is used to obtain the environment, since in some
>           cases the environment many contain non-ASCII characters and we see
> @@ -187,15 +188,21 @@ class Toolchain:
>             UnicodeEncodeError: 'utf-8' codec can't encode characters in position
>                569-570: surrogates not allowed
>
> +        When running inside a Python venv, care is taken not to put the
> +        toolchain path before the venv path, so that builds initiated by
> +        buildman will still respect the venv.
> +
>           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.
>           """
> -        env = dict(os.environb)
> +        env = dict(env or os.environb)
> +
>           wrapper = self.GetWrapper()
>
>           if self.override_toolchain:
> @@ -206,7 +213,23 @@ class Toolchain:
>                   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']
> +
> +            # Detect a Python virtualenv and avoid defeating it
> +            if sys.prefix != sys.base_prefix:
> +                paths = env[b'PATH'].split(b':')
> +                new_paths = []
> +                to_insert = tools.to_bytes(self.path)
> +                insert_after = tools.to_bytes(sys.prefix)
> +                for path in paths:
> +                    new_paths.append(path)
> +                    if to_insert and path.startswith(insert_after):
> +                        new_paths.append(to_insert)
> +                        to_insert = None
> +                if to_insert:
> +                    new_paths.append(to_insert)
> +                env[b'PATH'] = b':'.join(new_paths)
> +            else:
> +                env[b'PATH'] = tools.to_bytes(self.path) + b':' + env[b'PATH']
>
>           env[b'LC_ALL'] = b'C'
>

Re: [PATCH 1/5] Dockerfile: Add python3-coverage

[Tom Rini]

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

On Thu, Jun 20, 2024 at 07:19:33AM -0600, Simon Glass wrote:

> Add this package so we can run code-coverage tests for Binman.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

This (and same for patch 2) should be in tools/buildman/requirements.txt

-- 
Tom

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

Re: [PATCH 3/5] buildman: Support building within a Python venv

[Tom Rini]

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

On Thu, Jun 20, 2024 at 07:19:35AM -0600, Simon Glass wrote:

> The Python virtualenv tool sets up a few things in the envronment,
> putting its path first in the PATH environment variable and setting up
> a sys.prefix different from the sys.base_prefix value.
> 
> At present buildman puts the toolchain path first in PATH so that it can
> be found easily during the build. For sandbox this causes problems since
> /usr/bin/gcc (for example) results in '/usr/bin' being prepended to the
> PATH variable. As a result, the venv is partially disabled.
> 
> The result is that sandbox builds within a venv ignore the venv, e.g.
> when looking for packages.
> 
> Correct this by detecting the venv and adding the toolchain path after
> the venv path.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Why are we using PATH at all in this case? Shouldn't we just be setting
CROSS_COMPILE=/full/path/to/the/prefix ?

-- 
Tom

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

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

[Tom Rini]

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

On Thu, Jun 20, 2024 at 07:19:37AM -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>
> ---
> 
>  .gitlab-ci.yml | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

This misses Azure, please update as well.

-- 
Tom

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

Re: [PATCH 3/5] buildman: Support building within a Python venv

[Simon Glass]

Hi Tom,

On Thu, 20 Jun 2024 at 08:32, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Jun 20, 2024 at 07:19:35AM -0600, Simon Glass wrote:
>
> > The Python virtualenv tool sets up a few things in the envronment,
> > putting its path first in the PATH environment variable and setting up
> > a sys.prefix different from the sys.base_prefix value.
> >
> > At present buildman puts the toolchain path first in PATH so that it can
> > be found easily during the build. For sandbox this causes problems since
> > /usr/bin/gcc (for example) results in '/usr/bin' being prepended to the
> > PATH variable. As a result, the venv is partially disabled.
> >
> > The result is that sandbox builds within a venv ignore the venv, e.g.
> > when looking for packages.
> >
> > Correct this by detecting the venv and adding the toolchain path after
> > the venv path.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
>
> Why are we using PATH at all in this case? Shouldn't we just be setting
> CROSS_COMPILE=/full/path/to/the/prefix ?

This is the -p option to buildman. The original commit was:

commit bb1501f2c22c979961b735db775605cccedd98f6
Author: Simon Glass <sjg@chromium.org>
Date:   Mon Dec 1 17:34:00 2014 -0700

    buildman: Add an option to use the full tool chain path

    In some cases there may be multiple toolchains with the same name in the
    path. Provide an option to use the full path in the CROSS_COMPILE
    environment variable.

    Note: Wolfgang mentioned that this is dangerous since in some cases there
    may be other tools on the path that are needed. So this is set up as an
    option, not the default. I will need test confirmation (i.e. that this
    commit fixes a real problem) before merging it.

As to why we don't always do this, well that is back in the mists of
time, 10 years ago.

BTW, this is raising a point ("let's change the behaviour") separate
from the goal of this commit, which is to fix a problem with venv,
albeit that if we made -p the only option, then we could potentially
drop all PATH changes. Perhaps toolchains are built differently now,
such that they always invoke their tools using the same prefix and
dir?

Regards,
Simon

Re: [PATCH 3/5] buildman: Support building within a Python venv

[Simon Glass]

Hi Heinrich,

On Thu, 20 Jun 2024 at 07:38, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 20.06.24 15:19, Simon Glass wrote:
> > The Python virtualenv tool sets up a few things in the envronment,
>
> Nits
>
> %s/envronment/environment/
>
> > putting its path first in the PATH environment variable and setting up
> > a sys.prefix different from the sys.base_prefix value.
> >
> > At present buildman puts the toolchain path first in PATH so that it can
> > be found easily during the build. For sandbox this causes problems since
> > /usr/bin/gcc (for example) results in '/usr/bin' being prepended to the
> > PATH variable. As a result, the venv is partially disabled.
>
> If you want to remember a PATH, why don't you use a differnet variable
> like U_BOOT_TOOLPATH to convey this information instead of manipulating
> the PATH variable?

Remembering a path?

The goal here is to give buildman what it needs, i.e. the combination
of PATH changes (if needed) and CROSS_COMPILE that makes things work.

I am not proposing to change the mechanics of buildman, just deal with
this venv situation which I had not previously noticed.

>
> >
> > The result is that sandbox builds within a venv ignore the venv, e.g.
> > when looking for packages.
> >
> > Correct this by detecting the venv and adding the toolchain path after
> > the venv path.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >   tools/buildman/bsettings.py |  3 ++
> >   tools/buildman/test.py      | 83 +++++++++++++++++++++++++++++++++++++
> >   tools/buildman/toolchain.py | 31 ++++++++++++--
> >   3 files changed, 113 insertions(+), 4 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/test.py b/tools/buildman/test.py
> > index f92add7a7c5..83219182fe0 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,88 @@ class TestBuild(unittest.TestCase):
> >           self.assertEqual([
> >               ['MARY="mary"', 'Missing expected line: CONFIG_MARY="mary"']], result)
> >
> > +    def call_make_environment(self, tchn, full_path, in_env=None):
> > +        """Call Toolchain.MakeEnvironment() and process the result
> > +
> > +        Args:
> > +            tchn (Toolchain): Toolchain to use
> > +            full_path (bool): True to return the full path in CROSS_COMPILE
> > +                rather than adding it to the PATH variable
> > +            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(full_path, 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, so that full_path makes a difference
> > +        tchn = self.toolchains.Select('aarch64')
> > +
> > +        # Normal cases
> > +        diff = self.call_make_environment(tchn, full_path=False)[0]
> > +        self.assertEqual(
> > +            {b'CROSS_COMPILE': b'aarch64-linux-', b'LC_ALL': b'C',
> > +             b'PATH': b'/path/to'}, diff)
> > +
> > +        diff = self.call_make_environment(tchn, full_path=True)[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, full_path=True)[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, False, env)
> > +
> > +        self.assertIn(b'PATH', diff)
> > +        self.assertEqual([b'/some/venv/bin', b'/my/path', b'other/things'],
> > +                         diff_path)
> > +        self.assertEqual(
> > +            {b'CROSS_COMPILE': b'aarch64-linux-', b'LC_ALL': b'C',
> > +             b'PATH': b'/my/path'}, 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, full_path=True)[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 79c7c11a110..cd917d8b7eb 100644
> > --- a/tools/buildman/toolchain.py
> > +++ b/tools/buildman/toolchain.py
> > @@ -172,13 +172,14 @@ class Toolchain:
> >           else:
> >               raise ValueError('Unknown arg to GetEnvArgs (%d)' % which)
> >
> > -    def MakeEnvironment(self, full_path):
> > +    def MakeEnvironment(self, full_path, env=None):
> >           """Returns an environment for using the toolchain.
> >
> >           Thie takes the current environment and adds CROSS_COMPILE so that
>
> Maybe you could resolve these nits too:
>
> %s/Thie/This/
>
> >           the tool chain will operate correctly. This also disables localized
> >           output and possibly unicode encoded output of all build tools by
>
> %s/unicode/Unicode/

OK

>
> Best regards
>
> Heinrich
>
> > -        adding LC_ALL=C.
> > +        adding LC_ALL=C. For the case where full_path is False, it prepends
> > +        the toolchain to PATH
> >
> >           Note that os.environb is used to obtain the environment, since in some
> >           cases the environment many contain non-ASCII characters and we see
> > @@ -187,15 +188,21 @@ class Toolchain:
> >             UnicodeEncodeError: 'utf-8' codec can't encode characters in position
> >                569-570: surrogates not allowed
> >
> > +        When running inside a Python venv, care is taken not to put the
> > +        toolchain path before the venv path, so that builds initiated by
> > +        buildman will still respect the venv.
> > +
> >           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.
> >           """
> > -        env = dict(os.environb)
> > +        env = dict(env or os.environb)
> > +
> >           wrapper = self.GetWrapper()
> >
> >           if self.override_toolchain:
> > @@ -206,7 +213,23 @@ class Toolchain:
> >                   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']
> > +
> > +            # Detect a Python virtualenv and avoid defeating it
> > +            if sys.prefix != sys.base_prefix:
> > +                paths = env[b'PATH'].split(b':')
> > +                new_paths = []
> > +                to_insert = tools.to_bytes(self.path)
> > +                insert_after = tools.to_bytes(sys.prefix)
> > +                for path in paths:
> > +                    new_paths.append(path)
> > +                    if to_insert and path.startswith(insert_after):
> > +                        new_paths.append(to_insert)
> > +                        to_insert = None
> > +                if to_insert:
> > +                    new_paths.append(to_insert)
> > +                env[b'PATH'] = b':'.join(new_paths)
> > +            else:
> > +                env[b'PATH'] = tools.to_bytes(self.path) + b':' + env[b'PATH']
> >
> >           env[b'LC_ALL'] = b'C'
> >
>

Regards,
Simon

Re: [PATCH 3/5] buildman: Support building within a Python venv

[Tom Rini]

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

On Thu, Jun 20, 2024 at 05:05:30PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 20 Jun 2024 at 08:32, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Jun 20, 2024 at 07:19:35AM -0600, Simon Glass wrote:
> >
> > > The Python virtualenv tool sets up a few things in the envronment,
> > > putting its path first in the PATH environment variable and setting up
> > > a sys.prefix different from the sys.base_prefix value.
> > >
> > > At present buildman puts the toolchain path first in PATH so that it can
> > > be found easily during the build. For sandbox this causes problems since
> > > /usr/bin/gcc (for example) results in '/usr/bin' being prepended to the
> > > PATH variable. As a result, the venv is partially disabled.
> > >
> > > The result is that sandbox builds within a venv ignore the venv, e.g.
> > > when looking for packages.
> > >
> > > Correct this by detecting the venv and adding the toolchain path after
> > > the venv path.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> >
> > Why are we using PATH at all in this case? Shouldn't we just be setting
> > CROSS_COMPILE=/full/path/to/the/prefix ?
> 
> This is the -p option to buildman. The original commit was:
> 
> commit bb1501f2c22c979961b735db775605cccedd98f6
> Author: Simon Glass <sjg@chromium.org>
> Date:   Mon Dec 1 17:34:00 2014 -0700
> 
>     buildman: Add an option to use the full tool chain path
> 
>     In some cases there may be multiple toolchains with the same name in the
>     path. Provide an option to use the full path in the CROSS_COMPILE
>     environment variable.
> 
>     Note: Wolfgang mentioned that this is dangerous since in some cases there
>     may be other tools on the path that are needed. So this is set up as an
>     option, not the default. I will need test confirmation (i.e. that this
>     commit fixes a real problem) before merging it.
> 
> As to why we don't always do this, well that is back in the mists of
> time, 10 years ago.
> 
> BTW, this is raising a point ("let's change the behaviour") separate
> from the goal of this commit, which is to fix a problem with venv,
> albeit that if we made -p the only option, then we could potentially
> drop all PATH changes. Perhaps toolchains are built differently now,
> such that they always invoke their tools using the same prefix and
> dir?

Wait, I'm confused. buildman internally updates its own PATH to avoid
calling CROSS_COMPILE with the full path due to a concern about
toolchain bugs?

-- 
Tom

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

Re: [PATCH 3/5] buildman: Support building within a Python venv

[Simon Glass]

Hi Tom,

On Thu, 20 Jun 2024 at 17:30, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Jun 20, 2024 at 05:05:30PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 20 Jun 2024 at 08:32, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, Jun 20, 2024 at 07:19:35AM -0600, Simon Glass wrote:
> > >
> > > > The Python virtualenv tool sets up a few things in the envronment,
> > > > putting its path first in the PATH environment variable and setting up
> > > > a sys.prefix different from the sys.base_prefix value.
> > > >
> > > > At present buildman puts the toolchain path first in PATH so that it can
> > > > be found easily during the build. For sandbox this causes problems since
> > > > /usr/bin/gcc (for example) results in '/usr/bin' being prepended to the
> > > > PATH variable. As a result, the venv is partially disabled.
> > > >
> > > > The result is that sandbox builds within a venv ignore the venv, e.g.
> > > > when looking for packages.
> > > >
> > > > Correct this by detecting the venv and adding the toolchain path after
> > > > the venv path.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > >
> > > Why are we using PATH at all in this case? Shouldn't we just be setting
> > > CROSS_COMPILE=/full/path/to/the/prefix ?
> >
> > This is the -p option to buildman. The original commit was:
> >
> > commit bb1501f2c22c979961b735db775605cccedd98f6
> > Author: Simon Glass <sjg@chromium.org>
> > Date:   Mon Dec 1 17:34:00 2014 -0700
> >
> >     buildman: Add an option to use the full tool chain path
> >
> >     In some cases there may be multiple toolchains with the same name in the
> >     path. Provide an option to use the full path in the CROSS_COMPILE
> >     environment variable.
> >
> >     Note: Wolfgang mentioned that this is dangerous since in some cases there
> >     may be other tools on the path that are needed. So this is set up as an
> >     option, not the default. I will need test confirmation (i.e. that this
> >     commit fixes a real problem) before merging it.
> >
> > As to why we don't always do this, well that is back in the mists of
> > time, 10 years ago.
> >
> > BTW, this is raising a point ("let's change the behaviour") separate
> > from the goal of this commit, which is to fix a problem with venv,
> > albeit that if we made -p the only option, then we could potentially
> > drop all PATH changes. Perhaps toolchains are built differently now,
> > such that they always invoke their tools using the same prefix and
> > dir?
>
> Wait, I'm confused. buildman internally updates its own PATH to avoid
> calling CROSS_COMPILE with the full path due to a concern about
> toolchain bugs?

Not its own PATH: the one it passes to U-Boot's 'make'.

I'm not sure why, actually. It is such a long time ago that I don't remember.

I see:

~/.buildman-toolchains/gcc-13.2.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld

and

~/.buildman-toolchains/gcc-13.2.0-nolibc/arm-linux-gnueabi/arm-linux-gnueabi/bin/ld

but interestingly there is no gcc in the latter directory, which there
was in 4.6 (and presumably for some time after).

Certainly for sandbox there is no prefix, so we cannot add it in that
case, and sandbox is actually the arch used to run these tests.

What are you suggesting we change about this patch?

Regards,
Simon

Re: [PATCH 3/5] buildman: Support building within a Python venv

[Tom Rini]

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

On Fri, Jun 21, 2024 at 08:57:50AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 20 Jun 2024 at 17:30, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Jun 20, 2024 at 05:05:30PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 20 Jun 2024 at 08:32, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Thu, Jun 20, 2024 at 07:19:35AM -0600, Simon Glass wrote:
> > > >
> > > > > The Python virtualenv tool sets up a few things in the envronment,
> > > > > putting its path first in the PATH environment variable and setting up
> > > > > a sys.prefix different from the sys.base_prefix value.
> > > > >
> > > > > At present buildman puts the toolchain path first in PATH so that it can
> > > > > be found easily during the build. For sandbox this causes problems since
> > > > > /usr/bin/gcc (for example) results in '/usr/bin' being prepended to the
> > > > > PATH variable. As a result, the venv is partially disabled.
> > > > >
> > > > > The result is that sandbox builds within a venv ignore the venv, e.g.
> > > > > when looking for packages.
> > > > >
> > > > > Correct this by detecting the venv and adding the toolchain path after
> > > > > the venv path.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > >
> > > > Why are we using PATH at all in this case? Shouldn't we just be setting
> > > > CROSS_COMPILE=/full/path/to/the/prefix ?
> > >
> > > This is the -p option to buildman. The original commit was:
> > >
> > > commit bb1501f2c22c979961b735db775605cccedd98f6
> > > Author: Simon Glass <sjg@chromium.org>
> > > Date:   Mon Dec 1 17:34:00 2014 -0700
> > >
> > >     buildman: Add an option to use the full tool chain path
> > >
> > >     In some cases there may be multiple toolchains with the same name in the
> > >     path. Provide an option to use the full path in the CROSS_COMPILE
> > >     environment variable.
> > >
> > >     Note: Wolfgang mentioned that this is dangerous since in some cases there
> > >     may be other tools on the path that are needed. So this is set up as an
> > >     option, not the default. I will need test confirmation (i.e. that this
> > >     commit fixes a real problem) before merging it.
> > >
> > > As to why we don't always do this, well that is back in the mists of
> > > time, 10 years ago.
> > >
> > > BTW, this is raising a point ("let's change the behaviour") separate
> > > from the goal of this commit, which is to fix a problem with venv,
> > > albeit that if we made -p the only option, then we could potentially
> > > drop all PATH changes. Perhaps toolchains are built differently now,
> > > such that they always invoke their tools using the same prefix and
> > > dir?
> >
> > Wait, I'm confused. buildman internally updates its own PATH to avoid
> > calling CROSS_COMPILE with the full path due to a concern about
> > toolchain bugs?
> 
> Not its own PATH: the one it passes to U-Boot's 'make'.

OK, but the point stands.

> I'm not sure why, actually. It is such a long time ago that I don't remember.
> 
> I see:
> 
> ~/.buildman-toolchains/gcc-13.2.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld

Yes, prefixed version that's allowed to be called by users.

> and
> 
> ~/.buildman-toolchains/gcc-13.2.0-nolibc/arm-linux-gnueabi/arm-linux-gnueabi/bin/ld

Internal usage, here be dragons and all that.

> but interestingly there is no gcc in the latter directory, which there
> was in 4.6 (and presumably for some time after).
> 
> Certainly for sandbox there is no prefix, so we cannot add it in that
> case, and sandbox is actually the arch used to run these tests.

CROSS_COMPILE is empty for sandbox, yes.

> What are you suggesting we change about this patch?

That it's going about things backwards? If you're setting CROSS_COMPILE
_then_ it should be the full path that it already knows otherwise if not
setting CROSS_COMPILE then also not modifying PATH.

-- 
Tom

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

Re: [PATCH 3/5] buildman: Support building within a Python venv

[Simon Glass]

Hi Tom,

On Fri, 21 Jun 2024 at 09:22, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Jun 21, 2024 at 08:57:50AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 20 Jun 2024 at 17:30, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, Jun 20, 2024 at 05:05:30PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, 20 Jun 2024 at 08:32, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Thu, Jun 20, 2024 at 07:19:35AM -0600, Simon Glass wrote:
> > > > >
> > > > > > The Python virtualenv tool sets up a few things in the envronment,
> > > > > > putting its path first in the PATH environment variable and setting up
> > > > > > a sys.prefix different from the sys.base_prefix value.
> > > > > >
> > > > > > At present buildman puts the toolchain path first in PATH so that it can
> > > > > > be found easily during the build. For sandbox this causes problems since
> > > > > > /usr/bin/gcc (for example) results in '/usr/bin' being prepended to the
> > > > > > PATH variable. As a result, the venv is partially disabled.
> > > > > >
> > > > > > The result is that sandbox builds within a venv ignore the venv, e.g.
> > > > > > when looking for packages.
> > > > > >
> > > > > > Correct this by detecting the venv and adding the toolchain path after
> > > > > > the venv path.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > >
> > > > > Why are we using PATH at all in this case? Shouldn't we just be setting
> > > > > CROSS_COMPILE=/full/path/to/the/prefix ?
> > > >
> > > > This is the -p option to buildman. The original commit was:
> > > >
> > > > commit bb1501f2c22c979961b735db775605cccedd98f6
> > > > Author: Simon Glass <sjg@chromium.org>
> > > > Date:   Mon Dec 1 17:34:00 2014 -0700
> > > >
> > > >     buildman: Add an option to use the full tool chain path
> > > >
> > > >     In some cases there may be multiple toolchains with the same name in the
> > > >     path. Provide an option to use the full path in the CROSS_COMPILE
> > > >     environment variable.
> > > >
> > > >     Note: Wolfgang mentioned that this is dangerous since in some cases there
> > > >     may be other tools on the path that are needed. So this is set up as an
> > > >     option, not the default. I will need test confirmation (i.e. that this
> > > >     commit fixes a real problem) before merging it.
> > > >
> > > > As to why we don't always do this, well that is back in the mists of
> > > > time, 10 years ago.
> > > >
> > > > BTW, this is raising a point ("let's change the behaviour") separate
> > > > from the goal of this commit, which is to fix a problem with venv,
> > > > albeit that if we made -p the only option, then we could potentially
> > > > drop all PATH changes. Perhaps toolchains are built differently now,
> > > > such that they always invoke their tools using the same prefix and
> > > > dir?
> > >
> > > Wait, I'm confused. buildman internally updates its own PATH to avoid
> > > calling CROSS_COMPILE with the full path due to a concern about
> > > toolchain bugs?
> >
> > Not its own PATH: the one it passes to U-Boot's 'make'.
>
> OK, but the point stands.
>
> > I'm not sure why, actually. It is such a long time ago that I don't remember.
> >
> > I see:
> >
> > ~/.buildman-toolchains/gcc-13.2.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld
>
> Yes, prefixed version that's allowed to be called by users.
>
> > and
> >
> > ~/.buildman-toolchains/gcc-13.2.0-nolibc/arm-linux-gnueabi/arm-linux-gnueabi/bin/ld
>
> Internal usage, here be dragons and all that.
>
> > but interestingly there is no gcc in the latter directory, which there
> > was in 4.6 (and presumably for some time after).
> >
> > Certainly for sandbox there is no prefix, so we cannot add it in that
> > case, and sandbox is actually the arch used to run these tests.
>
> CROSS_COMPILE is empty for sandbox, yes.
>
> > What are you suggesting we change about this patch?
>
> That it's going about things backwards? If you're setting CROSS_COMPILE
> _then_ it should be the full path that it already knows otherwise if not
> setting CROSS_COMPILE then also not modifying PATH.

That is what the code does, yes. It either adds the toolchain path to
CROSS_COMPILE or to PATH, not both. The 'full_path' argument controls
which one it uses.

Regards,
Simon

Re: [PATCH 3/5] buildman: Support building within a Python venv

[Tom Rini]

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

On Fri, Jun 21, 2024 at 12:19:12PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 21 Jun 2024 at 09:22, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Jun 21, 2024 at 08:57:50AM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 20 Jun 2024 at 17:30, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Thu, Jun 20, 2024 at 05:05:30PM -0600, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Thu, 20 Jun 2024 at 08:32, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Thu, Jun 20, 2024 at 07:19:35AM -0600, Simon Glass wrote:
> > > > > >
> > > > > > > The Python virtualenv tool sets up a few things in the envronment,
> > > > > > > putting its path first in the PATH environment variable and setting up
> > > > > > > a sys.prefix different from the sys.base_prefix value.
> > > > > > >
> > > > > > > At present buildman puts the toolchain path first in PATH so that it can
> > > > > > > be found easily during the build. For sandbox this causes problems since
> > > > > > > /usr/bin/gcc (for example) results in '/usr/bin' being prepended to the
> > > > > > > PATH variable. As a result, the venv is partially disabled.
> > > > > > >
> > > > > > > The result is that sandbox builds within a venv ignore the venv, e.g.
> > > > > > > when looking for packages.
> > > > > > >
> > > > > > > Correct this by detecting the venv and adding the toolchain path after
> > > > > > > the venv path.
> > > > > > >
> > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > >
> > > > > > Why are we using PATH at all in this case? Shouldn't we just be setting
> > > > > > CROSS_COMPILE=/full/path/to/the/prefix ?
> > > > >
> > > > > This is the -p option to buildman. The original commit was:
> > > > >
> > > > > commit bb1501f2c22c979961b735db775605cccedd98f6
> > > > > Author: Simon Glass <sjg@chromium.org>
> > > > > Date:   Mon Dec 1 17:34:00 2014 -0700
> > > > >
> > > > >     buildman: Add an option to use the full tool chain path
> > > > >
> > > > >     In some cases there may be multiple toolchains with the same name in the
> > > > >     path. Provide an option to use the full path in the CROSS_COMPILE
> > > > >     environment variable.
> > > > >
> > > > >     Note: Wolfgang mentioned that this is dangerous since in some cases there
> > > > >     may be other tools on the path that are needed. So this is set up as an
> > > > >     option, not the default. I will need test confirmation (i.e. that this
> > > > >     commit fixes a real problem) before merging it.
> > > > >
> > > > > As to why we don't always do this, well that is back in the mists of
> > > > > time, 10 years ago.
> > > > >
> > > > > BTW, this is raising a point ("let's change the behaviour") separate
> > > > > from the goal of this commit, which is to fix a problem with venv,
> > > > > albeit that if we made -p the only option, then we could potentially
> > > > > drop all PATH changes. Perhaps toolchains are built differently now,
> > > > > such that they always invoke their tools using the same prefix and
> > > > > dir?
> > > >
> > > > Wait, I'm confused. buildman internally updates its own PATH to avoid
> > > > calling CROSS_COMPILE with the full path due to a concern about
> > > > toolchain bugs?
> > >
> > > Not its own PATH: the one it passes to U-Boot's 'make'.
> >
> > OK, but the point stands.
> >
> > > I'm not sure why, actually. It is such a long time ago that I don't remember.
> > >
> > > I see:
> > >
> > > ~/.buildman-toolchains/gcc-13.2.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld
> >
> > Yes, prefixed version that's allowed to be called by users.
> >
> > > and
> > >
> > > ~/.buildman-toolchains/gcc-13.2.0-nolibc/arm-linux-gnueabi/arm-linux-gnueabi/bin/ld
> >
> > Internal usage, here be dragons and all that.
> >
> > > but interestingly there is no gcc in the latter directory, which there
> > > was in 4.6 (and presumably for some time after).
> > >
> > > Certainly for sandbox there is no prefix, so we cannot add it in that
> > > case, and sandbox is actually the arch used to run these tests.
> >
> > CROSS_COMPILE is empty for sandbox, yes.
> >
> > > What are you suggesting we change about this patch?
> >
> > That it's going about things backwards? If you're setting CROSS_COMPILE
> > _then_ it should be the full path that it already knows otherwise if not
> > setting CROSS_COMPILE then also not modifying PATH.
> 
> That is what the code does, yes. It either adds the toolchain path to
> CROSS_COMPILE or to PATH, not both. The 'full_path' argument controls
> which one it uses.

I'm saying it should always use the full path to CROSS_COMPILE and never
modify PATH.

-- 
Tom

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

Re: [PATCH 3/5] buildman: Support building within a Python venv

[Simon Glass]

Hi Tom,

On Fri, 21 Jun 2024 at 13:26, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Jun 21, 2024 at 12:19:12PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 21 Jun 2024 at 09:22, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Fri, Jun 21, 2024 at 08:57:50AM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, 20 Jun 2024 at 17:30, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Thu, Jun 20, 2024 at 05:05:30PM -0600, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Thu, 20 Jun 2024 at 08:32, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Thu, Jun 20, 2024 at 07:19:35AM -0600, Simon Glass wrote:
> > > > > > >
> > > > > > > > The Python virtualenv tool sets up a few things in the envronment,
> > > > > > > > putting its path first in the PATH environment variable and setting up
> > > > > > > > a sys.prefix different from the sys.base_prefix value.
> > > > > > > >
> > > > > > > > At present buildman puts the toolchain path first in PATH so that it can
> > > > > > > > be found easily during the build. For sandbox this causes problems since
> > > > > > > > /usr/bin/gcc (for example) results in '/usr/bin' being prepended to the
> > > > > > > > PATH variable. As a result, the venv is partially disabled.
> > > > > > > >
> > > > > > > > The result is that sandbox builds within a venv ignore the venv, e.g.
> > > > > > > > when looking for packages.
> > > > > > > >
> > > > > > > > Correct this by detecting the venv and adding the toolchain path after
> > > > > > > > the venv path.
> > > > > > > >
> > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > >
> > > > > > > Why are we using PATH at all in this case? Shouldn't we just be setting
> > > > > > > CROSS_COMPILE=/full/path/to/the/prefix ?
> > > > > >
> > > > > > This is the -p option to buildman. The original commit was:
> > > > > >
> > > > > > commit bb1501f2c22c979961b735db775605cccedd98f6
> > > > > > Author: Simon Glass <sjg@chromium.org>
> > > > > > Date:   Mon Dec 1 17:34:00 2014 -0700
> > > > > >
> > > > > >     buildman: Add an option to use the full tool chain path
> > > > > >
> > > > > >     In some cases there may be multiple toolchains with the same name in the
> > > > > >     path. Provide an option to use the full path in the CROSS_COMPILE
> > > > > >     environment variable.
> > > > > >
> > > > > >     Note: Wolfgang mentioned that this is dangerous since in some cases there
> > > > > >     may be other tools on the path that are needed. So this is set up as an
> > > > > >     option, not the default. I will need test confirmation (i.e. that this
> > > > > >     commit fixes a real problem) before merging it.
> > > > > >
> > > > > > As to why we don't always do this, well that is back in the mists of
> > > > > > time, 10 years ago.
> > > > > >
> > > > > > BTW, this is raising a point ("let's change the behaviour") separate
> > > > > > from the goal of this commit, which is to fix a problem with venv,
> > > > > > albeit that if we made -p the only option, then we could potentially
> > > > > > drop all PATH changes. Perhaps toolchains are built differently now,
> > > > > > such that they always invoke their tools using the same prefix and
> > > > > > dir?
> > > > >
> > > > > Wait, I'm confused. buildman internally updates its own PATH to avoid
> > > > > calling CROSS_COMPILE with the full path due to a concern about
> > > > > toolchain bugs?
> > > >
> > > > Not its own PATH: the one it passes to U-Boot's 'make'.
> > >
> > > OK, but the point stands.
> > >
> > > > I'm not sure why, actually. It is such a long time ago that I don't remember.
> > > >
> > > > I see:
> > > >
> > > > ~/.buildman-toolchains/gcc-13.2.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld
> > >
> > > Yes, prefixed version that's allowed to be called by users.
> > >
> > > > and
> > > >
> > > > ~/.buildman-toolchains/gcc-13.2.0-nolibc/arm-linux-gnueabi/arm-linux-gnueabi/bin/ld
> > >
> > > Internal usage, here be dragons and all that.
> > >
> > > > but interestingly there is no gcc in the latter directory, which there
> > > > was in 4.6 (and presumably for some time after).
> > > >
> > > > Certainly for sandbox there is no prefix, so we cannot add it in that
> > > > case, and sandbox is actually the arch used to run these tests.
> > >
> > > CROSS_COMPILE is empty for sandbox, yes.
> > >
> > > > What are you suggesting we change about this patch?
> > >
> > > That it's going about things backwards? If you're setting CROSS_COMPILE
> > > _then_ it should be the full path that it already knows otherwise if not
> > > setting CROSS_COMPILE then also not modifying PATH.
> >
> > That is what the code does, yes. It either adds the toolchain path to
> > CROSS_COMPILE or to PATH, not both. The 'full_path' argument controls
> > which one it uses.
>
> I'm saying it should always use the full path to CROSS_COMPILE and never
> modify PATH.

That wouldn't work with toolchains that don't have a prefix, though.
If that is what you want I'm happy to modify the patch.

Regards,
Simon

Re: [PATCH 3/5] buildman: Support building within a Python venv

[Tom Rini]

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

On Fri, Jun 21, 2024 at 01:39:46PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 21 Jun 2024 at 13:26, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Jun 21, 2024 at 12:19:12PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Fri, 21 Jun 2024 at 09:22, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Fri, Jun 21, 2024 at 08:57:50AM -0600, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Thu, 20 Jun 2024 at 17:30, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Thu, Jun 20, 2024 at 05:05:30PM -0600, Simon Glass wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Thu, 20 Jun 2024 at 08:32, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Thu, Jun 20, 2024 at 07:19:35AM -0600, Simon Glass wrote:
> > > > > > > >
> > > > > > > > > The Python virtualenv tool sets up a few things in the envronment,
> > > > > > > > > putting its path first in the PATH environment variable and setting up
> > > > > > > > > a sys.prefix different from the sys.base_prefix value.
> > > > > > > > >
> > > > > > > > > At present buildman puts the toolchain path first in PATH so that it can
> > > > > > > > > be found easily during the build. For sandbox this causes problems since
> > > > > > > > > /usr/bin/gcc (for example) results in '/usr/bin' being prepended to the
> > > > > > > > > PATH variable. As a result, the venv is partially disabled.
> > > > > > > > >
> > > > > > > > > The result is that sandbox builds within a venv ignore the venv, e.g.
> > > > > > > > > when looking for packages.
> > > > > > > > >
> > > > > > > > > Correct this by detecting the venv and adding the toolchain path after
> > > > > > > > > the venv path.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > >
> > > > > > > > Why are we using PATH at all in this case? Shouldn't we just be setting
> > > > > > > > CROSS_COMPILE=/full/path/to/the/prefix ?
> > > > > > >
> > > > > > > This is the -p option to buildman. The original commit was:
> > > > > > >
> > > > > > > commit bb1501f2c22c979961b735db775605cccedd98f6
> > > > > > > Author: Simon Glass <sjg@chromium.org>
> > > > > > > Date:   Mon Dec 1 17:34:00 2014 -0700
> > > > > > >
> > > > > > >     buildman: Add an option to use the full tool chain path
> > > > > > >
> > > > > > >     In some cases there may be multiple toolchains with the same name in the
> > > > > > >     path. Provide an option to use the full path in the CROSS_COMPILE
> > > > > > >     environment variable.
> > > > > > >
> > > > > > >     Note: Wolfgang mentioned that this is dangerous since in some cases there
> > > > > > >     may be other tools on the path that are needed. So this is set up as an
> > > > > > >     option, not the default. I will need test confirmation (i.e. that this
> > > > > > >     commit fixes a real problem) before merging it.
> > > > > > >
> > > > > > > As to why we don't always do this, well that is back in the mists of
> > > > > > > time, 10 years ago.
> > > > > > >
> > > > > > > BTW, this is raising a point ("let's change the behaviour") separate
> > > > > > > from the goal of this commit, which is to fix a problem with venv,
> > > > > > > albeit that if we made -p the only option, then we could potentially
> > > > > > > drop all PATH changes. Perhaps toolchains are built differently now,
> > > > > > > such that they always invoke their tools using the same prefix and
> > > > > > > dir?
> > > > > >
> > > > > > Wait, I'm confused. buildman internally updates its own PATH to avoid
> > > > > > calling CROSS_COMPILE with the full path due to a concern about
> > > > > > toolchain bugs?
> > > > >
> > > > > Not its own PATH: the one it passes to U-Boot's 'make'.
> > > >
> > > > OK, but the point stands.
> > > >
> > > > > I'm not sure why, actually. It is such a long time ago that I don't remember.
> > > > >
> > > > > I see:
> > > > >
> > > > > ~/.buildman-toolchains/gcc-13.2.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld
> > > >
> > > > Yes, prefixed version that's allowed to be called by users.
> > > >
> > > > > and
> > > > >
> > > > > ~/.buildman-toolchains/gcc-13.2.0-nolibc/arm-linux-gnueabi/arm-linux-gnueabi/bin/ld
> > > >
> > > > Internal usage, here be dragons and all that.
> > > >
> > > > > but interestingly there is no gcc in the latter directory, which there
> > > > > was in 4.6 (and presumably for some time after).
> > > > >
> > > > > Certainly for sandbox there is no prefix, so we cannot add it in that
> > > > > case, and sandbox is actually the arch used to run these tests.
> > > >
> > > > CROSS_COMPILE is empty for sandbox, yes.
> > > >
> > > > > What are you suggesting we change about this patch?
> > > >
> > > > That it's going about things backwards? If you're setting CROSS_COMPILE
> > > > _then_ it should be the full path that it already knows otherwise if not
> > > > setting CROSS_COMPILE then also not modifying PATH.
> > >
> > > That is what the code does, yes. It either adds the toolchain path to
> > > CROSS_COMPILE or to PATH, not both. The 'full_path' argument controls
> > > which one it uses.
> >
> > I'm saying it should always use the full path to CROSS_COMPILE and never
> > modify PATH.
> 
> That wouldn't work with toolchains that don't have a prefix, though.
> If that is what you want I'm happy to modify the patch.

Yes, thanks.

-- 
Tom

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