[dpdk-dev,v2] ABI: Add abi checking utility

  There was a request for an abi validation utiltyfor the ongoing ABI stability
work.  As it turns out there is a abi compliance checker in development that
seems to be under active development and provides fairly detailed ABI compliance
reports.  Its not yet intellegent enough to understand symbol versioning, but it
does provide the ability to identify symbols which have changed between
releases, along with details of the change, and offers develoeprs the
opportunity to identify which symbols then need versioning and validation for a
given update via manaul testing.

This script automates the use of the compliance checker between two arbitrarily
specified tags within the dpdk tree.  To execute enter the $RTE_SDK directory
and run:

./scripts/validate_abi.sh $GIT_TAG1 $GIT_TAG2 $CONFIG

where $GIT_TAG1 and 2 are git tags and $CONFIG is a config specification
suitable for passing as the T= variable in the make config command.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

Change Notes:

v2) Fixed some typos as requested by Thomas
---
 scripts/validate_abi.sh | 241 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 241 insertions(+)
 create mode 100755 scripts/validate_abi.sh
  

On Mon, Feb 02, 2015 at 01:18:26PM -0500, Neil Horman wrote:
> There was a request for an abi validation utiltyfor the ongoing ABI stability
> work.  As it turns out there is a abi compliance checker in development that
> seems to be under active development and provides fairly detailed ABI compliance
> reports.  Its not yet intellegent enough to understand symbol versioning, but it
> does provide the ability to identify symbols which have changed between
> releases, along with details of the change, and offers develoeprs the
> opportunity to identify which symbols then need versioning and validation for a
> given update via manaul testing.
> 
> This script automates the use of the compliance checker between two arbitrarily
> specified tags within the dpdk tree.  To execute enter the $RTE_SDK directory
> and run:
> 
> ./scripts/validate_abi.sh $GIT_TAG1 $GIT_TAG2 $CONFIG
> 
> where $GIT_TAG1 and 2 are git tags and $CONFIG is a config specification
> suitable for passing as the T= variable in the make config command.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> Change Notes:
> 
> v2) Fixed some typos as requested by Thomas
> ---
>  scripts/validate_abi.sh | 241 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 241 insertions(+)
>  create mode 100755 scripts/validate_abi.sh
> 
> diff --git a/scripts/validate_abi.sh b/scripts/validate_abi.sh
> new file mode 100755
> index 0000000..31583df
> --- /dev/null
> +++ b/scripts/validate_abi.sh
> @@ -0,0 +1,241 @@
> +#!/bin/sh
> +#   BSD LICENSE
> +#
> +#   Copyright(c) 2015 Neil Horman. All rights reserved.
> +#   All rights reserved.
> +#
> +#   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.
> +#
> +#   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.
> +
> +TAG1=$1
> +TAG2=$2
> +TARGET=$3
> +ABI_DIR=`mktemp -d -p /tmp ABI.XXXXXX`
> +
> +usage() {
> +	echo "$0 <TAG1> <TAG2> <TARGET>"
> +}
> +
> +log() {
> +	local level=$1
> +	shift
> +	echo "$*"
> +}
> +
> +validate_tags() {
> +	git tag -l | grep -q "$TAG1"
> +	if [ $? -ne 0 ]
> +	then
> +		echo "$TAG1 is invalid"
> +		return
> +	fi
> +	git tag -l | grep -q "$TAG2"
> +	if [ $? -ne 0 ]
> +	then
> +		echo "$TAG2 is invalid"
> +		return
> +	fi
> +}
> +
> +validate_args() {
> +	if [ -z "$TAG1" ]
> +	then
> +		echo "Must Specify TAG1"
> +		return
> +	fi
> +	if [ -z "$TAG2" ]
> +	then
> +		echo "Must Specify TAG2"
> +		return
> +	fi
> +	if [ -z "$TARGET" ]
> +	then
> +		echo "Must Specify a build target"
> +	fi
> +}
> +
> +
> +cleanup_and_exit() {
> +	rm -rf $ABI_DIR
> +	exit $1
> +}
> +
> +###########################################
> +#START
> +############################################
> +
> +#Save the current branch
> +CURRENT_BRANCH=`git branch | grep \* | cut -d' ' -f2`
> +
> +if [ -n "$VERBOSE" ]
> +then
> +	export VERBOSE=/dev/stdout
> +else
> +	export VERBOSE=/dev/null
> +fi
> +
> +# Validate that we have all the arguments we need
> +res=$(validate_args)
> +if [ -n "$res" ]
> +then
> +	echo $res
> +	usage
> +	cleanup_and_exit 1
> +fi
> +
> +# Make sure our tags exist
> +res=$(validate_tags)
> +if [ -n "$res" ]
> +then
> +	echo $res
> +	cleanup_and_exit 1
> +fi
> +
> +ABICHECK=`which abi-compliance-checker 2>/dev/null`
> +if [ $? -ne 0 ]
> +then
> +	log "INFO" "Cant find abi-compliance-checker utility"
> +	cleanup_and_exit 1
> +fi
> +
> +ABIDUMP=`which abi-dumper 2>/dev/null`
> +if [ $? -ne 0 ]
> +then
> +	log "INFO" "Cant find abi-dumper utility"
> +	cleanup_and_exit 1
> +fi
> +
> +log "INFO" "We're going to check and make sure that applications built"
> +log "INFO" "against DPDK DSOs from tag $TAG1 will still run when executed"
> +log "INFO" "against DPDK DSOs built from tag $TAG2."
> +log "INFO" ""
> +
> +# Check to make sure we have a clean tree
> +git status | grep -q clean
> +if [ $? -ne 0 ]
> +then
> +	log "WARN" "Working directory not clean, aborting"
> +	cleanup_and_exit 1
> +fi
> +
> +if [ ! -d ./.git ]
> +then
> +	log "WARN" "You must be in the root of the dpdk git tree"
> +	log "WARN" "You are in $PWD"
> +	cleanup_and_exit 1
> +fi
> +
> +log "INFO" "Checking out version $TAG1 of the dpdk"
> +# Move to the old version of the tree
> +git checkout $TAG1
> +
> +# Make sure we configure SHARED libraries
> +# Also turn off IGB and KNI as those require kernel headers to build
> +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" config/defconfig_$TARGET
> +sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET
> +sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET
> +
> +export EXTRA_CFLAGS=-g
> +export EXTRA_LDFLAGS=-g
> +
> +# Now configure the build
> +log "INFO" "Configuring DPDK $TAG1"
> +make config T=$TARGET O=$TARGET > $VERBOSE 2>&1
> +
> +log "INFO" "Building DPDK $TAG1. This might take a moment"
> +make O=$TARGET > $VERBOSE 2>&1
> +
> +if [ $? -ne 0 ]
> +then
> +	log "INFO" "THE BUILD FAILED.  ABORTING"
> +	cleanup_and_exit 1
> +fi
> +
> +# Move to the lib directory
> +cd $TARGET/lib
> +log "INFO" "COLLECTING ABI INFORMATION FOR $TAG1"
> +for i in `ls *.so`
> +do
> +	$ABIDUMP $i -o $ABI_DIR/$i-ABI-0.dump -lver $TAG1
> +done
> +cd ../..
> +
> +# Now clean the tree, checkout the second tag, and rebuild
> +git clean -f -d
> +git reset --hard
> +# Move to the new version of the tree
> +log "INFO" "Checking out version $TAG2 of the dpdk"
> +git checkout $TAG2
> +
> +export EXTRA_CFLAGS=-g
> +export EXTRA_LDFLAGS=-g
> +
> +# Make sure we configure SHARED libraries
> +# Also turn off IGB and KNI as those require kernel headers to build
> +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" config/defconfig_$TARGET
> +sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET
> +sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET
> +
> +# Now configure the build
> +log "INFO" "Configuring DPDK $TAG2"
> +make config T=$TARGET O=$TARGET > $VERBOSE 2>&1
> +
> +log "INFO" "Building DPDK $TAG2. This might take a moment"
> +make O=$TARGET > $VERBOSE 2>&1
> +
> +if [ $? -ne 0 ]
> +then
> +	log "INFO" "THE BUILD FAILED.  ABORTING"
> +	cleanup_and_exit 1
> +fi
> +
> +cd $TARGET/lib
> +log "INFO" "COLLECTING ABI INFORMATION FOR $TAG2"
> +for i in `ls *.so`
> +do
> +	$ABIDUMP $i -o $ABI_DIR/$i-ABI-1.dump -lver $TAG2
> +done
> +cd ../..
> +
> +# Start comparison of ABI dumps
> +for i in `ls $ABI_DIR/*-1.dump`
> +do
> +	NEWNAME=`basename $i`
> +	OLDNAME=`basename $i | sed -e"s/1.dump/0.dump/"`
> +	LIBNAME=`basename $i | sed -e"s/-ABI-1.dump//"`
> +
> +	if [ ! -f $ABI_DIR/$OLDNAME ]
> +	then
> +		log "INFO" "$OLDNAME DOES NOT EXIST IN $TAG1. SKIPPING..."
> +	fi
> +
> +	#compare the abi dumps
> +	$ABICHECK -l $LIBNAME -old $ABI_DIR/$OLDNAME -new $ABI_DIR/$NEWNAME
> +done
> +
> +git reset --hard
> +git checkout $CURRENT_BRANCH
> +log "INFO" "ABI CHECK COMPLETE.  REPORTS ARE IN compat_report directory"
> +cleanup_and_exit 0
> +
> +
> -- 
> 2.1.0
> 
> 
Whats the disposition of this, I don't see it in the repository yet.

Neil
  

Hi Neil,

2015-02-27 08:48, Neil Horman:
> On Mon, Feb 02, 2015 at 01:18:26PM -0500, Neil Horman wrote:
> > There was a request for an abi validation utiltyfor the ongoing ABI stability
> > work.  As it turns out there is a abi compliance checker in development that
> > seems to be under active development and provides fairly detailed ABI compliance
> > reports.  Its not yet intellegent enough to understand symbol versioning, but it
> > does provide the ability to identify symbols which have changed between
> > releases, along with details of the change, and offers develoeprs the
> > opportunity to identify which symbols then need versioning and validation for a
> > given update via manaul testing.
> > 
> > This script automates the use of the compliance checker between two arbitrarily
> > specified tags within the dpdk tree.  To execute enter the $RTE_SDK directory
> > and run:
> > 
> > ./scripts/validate_abi.sh $GIT_TAG1 $GIT_TAG2 $CONFIG
> > 
> > where $GIT_TAG1 and 2 are git tags and $CONFIG is a config specification
> > suitable for passing as the T= variable in the make config command.
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> Whats the disposition of this, I don't see it in the repository yet.

I plan to review it carefully during next week.
We can integrate this tool until end of March, that's why it was not the
highest priority.
  

2015-02-02 13:18, Neil Horman:
> There was a request for an abi validation utiltyfor the ongoing ABI stability
> work.  As it turns out there is a abi compliance checker in development that
> seems to be under active development and provides fairly detailed ABI compliance
> reports.  Its not yet intellegent enough to understand symbol versioning, but it
> does provide the ability to identify symbols which have changed between
> releases, along with details of the change, and offers develoeprs the
> opportunity to identify which symbols then need versioning and validation for a
> given update via manaul testing.

There's a lot of typos in this text. Please check.

