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

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 5 May 2026 12:49:11 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> 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.

Yes, absolutely, and we can't run static checkers on pesto without it,
so we need something like this series.

If we (especially: you ;)) can fix whatever comment you have plus my
concern on 4/13 quickly enough, it makes sense to continue with it. If
it becomes a big effort maybe we should go for a less involved approach
though. I'm not sure.

-- 
Stefano