[dpdk-dev,1/2] scripts: add checkpatch wrapper

  This script can be used to call checkpatch.pl from Linux with some
custom DPDK options.

The path to the original Linux script must be set in an environment
variable. A script is added to load any configuration variables
required by development tools from a file .develconfig, or
~/.config/dpdk/devel.config or /etc/dpdk/devel.config.

Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
 scripts/checkpatches.sh      | 30 ++++++++++++++++++++++++++++++
 scripts/load-devel-config.sh | 14 ++++++++++++++
 2 files changed, 44 insertions(+)
 create mode 100755 scripts/checkpatches.sh
 create mode 100755 scripts/load-devel-config.sh
  

On Fri, 23 Oct 2015 17:53:25 +0200
Thomas Monjalon <thomas.monjalon@6wind.com> wrote:

> +
> +# override default Linux options
> +options="$options --max-line-length=$length"
> +options="$options --show-types"
> +options="$options --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\
> +VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,PREFER_KERNEL_TYPES,\
> +SPLIT_STRING,LINE_SPACING,PARENTHESIS_ALIGNMENT,NETWORKING_BLOCK_COMMENT_STYLE,\
> +NEW_TYPEDEFS,COMPLEX_MACRO,COMPARISON_TO_NULL"

Please keep some of these:
  PARENTHESIS_ALIGNMENT
  LINUX_VERSION_CODE
  COMPLEX_MACRO

All of these are indications of problems in code.
  

2015-10-23 09:03, Stephen Hemminger:
> Thomas Monjalon <thomas.monjalon@6wind.com> wrote:
> > +options="$options --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\
> > +VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,PREFER_KERNEL_TYPES,\
> > +SPLIT_STRING,LINE_SPACING,PARENTHESIS_ALIGNMENT,NETWORKING_BLOCK_COMMENT_STYLE,\
> > +NEW_TYPEDEFS,COMPLEX_MACRO,COMPARISON_TO_NULL"
> 
> Please keep some of these:
>   PARENTHESIS_ALIGNMENT

It is not written in the DPDK coding rules that
"Alignment should match open parenthesis"

>   LINUX_VERSION_CODE

It is used for out-of-tree modules.

>   COMPLEX_MACRO

I don't remember why I've ignored this one:
"Macros with complex values should be enclosed in parentheses"

> All of these are indications of problems in code.

I don't think so for the DPDK case, except COMPLEX_MACRO.
Thanks for catching.
  

On Fri, 23 Oct 2015 18:34:23 +0200
Thomas Monjalon <thomas.monjalon@6wind.com> wrote:

> > Please keep some of these:
> >   PARENTHESIS_ALIGNMENT  
> 
> It is not written in the DPDK coding rules that
> "Alignment should match open parenthesis"

I think code looks nicer if this rule is followed.
Ditto for the LINE_SPACING rule.
  

On Fri, 23 Oct 2015 18:34:23 +0200
Thomas Monjalon <thomas.monjalon@6wind.com> wrote:

> >   LINUX_VERSION_CODE  
> 
> It is used for out-of-tree modules.

Actually for the kernel modules, all flags should be enabled.
It is kernel code and it should follow the kernel style conventions.
  

2015-10-23 09:58, Stephen Hemminger:
> On Fri, 23 Oct 2015 18:34:23 +0200
> Thomas Monjalon <thomas.monjalon@6wind.com> wrote:
> 
> > >   LINUX_VERSION_CODE  
> > 
> > It is used for out-of-tree modules.
> 
> Actually for the kernel modules, all flags should be enabled.
> It is kernel code and it should follow the kernel style conventions.

No, version macros are needed in out of tree modules but forbidden
upstream.
  

On Fri, Oct 23, 2015 at 05:53:25PM +0200, Thomas Monjalon wrote:
> This script can be used to call checkpatch.pl from Linux with some
> custom DPDK options.
> 
> The path to the original Linux script must be set in an environment
> variable. A script is added to load any configuration variables
> required by development tools from a file .develconfig, or
> ~/.config/dpdk/devel.config or /etc/dpdk/devel.config.
> 
> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>

