[RFC PATCH 0/3] kbuild: In quiet mode, print the full command line if it fails

[RFC PATCH 0/3] kbuild: In quiet mode, print the full command line if it fails

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1277 bytes --]

The default (quiet) kbuild output is usually preferable to a log
showing every command line in full.  However, after a failure the
full command line may be more interesting.  When building from an
interactive shell, it's trivial to retry the build with V=1, but in
case the failed build was automated this is not so easy.

For example,
<https://buildd.debian.org/status/fetch.php?pkg=linux&arch=powerpcspe&ver=4.18%7Erc4-1%7Eexp1&stamp=1531410477&raw=0>
is a build failure triggered by a change in the default compiler
options for powerpc.  This failure is from a native build and I
cannot reproduce it in a cross-build environment.  From the quiet
build log I can't tell whether the same compiler options were used
in the native build.

This series changes the various standard macros used to run and
(depending on verbosity) echo a command.  In quiet mode, if a
command fails it will be echoed before exiting the shell.

Ben.

Ben Hutchings (3):
  kbuild: Move final argument to modpost into $(cmd_modpost)
  kbuild: Add $(run-cmd) macro for running and maybe echoing command
  kbuild: In quiet mode, print the full command line if it fails

 scripts/Kbuild.include   | 13 +++++++++----
 scripts/Makefile.modpost |  4 ++--
 2 files changed, 11 insertions(+), 6 deletions(-)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[RFC PATCH 1/3] kbuild: Move final argument to modpost into $(cmd_modpost)

[Ben Hutchings]

The following changes to the definition of $(cmd) will require that it
is given a complete command line, and no additional arguments can be
appended..

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 scripts/Makefile.modpost | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index df4174405feb..4e56df933b79 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -85,11 +85,11 @@ MODPOST_OPT=$(subst -i,-n,$(filter -i,$(MAKEFLAGS)))
 
 # We can go over command line length here, so be careful.
 quiet_cmd_modpost = MODPOST $(words $(filter-out vmlinux FORCE, $^)) modules
-      cmd_modpost = $(MODLISTCMD) | sed 's/\.ko$$/.o/' | $(modpost) $(MODPOST_OPT) -s -T -
+      cmd_modpost = $(MODLISTCMD) | sed 's/\.ko$$/.o/' | $(modpost) $(MODPOST_OPT) -s -T - $(wildcard vmlinux)
 
 PHONY += __modpost
 __modpost: $(modules:.ko=.o) FORCE
-	$(call cmd,modpost) $(wildcard vmlinux)
+	$(call cmd,modpost)
 
 quiet_cmd_kernel-mod = MODPOST $@
       cmd_kernel-mod = $(modpost) $@

[RFC PATCH 2/3] kbuild: Add $(run-cmd) macro for running and maybe echoing command

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2742 bytes --]

$(run-cmd) replaces a sequence of two macros that is used several
times in Kbuild.include.  This should be a no-op change, but the
following commit will change the definition to be more complex.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 scripts/Kbuild.include | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index c8156d61678c..8778ae4a3476 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -224,8 +224,10 @@ flags = $(foreach o,$($(1)),$(if $(filter -I%,$(o)),$(call addtree,$(o)),$(o)))
 echo-cmd = $(if $($(quiet)cmd_$(1)),\
 	echo '  $(call escsq,$($(quiet)cmd_$(1)))$(echo-why)';)
 
+run-cmd = $(echo-cmd) $(cmd_$(1))
+
 # printing commands
-cmd = @$(echo-cmd) $(cmd_$(1))
+cmd = @$(run-cmd)
 
 # Add $(obj)/ for paths that are not absolute
 objectify = $(foreach o,$(1),$(if $(filter /%,$(o)),$(o),$(obj)/$(o)))
@@ -262,7 +264,7 @@ any-prereq = $(filter-out $(PHONY),$?) $(filter-out $(PHONY) $(wildcard $^),$^)
 # Execute command if command has changed or prerequisite(s) are updated.
 if_changed = $(if $(strip $(any-prereq) $(arg-check)),                       \
 	@set -e;                                                             \
-	$(echo-cmd) $(cmd_$(1));                                             \
+	$(run-cmd);                                                          \
 	printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd, @:)
 
 # Execute the command and also postprocess generated .d dependencies file.