> This script automates the use of the compliance checker between two arbitrarily
> specified tags within the dpdk tree.  To execute enter the $RTE_SDK directory
> and run:
> 
> ./scripts/validate_abi.sh $GIT_TAG1 $GIT_TAG2 $CONFIG
> 
> where $GIT_TAG1 and 2 are git tags and $CONFIG is a config specification
> suitable for passing as the T= variable in the make config command.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> Change Notes:
> 
> v2) Fixed some typos as requested by Thomas
> ---
>  scripts/validate_abi.sh | 241 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 241 insertions(+)
>  create mode 100755 scripts/validate_abi.sh
> 
> diff --git a/scripts/validate_abi.sh b/scripts/validate_abi.sh
> new file mode 100755
> index 0000000..31583df
> --- /dev/null
> +++ b/scripts/validate_abi.sh
> @@ -0,0 +1,241 @@
> +#!/bin/sh
> +#   BSD LICENSE
> +#
> +#   Copyright(c) 2015 Neil Horman. All rights reserved.
> +#   All rights reserved.
> +#
> +#   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.
> +#
> +#   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.
> +
> +TAG1=$1
> +TAG2=$2
> +TARGET=$3
> +ABI_DIR=`mktemp -d -p /tmp ABI.XXXXXX`
> +
> +usage() {
> +	echo "$0 <TAG1> <TAG2> <TARGET>"
> +}
> +
> +log() {
> +	local level=$1

level is not used later?

> +	shift
> +	echo "$*"
> +}
> +
> +validate_tags() {
> +	git tag -l | grep -q "$TAG1"
> +	if [ $? -ne 0 ]
> +	then
> +		echo "$TAG1 is invalid"
> +		return
> +	fi
> +	git tag -l | grep -q "$TAG2"
> +	if [ $? -ne 0 ]
> +	then
> +		echo "$TAG2 is invalid"
> +		return
> +	fi
> +}
> +
> +validate_args() {
> +	if [ -z "$TAG1" ]
> +	then
> +		echo "Must Specify TAG1"
> +		return
> +	fi
> +	if [ -z "$TAG2" ]
> +	then
> +		echo "Must Specify TAG2"
> +		return
> +	fi
> +	if [ -z "$TARGET" ]
> +	then
> +		echo "Must Specify a build target"
> +	fi
> +}
> +
> +
> +cleanup_and_exit() {
> +	rm -rf $ABI_DIR
> +	exit $1
> +}

This function could be automatically invoked with trap.

> +###########################################
> +#START
> +############################################
> +
> +#Save the current branch
> +CURRENT_BRANCH=`git branch | grep \* | cut -d' ' -f2`

Will it work when not on any branch?

> +
> +if [ -n "$VERBOSE" ]
> +then
> +	export VERBOSE=/dev/stdout
> +else
> +	export VERBOSE=/dev/null
> +fi
> +
> +# Validate that we have all the arguments we need
> +res=$(validate_args)
> +if [ -n "$res" ]
> +then
> +	echo $res

Should be redirected to stderr >&2

> +	usage
> +	cleanup_and_exit 1
> +fi
> +
> +# Make sure our tags exist
> +res=$(validate_tags)
> +if [ -n "$res" ]
> +then
> +	echo $res
> +	cleanup_and_exit 1
> +fi
> +
> +ABICHECK=`which abi-compliance-checker 2>/dev/null`

Why not using the $() form like above?

I guess this is the tool:
	http://ispras.linuxbase.org/index.php/ABI_compliance_checker

> +if [ $? -ne 0 ]
> +then
> +	log "INFO" "Cant find abi-compliance-checker utility"
> +	cleanup_and_exit 1
> +fi
> +
> +ABIDUMP=`which abi-dumper 2>/dev/null`
> +if [ $? -ne 0 ]
> +then
> +	log "INFO" "Cant find abi-dumper utility"
> +	cleanup_and_exit 1
> +fi
> +
> +log "INFO" "We're going to check and make sure that applications built"
> +log "INFO" "against DPDK DSOs from tag $TAG1 will still run when executed"
> +log "INFO" "against DPDK DSOs built from tag $TAG2."
> +log "INFO" ""
> +
> +# Check to make sure we have a clean tree
> +git status | grep -q clean
> +if [ $? -ne 0 ]
> +then

You may compact in one line:
if git status | grep -q clean ; then

> +	log "WARN" "Working directory not clean, aborting"
> +	cleanup_and_exit 1
> +fi
> +
> +if [ ! -d ./.git ]
> +then
> +	log "WARN" "You must be in the root of the dpdk git tree"
> +	log "WARN" "You are in $PWD"
> +	cleanup_and_exit 1
> +fi

Why not cd $(dirname $0)/.. instead of returning an error?

> +log "INFO" "Checking out version $TAG1 of the dpdk"
> +# Move to the old version of the tree
> +git checkout $TAG1
> +
> +# Make sure we configure SHARED libraries
> +# Also turn off IGB and KNI as those require kernel headers to build
> +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" config/defconfig_$TARGET
> +sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET
> +sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET

Why not tuning configuration after make config in .config file?

> +export EXTRA_CFLAGS=-g
> +export EXTRA_LDFLAGS=-g

A comment is required (needed for abi-dumper?)

> +# Now configure the build
> +log "INFO" "Configuring DPDK $TAG1"
> +make config T=$TARGET O=$TARGET > $VERBOSE 2>&1
> +
> +log "INFO" "Building DPDK $TAG1. This might take a moment"
> +make O=$TARGET > $VERBOSE 2>&1

It would more efficient with a customizable -j option

> +if [ $? -ne 0 ]
> +then
> +	log "INFO" "THE BUILD FAILED.  ABORTING"
> +	cleanup_and_exit 1
> +fi
> +
> +# Move to the lib directory
> +cd $TARGET/lib
> +log "INFO" "COLLECTING ABI INFORMATION FOR $TAG1"
> +for i in `ls *.so`

I think ls is useless.

