[PATCH 3/5] buildman: Support building within a Python venv

[Simon Glass]

Hi Tom,

On Fri, 21 Jun 2024 at 09:22, Tom Rini <trini at 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 at 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 at 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 at 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 at 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