Hi Tom,
On Thu, 15 Aug 2024 at 13:57, Simon Glass <s...@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 <s...@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