> +do
> +	$ABIDUMP $i -o $ABI_DIR/$i-ABI-0.dump -lver $TAG1
> +done
> +cd ../..
> +
> +# Now clean the tree, checkout the second tag, and rebuild
> +git clean -f -d
> +git reset --hard
> +# Move to the new version of the tree
> +log "INFO" "Checking out version $TAG2 of the dpdk"
> +git checkout $TAG2
> +
> +export EXTRA_CFLAGS=-g
> +export EXTRA_LDFLAGS=-g
> +
> +# Make sure we configure SHARED libraries
> +# Also turn off IGB and KNI as those require kernel headers to build
> +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" config/defconfig_$TARGET
> +sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET
> +sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET
> +
> +# Now configure the build
> +log "INFO" "Configuring DPDK $TAG2"
> +make config T=$TARGET O=$TARGET > $VERBOSE 2>&1
> +
> +log "INFO" "Building DPDK $TAG2. This might take a moment"
> +make O=$TARGET > $VERBOSE 2>&1
> +
> +if [ $? -ne 0 ]
> +then
> +	log "INFO" "THE BUILD FAILED.  ABORTING"
> +	cleanup_and_exit 1
> +fi
> +
> +cd $TARGET/lib
> +log "INFO" "COLLECTING ABI INFORMATION FOR $TAG2"
> +for i in `ls *.so`
> +do
> +	$ABIDUMP $i -o $ABI_DIR/$i-ABI-1.dump -lver $TAG2
> +done
> +cd ../..
> +
> +# Start comparison of ABI dumps
> +for i in `ls $ABI_DIR/*-1.dump`

Why ls?

> +do
> +	NEWNAME=`basename $i`
> +	OLDNAME=`basename $i | sed -e"s/1.dump/0.dump/"`
> +	LIBNAME=`basename $i | sed -e"s/-ABI-1.dump//"`
> +
> +	if [ ! -f $ABI_DIR/$OLDNAME ]
> +	then
> +		log "INFO" "$OLDNAME DOES NOT EXIST IN $TAG1. SKIPPING..."
> +	fi
> +
> +	#compare the abi dumps
> +	$ABICHECK -l $LIBNAME -old $ABI_DIR/$OLDNAME -new $ABI_DIR/$NEWNAME

Do we need to do a visual check? I didn't try yet.

> +done
> +
> +git reset --hard
> +git checkout $CURRENT_BRANCH
> +log "INFO" "ABI CHECK COMPLETE.  REPORTS ARE IN compat_report directory"
> +cleanup_and_exit 0

So you compare the ABI dumps.
Do we also need to run an app from TAG2 with libs from TAG1?

Thanks Neil
  

On Tue, Mar 03, 2015 at 11:18:47PM +0100, Thomas Monjalon wrote:
> 2015-02-02 13:18, Neil Horman:
> > There was a request for an abi validation utiltyfor the ongoing ABI stability
> > work.  As it turns out there is a abi compliance checker in development that
> > seems to be under active development and provides fairly detailed ABI compliance
> > reports.  Its not yet intellegent enough to understand symbol versioning, but it
> > does provide the ability to identify symbols which have changed between
> > releases, along with details of the change, and offers develoeprs the
> > opportunity to identify which symbols then need versioning and validation for a
> > given update via manaul testing.
> 
> There's a lot of typos in this text. Please check.
> 
Three.  Theres 3 typos.  But sure, I'll fix them.

><snip>
> > +
> > +usage() {
> > +	echo "$0 <TAG1> <TAG2> <TARGET>"
> > +}
> > +
> > +log() {
> > +	local level=$1
> 
> level is not used later?
> 
Not yet, but you'll note all the log calls start with a log level to add
filtering.  I'd rather leave this here as it doesn't hurt anything and
effectively documents the paramter.

><snip>
> > +	shift
> > +	echo "$*"
> > +}
> > +
> > +validate_tags() {
> > +	git tag -l | grep -q "$TAG1"
> > +	if [ $? -ne 0 ]
> > +	then
> > +		echo "$TAG1 is invalid"
> > +		return
> > +	fi
> > +	git tag -l | grep -q "$TAG2"
> > +	if [ $? -ne 0 ]
> > +	then
> > +		echo "$TAG2 is invalid"
> > +		return
> > +	fi
> > +}
> > +
> > +validate_args() {
> > +	if [ -z "$TAG1" ]
> > +	then
> > +		echo "Must Specify TAG1"
> > +		return
> > +	fi
> > +	if [ -z "$TAG2" ]
> > +	then
> > +		echo "Must Specify TAG2"
> > +		return
> > +	fi
> > +	if [ -z "$TARGET" ]
> > +	then
> > +		echo "Must Specify a build target"
> > +	fi
> > +}
> > +
> > +
> > +cleanup_and_exit() {
> > +	rm -rf $ABI_DIR
> > +	exit $1
> > +}
> 
> This function could be automatically invoked with trap.
> 
Yes, I can add that.

> > +###########################################
> > +#START
> > +############################################
> > +
> > +#Save the current branch
> > +CURRENT_BRANCH=`git branch | grep \* | cut -d' ' -f2`
> 
> Will it work when not on any branch?
> 
No it won't, and I honestly wasn't that worried about it, as people
don't/shouldn't make changes in detached head state.  I can add a check to
ensure you're on a branch though.

> > +
> > +if [ -n "$VERBOSE" ]
> > +then
> > +	export VERBOSE=/dev/stdout
> > +else
> > +	export VERBOSE=/dev/null
> > +fi
> > +
> > +# Validate that we have all the arguments we need
> > +res=$(validate_args)
> > +if [ -n "$res" ]
> > +then
> > +	echo $res
> 
> Should be redirected to stderr >&2
> 
Why? this is eactly what I intended.  All the other messages from log are
directed to stdout, so should this be.

