Hi Heinrich, On Thu, 20 Jun 2024 at 07:38, Heinrich Schuchardt <xypron.g...@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 <s...@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