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

[Joe Perches]

On Tue, 2017-11-21 at 12:48 -0700, Jim Davis wrote:
> On Tue, Nov 21, 2017 at 1:10 AM, Knut Omang  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.

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

[Joe Perches]

On Tue, 2017-11-21 at 12:48 -0700, Jim Davis wrote:
> On Tue, Nov 21, 2017 at 1:10 AM, Knut Omang  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.

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

[Jim Davis]

On Tue, Nov 21, 2017 at 1:10 AM, Knut Omang  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.

-- 
Jim

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

[Jim Davis]

On Tue, Nov 21, 2017 at 1:10 AM, Knut Omang  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.

-- 
Jim

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

[Knut Omang]

On Mon, 2017-11-20 at 17:00 -0700, Jim Davis wrote:
> On Mon, Nov 20, 2017 at 2:22 PM, Luc Van Oostenryck
>  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

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

[Knut Omang]

On Mon, 2017-11-20 at 17:00 -0700, Jim Davis wrote:
> On Mon, Nov 20, 2017 at 2:22 PM, Luc Van Oostenryck
>  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

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

[Jim Davis]

On Mon, Nov 20, 2017 at 2:22 PM, Luc Van Oostenryck
 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.

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.


-- 
Jim

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

[Jim Davis]

On Mon, Nov 20, 2017 at 2:22 PM, Luc Van Oostenryck
 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.

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.


-- 
Jim

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

[Luc Van Oostenryck]

On Mon, Nov 20, 2017 at 10:10:12PM +0100, Knut Omang wrote:
> On Mon, 2017-11-20 at 21:08 +0100, Luc Van Oostenryck wrote:
> > 
> > It should be noted though that CHECKFLAGS contains very very few
> > sparse specific things. It's mainly flags for the compiler
> > coming from  KBUILD_CFLAGS (which of course, sparse needs to
> > do its job properly).
> 
> Yes, and we would want some arguments passed to checkpatch by default as well.
> 
> A wrapper script (which by the way was what I started this with..)
> could of course solve this and other issues such as the ability 
> to run multiple checkers, but I am not convinced that that would be 
> less ugly?

A wrapper script is something else that need to be maintained
but of course, it's very flexible.
I don't have a strong opinion on this and prefer to let speak
the people who maintain kbuild.

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


-- Luc Van Oostenryck

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

[Luc Van Oostenryck]

On Mon, Nov 20, 2017 at 10:10:12PM +0100, Knut Omang wrote:
> On Mon, 2017-11-20 at 21:08 +0100, Luc Van Oostenryck wrote:
> > 
> > It should be noted though that CHECKFLAGS contains very very few
> > sparse specific things. It's mainly flags for the compiler
> > coming from  KBUILD_CFLAGS (which of course, sparse needs to
> > do its job properly).
> 
> Yes, and we would want some arguments passed to checkpatch by default as well.
> 
> A wrapper script (which by the way was what I started this with..)
> could of course solve this and other issues such as the ability 
> to run multiple checkers, but I am not convinced that that would be 
> less ugly?

A wrapper script is something else that need to be maintained
but of course, it's very flexible.
I don't have a strong opinion on this and prefer to let speak
the people who maintain kbuild.

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


-- Luc Van Oostenryck

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

[Knut Omang]

On Mon, 2017-11-20 at 21:08 +0100, Luc Van Oostenryck wrote:
> On Mon, Nov 20, 2017 at 12:48:35PM -0700, Jim Davis wrote:
> > 
> > I'd be nice if people could just specify CHECK and CHECKFLAGS to run
> > their favorite checker, but currently CHECKFLAGS seems hardwired for
> > running sparse.  So something liike
> > 
> > make C=1 CHECK="scripts/checkpatch.pl" CHECKFLAGS="--quiet --file"
> > 
> > fails when checkpatch is passed lots of arguments like -D__linux__
> > -Dlinux -D__STDC__ .  A little shell wrapper to grab the last argument
> > in that long list is a workaround, but perhaps CHECKFLAGS should be
> > made less sparse-specific?
> 
> It should be noted though that CHECKFLAGS contains very very few
> sparse specific things. It's mainly flags for the compiler
> coming from  KBUILD_CFLAGS (which of course, sparse needs to
> do its job properly).

Yes, and we would want some arguments passed to checkpatch by default as well.

A wrapper script (which by the way was what I started this with..)
could of course solve this and other issues such as the ability 
to run multiple checkers, but I am not convinced that that would be 
less ugly?

Thanks,
Knut

> 
> -- Luc Van Oostenryck

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

[Knut Omang]