> > +	usage
> > +	cleanup_and_exit 1
> > +fi
> > +
> > +# Make sure our tags exist
> > +res=$(validate_tags)
> > +if [ -n "$res" ]
> > +then
> > +	echo $res
> > +	cleanup_and_exit 1
> > +fi
> > +
> > +ABICHECK=`which abi-compliance-checker 2>/dev/null`
> 
> Why not using the $() form like above?
> 
I don't honestly recall, but I do remember fighting trying to get output from
that format for some reason, and just left this as it was, as it wasn't
particularly relevant.

> I guess this is the tool:
> 	http://ispras.linuxbase.org/index.php/ABI_compliance_checker
> 
Correct.

> > +if [ $? -ne 0 ]
> > +then
> > +	log "INFO" "Cant find abi-compliance-checker utility"
> > +	cleanup_and_exit 1
> > +fi
> > +
> > +ABIDUMP=`which abi-dumper 2>/dev/null`
> > +if [ $? -ne 0 ]
> > +then
> > +	log "INFO" "Cant find abi-dumper utility"
> > +	cleanup_and_exit 1
> > +fi
> > +
> > +log "INFO" "We're going to check and make sure that applications built"
> > +log "INFO" "against DPDK DSOs from tag $TAG1 will still run when executed"
> > +log "INFO" "against DPDK DSOs built from tag $TAG2."
> > +log "INFO" ""
> > +
> > +# Check to make sure we have a clean tree
> > +git status | grep -q clean
> > +if [ $? -ne 0 ]
> > +then
> 
> You may compact in one line:
> if git status | grep -q clean ; then
> 
I explicitly do execution and error checking on separate lines as I think its
more clear.  You'll find this style consistent in the script.

> > +	log "WARN" "Working directory not clean, aborting"
> > +	cleanup_and_exit 1
> > +fi
> > +
> > +if [ ! -d ./.git ]
> > +then
> > +	log "WARN" "You must be in the root of the dpdk git tree"
> > +	log "WARN" "You are in $PWD"
> > +	cleanup_and_exit 1
> > +fi
> 
> Why not cd $(dirname $0)/.. instead of returning an error?
> 
Why would that help in finding the base of the git tree.  Theres no guarantee
that you are in a subdirectory of a git tree.  I suppose we can try it
recursively until we hit /, but it seems just as easy and clear to tell the user
whats needed.

> > +log "INFO" "Checking out version $TAG1 of the dpdk"
> > +# Move to the old version of the tree
> > +git checkout $TAG1
> > +
> > +# Make sure we configure SHARED libraries
> > +# Also turn off IGB and KNI as those require kernel headers to build
> > +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" config/defconfig_$TARGET
> > +sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET
> > +sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET
> 
> Why not tuning configuration after make config in .config file?
> 
Because this way we save a reconfig (from a developer viewpoint), you should run
make config again after changing configs, and so this way you save doing that.

> > +export EXTRA_CFLAGS=-g
> > +export EXTRA_LDFLAGS=-g
> 
> A comment is required (needed for abi-dumper?)
> 
Sure.

> > +# Now configure the build
> > +log "INFO" "Configuring DPDK $TAG1"
> > +make config T=$TARGET O=$TARGET > $VERBOSE 2>&1
> > +
> > +log "INFO" "Building DPDK $TAG1. This might take a moment"
> > +make O=$TARGET > $VERBOSE 2>&1
> 
> It would more efficient with a customizable -j option
> 
I'm sure it would, I'll look at that in future enhancement. 

> > +if [ $? -ne 0 ]
> > +then
> > +	log "INFO" "THE BUILD FAILED.  ABORTING"
> > +	cleanup_and_exit 1
> > +fi
> > +
> > +# Move to the lib directory
> > +cd $TARGET/lib
> > +log "INFO" "COLLECTING ABI INFORMATION FOR $TAG1"
> > +for i in `ls *.so`
> 
> I think ls is useless.
> 
Um, I don't?  Not sure what you're getting at here.

> > +do
> > +	$ABIDUMP $i -o $ABI_DIR/$i-ABI-0.dump -lver $TAG1
> > +done
> > +cd ../..
> > +
> > +# Now clean the tree, checkout the second tag, and rebuild
> > +git clean -f -d
> > +git reset --hard
> > +# Move to the new version of the tree
> > +log "INFO" "Checking out version $TAG2 of the dpdk"
> > +git checkout $TAG2
> > +
> > +export EXTRA_CFLAGS=-g
> > +export EXTRA_LDFLAGS=-g
> > +
> > +# Make sure we configure SHARED libraries
> > +# Also turn off IGB and KNI as those require kernel headers to build
> > +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" config/defconfig_$TARGET
> > +sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET
> > +sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET
> > +
> > +# Now configure the build
> > +log "INFO" "Configuring DPDK $TAG2"
> > +make config T=$TARGET O=$TARGET > $VERBOSE 2>&1
> > +
> > +log "INFO" "Building DPDK $TAG2. This might take a moment"
> > +make O=$TARGET > $VERBOSE 2>&1
> > +
> > +if [ $? -ne 0 ]
> > +then
> > +	log "INFO" "THE BUILD FAILED.  ABORTING"
> > +	cleanup_and_exit 1
> > +fi
> > +
> > +cd $TARGET/lib
> > +log "INFO" "COLLECTING ABI INFORMATION FOR $TAG2"
> > +for i in `ls *.so`
> > +do
> > +	$ABIDUMP $i -o $ABI_DIR/$i-ABI-1.dump -lver $TAG2
> > +done
> > +cd ../..
> > +
> > +# Start comparison of ABI dumps
> > +for i in `ls $ABI_DIR/*-1.dump`
> 
> Why ls?
> 
Because it preforms the needed action for what I want to do here. Not sure what
you're proposing