@@ -273,7 +275,7 @@ if_changed_dep = $(if $(strip $(any-prereq) $(arg-check) ),                  \
 ifndef CONFIG_TRIM_UNUSED_KSYMS
 
 cmd_and_fixdep =                                                             \
-	$(echo-cmd) $(cmd_$(1));                                             \
+	$(run-cmd);                                                          \
 	scripts/basic/fixdep $(depfile) $@ '$(make-cmd)' > $(dot-target).tmp;\
 	rm -f $(depfile);                                                    \
 	mv -f $(dot-target).tmp $(dot-target).cmd;
@@ -296,7 +298,7 @@ ksym_dep_filter =                                                            \
 	esac | tr ";" "\n" | sed -n 's/^.*=== __KSYM_\(.*\) ===.*$$/_\1/p'
 
 cmd_and_fixdep =                                                             \
-	$(echo-cmd) $(cmd_$(1));                                             \
+	$(run-cmd);                                                          \
 	$(ksym_dep_filter) |                                                 \
 		scripts/basic/fixdep -e $(depfile) $@ '$(make-cmd)'          \
 			> $(dot-target).tmp;	                             \


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[RFC PATCH 3/3] kbuild: In quiet mode, print the full command line if it fails

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1441 bytes --]

In the $(run-cmd) macro, add a trap on EXIT that prints the
full command line.  Remove the trap after running the command(s)
successfully.

(A more straightforward approach would be to use "if" or "||" to test
for failure, but that doesn't work.  Some command lines given to
$(run-cmd) have multiple commands separated by semi-colons, and the
caller must run "set -e" to enable exit-on-error.  Testing the result
of such a command line, even if it is probably grouped and run in a
sub-shell, inhibits exit-on-error and would cause some errors to be
ignored.)

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 scripts/Kbuild.include | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 8778ae4a3476..c2525aaa36ac 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -224,7 +224,10 @@ flags = $(foreach o,$($(1)),$(if $(filter -I%,$(o)),$(call addtree,$(o)),$(o)))
 echo-cmd = $(if $($(quiet)cmd_$(1)),\
 	echo '  $(call escsq,$($(quiet)cmd_$(1)))$(echo-why)';)
 
-run-cmd = $(echo-cmd) $(cmd_$(1))
+trap-cmd-failed = trap 'test $$? = 0 || echo Failed command: '\''$(call escsq,$(call escsq,$(cmd_$(1))))'\' EXIT
+untrap-cmd-failed = trap - EXIT
+
+run-cmd = $(echo-cmd) $(if $(cmd_$(1)), $(if $(quiet), $(trap-cmd-failed); $(cmd_$(1)); $(untrap-cmd-failed), $(cmd_$(1))))
 
 # printing commands
 cmd = @$(run-cmd)

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [RFC PATCH 3/3] kbuild: In quiet mode, print the full command line if it fails

[Masahiro Yamada]

2018-07-13 7:17 GMT+09:00 Ben Hutchings <ben@decadent.org.uk>:
> In the $(run-cmd) macro, add a trap on EXIT that prints the
> full command line.  Remove the trap after running the command(s)
> successfully.
>
> (A more straightforward approach would be to use "if" or "||" to test
> for failure, but that doesn't work.  Some command lines given to
> $(run-cmd) have multiple commands separated by semi-colons, and the
> caller must run "set -e" to enable exit-on-error.  Testing the result
> of such a command line, even if it is probably grouped and run in a
> sub-shell, inhibits exit-on-error and would cause some errors to be
> ignored.)
>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

OK, this seems useful to me.


> ---
>  scripts/Kbuild.include | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
> index 8778ae4a3476..c2525aaa36ac 100644
> --- a/scripts/Kbuild.include
> +++ b/scripts/Kbuild.include
> @@ -224,7 +224,10 @@ flags = $(foreach o,$($(1)),$(if $(filter -I%,$(o)),$(call addtree,$(o)),$(o)))
>  echo-cmd = $(if $($(quiet)cmd_$(1)),\
>         echo '  $(call escsq,$($(quiet)cmd_$(1)))$(echo-why)';)
>
> -run-cmd = $(echo-cmd) $(cmd_$(1))
> +trap-cmd-failed = trap 'test $$? = 0 || echo Failed command: '\''$(call escsq,$(call escsq,$(cmd_$(1))))'\' EXIT
> +untrap-cmd-failed = trap - EXIT


