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

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

Posted by Bertrand Marquis 1 year, 11 months ago

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(-)

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:
-- 
2.25.1

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

Posted by Jan Beulich 1 year, 11 months ago

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

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

Posted by Bertrand Marquis 1 year, 11 months ago

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

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

Posted by Jan Beulich 1 year, 11 months ago

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

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

Posted by Bertrand Marquis 1 year, 11 months ago

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
>