> > +do
> > +	NEWNAME=`basename $i`
> > +	OLDNAME=`basename $i | sed -e"s/1.dump/0.dump/"`
> > +	LIBNAME=`basename $i | sed -e"s/-ABI-1.dump//"`
> > +
> > +	if [ ! -f $ABI_DIR/$OLDNAME ]
> > +	then
> > +		log "INFO" "$OLDNAME DOES NOT EXIST IN $TAG1. SKIPPING..."
> > +	fi
> > +
> > +	#compare the abi dumps
> > +	$ABICHECK -l $LIBNAME -old $ABI_DIR/$OLDNAME -new $ABI_DIR/$NEWNAME
> 
> Do we need to do a visual check? I didn't try yet.
> 
Yes, it generates an html report of all the symbols exported in a build and
compares them with the alternate version.  That needs manual review.

> > +done
> > +
> > +git reset --hard
> > +git checkout $CURRENT_BRANCH
> > +log "INFO" "ABI CHECK COMPLETE.  REPORTS ARE IN compat_report directory"
> > +cleanup_and_exit 0
> 
> So you compare the ABI dumps.
> Do we also need to run an app from TAG2 with libs from TAG1?
> 
I started down that path, but its not really that helpful, as all it will do is
refuse to run if there is a symbol missing from a later version.  While that
might be helpful, its no where near as through as the full report from the
compliance checker.

The bottom line is that real ABI compliance requires a developer to be aware of
the changes going into the code and how they affect binary layout. A simple
"does it still work" test isn't sufficient.

Neil
> Thanks Neil
>
  

Hi Neil,

I remove parts that I agree and reply to those which deserve more discussion.

2015-03-04 06:49, Neil Horman:
> On Tue, Mar 03, 2015 at 11:18:47PM +0100, Thomas Monjalon wrote:
> > 2015-02-02 13:18, Neil Horman:
> > > +# Validate that we have all the arguments we need
> > > +res=$(validate_args)
> > > +if [ -n "$res" ]
> > > +then
> > > +	echo $res
> > 
> > Should be redirected to stderr >&2
> > 
> Why? this is eactly what I intended.  All the other messages from log are
> directed to stdout, so should this be.

I'm wondering if there's some normal output which could be redirected for
further processing, and some error output.
My comment was not only for this log but also for every error message.

> > I guess this is the tool:
> > 	http://ispras.linuxbase.org/index.php/ABI_compliance_checker
> 
> Correct.

So maybe you should add this URL in the commit log.

> > > +log "INFO" "We're going to check and make sure that applications built"
> > > +log "INFO" "against DPDK DSOs from tag $TAG1 will still run when executed"
> > > +log "INFO" "against DPDK DSOs built from tag $TAG2."
> > > +log "INFO" ""

> > > +if [ ! -d ./.git ]
> > > +then
> > > +	log "WARN" "You must be in the root of the dpdk git tree"
> > > +	log "WARN" "You are in $PWD"
> > > +	cleanup_and_exit 1
> > > +fi
> > 
> > Why not cd $(dirname $0)/.. instead of returning an error?
> 
> Why would that help in finding the base of the git tree.  Theres no guarantee
> that you are in a subdirectory of a git tree.  I suppose we can try it
> recursively until we hit /, but it seems just as easy and clear to tell the user
> whats needed.

No I'm saying that you could avoid this check by going into the right
directory from the beginning.
We know that the root dir is $(dirname $0)/.. because this script is in
scripts/ directory.

> > > +# Make sure we configure SHARED libraries
> > > +# Also turn off IGB and KNI as those require kernel headers to build
> > > +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" config/defconfig_$TARGET
> > > +sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET
> > > +sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET
> > 
> > Why not tuning configuration after make config in .config file?
> > 
> Because this way we save a reconfig (from a developer viewpoint), you should run
> make config again after changing configs, and so this way you save doing that.

No, you run make config once and update .config file. That's the recommended
way to configure DPDK.
defconfig files are default configurations and should stay read-only.

> > > +for i in `ls *.so`
> > 
> > I think ls is useless.
> > 
> Um, I don't?  Not sure what you're getting at here.

I think "for i in *.so" should work.

> > > +	#compare the abi dumps
> > > +	$ABICHECK -l $LIBNAME -old $ABI_DIR/$OLDNAME -new $ABI_DIR/$NEWNAME
> > 
> > Do we need to do a visual check? I didn't try yet.
> > 
> Yes, it generates an html report of all the symbols exported in a build and
> compares them with the alternate version.  That needs manual review.

OK I think it's important to explain in the commit log.

> > So you compare the ABI dumps.
> > Do we also need to run an app from TAG2 with libs from TAG1?
> 
> I started down that path, but its not really that helpful, as all it will do is
> refuse to run if there is a symbol missing from a later version.  While that
> might be helpful, its no where near as through as the full report from the
> compliance checker.
> 
> The bottom line is that real ABI compliance requires a developer to be aware of
> the changes going into the code and how they affect binary layout. A simple
> "does it still work" test isn't sufficient.

I hope we'll be able to integrate this kind of tool in an automated sanity
check in order to find obvious errors.

Thanks
  

On Wed, Mar 04, 2015 at 01:54:49PM +0100, Thomas Monjalon wrote:
> Hi Neil,
> 
> I remove parts that I agree and reply to those which deserve more discussion.
> 
> 2015-03-04 06:49, Neil Horman:
> > On Tue, Mar 03, 2015 at 11:18:47PM +0100, Thomas Monjalon wrote:
> > > 2015-02-02 13:18, Neil Horman:
> > > > +# Validate that we have all the arguments we need
> > > > +res=$(validate_args)
> > > > +if [ -n "$res" ]
> > > > +then
> > > > +	echo $res
> > > 
> > > Should be redirected to stderr >&2
> > > 
> > Why? this is eactly what I intended.  All the other messages from log are
> > directed to stdout, so should this be.
> 
> I'm wondering if there's some normal output which could be redirected for
> further processing, and some error output.
> My comment was not only for this log but also for every error message.
> 

No, the report output is in html format and always to a file, so stdout isn't
used for any inline informational reporting.

