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