[dpdk-dev] [PATCH 0/2] more basic tests

[dpdk-dev] [PATCH 0/2] more basic tests

[Thomas Monjalon]

I use some scripts to make some basic checks before committing
or submitting some patches.
I think they can be useful for every DPDK developers and especially
for new committers / tree owners.

The problem when sharing such tool is how to make it customizable
while fitting for everyone. The proposed solution is to use a
development configuration file.

Thomas Monjalon (2):
  scripts: add checkpatch wrapper
  scripts: add build tests

 scripts/checkpatches.sh      |  30 ++++++++++++
 scripts/load-devel-config.sh |  14 ++++++
 scripts/test-build.sh        | 112 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 156 insertions(+)
 create mode 100755 scripts/checkpatches.sh
 create mode 100755 scripts/load-devel-config.sh
 create mode 100755 scripts/test-build.sh

-- 
2.5.2

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

[Thomas Monjalon]

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

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

[dpdk-dev] [PATCH 2/2] scripts: add build tests

[Thomas Monjalon]

This script helps to build a list of target with some custom options.
It tries to enable most of the options.
The examples and documentation are also built.

It uses some configuration from exported variables.
This config works on my machine:
export DPDK_DEP_PCAP=y
export DPDK_DEP_MOFED=y
       mlxdep=/opt/mofed-3.0
export DPDK_DEP_CFLAGS=-I$mlxdep/include
export DPDK_DEP_LDFLAGS=-L$mlxdep/lib
export DPDK_BUILD_TEST_CONFIGS='x86_64-native-linuxapp-gcc+shared+next
       x86_64-native-linuxapp-clang+shared+combined
       i686-native-linuxapp-gcc+combined'
export DPDK_MAKE_JOBS=8
export DPDK_NOTIFY=notify-send

Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
 scripts/test-build.sh | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)
 create mode 100755 scripts/test-build.sh