> > > I guess this is the tool:
> > > 	http://ispras.linuxbase.org/index.php/ABI_compliance_checker
> > 
> > Correct.
> 
> So maybe you should add this URL in the commit log.
> 
sure, fine.

> > > > +log "INFO" "We're going to check and make sure that applications built"
> > > > +log "INFO" "against DPDK DSOs from tag $TAG1 will still run when executed"
> > > > +log "INFO" "against DPDK DSOs built from tag $TAG2."
> > > > +log "INFO" ""
> 
> > > > +if [ ! -d ./.git ]
> > > > +then
> > > > +	log "WARN" "You must be in the root of the dpdk git tree"
> > > > +	log "WARN" "You are in $PWD"
> > > > +	cleanup_and_exit 1
> > > > +fi
> > > 
> > > Why not cd $(dirname $0)/.. instead of returning an error?
> > 
> > Why would that help in finding the base of the git tree.  Theres no guarantee
> > that you are in a subdirectory of a git tree.  I suppose we can try it
> > recursively until we hit /, but it seems just as easy and clear to tell the user
> > whats needed.
> 
> No I'm saying that you could avoid this check by going into the right
> directory from the beginning.
> We know that the root dir is $(dirname $0)/.. because this script is in
> scripts/ directory.
> 
That only helps if you start from the right directory.  If you run this command
from some other location, your solution just breaks.

> > > > +# Make sure we configure SHARED libraries
> > > > +# Also turn off IGB and KNI as those require kernel headers to build
> > > > +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" config/defconfig_$TARGET
> > > > +sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET
> > > > +sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET
> > > 
> > > Why not tuning configuration after make config in .config file?
> > > 
> > Because this way we save a reconfig (from a developer viewpoint), you should run
> > make config again after changing configs, and so this way you save doing that.
> 
> No, you run make config once and update .config file. That's the recommended
> way to configure DPDK.
> defconfig files are default configurations and should stay read-only.
They get overwritten when we do the git resets.  Its silly to modify your config
file after you run make config, in the event the make target has to re-read any
modified options and adjust dependent config files accordingly.  I understand
that doesn't happen now, but its common practice for every open source project
in existance.

> 
> > > > +for i in `ls *.so`
> > > 
> > > I think ls is useless.
> > > 
> > Um, I don't?  Not sure what you're getting at here.
> 
> I think "for i in *.so" should work.
> 
Then its irrelevant in my mind.  They both work equally well.


> > > > +	#compare the abi dumps
> > > > +	$ABICHECK -l $LIBNAME -old $ABI_DIR/$OLDNAME -new $ABI_DIR/$NEWNAME
> > > 
> > > Do we need to do a visual check? I didn't try yet.
> > > 
> > Yes, it generates an html report of all the symbols exported in a build and
> > compares them with the alternate version.  That needs manual review.
> 
> OK I think it's important to explain in the commit log.
Ok.

> 
> > > So you compare the ABI dumps.
> > > Do we also need to run an app from TAG2 with libs from TAG1?
> > 
> > I started down that path, but its not really that helpful, as all it will do is
> > refuse to run if there is a symbol missing from a later version.  While that
> > might be helpful, its no where near as through as the full report from the
> > compliance checker.
> > 
> > The bottom line is that real ABI compliance requires a developer to be aware of
> > the changes going into the code and how they affect binary layout. A simple
> > "does it still work" test isn't sufficient.
> 
> I hope we'll be able to integrate this kind of tool in an automated sanity
> check in order to find obvious errors.
> 
> Thanks
>
  

2015-03-04 09:39, Neil Horman:
> On Wed, Mar 04, 2015 at 01:54:49PM +0100, Thomas Monjalon wrote:
> > Hi Neil,
> > 
> > I remove parts that I agree and reply to those which deserve more discussion.
> > 
> > 2015-03-04 06:49, Neil Horman:
> > > On Tue, Mar 03, 2015 at 11:18:47PM +0100, Thomas Monjalon wrote:
> > > > 2015-02-02 13:18, Neil Horman:
> > > > > +# Validate that we have all the arguments we need
> > > > > +if [ ! -d ./.git ]
> > > > > +then
> > > > > +	log "WARN" "You must be in the root of the dpdk git tree"
> > > > > +	log "WARN" "You are in $PWD"
> > > > > +	cleanup_and_exit 1
> > > > > +fi
> > > > 
> > > > Why not cd $(dirname $0)/.. instead of returning an error?
> > > 
> > > Why would that help in finding the base of the git tree.  Theres no guarantee
> > > that you are in a subdirectory of a git tree.  I suppose we can try it
> > > recursively until we hit /, but it seems just as easy and clear to tell the user
> > > whats needed.
> > 
> > No I'm saying that you could avoid this check by going into the right
> > directory from the beginning.
> > We know that the root dir is $(dirname $0)/.. because this script is in
> > scripts/ directory.
> > 
> That only helps if you start from the right directory.  If you run this command
> from some other location, your solution just breaks.

Why it would break? $(dirname $0) is always reachable because you launched $0.
The only exception is for the case the PATH variable is used to find the DPDK
scripts/ directory (should not happen).

> > > > > +# Make sure we configure SHARED libraries
> > > > > +# Also turn off IGB and KNI as those require kernel headers to build
> > > > > +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" config/defconfig_$TARGET
> > > > > +sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET
> > > > > +sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET
> > > > 
> > > > Why not tuning configuration after make config in .config file?
> > > > 
> > > Because this way we save a reconfig (from a developer viewpoint), you should run
> > > make config again after changing configs, and so this way you save doing that.
> > 
> > No, you run make config once and update .config file. That's the recommended
> > way to configure DPDK.
> > defconfig files are default configurations and should stay read-only.
> 
> They get overwritten when we do the git resets.  Its silly to modify your config
> file after you run make config, in the event the make target has to re-read any
> modified options and adjust dependent config files accordingly.  I understand
> that doesn't happen now, but its common practice for every open source project
> in existance.

