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

quoted

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
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help