Be great to have this in the repo for checking patches before ack.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>
  

On Fri, Oct 23, 2015 at 5:53 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> +for p in "$@" ; do
> +       printf -- "\n### $p\n\n"
> +       report=$($DPDK_CHECKPATCH_PATH $options "$p" 2>/dev/null)
> +       [ $? -ne 0 ] || continue
> +       printf '%s\n' "$report" | head -n -6
> +       status=$(($status + 1))
> +done
> +exit $status
>

I prefer when checking scripts only complain when something is wrong :-)
So I would only display the file name if checkpatch complains.
  

2015-10-29 13:33, David Marchand:
> On Fri, Oct 23, 2015 at 5:53 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
> wrote:
> 
> > +for p in "$@" ; do
> > +       printf -- "\n### $p\n\n"
> > +       report=$($DPDK_CHECKPATCH_PATH $options "$p" 2>/dev/null)
> > +       [ $? -ne 0 ] || continue
> > +       printf '%s\n' "$report" | head -n -6
> > +       status=$(($status + 1))
> > +done
> > +exit $status
> >
> 
> I prefer when checking scripts only complain when something is wrong :-)
> So I would only display the file name if checkpatch complains.

Yes I'll move the first printf after the "continue".
  

On Thu, Oct 29, 2015 at 02:03:59PM +0100, Thomas Monjalon wrote:
> 2015-10-29 13:33, David Marchand:
> > On Fri, Oct 23, 2015 at 5:53 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
> > wrote:
> > 
> > > +for p in "$@" ; do
> > > +       printf -- "\n### $p\n\n"
> > > +       report=$($DPDK_CHECKPATCH_PATH $options "$p" 2>/dev/null)
> > > +       [ $? -ne 0 ] || continue
> > > +       printf '%s\n' "$report" | head -n -6
> > > +       status=$(($status + 1))
> > > +done
> > > +exit $status
> > >
> > 
> > I prefer when checking scripts only complain when something is wrong :-)
> > So I would only display the file name if checkpatch complains.
> 
> Yes I'll move the first printf after the "continue".

Ok, but perhaps instead we can get a print at the end of how many files were
checked. I'm concerned about the case where we think we have checked something and
it's ok, when in fact we have actually had an error in our command and e.g. not checked
any files at all. The printing of the filename helps give a guarantee that the
script is doing the right thing, so if it goes away, I'd hope for some other method
to ensure that.

/Bruce
  

2015-10-29 13:24, Bruce Richardson:
> On Thu, Oct 29, 2015 at 02:03:59PM +0100, Thomas Monjalon wrote:
> > 2015-10-29 13:33, David Marchand:
> > > On Fri, Oct 23, 2015 at 5:53 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
> > > wrote:
> > > 
> > > > +for p in "$@" ; do
> > > > +       printf -- "\n### $p\n\n"
> > > > +       report=$($DPDK_CHECKPATCH_PATH $options "$p" 2>/dev/null)
> > > > +       [ $? -ne 0 ] || continue
> > > > +       printf '%s\n' "$report" | head -n -6
> > > > +       status=$(($status + 1))
> > > > +done
> > > > +exit $status
> > > >
> > > 
> > > I prefer when checking scripts only complain when something is wrong :-)
> > > So I would only display the file name if checkpatch complains.
> > 
> > Yes I'll move the first printf after the "continue".
> 
> Ok, but perhaps instead we can get a print at the end of how many files were
> checked. I'm concerned about the case where we think we have checked something and
> it's ok, when in fact we have actually had an error in our command and e.g. not checked
> any files at all. The printing of the filename helps give a guarantee that the
> script is doing the right thing, so if it goes away, I'd hope for some other method
> to ensure that.

I agree with both of you.
I could suggest something but I'm afraid it will be difficult to have a
consensus between a "quiet tool" and a "double check verbose tool".
As it is a really critical piece of code, I think we should have a meeting
with a technical steering comittee ;)
... or we can add an option: -q or -v ? Debate is open :D
  

