[PATCH 3/5] buildman: Support building within a Python venv
Simon Glass
sjg at chromium.org
Fri Jun 21 01:05:34 CEST 2024
Hi Heinrich,
On Thu, 20 Jun 2024 at 07:38, Heinrich Schuchardt <xypron.glpk at 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 at 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
More information about the U-Boot
mailing list