[Buildroot] [PATCH 0/1] Build issue related to "command -v"

Markus Mayer mmayer at broadcom.com
Fri Oct 1 17:53:29 UTC 2021


Hi Petr, Arnout, all,

Thanks for all the feedback and for looking into this issue.

On Wed, Sep 29, 2021 at 10:11:46PM +0200, Petr Vorel wrote:
> Hi Markus, Arnout, all,
> 
> >  Hi Markus,
> 
> >  Thank you for this extensive investigation!

You are welcome. It was an interesting one as you can probably
imagine. :-)
 
> > On 28/09/2021 21:55, Markus Mayer wrote:
> 
> > > After commit ca6a2907c27c[1], our automated nightly builds
> > > started experiencing build failures. It took a little while to
> > > track down what was happening. I think I understand now what is
> > > going on.
> 
> > [snip]
> 
> > > One thing of note is that our post-build script calls "make
> > > legal-info", and that is when the problem happens. The purpose
> > > of doing it like this is to include the result of "make
> > > legal-info" in the image.
> 
> > Calling into Buildroot's Makefile recursively from within a
> > post-build script (or a package or whatever) is not something that
> > is supported, in the sense that it's not something that anybody
> > ever tests. The use case definitely makes sense though. I even
> > heard of people starting the build of a different configuration in
> > a post-build script (e.g. for an initramfs).
> 
> > So maybe we should add a test in support/testing that validates
> > this scenario.
> +1

Sounds good to me, as well. It does seem to be quite useful to support
this scenario.

> > > However, when make is invoked a second time with HOSTCC already
> > > defined to call ccache, it'll still assign
> > >     HOSTCC_NOCCACHE := $(HOSTCC)
> > > which now redefines HOSTCC_NOCCACHE to *INCLUDE* ccache (since
> > > HOSTCC does, from earlier)!
> 
> > This is clearly wrong. Your patch helps, but we still have a
> > similar situation with HOSTCC which will be .../ccache .../ccache
> > /usr/bin/gcc in the recursive invocation (i.e. with two times
> > ccache). Although maybe that's not really an issue - "ccache
> > ccache gcc -v" at least gives the expected results.
> Ah, sorry for not catching this.
> 
> > I'm thinking that maybe we should detect recursive invocation in
> > the top-level Makefile and behave differently. For example,
> > everything that is exported doesn't need to be exported again, and
> > stuff like that. Or at least we could protect the entire HOSTCC
> > etc. block against recursive override.
> +1

Definitely no objections from me. I'll defer to those more
knowledgeable about Makefile Magic to make the call what else might
need protection.
 
> > Also, the entire handling of HOSTCC is a bit flaky. It is still
> > from prehistoric commit 8027784 with no clear requirements (e.g. 
> > is HOSTCC allowed to be a command with arguments?  Is it allowed to
> > be a relative path?). It has been documented after-the-fact in
> > docs/manual/common-usage.txt, but I wonder if we really still need
> > it?  I guess it's a way for people to use a host compiler that is
> > more recent that the distro-provided one, but it could just as well
> > be added to $PATH and be done with it...

I think having a documented way of specifying a custom host compiler
(without "poisoning" the environment or being forced to use wrapper
scripts) is a handy thing to have.

Also, one thing you cannot do using PATH is specifying a different
compiler *name*. You can make it search different directories (such as
/opt/toolchains/gcc-x.y/bin or similar), but you can't make it look for
something other than gcc (or cc) or whatever compiler name is
hard-coded into the makefile. If you have HOSTCC, you can name the
actual compiler binary.

Somebody might just be willing to really live on the edge to try out
clang as host compiler. :-)

BTW, we are experimenting with clang as target compiler, but that is a
story for another day.
 
> > [snip]
> 
> > > Here is where it gets interesting. "which" will return two
> > > lines, one for each of the commands:
> 
> > > $ which $HOSTCC_NOCCACHE
> > > /local/users/mmayer/buildroot/output/arm64/host/bin/ccache
> > > /usr/bin/gcc
> 
> >  As mentioned by Nicolas, we really should quote the argument to
> > "command -v", or apply $(firstword ...) to avoid this issue. 
> > Indepedent of which or command -v.
> +1 
> IMHO quoting would require fixing unwanted redefinition
> HOSTCC_NOCCACHE, ($(firstword ...) would not but hide the issue, thus
> I'd prefer fixing the redefinition.

Personally, I like the $(firstword ...) idea.
 
> > [snip]
> 
> > > As such, relying on "command -v" seems a little risky in that it
> > > opens up the possibility for strange build errors that others
> > > cannot reproduce and that nobody would ever think to investigate
> > > as being related to the "command -v" implementation of a specific
> > > shell.
> 
> > It is solving an actual problem (i.e. that "which" is deprecated
> > in some distros), so the best we can do is make the change early
> > enough before a release so people can discover problems with it.

Fair enough. And with all the suggestions in this thread, it is looking
like it can be done safely and without causing nasty surprises down the
road. I think it's fine to proceed with "command -v" in lieu of "which".
 
> > > There is also the issue of some developers working with different
> > > distributions. Somebody developing a feature on distro 1 might
> > > create build problems for others using distro 2 and vice versa. 
> > > Neither would have a way of knowing ahead of time that there will
> > > be an issue.
> 
> > That issue exists regardless of "which" (which BTW has different
> > implementations on different distros anyway, so it can also cause
> > problems).

Good point.

> +1 FYI there is LTP script to cover some which functionality [1]. 
> IMHO type or command -v are more tested in shell implementation than
> this simple test.
> > We try to minimise the external dependencies of Buildroot, and
> > removing "which" from the external dependencies is a good thing
> > IMHO.
> > Bottom line: I think we need to take four actions here.
> 
> > 1. Apply your patch.
> > 2. Improve on it by detecting that the Buildroot overrides have
> > already been exported and don't need to be exported again.
> > 3. Verify all calls to "command -v" and make sure the argument is
> > either quoted or uses $(firstword).
> > 4. Consider the removal of HOSTCC and friends as user-settable
> > variables.

Definitely +1 on points 1-3. I am a little wary about 4 as mentioned
above. While most use cases it be addressed using PATH, it does seem
to be a bit of a heavy-handed approach to tweak a system-wide variable.
And you won't be able to set the compiler name that way.

Regards,
-Markus

> [1] https://github.com/linux-test-project/ltp/blob/master/testcases/commands/which/which01.sh


More information about the buildroot mailing list