| Message ID | 20240620131937.1130446-4-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Add Binman code-coverage test to CI | expand |
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 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/ 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' >
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
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 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
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.
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'
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(-)