Re: [PATCH v2 01/13] Makefile: Use make variables for static checker configuration

[Laurent Vivier <lvivier@redhat.com>]

On 5/5/26 12:20, Stefano Brivio wrote:
> On Tue, 5 May 2026 12:14:02 +0200
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
>> On 4/21/26 05:23, David Gibson wrote:
>>> Our cppcheck and clang-tidy rules don't really follow normal Makefile
>>> conventions.  Usually any commands other than the very basics have their
>>> binary specified in a variable so it can be overridden on the command line
>>> if they're in an unusual location.  Implement that for $(CPPCHECK) and
>>> $(CLANG_TIDY)
>>>
>>> Likewise flags to tools usually have their own Make variable.  Do the same
>>> with $(CLANG_TIDY_FLAGS) and $(CPPCHECK_FLAGS).  Note that these only have
>>> the options specifically for the static checker, not compiler flags which
>>> we are also supplying to the static checker - those are derived from
>>> FLAGS / CFLAGS / CPPFLAGS as before.
>>>
>>> As part of that we change the probing for --check-level=exhaustive from
>>> being run as part of the cppcheck target, to being run when we build the
>>> CPPCHECK_FLAGS variable.  That doesn't make any real difference now, but
>>> will make things nicer if we need multiple cppcheck targets in future (e.g.
>>> for passt-repair).
>>>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>>    Makefile | 32 +++++++++++++++++++-------------
>>>    1 file changed, 19 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/Makefile b/Makefile
>>> index f697c12b..17e70d22 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -174,21 +174,27 @@ docs: README.md
>>>    		done < README.md;					\
>>>    	) > README.plain.md
>>>    
>>> +CLANG_TIDY = clang-tidy
>>> +CLANG_TIDY_FLAGS = -DCLANG_TIDY_58992
>>> +
>>>    clang-tidy: $(PASST_SRCS)
>>> -	clang-tidy $^ -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) \
>>> -	           -DCLANG_TIDY_58992
>>> +	$(CLANG_TIDY) $^ -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) \
>>> +		$(CLANG_TIDY_FLAGS)
>>>    
>>> -cppcheck: $(PASST_SRCS) $(HEADERS)
>>> -	if cppcheck --check-level=exhaustive /dev/null > /dev/null 2>&1; then \
>>> -		CPPCHECK_EXHAUSTIVE="--check-level=exhaustive";		\
>>> -	else								\
>>> -		CPPCHECK_EXHAUSTIVE=;					\
>>> -	fi;								\
>>> -	cppcheck --std=c11 --error-exitcode=1 --enable=all --force	\
>>> +CPPCHECK = cppcheck
>>> +CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force	\
>>>    	--inconclusive --library=posix --quiet				\
>>> -	$${CPPCHECK_EXHAUSTIVE}						\
>>>    	--inline-suppr							\
>>> -	--suppress=missingIncludeSystem \
>>> +	$(shell if $(CPPCHECK) --quiet --check-level=exhaustive /dev/null; then \
>>> +		echo "--check-level=exhaustive";			\
>>> +	else								\
>>> +		echo "";						\
>>> +	fi)			
>>
>> Perhaps we can add a reusable function to check flags:
>>
>> check_flag = $(shell $(1) $(2) >/dev/null 2>&1 && echo $(2))
>>
>> Then
>> 	$(call check_flag, $(CPPCHECK) /dev/null, --check-level=exhaustive)
>> 				\
>>> +	--suppress=missingIncludeSystem					\
>>>    	--suppress=unusedStructMember					\
>>> -	$(filter -D%,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -D CPPCHECK_6936	\
>>> -	$^
>>> +	 -D CPPCHECK_6936
>>> +
>>> +cppcheck: $(PASST_SRCS) $(HEADERS)
>>> +	$(CPPCHECK) $(CPPCHECK_FLAGS) 					\
>>> +		$(filter -D%,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) $^		\
>>> +		$^
>>
>> You have duplicate '$^' here.
> 
> Just note that it's not necessarily important to review this series
> right now (I'm not sure).

okay, I was reviewing it because "Dynamic configuration" series doesn't apply cleanly 
without it.

> 
> I guess if we can "fix" it (see my concerns about 4/13) quickly, then
> it's useful, and I'll try to pick it up like that (feel free to post a
> new version yourself, of course!).
> 
> But if it needs major reworks I'd rather try to come up with something
> minimal instead, for the moment.
> 

Thanks,
Laurent