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

[Carl-Daniel Hailfinger]

On 05.01.2016 07:29, Stefan Tauner wrote:
> 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)

Heh.


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

Definitely.

 
>>> Signed-off-by: Stefan Tauner <stefan.tauner at alumni.tuwien.ac.at>
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>


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

Thanks!

 
>>> +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" :)

Well, now it's at least present in patchwork.


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

Mostly because I didn't have that idea. Such a change would have my
immediate Ack (provided it works).


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

Argh. Should we rewrite the makefile in Python? ;-)


Regards,
Carl-Daniel