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

From: Bertrand Marquis <Bertrand.Marquis@arm.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2] build: Fix make warning if there is no cppcheck
Date: Fri, 20 May 2022 14:17:07 +0000	[thread overview]
Message-ID: <A15CA4FE-0D8F-4291-A245-58915FF83438@arm.com> (raw)
In-Reply-To: <4f9343a2-7ac1-11ea-276e-32b7207db072@suse.com>

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
> 