diff --git a/scripts/test-build.sh b/scripts/test-build.sh
new file mode 100755
index 0000000..517141d
--- /dev/null
+++ b/scripts/test-build.sh
@@ -0,0 +1,112 @@
+#! /bin/sh -e
+
+# Load config options:
+# - DPDK_BUILD_TEST_CONFIGS (target1+option1+option2 target2)
+# - DPDK_DEP_CFLAGS
+# - DPDK_DEP_LDFLAGS
+# - DPDK_DEP_MOFED (y/[n])
+# - DPDK_DEP_PCAP (y/[n])
+# - DPDK_NOTIFY (notify-send)
+. scripts/load-devel-config.sh
+
+print_usage () {
+	echo "usage: $(basename $0) [-jX] [-q] [config1 [config2] ...]]"
+}
+
+J=$DPDK_MAKE_JOBS
+quick=false
+while getopts hj:q ARG ; do
+	case $ARG in
+		j ) J=$OPTARG ;;
+		q ) quick=true ;;
+		h ) print_usage ; exit 0 ;;
+		? ) print_usage ; exit 1 ;;
+	esac
+done
+shift $(($OPTIND - 1))
+configs=${*:-$DPDK_BUILD_TEST_CONFIGS}
+
+success=false
+on_exit ()
+{
+	if [ "$DPDK_NOTIFY" = notify-send ] ; then
+		if $success ; then
+			notify-send -u low --icon=dialog-information 'DPDK build' 'finished'
+		elif [ -z "$signal" ] ; then
+			notify-send -u low --icon=dialog-error 'DPDK build' 'failed'
+		fi
+	fi
+}
+# catch manual interrupt to ignore notification
+trap "signal=INT ; trap - INT ; kill -INT $$" INT
+# notify result on exit
+trap on_exit EXIT
+
+cd $(dirname $(readlink -m $0))/..
+
+config () # <directory> <target> <options>
+{
+	if [ ! -e $1/.config ] ; then
+		echo Custom configuration
+		make T=$2 O=$1 config
+		echo $3 | grep -q next || \
+		sed -ri           's,(NEXT_ABI=)y,\1n,' $1/.config
+		! echo $3 | grep -q shared || \
+		sed -ri         's,(SHARED_LIB=)n,\1y,' $1/.config
+		! echo $3 | grep -q combined || \
+		sed -ri       's,(COMBINE_LIBS=)n,\1y,' $1/.config
+		echo $2 | grep -q '^i686' || \
+		sed -ri               's,(NUMA=)n,\1y,' $1/.config
+		sed -ri         's,(PCI_CONFIG=)n,\1y,' $1/.config
+		sed -ri    's,(LIBRTE_IEEE1588=)n,\1y,' $1/.config
+		sed -ri             's,(BYPASS=)n,\1y,' $1/.config
+		test "$DPDK_DEP_MOFED" != y || \
+		echo $2 | grep -q '^clang$' || \
+		echo $3 | grep -q 'shared.*combined' || \
+		sed -ri           's,(MLX4_PMD=)n,\1y,' $1/.config
+		echo $2 | grep -q '^i686' || \
+		sed -ri          's,(BNX2X_PMD=)n,\1y,' $1/.config
+		test "$DPDK_DEP_PCAP" != y || \
+		sed -ri               's,(PCAP=)n,\1y,' $1/.config
+		sed -ri        's,(KNI_VHOST.*=)n,\1y,' $1/.config
+		sed -ri           's,(SCHED_.*=)n,\1y,' $1/.config
+		! echo $2 | grep -q '^i686' || \
+		sed -ri              's,(POWER=)y,\1n,' $1/.config
+		sed -ri 's,(TEST_PMD_RECORD_.*=)n,\1y,' $1/.config
+		sed -ri            's,(DEBUG.*=)n,\1y,' $1/.config
+	fi
+}
+
+for conf in $configs ; do
+	target=$(echo $conf | cut -d'+' -f1)
+	options=$(echo $conf | cut -d'+' -sf2- --output-delimiter='-')
+	if [ -z "$options" ] ; then
+		dir=$target
+		config $dir $target
+		# Use install rule
+		make -j$J T=$target install EXTRA_CFLAGS=$DPDK_DEP_CFLAGS EXTRA_LDFLAGS=$DPDK_DEP_LDFLAGS
+		$quick || make -j$J T=$target examples O=$dir/examples EXTRA_LDFLAGS=$DPDK_DEP_LDFLAGS
+	else
+		dir=$target-$options
+		config $dir $target $options
+		echo "================== Build $dir"
+		# Use O variable without install
+		make -j$J O=$dir EXTRA_CFLAGS=$DPDK_DEP_CFLAGS EXTRA_LDFLAGS=$DPDK_DEP_LDFLAGS
+		echo "================== Build examples for $dir"
+		make -j$J -sC examples RTE_SDK=$(pwd) RTE_TARGET=$dir O=$(readlink -m $dir/examples) EXTRA_LDFLAGS=$DPDK_DEP_LDFLAGS
+	fi
+	echo "################## $dir done."
+	! $quick || break
+done
+
+if ! $quick ; then
+	mkdir -p .check
+	echo "================== Build doxygen HTML API"
+	make doc-api-html >/dev/null 2>.check/doc.txt
+	echo "================== Build sphinx HTML guides"
+	make doc-guides-html >/dev/null 2>>.check/doc.txt
+	echo "================== Check docs"
+	diff -u /dev/null .check/doc.txt
+fi
+
+success=true
-- 
2.5.2

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

[Stephen Hemminger]

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.

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

[Thomas Monjalon]

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.

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

[Stephen Hemminger]

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.

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

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

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

[Thomas Monjalon]

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.

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

[Bruce Richardson]

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>

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

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

-- 
David Marchand

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

[Thomas Monjalon]

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

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

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

/Bruce

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

[Thomas Monjalon]

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

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

[Bruce Richardson]

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

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

[David Marchand]

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.


-- 
David Marchand

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

[Thomas Monjalon]

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 ;)

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

[Bruce Richardson]

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

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

[Thomas Monjalon]

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

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

[Van Haaren, Harry]

> 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

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

[Bruce Richardson]

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

[dpdk-dev] [PATCH v2 0/2] more basic tests

[Thomas Monjalon]

I use some scripts to make some basic checks before committing or
submitting some patches.
As they can be useful for every DPDK developers and especially for
new committers / tree owners, they are added to the scripts directory.

The problem, when sharing such tools, is how to make them customizable
while fitting for everyone. The proposed solution is to use a
development configuration file.

Thomas Monjalon (2):
  scripts: add checkpatch wrapper
  scripts: add build tests

 scripts/checkpatches.sh      |  82 ++++++++++++++++++++++++
 scripts/load-devel-config.sh |  14 +++++
 scripts/test-build.sh        | 145 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 241 insertions(+)
 create mode 100755 scripts/checkpatches.sh
 create mode 100755 scripts/load-devel-config.sh
 create mode 100755 scripts/test-build.sh

-- 
2.5.2

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

[Thomas Monjalon]

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>
---
v2:
- do not ignore COMPLEX_MACRO
- use option --no-tree to avoid silent failure
- add -q and -v options

 scripts/checkpatches.sh      | 82 ++++++++++++++++++++++++++++++++++++++++++++
 scripts/load-devel-config.sh | 14 ++++++++
 2 files changed, 96 insertions(+)
 create mode 100755 scripts/checkpatches.sh
 create mode 100755 scripts/load-devel-config.sh

diff --git a/scripts/checkpatches.sh b/scripts/checkpatches.sh
new file mode 100755
index 0000000..afc611b
--- /dev/null
+++ b/scripts/checkpatches.sh
@@ -0,0 +1,82 @@
+#! /bin/sh
+
+# BSD LICENSE
+#
+# Copyright 2015 6WIND S.A.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions
+# are met:
+#
+#   * Redistributions of source code must retain the above copyright
+#     notice, this list of conditions and the following disclaimer.
+#   * Redistributions in binary form must reproduce the above copyright
+#     notice, this list of conditions and the following disclaimer in
+#     the documentation and/or other materials provided with the
+#     distribution.
+#   * Neither the name of 6WIND S.A. nor the names of its
+#     contributors may be used to endorse or promote products derived
+#     from this software without specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+# 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="--no-tree"
+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,COMPARISON_TO_NULL"
+
+print_usage () {
+	echo "usage: $(basename $0) [-q] [-v] [patch1 [patch2] ...]]"
+}
+
+quiet=false
+verbose=false
+while getopts hqv ARG ; do
+	case $ARG in
+		q ) quiet=true ;;
+		v ) verbose=true ;;
+		h ) print_usage ; exit 0 ;;
+		? ) print_usage ; exit 1 ;;
+	esac
+done
+shift $(($OPTIND - 1))
+
+status=0
+for p in "$@" ; do
+	! $verbose || printf '\n### %s\n\n' "$p"
+	report=$($DPDK_CHECKPATCH_PATH $options "$p" 2>/dev/null)
+	[ $? -ne 0 ] || continue
+	$verbose || printf '\n### %s\n\n' "$p"
+	printf '%s\n' "$report" | head -n -6
+	status=$(($status + 1))
+done
+pass=$(($# - $status))
+$quiet || printf '%d/%d valid patch' $pass $#
+$quiet || [ $pass -le 1 ] || printf 'es'
+$quiet || printf '\n'
+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
-- 
2.5.2

[dpdk-dev] [PATCH v2 2/2] scripts: add build tests

[Thomas Monjalon]

This script helps to build a list of target with some custom options.
It tries to enable most of the options.
The examples and documentation are also built.

It uses some configuration from exported variables.
This config works on my machine:
export DPDK_DEP_PCAP=y
export DPDK_DEP_MOFED=y
       mlxdep=/opt/mofed-3.0
export DPDK_DEP_CFLAGS=-I$mlxdep/include
export DPDK_DEP_LDFLAGS=-L$mlxdep/lib
export DPDK_BUILD_TEST_CONFIGS='x86_64-native-linuxapp-gcc+shared+next
       x86_64-native-linuxapp-clang+shared+combined
       i686-native-linuxapp-gcc+combined'
export DPDK_MAKE_JOBS=8
export DPDK_NOTIFY=notify-send

Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
v2:
- conditionally enable szedata2
- add quotes for CFLAGS and LDFLAGS

 scripts/test-build.sh | 145 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 145 insertions(+)
 create mode 100755 scripts/test-build.sh

diff --git a/scripts/test-build.sh b/scripts/test-build.sh
new file mode 100755
index 0000000..3d3cf9f
--- /dev/null
+++ b/scripts/test-build.sh
@@ -0,0 +1,145 @@
+#! /bin/sh -e
+
+# BSD LICENSE
+#
+# Copyright 2015 6WIND S.A.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions
+# are met:
+#
+#   * Redistributions of source code must retain the above copyright
+#     notice, this list of conditions and the following disclaimer.
+#   * Redistributions in binary form must reproduce the above copyright
+#     notice, this list of conditions and the following disclaimer in
+#     the documentation and/or other materials provided with the
+#     distribution.
+#   * Neither the name of 6WIND S.A. nor the names of its
+#     contributors may be used to endorse or promote products derived
+#     from this software without specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+# Load config options:
+# - DPDK_BUILD_TEST_CONFIGS (target1+option1+option2 target2)
+# - DPDK_DEP_CFLAGS
+# - DPDK_DEP_LDFLAGS
+# - DPDK_DEP_MOFED (y/[n])
+# - DPDK_DEP_PCAP (y/[n])
+# - DPDK_NOTIFY (notify-send)
+. scripts/load-devel-config.sh
+
+print_usage () {
+	echo "usage: $(basename $0) [-jX] [-q] [config1 [config2] ...]]"
+}
+
+J=$DPDK_MAKE_JOBS
+quick=false
+while getopts hj:q ARG ; do
+	case $ARG in
+		j ) J=$OPTARG ;;
+		q ) quick=true ;;
+		h ) print_usage ; exit 0 ;;
+		? ) print_usage ; exit 1 ;;
+	esac
+done
+shift $(($OPTIND - 1))
+configs=${*:-$DPDK_BUILD_TEST_CONFIGS}
+
+success=false
+on_exit ()
+{
+	if [ "$DPDK_NOTIFY" = notify-send ] ; then
+		if $success ; then
+			notify-send -u low --icon=dialog-information 'DPDK build' 'finished'
+		elif [ -z "$signal" ] ; then
+			notify-send -u low --icon=dialog-error 'DPDK build' 'failed'
+		fi
+	fi
+}
+# catch manual interrupt to ignore notification
+trap "signal=INT ; trap - INT ; kill -INT $$" INT
+# notify result on exit
+trap on_exit EXIT
+
+cd $(dirname $(readlink -m $0))/..
+
+config () # <directory> <target> <options>
+{
+	if [ ! -e $1/.config ] ; then
+		echo Custom configuration
+		make T=$2 O=$1 config
+		echo $3 | grep -q next || \
+		sed -ri           's,(NEXT_ABI=)y,\1n,' $1/.config
+		! echo $3 | grep -q shared || \
+		sed -ri         's,(SHARED_LIB=)n,\1y,' $1/.config
+		! echo $3 | grep -q combined || \
+		sed -ri       's,(COMBINE_LIBS=)n,\1y,' $1/.config
+		echo $2 | grep -q '^i686' || \
+		sed -ri               's,(NUMA=)n,\1y,' $1/.config
+		sed -ri         's,(PCI_CONFIG=)n,\1y,' $1/.config
+		sed -ri    's,(LIBRTE_IEEE1588=)n,\1y,' $1/.config
+		sed -ri             's,(BYPASS=)n,\1y,' $1/.config
+		test "$DPDK_DEP_MOFED" != y || \
+		echo $2 | grep -q '^clang$' || \
+		echo $3 | grep -q 'shared.*combined' || \
+		sed -ri           's,(MLX4_PMD=)n,\1y,' $1/.config
+		test "$DPDK_DEP_SZE" != y || \
+		echo $2 | grep -q '^i686' || \
+		sed -ri       's,(PMD_SZEDATA2=)n,\1y,' $1/.config
+		echo $2 | grep -q '^i686' || \
+		sed -ri          's,(BNX2X_PMD=)n,\1y,' $1/.config
+		test "$DPDK_DEP_PCAP" != y || \
+		sed -ri               's,(PCAP=)n,\1y,' $1/.config
+		sed -ri        's,(KNI_VHOST.*=)n,\1y,' $1/.config
+		sed -ri           's,(SCHED_.*=)n,\1y,' $1/.config
+		! echo $2 | grep -q '^i686' || \
+		sed -ri              's,(POWER=)y,\1n,' $1/.config
+		sed -ri 's,(TEST_PMD_RECORD_.*=)n,\1y,' $1/.config
+		sed -ri            's,(DEBUG.*=)n,\1y,' $1/.config
+	fi
+}
+
+for conf in $configs ; do
+	target=$(echo $conf | cut -d'+' -f1)
+	options=$(echo $conf | cut -d'+' -sf2- --output-delimiter='-')
+	if [ -z "$options" ] ; then
+		dir=$target
+		config $dir $target
+		# Use install rule
+		make -j$J T=$target install EXTRA_CFLAGS="$DPDK_DEP_CFLAGS" EXTRA_LDFLAGS="$DPDK_DEP_LDFLAGS"
+		$quick || make -j$J T=$target examples O=$dir/examples EXTRA_LDFLAGS="$DPDK_DEP_LDFLAGS"
+	else
+		dir=$target-$options
+		config $dir $target $options
+		echo "================== Build $dir"
+		# Use O variable without install
+		make -j$J O=$dir EXTRA_CFLAGS="$DPDK_DEP_CFLAGS" EXTRA_LDFLAGS="$DPDK_DEP_LDFLAGS"
+		echo "================== Build examples for $dir"
+		make -j$J -sC examples RTE_SDK=$(pwd) RTE_TARGET=$dir O=$(readlink -m $dir/examples) EXTRA_LDFLAGS="$DPDK_DEP_LDFLAGS"
+	fi
+	echo "################## $dir done."
+	! $quick || break
+done
+
+if ! $quick ; then
+	mkdir -p .check
+	echo "================== Build doxygen HTML API"
+	make doc-api-html >/dev/null 2>.check/doc.txt
+	echo "================== Build sphinx HTML guides"
+	make doc-guides-html >/dev/null 2>>.check/doc.txt
+	echo "================== Check docs"
+	diff -u /dev/null .check/doc.txt
+fi
+
+success=true
-- 
2.5.2

Re: [dpdk-dev] [PATCH v2 1/2] scripts: add checkpatch wrapper

[Bruce Richardson]

On Mon, Nov 23, 2015 at 02:40:07AM +0100, 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>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH v2 2/2] scripts: add build tests

[Mcnamara, John]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Monday, November 23, 2015 1:40 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v2 2/2] scripts: add build tests
> 
> ...
>
> +while getopts hj:q ARG ; do
> +	case $ARG in
> +		j ) J=$OPTARG ;;
> +		q ) quick=true ;;
> +		h ) print_usage ; exit 0 ;;
> +		? ) print_usage ; exit 1 ;;

Hi John,

Very useful. We should add these scripts to the contributing guide at some point.

One minor nit. "q" is used for "quick" in this script and "quiet" in the checkpatch script. Maybe for consistency use "f" for fast or "n" for no-doc or something else.

John.
-- 
 

Re: [dpdk-dev] [PATCH v2 2/2] scripts: add build tests

[Thomas Monjalon]

2015-11-24 17:49, Mcnamara, John:
> Very useful. We should add these scripts to the contributing guide at some point.

Yes

> One minor nit. "q" is used for "quick" in this script and "quiet" in the checkpatch script. Maybe for consistency use "f" for fast or "n" for no-doc or something else.

Consistency of short options is a difficult job.
-f may work but can also be used to "force" in another script.
I'll change it to -s (short) if there is no other comment.

Re: [dpdk-dev] [PATCH v2 0/2] more basic tests

[Thomas Monjalon]

2015-11-23 02:40, Thomas Monjalon:
> I use some scripts to make some basic checks before committing or
> submitting some patches.
> As they can be useful for every DPDK developers and especially for
> new committers / tree owners, they are added to the scripts directory.
> 
> The problem, when sharing such tools, is how to make them customizable
> while fitting for everyone. The proposed solution is to use a
> development configuration file.
> 
> Thomas Monjalon (2):
>   scripts: add checkpatch wrapper
>   scripts: add build tests

Applied with some minor changes.