On Mon, 2017-11-20 at 21:08 +0100, Luc Van Oostenryck wrote:
> On Mon, Nov 20, 2017 at 12:48:35PM -0700, Jim Davis wrote:
> > 
> > I'd be nice if people could just specify CHECK and CHECKFLAGS to run
> > their favorite checker, but currently CHECKFLAGS seems hardwired for
> > running sparse.  So something liike
> > 
> > make C=1 CHECK="scripts/checkpatch.pl" CHECKFLAGS="--quiet --file"
> > 
> > fails when checkpatch is passed lots of arguments like -D__linux__
> > -Dlinux -D__STDC__ .  A little shell wrapper to grab the last argument
> > in that long list is a workaround, but perhaps CHECKFLAGS should be
> > made less sparse-specific?
> 
> It should be noted though that CHECKFLAGS contains very very few
> sparse specific things. It's mainly flags for the compiler
> coming from  KBUILD_CFLAGS (which of course, sparse needs to
> do its job properly).

Yes, and we would want some arguments passed to checkpatch by default as well.

A wrapper script (which by the way was what I started this with..)
could of course solve this and other issues such as the ability 
to run multiple checkers, but I am not convinced that that would be 
less ugly?

Thanks,
Knut

> 
> -- Luc Van Oostenryck

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

[Knut Omang]

On Tue, 2017-11-21 at 01:18 +0900, Masahiro Yamada wrote:
> 2017-11-17 2:01 GMT+09:00 Knut Omang :
> > Add interpretation of a new environment variable P={1,2} in spirit of the
> > C= option, but executing checkpatch instead of sparse.
> > 
> > Signed-off-by: Knut Omang 
> > Reviewed-by: Håkon Bugge 
> > Acked-by: Åsmund Østvold 
> > ---
> >  Makefile   | 20 +++-
> >  scripts/Makefile.build | 13 +
> >  2 files changed, 32 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index ccd9818..eb4bca9 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -176,6 +176,20 @@ ifndef KBUILD_CHECKSRC
> >KBUILD_CHECKSRC = 0
> >  endif
> > 
> > +# Run scripts/checkpatch.pl with --ignore-cfg checkpatch.cfg
> > +#
> > +# Use 'make P=1' to enable checking of only re-compiled files.
> > +# Use 'make P=2' to enable checking of *all* source files, regardless
> > +#
> > +# See the file "Documentation/dev-tools/run-checkpatch.rst" for more
> > details,
> > +#
> > +ifeq ("$(origin P)", "command line")
> > +  KBUILD_CHECKPATCH = $(P)
> > +endif
> > +ifndef KBUILD_CHECKPATCH
> > +  KBUILD_CHECKPATCH = 0
> > +endif
> 
> 
> I am unhappy about adding a new interface
> for each checker.

I see your point. I wanted to extend C= to handle checkpatch as well but see no
obvious good non-intrusive solution.

> The default of CHECK is "sparse", but
> users can override it to use another checker.
> 
> 
> 
> As Decumentation/dev-tools/coccinelle.rst says,
> if you want to use coccinelle as a checker,
> 
> make C=1 CHECK="scripts/coccicheck"

That works well with coccinelle since both sparse and coccinelle rely on getting
the same command line options as what's passed to the compiler, while checkpatch
is quite different:

  make C=2 CHECK="\$(srctree)/scripts/checkpatch.pl" 

fails, complaining about every single compiler flag.

Thanks,
Knut

> Recently, I saw a patch to use scripts/kernel-doc as a checker.
> https://patchwork.kernel.org/patch/10030521/
> 
> 
> 
> If I accept your patch,
> we would end up with
> 
> KBUILD_CHECKPATCH,
> KBUILD_CHECKCOCCI
> KBUILD_CHECKDOC,
> ...
> 
> This is ugly.

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

[Knut Omang]

On Tue, 2017-11-21 at 01:18 +0900, Masahiro Yamada wrote:
> 2017-11-17 2:01 GMT+09:00 Knut Omang :
> > Add interpretation of a new environment variable P={1,2} in spirit of the
> > C= option, but executing checkpatch instead of sparse.
> > 
> > Signed-off-by: Knut Omang 
> > Reviewed-by: Håkon Bugge 
> > Acked-by: Åsmund Østvold 
> > ---
> >  Makefile   | 20 +++-
> >  scripts/Makefile.build | 13 +
> >  2 files changed, 32 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index ccd9818..eb4bca9 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -176,6 +176,20 @@ ifndef KBUILD_CHECKSRC
> >KBUILD_CHECKSRC = 0
> >  endif
> > 
> > +# Run scripts/checkpatch.pl with --ignore-cfg checkpatch.cfg
> > +#
> > +# Use 'make P=1' to enable checking of only re-compiled files.
> > +# Use 'make P=2' to enable checking of *all* source files, regardless
> > +#
> > +# See the file "Documentation/dev-tools/run-checkpatch.rst" for more
> > details,
> > +#
> > +ifeq ("$(origin P)", "command line")
> > +  KBUILD_CHECKPATCH = $(P)
> > +endif
> > +ifndef KBUILD_CHECKPATCH
> > +  KBUILD_CHECKPATCH = 0
> > +endif
> 
> 
> I am unhappy about adding a new interface
> for each checker.