Why $(call escsq,...) twice?


I think
... echo Failed command: '\''$(call escsq,$(cmd_$(1)))'\' EXIT
is enough.



I see another problem for this.
Make cannot detect the failure of $(call cmd,...)



--------------(sample code)---------------

extra-y := foo.txt

quiet_cmd_always_fail = FAIL    $@
      cmd_always_fail = /bin/false

$(obj)/foo.txt:
        $(call cmd,always_fail)

------------(sample code end)-------------



This should terminate the build.


With your series, this always succeeds
because it returns the exit code of 'trap - EXIT', which is 0.


Maybe,
$(cmd_$(1)) && $(untrap-cmd-failed)
?







> +
> +run-cmd = $(echo-cmd) $(if $(cmd_$(1)), $(if $(quiet), $(trap-cmd-failed); $(cmd_$(1)); $(untrap-cmd-failed), $(cmd_$(1))))
>
>  # printing commands
>  cmd = @$(run-cmd)



-- 
Best Regards
Masahiro Yamada

[RFC PATCH 1/3] kbuild: Move final argument to modpost into $(cmd_modpost)

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1101 bytes --]

The following changes to the definition of $(cmd) will require that it
is given a complete command line, and no additional arguments can be
appended..

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 scripts/Makefile.modpost | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index df4174405feb..4e56df933b79 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -85,11 +85,11 @@ MODPOST_OPT=$(subst -i,-n,$(filter -i,$(MAKEFLAGS)))
 
 # We can go over command line length here, so be careful.
 quiet_cmd_modpost = MODPOST $(words $(filter-out vmlinux FORCE, $^)) modules
-      cmd_modpost = $(MODLISTCMD) | sed 's/\.ko$$/.o/' | $(modpost) $(MODPOST_OPT) -s -T -
+      cmd_modpost = $(MODLISTCMD) | sed 's/\.ko$$/.o/' | $(modpost) $(MODPOST_OPT) -s -T - $(wildcard vmlinux)
 
 PHONY += __modpost
 __modpost: $(modules:.ko=.o) FORCE
-	$(call cmd,modpost) $(wildcard vmlinux)
+	$(call cmd,modpost)
 
 quiet_cmd_kernel-mod = MODPOST $@
       cmd_kernel-mod = $(modpost) $@


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [RFC PATCH 1/3] kbuild: Move final argument to modpost into $(cmd_modpost)

[Masahiro Yamada]

2018-07-13 7:17 GMT+09:00 Ben Hutchings <ben@decadent.org.uk>:
> The following changes to the definition of $(cmd) will require that it
> is given a complete command line, and no additional arguments can be
> appended..

Nit:  is only one period enough?


Otherwise, looks good to me.



> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
>  scripts/Makefile.modpost | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> index df4174405feb..4e56df933b79 100644
> --- a/scripts/Makefile.modpost
> +++ b/scripts/Makefile.modpost
> @@ -85,11 +85,11 @@ MODPOST_OPT=$(subst -i,-n,$(filter -i,$(MAKEFLAGS)))
>
>  # We can go over command line length here, so be careful.
>  quiet_cmd_modpost = MODPOST $(words $(filter-out vmlinux FORCE, $^)) modules
> -      cmd_modpost = $(MODLISTCMD) | sed 's/\.ko$$/.o/' | $(modpost) $(MODPOST_OPT) -s -T -
> +      cmd_modpost = $(MODLISTCMD) | sed 's/\.ko$$/.o/' | $(modpost) $(MODPOST_OPT) -s -T - $(wildcard vmlinux)
>
>  PHONY += __modpost
>  __modpost: $(modules:.ko=.o) FORCE
> -       $(call cmd,modpost) $(wildcard vmlinux)
> +       $(call cmd,modpost)
>
>  quiet_cmd_kernel-mod = MODPOST $@
>        cmd_kernel-mod = $(modpost) $@
>



-- 
Best Regards
Masahiro Yamada