I'm not sure to understand. Maybe an example would help.
By the way, your method works.
  

On Wed, Mar 04, 2015 at 04:15:18PM +0100, Thomas Monjalon wrote:
> 2015-03-04 09:39, Neil Horman:
> > On Wed, Mar 04, 2015 at 01:54:49PM +0100, Thomas Monjalon wrote:
> > > Hi Neil,
> > > 
> > > I remove parts that I agree and reply to those which deserve more discussion.
> > > 
> > > 2015-03-04 06:49, Neil Horman:
> > > > On Tue, Mar 03, 2015 at 11:18:47PM +0100, Thomas Monjalon wrote:
> > > > > 2015-02-02 13:18, Neil Horman:
> > > > > > +# Validate that we have all the arguments we need
> > > > > > +if [ ! -d ./.git ]
> > > > > > +then
> > > > > > +	log "WARN" "You must be in the root of the dpdk git tree"
> > > > > > +	log "WARN" "You are in $PWD"
> > > > > > +	cleanup_and_exit 1
> > > > > > +fi
> > > > > 
> > > > > Why not cd $(dirname $0)/.. instead of returning an error?
> > > > 
> > > > Why would that help in finding the base of the git tree.  Theres no guarantee
> > > > that you are in a subdirectory of a git tree.  I suppose we can try it
> > > > recursively until we hit /, but it seems just as easy and clear to tell the user
> > > > whats needed.
> > > 
> > > No I'm saying that you could avoid this check by going into the right
> > > directory from the beginning.
> > > We know that the root dir is $(dirname $0)/.. because this script is in
> > > scripts/ directory.
> > > 
> > That only helps if you start from the right directory.  If you run this command
> > from some other location, your solution just breaks.
> 
> Why it would break? $(dirname $0) is always reachable because you launched $0.
> The only exception is for the case the PATH variable is used to find the DPDK
> scripts/ directory (should not happen).
> 
Ah!  Sorry, misunderstood, for some reason I was conflating $0 with $PWD.  Yes,
this will work and I'll update it

> > > > > > +# Make sure we configure SHARED libraries
> > > > > > +# Also turn off IGB and KNI as those require kernel headers to build
> > > > > > +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" config/defconfig_$TARGET
> > > > > > +sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET
> > > > > > +sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET
> > > > > 
> > > > > Why not tuning configuration after make config in .config file?
> > > > > 
> > > > Because this way we save a reconfig (from a developer viewpoint), you should run
> > > > make config again after changing configs, and so this way you save doing that.
> > > 
> > > No, you run make config once and update .config file. That's the recommended
> > > way to configure DPDK.
> > > defconfig files are default configurations and should stay read-only.
> > 
> > They get overwritten when we do the git resets.  Its silly to modify your config
> > file after you run make config, in the event the make target has to re-read any
> > modified options and adjust dependent config files accordingly.  I understand
> > that doesn't happen now, but its common practice for every open source project
> > in existance.
> 
> I'm not sure to understand. Maybe an example would help.
> By the way, your method works.
For example, the linux kernel.  The .config file that is generated in the root
directory is converted to an autoconf.h in parallel with its generation, for
applications to key off of.  If you change something in .config, you need to run
make config again so that those changes are reflected into the other
auto-generated files.  Thats common practice.  So its counter intuitive to
assume that altering the generated .config file is automatically recognized by
the rest of the build, without a subsequent make config (be it explicit or and
implicit dependency of the make all target).

Neil


> 
>
  

2015-03-04 10:42, Neil Horman:
> On Wed, Mar 04, 2015 at 04:15:18PM +0100, Thomas Monjalon wrote:
> > 2015-03-04 09:39, Neil Horman:
> > > On Wed, Mar 04, 2015 at 01:54:49PM +0100, Thomas Monjalon wrote:
> > > > Hi Neil,
> > > > 
> > > > I remove parts that I agree and reply to those which deserve more discussion.
> > > > 
> > > > 2015-03-04 06:49, Neil Horman:
> > > > > On Tue, Mar 03, 2015 at 11:18:47PM +0100, Thomas Monjalon wrote:
> > > > > > 2015-02-02 13:18, Neil Horman:
> > > > > > > +# Make sure we configure SHARED libraries
> > > > > > > +# Also turn off IGB and KNI as those require kernel headers to build
> > > > > > > +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" config/defconfig_$TARGET
> > > > > > > +sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET
> > > > > > > +sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET
> > > > > > 
> > > > > > Why not tuning configuration after make config in .config file?
> > > > > > 
> > > > > Because this way we save a reconfig (from a developer viewpoint), you should run
> > > > > make config again after changing configs, and so this way you save doing that.
> > > > 
> > > > No, you run make config once and update .config file. That's the recommended
> > > > way to configure DPDK.
> > > > defconfig files are default configurations and should stay read-only.
> > > 
> > > They get overwritten when we do the git resets.  Its silly to modify your config
> > > file after you run make config, in the event the make target has to re-read any
> > > modified options and adjust dependent config files accordingly.  I understand
> > > that doesn't happen now, but its common practice for every open source project
> > > in existance.
> > 
> > I'm not sure to understand. Maybe an example would help.
> > By the way, your method works.
> 
> For example, the linux kernel.  The .config file that is generated in the root
> directory is converted to an autoconf.h in parallel with its generation, for
> applications to key off of.  If you change something in .config, you need to run
> make config again so that those changes are reflected into the other
> auto-generated files.  Thats common practice.  So its counter intuitive to
> assume that altering the generated .config file is automatically recognized by
> the rest of the build, without a subsequent make config (be it explicit or and
> implicit dependency of the make all target).

OK thanks, now I better understand how you think about DPDK config.
Note that in Linux you are modifying .config, not the defconfig.
I'm not going to debate how it could be improved now but I think we shouldn't
dynamically modify defconfig files to avoid confusion about their purpose.