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

[Petr Vorel <petr.vorel@gmail.com>]

Hi Markus, Arnout, all,

>  Hi Markus,

>  Thank you for this extensive investigation!

> On 28/09/2021 21:55, Markus Mayer wrote:
> > Hi all,

> > 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.

> > This is a bit of a lengthy email as it was a bit of a lengthy
> > investigation, and I want to relay my findings and some concerns.

> [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


> > 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

>  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...

> [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.

> [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.


> > 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).
+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.

Thanks for further investigation.

Kind regards,
Petr

>  Regards,
>  Arnout

[1] https://github.com/linux-test-project/ltp/blob/master/testcases/commands/which/which01.sh
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot