Re: [flashrom] [PATCH] Makefile: fix and simplify test program compilations

[Stefan Tauner]

On Wed, 17 Aug 2011 22:51:31 +0200
Carl-Daniel Hailfinger  wrote:
> Am 12.08.2011 21:38 schrieb Stefan Tauner:
> > this was totally broken due to the time behavior of make's shell
> > function's temporal behavior.
> > quote from the gnu make documentation
> > http://www.gnu.org/s/hello/manual/make/Shell-Function.html):
> > "The commands run by calls to the shell function are run when the
> > function calls are expanded"
> > we have used the shell function to echo the test programs to a file.
> > the file name used was equal for all tests and was overwritten for
> > each test. the result was that all tests (in a single target?) used
> > the last test program because the echoing of the test programs was
> > done before all test compilations(!)
>
> Thanks for opening that bug report. Apparently the gnu make
> documentation was incorrect.

just for documentation... this is the report/mail:
http://lists.gnu.org/archive/html/bug-make/2011-08/msg00010.html
> > also the branching for testing ifeq ($(CONFIG_FT2232_SPI), yes) was
> > unnecessary.
>
> Branching never worked for me inside a target, but I also never tried it
> your way.

hm well it depends on the variables used i guess. the documentation
says "Conditionals control what make actually “sees” in the makefile,
so they cannot be used to control recipes at the time of execution."
maybe another temporal anomaly... they should ask kirk or picard for help ;)

the examples clearly show that it should work:
http://www.gnu.org/s/hello/manual/make/Conditional-Example.html
> > side note: we use $(CC) all over the place before we even check if
> > it is there. e.g.:
> > override ARCH = $(strip $(shell LC_ALL=C $(CC) -E arch.h|grep -v '^#'))
> >
> > this creates unwanted output like:
> > make -j1 CC=eiugthes
> > /bin/sh: eiugthes: not found
> > /bin/sh: eiugthes: not found
> > /bin/sh: eiugthes: not found
> > /bin/sh: eiugthes: not found
> > Checking for a C compiler... not found.
>
> Hm yes.

i have mitigated the problem somewhat (see my other mail(s) in this
thread) and will commit it now. please note that this is not fixed
completely though.
> > make: *** [compiler] Error 1
> > Signed-off-by: Stefan Tauner
> >
> > Makefile |   91 ++++++++++++++++++++++++++++++++++++++++----------------------
> >  1 files changed, 59 insertions(+), 32 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index c72d10a..672a0df 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -531,11 +531,62 @@ distclean: clean
> >  strip: $(PROGRAM)$(EXEC_SUFFIX)
> >    $(STRIP) $(STRIP_ARGS) $(PROGRAM)$(EXEC_SUFFIX)
> >
> > +# below are the definitions of test programs as verbatim variables, which get
> > +# exported to environment variables and are referenced with $$<varname> later
> > +define COMPILER_TEST
> > +int main(int argc, char **argv)
> > +{
> > (void) argafc;
>
> Typo?

just to test the consciousness of the reviewer!
(really: remains of my testing)
> > +define UTSNAME_TEST
> > +#include <sys/utsname.h>
> > +struct utsname osinfo;
> > +int main(int argc, char **argv)
> > +{
> > (void) argc;
> > (void) argv;
> > uname (&osinfo);
> > return 0;
> >
> > +}
> > +endef
> > +export UTSNAME_TEST
>
> Hm... the alternative would be to have each of those C programs in a
> separate file in a config/ directory or somesuch. I don't have a strong
> preference either way.
>
> Given that you think verbatim variables are superior, please move them
> directly above the targets which use them.

i dont think they are superior. i said we "could just use different
file names" i.e. keep the current inline shell echo stuff, but just use
different files to store the test programs. that way the shell
expansion could be done at any time without breaking the tests. but
that would still be totally unreadable (the test itself and the recipe
due to temporal behavior).

if we would have more complex programs then all should probably move to
a test dir. we should clean up the main directory before though... (yes
i know.. previous patch, no one cares, no one has time for doing it or
reviewing it... :)
> > compiler: featuresavailable
> >    @printf "Checking for a C compiler... "
> > @$(shell ( echo "int main(int argc, char **argv)"; \
> >    echo "{ (void) argc; (void) argv; return 0; }"; ) > .test.c )
> > @$(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) .test.c -o .test$(EXEC_SUFFIX) >/dev/null &&	\
> > @echo "$$COMPILER_TEST" > .test.c
> > @$(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) .test.c -o .test$(EXEC_SUFFIX) >/dev/null 2>&1 &&	\
> > echo "found." || ( echo "not found."; \
> > rm -f .test.c .test$(EXEC_SUFFIX); exit 1)
> > @rm -f .test.c .test$(EXEC_SUFFIX)
> >
> > @@ -548,12 +599,7 @@ compiler: featuresavailable
> >  ifeq ($(CHECK_LIBPCI), yes)
> >  pciutils: compiler
> >    @printf "Checking for libpci headers... "
> > @# Avoid a failing test due to libpci header symbol shadowing breakage
> > @$(shell ( echo "#define index shadow_workaround_index"; \
> >    echo "#include <pci/pci.h>";		   \
> >    echo "struct pci_access *pacc;";	   \
> >    echo "int main(int argc, char **argv)"; \
> >    echo "{ (void) argc; (void) argv; pacc = pci_alloc(); return 0; }"; ) > .test.c )
> > @echo "$$LIBPCI_TEST" > .test.c
> > @$(CC) -c $(CPPFLAGS) $(CFLAGS) .test.c -o .test.o >/dev/null 2>&1 &&		\
> > echo "found." || ( echo "not found."; echo;			\
> > echo "Please install libpci headers (package pciutils-devel).";	\
> >
> > @@ -588,41 +634,22 @@ featuresavailable:
> >    @false
> >  endif
> >
> > -ifeq ($(CONFIG_FT2232_SPI), yes)
> >  features: compiler
> >    @echo "FEATURES := yes" > .features.tmp
> > +ifeq ($(CONFIG_FT2232_SPI), yes)
>
> I'm really surprised that ifeq inside a target works. IIRC I tried this
> some time ago on my older machine and it did not work. However, if this
> works reliably, the code is indeed a lot more readable.

see above
> Acked-by: Carl-Daniel Hailfinger

thanks! committed in r1416