On Thu, Oct 29, 2015 at 02:34:32PM +0100, Thomas Monjalon wrote:
> 2015-10-29 13:24, Bruce Richardson:
> > On Thu, Oct 29, 2015 at 02:03:59PM +0100, Thomas Monjalon wrote:
> > > 2015-10-29 13:33, David Marchand:
> > > > On Fri, Oct 23, 2015 at 5:53 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
> > > > wrote:
> > > > 
> > > > > +for p in "$@" ; do
> > > > > +       printf -- "\n### $p\n\n"
> > > > > +       report=$($DPDK_CHECKPATCH_PATH $options "$p" 2>/dev/null)
> > > > > +       [ $? -ne 0 ] || continue
> > > > > +       printf '%s\n' "$report" | head -n -6
> > > > > +       status=$(($status + 1))
> > > > > +done
> > > > > +exit $status
> > > > >
> > > > 
> > > > I prefer when checking scripts only complain when something is wrong :-)
> > > > So I would only display the file name if checkpatch complains.
> > > 
> > > Yes I'll move the first printf after the "continue".
> > 
> > Ok, but perhaps instead we can get a print at the end of how many files were
> > checked. I'm concerned about the case where we think we have checked something and
> > it's ok, when in fact we have actually had an error in our command and e.g. not checked
> > any files at all. The printing of the filename helps give a guarantee that the
> > script is doing the right thing, so if it goes away, I'd hope for some other method
> > to ensure that.
> 
> I agree with both of you.
> I could suggest something but I'm afraid it will be difficult to have a
> consensus between a "quiet tool" and a "double check verbose tool".
> As it is a really critical piece of code, I think we should have a meeting
> with a technical steering comittee ;)
> ... or we can add an option: -q or -v ? Debate is open :D
> 
Yes, the whole future of the project could hinge on this decision :-)

Ok, my suggestion is both! 
1) Have the default (in case of no errors), be a single line print out at the end
stating number of files scanned
2) If "-q" flag specified, skip this
3) If "-v" flag specified, do current behaviour with a line per file.

Regards,

/Bruce
  

On Thu, Oct 29, 2015 at 2:48 PM, Bruce Richardson <
bruce.richardson@intel.com> wrote:

> On Thu, Oct 29, 2015 at 02:34:32PM +0100, Thomas Monjalon wrote:
> > I agree with both of you.
> > I could suggest something but I'm afraid it will be difficult to have a
> > consensus between a "quiet tool" and a "double check verbose tool".
> > As it is a really critical piece of code, I think we should have a
> meeting
> > with a technical steering comittee ;)
> > ... or we can add an option: -q or -v ? Debate is open :D
> >
> Yes, the whole future of the project could hinge on this decision :-)
>

Eheh :-)


> Ok, my suggestion is both!
> 1) Have the default (in case of no errors), be a single line print out at
> the end
> stating number of files scanned
> 2) If "-q" flag specified, skip this
> 3) If "-v" flag specified, do current behaviour with a line per file.
>

Ok for me.
  

2015-10-29 14:54, David Marchand:
> On Thu, Oct 29, 2015 at 2:48 PM, Bruce Richardson wrote:
> > On Thu, Oct 29, 2015 at 02:34:32PM +0100, Thomas Monjalon wrote:
> > > I agree with both of you.
> > > I could suggest something but I'm afraid it will be difficult to have a
> > > consensus between a "quiet tool" and a "double check verbose tool".
> > > As it is a really critical piece of code, I think we should have a
> > meeting
> > > with a technical steering comittee ;)
> > > ... or we can add an option: -q or -v ? Debate is open :D
> > 
> > Yes, the whole future of the project could hinge on this decision :-)
> 
> Eheh :-)
> 
> > Ok, my suggestion is both!
> > 1) Have the default (in case of no errors), be a single line print out at
> > the end
> > stating number of files scanned
> > 2) If "-q" flag specified, skip this
> > 3) If "-v" flag specified, do current behaviour with a line per file.
> 
> Ok for me.

I'm really happy we can have a sane consensus to this difficult question,
with just few emails!
Thanks guys :)

PS: I will send a v2 when the easy task of RC1 integration will be done ;)
  

