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?
Also, disable implicit rules because we don't need them and they just make the build slower.
Good idea.
Signed-off-by: Stefan Tauner stefan.tauner@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?
+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?
############################################################################### # 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
Hm. Is this compatible with classic sh, or do you introduce a dependency on bash here?
Regards, Carl-Daniel
On Tue, 5 Jan 2016 01:54:52 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@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@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>&1Hm. 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.
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@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@alumni.tuwien.ac.at
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@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>&1Hm. 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
On Thu, 7 Jan 2016 13:41:58 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
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@gmx.net wrote:
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@alumni.tuwien.ac.at
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
I have found some minor additional bugs and committed it after fixing those in r1911, 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.
Will comment on it in a future patch as well.
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).
I guess it works... http://lkml.iu.edu/hypermail/linux/kernel/0912.3/00246.html
-
Carl-Daniel Hailfinger -
Stefan Tauner