[PATCH 0/5] Add Binman code-coverage test to CI
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(-)
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 \
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
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 \
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'
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 resultArgs:tchn (Toolchain): Toolchain to usefull_path (bool): True to return the full path in CROSS_COMPILErather than adding it to the PATH variablein_env (dict): Input environment to use, None to use current envReturns:tuple:dict: Changes that MakeEnvironment has made to the environmentkey: Environment variable that was changedvalue: New value (for PATH this only includes componentswhich were added)str: Full value of the new PATH variable"""env = tchn.MakeEnvironment(full_path, env=in_env)# Get the original environmentorig_env = dict(os.environb if in_env is None else in_env)orig_path = orig_env[b'PATH'].split(b':')# Find new variablesdiff = dict((k, env[k]) for k in env if orig_env.get(k) != env[k])# Find new / different path componentsdiff_path = Nonenew_path = Noneif 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_pathelse:del diff[b'PATH']return diff, new_pathdef 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 differencetchn = self.toolchains.Select('aarch64')# Normal casesdiff = 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 settchn.override_toolchain = Truediff = self.call_make_environment(tchn, full_path=True)[0]self.assertEqual({b'LC_ALL': b'C'}, diff)# Test that virtualenv is handled correctlytchn.override_toolchain = Falsesys.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 wrappertchn.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 prependsthe 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 thetoolchain path before the venv path, so that builds initiated bybuildman will still respect the venv.Args: full_path: Return the full path in CROSS_COMPILE and don't set PATHenv (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 itif 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 = Noneif 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'
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 resultArgs:tchn (Toolchain): Toolchain to usefull_path (bool): True to return the full path in CROSS_COMPILErather than adding it to the PATH variablein_env (dict): Input environment to use, None to use current envReturns:tuple:dict: Changes that MakeEnvironment has made to the environmentkey: Environment variable that was changedvalue: New value (for PATH this only includes componentswhich were added)str: Full value of the new PATH variable"""env = tchn.MakeEnvironment(full_path, env=in_env)# Get the original environmentorig_env = dict(os.environb if in_env is None else in_env)orig_path = orig_env[b'PATH'].split(b':')# Find new variablesdiff = dict((k, env[k]) for k in env if orig_env.get(k) != env[k])# Find new / different path componentsdiff_path = Nonenew_path = Noneif 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_pathelse:del diff[b'PATH']return diff, new_pathdef 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 differencetchn = self.toolchains.Select('aarch64')# Normal casesdiff = 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 settchn.override_toolchain = Truediff = self.call_make_environment(tchn, full_path=True)[0]self.assertEqual({b'LC_ALL': b'C'}, diff)# Test that virtualenv is handled correctlytchn.override_toolchain = Falsesys.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 wrappertchn.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 thatMaybe 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 prependsthe 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 thetoolchain path before the venv path, so that builds initiated bybuildman will still respect the venv.Args: full_path: Return the full path in CROSS_COMPILE and don't set PATHenv (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 itif 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 = Noneif 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
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 ?
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
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?
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
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.
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
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.
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
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.
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'
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;
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.
participants (3)
-
Heinrich Schuchardt -
Simon Glass -
Tom Rini