On Thu, Oct 29, 2015 at 02:57:42PM +0100, Thomas Monjalon wrote:
> 2015-10-29 14:54, David Marchand:
> > On Thu, Oct 29, 2015 at 2:48 PM, Bruce Richardson wrote:
> > > On Thu, Oct 29, 2015 at 02:34:32PM +0100, Thomas Monjalon wrote:
> > > > I agree with both of you.
> > > > I could suggest something but I'm afraid it will be difficult to have a
> > > > consensus between a "quiet tool" and a "double check verbose tool".
> > > > As it is a really critical piece of code, I think we should have a
> > > meeting
> > > > with a technical steering comittee ;)
> > > > ... or we can add an option: -q or -v ? Debate is open :D
> > > 
> > > Yes, the whole future of the project could hinge on this decision :-)
> > 
> > Eheh :-)
> > 
> > > Ok, my suggestion is both!
> > > 1) Have the default (in case of no errors), be a single line print out at
> > > the end
> > > stating number of files scanned
> > > 2) If "-q" flag specified, skip this
> > > 3) If "-v" flag specified, do current behaviour with a line per file.
> > 
> > Ok for me.
> 
> I'm really happy we can have a sane consensus to this difficult question,
> with just few emails!
> Thanks guys :)
> 
> PS: I will send a v2 when the easy task of RC1 integration will be done ;)

Another request, can you perhaps also fix the script for situations where 
checkpatch.pl is not in the kernel tree. I've used this script now to check a
couple of patchsets, which came back clean, but it turns out that because I was
using checkpatch.pl outside the kernel directory, it is passing things it
shouldn't. [Thanks to Sergio for pointing this out]. 

Testing with a known-broken patch, this script indicates all ok, and only
reports an error with the --no-tree added to the options inside the script. :-(

/Bruce
  

2015-10-30 16:16, Bruce Richardson:
> Another request, can you perhaps also fix the script for situations where 
> checkpatch.pl is not in the kernel tree. I've used this script now to check a
> couple of patchsets, which came back clean, but it turns out that because I was
> using checkpatch.pl outside the kernel directory, it is passing things it
> shouldn't. [Thanks to Sergio for pointing this out]. 
> 
> Testing with a known-broken patch, this script indicates all ok, and only
> reports an error with the --no-tree added to the options inside the script. :-(

OK
I prefer using it in the Linux tree because it is updated with "git pull".
  

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Subject: Re: [dpdk-dev] [PATCH 1/2] scripts: add checkpatch wrapper
> 
> 2015-10-30 16:16, Bruce Richardson:
> > Another request, can you perhaps also fix the script for situations where
> > checkpatch.pl is not in the kernel tree.
> > <snip>

> OK
> I prefer using it in the Linux tree because it is updated with "git pull".

I would like to suggest including the checkpatch.pl script itself in the dpdk tree, as this would ensure that we are all running the exact same version of checkpatch.

My previous patchset had errors that I had not detected because I ran an older checkpatch.pl, and I think there are others who have similar issues that the checkpatch version provides more/less errors.

If included in the repo, we would all automatically upgrade when the next checkpatch.pl is merged - providing consistency.

-Harry
  

On Mon, Nov 02, 2015 at 10:28:35AM +0000, Van Haaren, Harry wrote:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > Subject: Re: [dpdk-dev] [PATCH 1/2] scripts: add checkpatch wrapper
> > 
> > 2015-10-30 16:16, Bruce Richardson:
> > > Another request, can you perhaps also fix the script for situations where
> > > checkpatch.pl is not in the kernel tree.
> > > <snip>
> 
> > OK
> > I prefer using it in the Linux tree because it is updated with "git pull".
> 
> I would like to suggest including the checkpatch.pl script itself in the dpdk tree, as this would ensure that we are all running the exact same version of checkpatch.
> 
> My previous patchset had errors that I had not detected because I ran an older checkpatch.pl, and I think there are others who have similar issues that the checkpatch version provides more/less errors.
> 
> If included in the repo, we would all automatically upgrade when the next checkpatch.pl is merged - providing consistency.
> 
> -Harry

+1 

/Bruce