I see your point. I wanted to extend C= to handle checkpatch as well but see no
obvious good non-intrusive solution.

> The default of CHECK is "sparse", but
> users can override it to use another checker.
> 
> 
> 
> As Decumentation/dev-tools/coccinelle.rst says,
> if you want to use coccinelle as a checker,
> 
> make C=1 CHECK="scripts/coccicheck"

That works well with coccinelle since both sparse and coccinelle rely on getting
the same command line options as what's passed to the compiler, while checkpatch
is quite different:

  make C=2 CHECK="\$(srctree)/scripts/checkpatch.pl" 

fails, complaining about every single compiler flag.

Thanks,
Knut

> Recently, I saw a patch to use scripts/kernel-doc as a checker.
> https://patchwork.kernel.org/patch/10030521/
> 
> 
> 
> If I accept your patch,
> we would end up with
> 
> KBUILD_CHECKPATCH,
> KBUILD_CHECKCOCCI
> KBUILD_CHECKDOC,
> ...
> 
> This is ugly.

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

[Luc Van Oostenryck]

On Mon, Nov 20, 2017 at 12:48:35PM -0700, Jim Davis wrote:
> 
> I'd be nice if people could just specify CHECK and CHECKFLAGS to run
> their favorite checker, but currently CHECKFLAGS seems hardwired for
> running sparse.  So something liike
> 
> make C=1 CHECK="scripts/checkpatch.pl" CHECKFLAGS="--quiet --file"
> 
> fails when checkpatch is passed lots of arguments like -D__linux__
> -Dlinux -D__STDC__ .  A little shell wrapper to grab the last argument
> in that long list is a workaround, but perhaps CHECKFLAGS should be
> made less sparse-specific?

It should be noted though that CHECKFLAGS contains very very few
sparse specific things. It's mainly flags for the compiler
coming from  KBUILD_CFLAGS (which of course, sparse needs to
do its job properly).

-- Luc Van Oostenryck

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

[Luc Van Oostenryck]

On Mon, Nov 20, 2017 at 12:48:35PM -0700, Jim Davis wrote:
> 
> I'd be nice if people could just specify CHECK and CHECKFLAGS to run
> their favorite checker, but currently CHECKFLAGS seems hardwired for
> running sparse.  So something liike
> 
> make C=1 CHECK="scripts/checkpatch.pl" CHECKFLAGS="--quiet --file"
> 
> fails when checkpatch is passed lots of arguments like -D__linux__
> -Dlinux -D__STDC__ .  A little shell wrapper to grab the last argument
> in that long list is a workaround, but perhaps CHECKFLAGS should be
> made less sparse-specific?

It should be noted though that CHECKFLAGS contains very very few
sparse specific things. It's mainly flags for the compiler
coming from  KBUILD_CFLAGS (which of course, sparse needs to
do its job properly).

-- Luc Van Oostenryck

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

[Jim Davis]

On Mon, Nov 20, 2017 at 9:18 AM, Masahiro Yamada
 wrote:

>
> I am unhappy about adding a new interface
> for each checker.
>
> The default of CHECK is "sparse", but
> users can override it to use another checker.
>
>
>
> As Decumentation/dev-tools/coccinelle.rst says,
> if you want to use coccinelle as a checker,
>
> make C=1 CHECK="scripts/coccicheck"
>

I'd be nice if people could just specify CHECK and CHECKFLAGS to run
their favorite checker, but currently CHECKFLAGS seems hardwired for
running sparse.  So something liike

make C=1 CHECK="scripts/checkpatch.pl" CHECKFLAGS="--quiet --file"

fails when checkpatch is passed lots of arguments like -D__linux__
-Dlinux -D__STDC__ .  A little shell wrapper to grab the last argument
in that long list is a workaround, but perhaps CHECKFLAGS should be
made less sparse-specific?

-- 
Jim

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

[Jim Davis]

On Mon, Nov 20, 2017 at 9:18 AM, Masahiro Yamada
 wrote:

