[flashrom] [PATCH 2/3] Make make errors more less flashily.

[Stefan Tauner]

On Tue, 5 Jan 2016 01:54:52 +0100
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> On 15.11.2015 16:40, Stefan Tauner wrote:
> > Previously we have hid almost all error messages from called tools like
> > pkg-config or the compiler in the "configure" step. Only exceptions were
> > the library checks because Carl-Daniel felt it improved usability in
> > case of missing headers etc.
> >
> > However, this makes the output of the makefile less readable and in the
> > case of pkg-config fallbacks the error messages are actually expected.
> > Instead of throwing a part of the detailed stderr outputs of the tools into the
> > face of the users and completely hiding others, write all of them into
> > a separate log file instead.
> 
> Agreed.
> 
> Should we eventually move to "make config; make" to simplify the makefile?

As long as we can sustain the single run make, I'd like to keep it. But
I'd rather get rid of much of the auto-enabling if it gets even more
in the way because it is a large part of the problem and is
actually reducing usability in some cases (have a chat with mrnuke about
that :P)

The alternative would be to provide an easy way for the user to build
sets of programmers supported by the respective libraries, e.g.
  make CONFIG_LIBUSB0 CONFIG_LIBUSB1
would build all programmers requiring libusb-0.1 and libusb-1.0 (only).
We should of course give the user a good indication of what libraries
would actually work but if he does not want to or can not install all
of them he should (still) have an easy way to disable programmers that
do require certain libraries (even without auto-detection).

The most important part (and the one I would like to focus on for now)
is to provide better diagnostics in case not all libraries are
available but the user actually wants them to install/fix.

> > Also, disable implicit rules because we don't need them and they just
> > make the build slower.
> 
> Good idea.

Although not really making a difference. You basically just get less
output from make -d that's all.
 
> 
> > Signed-off-by: Stefan Tauner <stefan.tauner at alumni.tuwien.ac.at>
> 
> Comments inline.
> 
> 
> > diff --git a/Makefile b/Makefile
> > index e68b106..3f96a15 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -37,6 +37,8 @@ MANDIR  ?= $(PREFIX)/share/man
> >  CFLAGS  ?= -Os -Wall -Wshadow
> >  EXPORTDIR ?= .
> >  RANLIB  ?= ranlib
> > +PKG_CONFIG ?= pkg-config
> > +BUILD_DETAILS_FILE ?= build_details.txt
> >  
> >  # The following parameter changes the default programmer that will be used if there is no -p/--programmer
> >  # argument given when running flashrom. The predefined setting does not enable any default so that every
> > @@ -70,6 +72,8 @@ LDFLAGS += -L$(LIBS_BASE)/lib -Wl,-rpath -Wl,$(LIBS_BASE)/lib
> >  PKG_CONFIG_LIBDIR ?= $(LIBS_BASE)/lib/pkgconfig
> >  endif
> >  
> > +dummy_for_make_3_80:=$(shell printf "Build started on " >$(BUILD_DETAILS_FILE) ; date >>$(BUILD_DETAILS_FILE) ; echo >>$(BUILD_DETAILS_FILE))
> 
> What's the oldest make version where the new makefile works reliably? Do
> we have to document that somewhere or test+abort?

Tell me. I have tested 3.80 apparently, else I would not have added
that workaround ;) I have tested it on the build bot of course and
there are no issues:
http://buildbot.flashrom.org/buildresults/flashrom-000273-L4o/results.html

> > +debug_shell = $(shell export LC_ALL=C ; { echo 'exec: export LC_ALL=C ; { $(1) ; }' >&2;  { $(1) ; } | tee -a $(BUILD_DETAILS_FILE) ; echo >&2 ; } 2>>$(BUILD_DETAILS_FILE))
> 
> I'd love to learn how this works. Do you have any pointers?

It gets a command line in form of a string as first parameter, notes
that it will execute that command by echoing it to stderr. It then goes
forward and executes the command copying its stdout to the build
details file. It then adds a line break for better formatting to
stderr... and all that stderr output (the echo in the beginning as well
as all stderr from the execution of the command) will be appended to
the build details file as well.
I definitely should have commented that "better" :)

BTW why don't we set LC_ALL=C globally? Please don't tell me because of
i10n of compiler errors...

> >  ###############################################################################
> >  # General OS-specific settings.
> >  # 1. Prepare for later by gathering information about host and target OS
> > @@ -90,7 +94,7 @@ endif
> >  # IMPORTANT: The following line must be placed before TARGET_OS is ever used
> >  # (of course), but should come after any lines setting CC because the line
> >  # below uses CC itself.
> > -override TARGET_OS := $(strip $(shell LC_ALL=C $(CC) $(CPPFLAGS) -E os.h 2>/dev/null | grep -v '^\#' | grep '"' | cut -f 2 -d'"'))
> > +override TARGET_OS := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E os.h 2>/dev/null | grep -v '^\#' | grep '"' | cut -f 2 -d'"'))
> >  
> >  ifeq ($(TARGET_OS), Darwin)
> >  CPPFLAGS += -I/opt/local/include -I/usr/local/include
> > [...]
> > @@ -824,11 +834,11 @@ endef
> >  export COMPILER_TEST
> >  
> >  compiler: featuresavailable
> > -	@printf "Checking for a C compiler... "
> > +	@printf "Checking for a C compiler... " | tee -a $(BUILD_DETAILS_FILE)
> >  	@echo "$$COMPILER_TEST" > .test.c
> > -	@$(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) .test.c -o .test$(EXEC_SUFFIX) >/dev/null &&	\
> > -		echo "found." || ( echo "not found."; \
> > -		rm -f .test.c .test$(EXEC_SUFFIX); exit 1)
> > +	@{ { { { { $(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) .test.c -o .test$(EXEC_SUFFIX) >&2 && \
> > +		echo "found." || { echo "not found."; \
> > +		rm -f .test.c .test$(EXEC_SUFFIX); exit 1; }; } 2>>$(BUILD_DETAILS_FILE); echo $? >&3 ; } | tee -a $(BUILD_DETAILS_FILE) >&4; } 3>&1;} | { read rc ; exit ${rc}; } } 4>&1
> >  	@rm -f .test.c .test$(EXEC_SUFFIX)
> >  	@printf "Target arch is "
> >  	@# FreeBSD wc will output extraneous whitespace.
> 
> Hm. Is this compatible with classic sh, or do you introduce a dependency
> on bash here?

The reason why this awful thing exists is because I wanted it to be
POSIXly! In bash one can easily access the exit state of any pipe stage
(PIPESTATUS array). It basically forwards the exist status of the
test commands as a string through an explicit pipe (FD 3) and retrieves
it at the end again to convert it to the exit value of the whole thing.
All that simply to capture the return code of the test commands.
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner