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 <tr...@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 <tr...@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 <tr...@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 <tr...@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 <s...@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 <s...@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. -- Tom
signature.asc
Description: PGP signature