diff mbox

[2/7] kbuild: Add P= command line flag to run checkpatch

Message ID CA+r1Zhj295pmVTmh_TVMuy37qRQ7Q6Mxipk7vhyAVkuXFBU8uw@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jim Davis Nov. 21, 2017, midnight UTC
On Mon, Nov 20, 2017 at 2:22 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:

> Should it be possible to somehow keep the distinction between
> the flags coming from KBUILD_CFLAGS and the pure CHECKFLAGS?

Well, the practical problem seems to be that $(CHECK) is called in
scripts/Makefile.build with both $(CHECKFLAGS) and $(c_flags) as
arguments, regardless of what $(CHECK) is.  That can be hacked around
with something inelegant like


and then if I run scripts/checkpatch.pl as $CHECK and pass --quiet
--file as before it works -- until checkpatch returns with a non-zero
exit code, which causes the Makefile to bail at that point.

So maybe a wrapper script, with an exit 0 fixup to keep on keeping on
in case of checkpatch warnings or errors, would be better after all.

Comments

Knut Omang Nov. 21, 2017, 8:10 a.m. UTC | #1
On Mon, 2017-11-20 at 17:00 -0700, Jim Davis wrote:
> On Mon, Nov 20, 2017 at 2:22 PM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> 
> > Should it be possible to somehow keep the distinction between
> > the flags coming from KBUILD_CFLAGS and the pure CHECKFLAGS?
> 
> Well, the practical problem seems to be that $(CHECK) is called in
> scripts/Makefile.build with both $(CHECKFLAGS) and $(c_flags) as
> arguments, regardless of what $(CHECK) is.  That can be hacked around
> with something inelegant like
> 
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index bb831d49bcfd..102194f9afb9 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -98,14 +98,20 @@ __build: $(if $(KBUILD_BUILTIN),$(builtin-target)
> $(lib-target) $(extra-y)) \
>          $(subdir-ym) $(always)
>         @:
> 
> -# Linus' kernel sanity checking tool
> +# Linus' kernel sanity checking tool, or $CHECK program of choice
> +ifneq ($(KBUILD_CHECKSRC),)
> +  add_to_checkflags =
> +  ifeq ($(CHECK),sparse)
> +    add_to_checkflags = $(c_flags)
> +  endif
> +endif
>  ifneq ($(KBUILD_CHECKSRC),0)
>    ifeq ($(KBUILD_CHECKSRC),2)
>      quiet_cmd_force_checksrc = CHECK   $<
> -          cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
> +          cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(add_to_checkflags) $<
> ;
>    else
>        quiet_cmd_checksrc     = CHECK   $<
> -            cmd_checksrc     = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
> +            cmd_checksrc     = $(CHECK) $(CHECKFLAGS) $(add_to_checkflags) $<
> ;
>    endif
>  endif
> 
> and then if I run scripts/checkpatch.pl as $CHECK and pass --quiet
> --file as before it works -- until checkpatch returns with a non-zero
> exit code, which causes the Makefile to bail at that point.

yes, in an earlier version, I added a --no-errors flag to checkpatch to handle
that, but based on feedback this version promotes make -k instead. It seems
however that that only holds to the next linker step. It is useful to be able to
select whether checkpatch should cause make to stop or not. While fixing issues,
failure is a better strategy while to assess the state or generate a report, a
return 0 behavior is better.

> So maybe a wrapper script, with an exit 0 fixup to keep on keeping on
> in case of checkpatch warnings or errors, would be better after all.

I can prepare a v2 based on the wrapper script I have already.

In that version, all added functionality was implemented in the wrapper
(including the configuration file parsing) 

Would you like to keep the checkpatch changes in some form, or would you rather
see everything happening in the wrapper?

A 3rd possibility that strikes me is that maybe what's needed is a general
checker runner that can also take care of running other checks, running multiple
checks, and maybe also handling temporary or decided suppression of sparse
errors in a similar fashion as I implemented for checkpatch errors. But maybe
that can be left to a later change set..

Thanks,
Knut
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jim Davis Nov. 21, 2017, 7:48 p.m. UTC | #2
On Tue, Nov 21, 2017 at 1:10 AM, Knut Omang <knut.omang@oracle.com> wrote:

> Would you like to keep the checkpatch changes in some form, or would you rather
> see everything happening in the wrapper?

I don't have a strong preference one way or another, but keeping
everything in a wrapper script might be easier if only because you
wouldn't need to get signoffs from whoever maintains checkpatch too.
Joe Perches Nov. 21, 2017, 8:03 p.m. UTC | #3
On Tue, 2017-11-21 at 12:48 -0700, Jim Davis wrote:
> On Tue, Nov 21, 2017 at 1:10 AM, Knut Omang <knut.omang@oracle.com> wrote:
> 
> > Would you like to keep the checkpatch changes in some form, or would you rather
> > see everything happening in the wrapper?
> 
> I don't have a strong preference one way or another, but keeping
> everything in a wrapper script might be easier if only because you
> wouldn't need to get signoffs from whoever maintains checkpatch too.

Keeping everything in a wrapper script would also allow
any arbitrary checker to be run with minimal changes to
the Makefile/build subsystem.

--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index bb831d49bcfd..102194f9afb9 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -98,14 +98,20 @@  __build: $(if $(KBUILD_BUILTIN),$(builtin-target)
$(lib-target) $(extra-y)) \
         $(subdir-ym) $(always)
        @:

-# Linus' kernel sanity checking tool
+# Linus' kernel sanity checking tool, or $CHECK program of choice
+ifneq ($(KBUILD_CHECKSRC),)
+  add_to_checkflags =
+  ifeq ($(CHECK),sparse)
+    add_to_checkflags = $(c_flags)
+  endif
+endif
 ifneq ($(KBUILD_CHECKSRC),0)
   ifeq ($(KBUILD_CHECKSRC),2)
     quiet_cmd_force_checksrc = CHECK   $<
-          cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
+          cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(add_to_checkflags) $< ;
   else
       quiet_cmd_checksrc     = CHECK   $<
-            cmd_checksrc     = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
+            cmd_checksrc     = $(CHECK) $(CHECKFLAGS) $(add_to_checkflags) $< ;
   endif
 endif