Message ID | 1445615606-3885-2-git-send-email-thomas.monjalon@6wind.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 7FD635A17; Fri, 23 Oct 2015 17:54:40 +0200 (CEST) Received: from mail-wi0-f178.google.com (mail-wi0-f178.google.com [209.85.212.178]) by dpdk.org (Postfix) with ESMTP id 8DF6F5963 for <dev@dpdk.org>; Fri, 23 Oct 2015 17:54:38 +0200 (CEST) Received: by wicfx6 with SMTP id fx6so37327265wic.1 for <dev@dpdk.org>; Fri, 23 Oct 2015 08:54:38 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:subject:date:message-id:in-reply-to :references; bh=LRRhX5OlSNn1KyJm71AhQSEn6zR7L4OSlJWovRYsNU0=; b=Ys/ceZUUWS31xuRrSIlpApwBTyt5xnFClV3HS7P7NwFYsE5NLIDUt8WPYwz7ZdnFKu UvlzuH0IrNRPtx+tlHDMZIzT3uJV5UyGSNPwVg1e6ZfgnHZ42UZp6BvhfTgFjcBBh3nR x9owLUS8J+3VwjVUvwBk8rAAwdS2IwFY2sBYWV/fTMwYxbNZGKDJuExBbdpNe7I7bwWu Fhqs33rmdZ58fCgy4XtBaQkxuo6FwCcFtEDGjqzJirPKQYjtqhl0+x2NtJp13Z9f7ccQ xbR/KrApYd8igk2ocJUa6F0RsP/xnMkoPHapHrYKCcuJmzlKlES2xWAX0eiSE1CaKGKi zPwQ== X-Gm-Message-State: ALoCoQlM2eQsz6VOpq4/k6lCn2vVy+InLO/9ovseR2E6laecEwPkfqZts7CoX+qM/g2JdpEbEz7k X-Received: by 10.194.179.165 with SMTP id dh5mr6524321wjc.134.1445615678393; Fri, 23 Oct 2015 08:54:38 -0700 (PDT) Received: from localhost.localdomain (136-92-190-109.dsl.ovh.fr. [109.190.92.136]) by smtp.gmail.com with ESMTPSA id cq8sm2187837wib.12.2015.10.23.08.54.37 for <dev@dpdk.org> (version=TLSv1/SSLv3 cipher=OTHER); Fri, 23 Oct 2015 08:54:37 -0700 (PDT) From: Thomas Monjalon <thomas.monjalon@6wind.com> To: dev@dpdk.org Date: Fri, 23 Oct 2015 17:53:25 +0200 Message-Id: <1445615606-3885-2-git-send-email-thomas.monjalon@6wind.com> X-Mailer: git-send-email 2.5.2 In-Reply-To: <1445615606-3885-1-git-send-email-thomas.monjalon@6wind.com> References: <1445615606-3885-1-git-send-email-thomas.monjalon@6wind.com> Subject: [dpdk-dev] [PATCH 1/2] scripts: add checkpatch wrapper X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Thomas Monjalon
Oct. 23, 2015, 3:53 p.m. UTC
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
Comments
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
diff --git a/scripts/checkpatches.sh b/scripts/checkpatches.sh new file mode 100755 index 0000000..192b931 --- /dev/null +++ b/scripts/checkpatches.sh @@ -0,0 +1,30 @@ +#! /bin/sh + +# Load config options: +# - DPDK_CHECKPATCH_PATH +# - DPDK_CHECKPATCH_LINE_LENGTH +. scripts/load-devel-config.sh +if [ ! -x "$DPDK_CHECKPATCH_PATH" ] ; then + echo 'Cannot execute DPDK_CHECKPATCH_PATH' >&2 + exit 1 +fi + +length=${DPDK_CHECKPATCH_LINE_LENGTH:-80} + +# 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" + +status=0 +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 diff --git a/scripts/load-devel-config.sh b/scripts/load-devel-config.sh new file mode 100755 index 0000000..489f007 --- /dev/null +++ b/scripts/load-devel-config.sh @@ -0,0 +1,14 @@ +#! /bin/echo must be loaded with . + +# Load DPDK devel config and allow override +# from system file +test ! -r /etc/dpdk/devel.config || + . /etc/dpdk/devel.config +# from user file +test ! -r ~/.config/dpdk/devel.config || + . ~/.config/dpdk/devel.config +# from local file +test ! -r $(dirname $(readlink -m $0))/../.develconfig || + . $(dirname $(readlink -m $0))/../.develconfig + +# The config files must export variables in the shell style