| Message ID | 1528121273-22041-1-git-send-email-luca@lucaceresoli.net |
|---|---|
| State | Rejected |
| Delegated to: | Masahiro Yamada |
| Headers | show |
| Series | [U-Boot] if_changed: fix error handling | expand |
Hi, On Tue, Jun 5, 2018 at 2:08 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: > > The commands in if_changed and if_changed_dep are concatenated with a > ';', thus any error in any command except the last one will be > silently ignored. This is particularly relevant for the actual > payload, cmd_$(1), whose errors should not be unnoticed. > > Fix by replacing the ';' with '&&'. > > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > > --- > > Note: I'm not aware of any situation in which this bug has any visible > effect. I noticed the problem while working on a different topic, but > later I did that job in a different way, not involving if_changed > usages. But this is a bug anyway, so let's fix it. > --- > scripts/Kbuild.include | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include > index 2c7918ad3721..f722f75611df 100644 > --- a/scripts/Kbuild.include > +++ b/scripts/Kbuild.include > @@ -255,16 +255,16 @@ 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)); \ > + @set -e; \ > + $(echo-cmd) $(cmd_$(1)) && \ > printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd) > > # Execute the command and also postprocess generated .d dependencies file. > if_changed_dep = $(if $(strip $(any-prereq) $(arg-check) ), \ > - @set -e; \ > - $(echo-cmd) $(cmd_$(1)); \ > - scripts/basic/fixdep $(depfile) $@ '$(make-cmd)' > $(dot-target).tmp;\ > - rm -f $(depfile); \ > + @set -e; \ > + $(echo-cmd) $(cmd_$(1)) && \ > + scripts/basic/fixdep $(depfile) $@ '$(make-cmd)' > $(dot-target).tmp && \ > + rm -f $(depfile) && \ > mv -f $(dot-target).tmp $(dot-target).cmd) > > # Usage: $(call if_changed_rule,foo) The 'set -e' should cause the sub-shell to exit immediately if any of the commands fail, regardless of the ';' vs '&&'. Assuming I'm wrong above, this patch should probably go to the kbuild mailing list http://vger.kernel.org/vger-lists.html#linux-kbuild
Hi Chris, On 06/06/2018 05:23, Chris Packham wrote: > Hi, > > On Tue, Jun 5, 2018 at 2:08 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: >> >> The commands in if_changed and if_changed_dep are concatenated with a >> ';', thus any error in any command except the last one will be >> silently ignored. This is particularly relevant for the actual >> payload, cmd_$(1), whose errors should not be unnoticed. >> >> Fix by replacing the ';' with '&&'. >> >> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> >> >> --- >> >> Note: I'm not aware of any situation in which this bug has any visible >> effect. I noticed the problem while working on a different topic, but >> later I did that job in a different way, not involving if_changed >> usages. But this is a bug anyway, so let's fix it. >> --- >> scripts/Kbuild.include | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include >> index 2c7918ad3721..f722f75611df 100644 >> --- a/scripts/Kbuild.include >> +++ b/scripts/Kbuild.include >> @@ -255,16 +255,16 @@ any-pThis is tricky, arereq = $(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)); \ >> + @set -e; \ >> + $(echo-cmd) $(cmd_$(1)) && \ >> printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd) >> >> # Execute the command and also postprocess generated .d dependencies file. >> if_changed_dep = $(if $(strip $(any-prereq) $(arg-check) ), \ >> - @set -e; \ >> - $(echo-cmd) $(cmd_$(1)); \ >> - scripts/basic/fixdep $(depfile) $@ '$(make-cmd)' > $(dot-target).tmp;\ >> - rm -f $(depfile); \ >> + @set -e; \ >> + $(echo-cmd) $(cmd_$(1)) && \ >> + scripts/basic/fixdep $(depfile) $@ '$(make-cmd)' > $(dot-target).tmp && \ >> + rm -f $(depfile) && \ >> mv -f $(dot-target).tmp $(dot-target).cmd) >> >> # Usage: $(call if_changed_rule,foo) > > The 'set -e' should cause the sub-shell to exit immediately if any of > the commands fail, regardless of the ';' vs '&&'. Thanks for your comment, it triggered me to go deeper in understanding the issue. Apologies for not having clarified it all in the first place. Actually ';' and '&&' are not treated the same way. The bash manpage states about 'set -e': > The shell does not exit if the command that fails is [...] part of > any command executed in a && or || list except the command following > the final && or || The issue I had initially happened when I wrote something like this: cmd_mkimage = test -r /no/such/file && \ $(objtree)/tools/mkimage $(MKIMAGEFLAGS_$(@F)) -d $< $@ \ >$(MKIMAGEOUTPUT) $(if $(KBUILD_VERBOSE:0=), && cat $(MKIMAGEOUTPUT)) If the 'test' command fails, set -e won't stop the build. But replacing '&&' with ';' makes the build stop immediately as expected. So I guess cmd_mkimage is just meant to be used with ';'. > Assuming I'm wrong above, this patch should probably go to the kbuild > mailing list http://vger.kernel.org/vger-lists.html#linux-kbuild
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include index 2c7918ad3721..f722f75611df 100644 --- a/scripts/Kbuild.include +++ b/scripts/Kbuild.include @@ -255,16 +255,16 @@ 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)); \ + @set -e; \ + $(echo-cmd) $(cmd_$(1)) && \ printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd) # Execute the command and also postprocess generated .d dependencies file. if_changed_dep = $(if $(strip $(any-prereq) $(arg-check) ), \ - @set -e; \ - $(echo-cmd) $(cmd_$(1)); \ - scripts/basic/fixdep $(depfile) $@ '$(make-cmd)' > $(dot-target).tmp;\ - rm -f $(depfile); \ + @set -e; \ + $(echo-cmd) $(cmd_$(1)) && \ + scripts/basic/fixdep $(depfile) $@ '$(make-cmd)' > $(dot-target).tmp && \ + rm -f $(depfile) && \ mv -f $(dot-target).tmp $(dot-target).cmd) # Usage: $(call if_changed_rule,foo)
The commands in if_changed and if_changed_dep are concatenated with a ';', thus any error in any command except the last one will be silently ignored. This is particularly relevant for the actual payload, cmd_$(1), whose errors should not be unnoticed. Fix by replacing the ';' with '&&'. Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> --- Note: I'm not aware of any situation in which this bug has any visible effect. I noticed the problem while working on a different topic, but later I did that job in a different way, not involving if_changed usages. But this is a bug anyway, so let's fix it. --- scripts/Kbuild.include | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)