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

Hi Tom,

On Thu, 15 Aug 2024 at 13:57, Simon Glass sjg@chromium.org wrote:

The Python virtualenv tool sets up a few things in the 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.

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

(no changes since v2)

Changes in v2:

• Fix 'envronment' typo

tools/buildman/bsettings.py | 3 ++ tools/buildman/test.py | 84 +++++++++++++++++++++++++++++++++++++ tools/buildman/toolchain.py | 31 ++++++++++++-- 3 files changed, 114 insertions(+), 4 deletions(-)

diff --git a/tools/buildman/bsettings.py b/tools/buildman/bsettings.py index aea724fc559..a7358cfc08a 100644 --- a/tools/buildman/bsettings.py +++ b/tools/buildman/bsettings.py @@ -31,6 +31,9 @@ def setup(fname=''): def add_file(data): settings.read_file(io.StringIO(data))

+def add_section(name):

• settings.add_section(name)

def get_items(section): """Get the items from a section of the config.

diff --git a/tools/buildman/test.py b/tools/buildman/test.py index bfad3093030..6191af95445 100644 --- a/tools/buildman/test.py +++ b/tools/buildman/test.py @@ -148,6 +148,7 @@ class TestBuild(unittest.TestCase): self.toolchains.Add('arm-linux-gcc', test=False) self.toolchains.Add('sparc-linux-gcc', test=False) self.toolchains.Add('powerpc-linux-gcc', test=False)

•    self.toolchains.Add('/path/to/aarch64-linux-gcc', test=False)
     self.toolchains.Add('gcc', test=False)
  
     # Avoid sending any output
  

@@ -868,6 +869,89 @@ class TestBuild(unittest.TestCase): self.assertEqual([4, 5], control.read_procs(tmpdir)) self.assertEqual(self.finish_time, self.cur_time)

• def call_make_environment(self, tchn, 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 324ad0e0821..6ca79c2c0f9 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.

  This 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

Can this one be picked up please? It fixes something and Jerome's series should really go on top of it.

Regards, Simon