| Message ID | 20277da971c1050d0464410e24ecff9d5d659990.1653048581.git.bertrand.marquis@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] build: Fix make warning if there is no cppcheck | expand |
On 20.05.2022 14:14, Bertrand Marquis wrote: > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -694,12 +694,14 @@ $(objtree)/%.c.cppcheck: $(srctree)/%.c $(objtree)/include/generated/autoconf.h > $(call if_changed,cppcheck_xml) > > cppcheck-version: > -ifeq ($(shell which $(CPPCHECK)),) > - $(error Cannot find cppcheck executable: $(CPPCHECK)) > -endif > -ifeq ($(shell $(CPPCHECK) --version | awk '{print ($$2 < 2.7)}'),1) > - $(error Please upgrade your cppcheck to version 2.7 or greater) > -endif > + @if ! which $(CPPCHECK) > /dev/null 2>&1; then \ > + echo "Cannot find cppcheck executable: $(CPPCHECK)"; \ > + exit 1; \ > + fi > + @if [ "$$($(CPPCHECK) --version | awk '{print ($$2 < 2.7)}')" -eq 1 ]; then \ > + echo "Please upgrade your cppcheck to version 2.7 or greater"; \ > + exit 1; \ > + fi > > # Put this in generated headers this way it is cleaned by include/Makefile > $(objtree)/include/generated/compiler-def.h: Fine with me, even if - as said on v1 - I would have preferred $(if ...). One question though: Wouldn't it better be $(Q) instead of the two plain @? Preferably with that adjustment (which I guess can be made while committing): Acked-by: Jan Beulich <jbeulich@suse.com> Jan
Hi, > On 20 May 2022, at 13:51, Jan Beulich <jbeulich@suse.com> wrote: > > On 20.05.2022 14:14, Bertrand Marquis wrote: >> --- a/xen/Makefile >> +++ b/xen/Makefile >> @@ -694,12 +694,14 @@ $(objtree)/%.c.cppcheck: $(srctree)/%.c $(objtree)/include/generated/autoconf.h >> $(call if_changed,cppcheck_xml) >> >> cppcheck-version: >> -ifeq ($(shell which $(CPPCHECK)),) >> - $(error Cannot find cppcheck executable: $(CPPCHECK)) >> -endif >> -ifeq ($(shell $(CPPCHECK) --version | awk '{print ($$2 < 2.7)}'),1) >> - $(error Please upgrade your cppcheck to version 2.7 or greater) >> -endif >> + @if ! which $(CPPCHECK) > /dev/null 2>&1; then \ >> + echo "Cannot find cppcheck executable: $(CPPCHECK)"; \ >> + exit 1; \ >> + fi >> + @if [ "$$($(CPPCHECK) --version | awk '{print ($$2 < 2.7)}')" -eq 1 ]; then \ >> + echo "Please upgrade your cppcheck to version 2.7 or greater"; \ >> + exit 1; \ >> + fi >> >> # Put this in generated headers this way it is cleaned by include/Makefile >> $(objtree)/include/generated/compiler-def.h: > > Fine with me, even if - as said on v1 - I would have preferred $(if ...). Could you explain why and what you mean exactly ? I thought the code would be more complex and less clear using if and I do not see how it would solve the issue with which being called. > One question though: Wouldn't it better be $(Q) instead of the two plain > @? Preferably with that adjustment (which I guess can be made while > committing): I thought of it but who would be interested in actually seeing those commands which are not “building” anything. > Acked-by: Jan Beulich <jbeulich@suse.com> > Thanks Bertrand
On 20.05.2022 15:23, Bertrand Marquis wrote: >> On 20 May 2022, at 13:51, Jan Beulich <jbeulich@suse.com> wrote: >> On 20.05.2022 14:14, Bertrand Marquis wrote: >>> --- a/xen/Makefile >>> +++ b/xen/Makefile >>> @@ -694,12 +694,14 @@ $(objtree)/%.c.cppcheck: $(srctree)/%.c $(objtree)/include/generated/autoconf.h >>> $(call if_changed,cppcheck_xml) >>> >>> cppcheck-version: >>> -ifeq ($(shell which $(CPPCHECK)),) >>> - $(error Cannot find cppcheck executable: $(CPPCHECK)) >>> -endif >>> -ifeq ($(shell $(CPPCHECK) --version | awk '{print ($$2 < 2.7)}'),1) >>> - $(error Please upgrade your cppcheck to version 2.7 or greater) >>> -endif >>> + @if ! which $(CPPCHECK) > /dev/null 2>&1; then \ >>> + echo "Cannot find cppcheck executable: $(CPPCHECK)"; \ >>> + exit 1; \ >>> + fi >>> + @if [ "$$($(CPPCHECK) --version | awk '{print ($$2 < 2.7)}')" -eq 1 ]; then \ >>> + echo "Please upgrade your cppcheck to version 2.7 or greater"; \ >>> + exit 1; \ >>> + fi >>> >>> # Put this in generated headers this way it is cleaned by include/Makefile >>> $(objtree)/include/generated/compiler-def.h: >> >> Fine with me, even if - as said on v1 - I would have preferred $(if ...). > > Could you explain why and what you mean exactly ? I generally think that make scripts should resort to shell language only if things cannot reasonably be expressed in make language. > I thought the code would be more complex and less clear using if and I > do not see how it would solve the issue with which being called. The problem to deal with was to move the shell invocation from makefile parsing time to rule execution time. Hence I don't see why cppcheck-version: $(if $(shell which ...),,$(error ...)) wouldn't deal with the problem equally well. But I guess I may not be understanding your question / concern. >> One question though: Wouldn't it better be $(Q) instead of the two plain >> @? Preferably with that adjustment (which I guess can be made while >> committing): > > I thought of it but who would be interested in actually seeing those > commands which are not “building” anything. You never know what's relevant to see when hunting down some obscure build system issue. Jan
Hi Jan, > On 20 May 2022, at 14:56, Jan Beulich <jbeulich@suse.com> wrote: > > On 20.05.2022 15:23, Bertrand Marquis wrote: >>> On 20 May 2022, at 13:51, Jan Beulich <jbeulich@suse.com> wrote: >>> On 20.05.2022 14:14, Bertrand Marquis wrote: >>>> --- a/xen/Makefile >>>> +++ b/xen/Makefile >>>> @@ -694,12 +694,14 @@ $(objtree)/%.c.cppcheck: $(srctree)/%.c $(objtree)/include/generated/autoconf.h >>>> $(call if_changed,cppcheck_xml) >>>> >>>> cppcheck-version: >>>> -ifeq ($(shell which $(CPPCHECK)),) >>>> - $(error Cannot find cppcheck executable: $(CPPCHECK)) >>>> -endif >>>> -ifeq ($(shell $(CPPCHECK) --version | awk '{print ($$2 < 2.7)}'),1) >>>> - $(error Please upgrade your cppcheck to version 2.7 or greater) >>>> -endif >>>> + @if ! which $(CPPCHECK) > /dev/null 2>&1; then \ >>>> + echo "Cannot find cppcheck executable: $(CPPCHECK)"; \ >>>> + exit 1; \ >>>> + fi >>>> + @if [ "$$($(CPPCHECK) --version | awk '{print ($$2 < 2.7)}')" -eq 1 ]; then \ >>>> + echo "Please upgrade your cppcheck to version 2.7 or greater"; \ >>>> + exit 1; \ >>>> + fi >>>> >>>> # Put this in generated headers this way it is cleaned by include/Makefile >>>> $(objtree)/include/generated/compiler-def.h: >>> >>> Fine with me, even if - as said on v1 - I would have preferred $(if ...). >> >> Could you explain why and what you mean exactly ? > > I generally think that make scripts should resort to shell language > only if things cannot reasonably be expressed in make language. Agree hence my first implementation. > >> I thought the code would be more complex and less clear using if and I >> do not see how it would solve the issue with which being called. > > The problem to deal with was to move the shell invocation from > makefile parsing time to rule execution time. Hence I don't see > why > > cppcheck-version: > $(if $(shell which ...),,$(error ...)) > > wouldn't deal with the problem equally well. But I guess I may > not be understanding your question / concern. There are always thousands of ways to achieve the same and here this is only a matter of taste. I must admit that I did not think of using that solution this way. If you prefer this I have nothing against it and I will ack a patch changing to this. > >>> One question though: Wouldn't it better be $(Q) instead of the two plain >>> @? Preferably with that adjustment (which I guess can be made while >>> committing): >> >> I thought of it but who would be interested in actually seeing those >> commands which are not “building” anything. > > You never know what's relevant to see when hunting down some > obscure build system issue. > Feel free to replace @ by $(Q) in my patch on commit. Cheers Bertrand > Jan >
diff --git a/xen/Makefile b/xen/Makefile index 15388703bc..e8d8ed71bc 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -694,12 +694,14 @@ $(objtree)/%.c.cppcheck: $(srctree)/%.c $(objtree)/include/generated/autoconf.h $(call if_changed,cppcheck_xml) cppcheck-version: -ifeq ($(shell which $(CPPCHECK)),) - $(error Cannot find cppcheck executable: $(CPPCHECK)) -endif -ifeq ($(shell $(CPPCHECK) --version | awk '{print ($$2 < 2.7)}'),1) - $(error Please upgrade your cppcheck to version 2.7 or greater) -endif + @if ! which $(CPPCHECK) > /dev/null 2>&1; then \ + echo "Cannot find cppcheck executable: $(CPPCHECK)"; \ + exit 1; \ + fi + @if [ "$$($(CPPCHECK) --version | awk '{print ($$2 < 2.7)}')" -eq 1 ]; then \ + echo "Please upgrade your cppcheck to version 2.7 or greater"; \ + exit 1; \ + fi # Put this in generated headers this way it is cleaned by include/Makefile $(objtree)/include/generated/compiler-def.h:
If cppcheck is not present, the following warning appears during build: which: no cppcheck in ([...]) /bin/sh: cppcheck: command not found Fix the problem by using shell code inside the cppcheck-version rule to also prevent unneeded call of which when something else than cppcheck is built. Reported-by: Julien Grall <julien@xen.org> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> --- xen/Makefile | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)