>
> I am unhappy about adding a new interface
> for each checker.
>
> The default of CHECK is "sparse", but
> users can override it to use another checker.
>
>
>
> As Decumentation/dev-tools/coccinelle.rst says,
> if you want to use coccinelle as a checker,
>
> make C=1 CHECK="scripts/coccicheck"
>

I'd be nice if people could just specify CHECK and CHECKFLAGS to run
their favorite checker, but currently CHECKFLAGS seems hardwired for
running sparse.  So something liike

make C=1 CHECK="scripts/checkpatch.pl" CHECKFLAGS="--quiet --file"

fails when checkpatch is passed lots of arguments like -D__linux__
-Dlinux -D__STDC__ .  A little shell wrapper to grab the last argument
in that long list is a workaround, but perhaps CHECKFLAGS should be
made less sparse-specific?

-- 
Jim

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

[Masahiro Yamada]

2017-11-17 2:01 GMT+09:00 Knut Omang :
> Add interpretation of a new environment variable P={1,2} in spirit of the
> C= option, but executing checkpatch instead of sparse.
>
> Signed-off-by: Knut Omang 
> Reviewed-by: Håkon Bugge 
> Acked-by: Åsmund Østvold 
> ---
>  Makefile   | 20 +++-
>  scripts/Makefile.build | 13 +
>  2 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index ccd9818..eb4bca9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -176,6 +176,20 @@ ifndef KBUILD_CHECKSRC
>KBUILD_CHECKSRC = 0
>  endif
>
> +# Run scripts/checkpatch.pl with --ignore-cfg checkpatch.cfg
> +#
> +# Use 'make P=1' to enable checking of only re-compiled files.
> +# Use 'make P=2' to enable checking of *all* source files, regardless
> +#
> +# See the file "Documentation/dev-tools/run-checkpatch.rst" for more details,
> +#
> +ifeq ("$(origin P)", "command line")
> +  KBUILD_CHECKPATCH = $(P)
> +endif
> +ifndef KBUILD_CHECKPATCH
> +  KBUILD_CHECKPATCH = 0
> +endif


I am unhappy about adding a new interface
for each checker.

The default of CHECK is "sparse", but
users can override it to use another checker.



As Decumentation/dev-tools/coccinelle.rst says,
if you want to use coccinelle as a checker,

make C=1 CHECK="scripts/coccicheck"


Recently, I saw a patch to use scripts/kernel-doc as a checker.
https://patchwork.kernel.org/patch/10030521/



If I accept your patch,
we would end up with

KBUILD_CHECKPATCH,
KBUILD_CHECKCOCCI
KBUILD_CHECKDOC,
...


This is ugly.



-- 
Best Regards
Masahiro Yamada

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

[Masahiro Yamada]

2017-11-17 2:01 GMT+09:00 Knut Omang :
> Add interpretation of a new environment variable P={1,2} in spirit of the
> C= option, but executing checkpatch instead of sparse.
>
> Signed-off-by: Knut Omang 
> Reviewed-by: Håkon Bugge 
> Acked-by: Åsmund Østvold 
> ---
>  Makefile   | 20 +++-
>  scripts/Makefile.build | 13 +
>  2 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index ccd9818..eb4bca9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -176,6 +176,20 @@ ifndef KBUILD_CHECKSRC
>KBUILD_CHECKSRC = 0
>  endif
>
> +# Run scripts/checkpatch.pl with --ignore-cfg checkpatch.cfg
> +#
> +# Use 'make P=1' to enable checking of only re-compiled files.
> +# Use 'make P=2' to enable checking of *all* source files, regardless
> +#
> +# See the file "Documentation/dev-tools/run-checkpatch.rst" for more details,
> +#
> +ifeq ("$(origin P)", "command line")
> +  KBUILD_CHECKPATCH = $(P)
> +endif
> +ifndef KBUILD_CHECKPATCH
> +  KBUILD_CHECKPATCH = 0
> +endif


I am unhappy about adding a new interface
for each checker.

The default of CHECK is "sparse", but
users can override it to use another checker.



As Decumentation/dev-tools/coccinelle.rst says,
if you want to use coccinelle as a checker,

make C=1 CHECK="scripts/coccicheck"


Recently, I saw a patch to use scripts/kernel-doc as a checker.
https://patchwork.kernel.org/patch/10030521/



If I accept your patch,
we would end up with

KBUILD_CHECKPATCH,
KBUILD_CHECKCOCCI
KBUILD_CHECKDOC,
...


This is ugly.



-- 
Best Regards
Masahiro Yamada