Re: [PATCH v2] build: Fix make warning if there is no cppcheck

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jan,

> On 20 May 2022, at 14:56, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 20.05.2022 15:23, Bertrand Marquis wrote:
>>> On 20 May 2022, at 13:51, Jan Beulich <jbeulich@xxxxxxxx> 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
>