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

[Thomas Monjalon <thomas.monjalon@6wind.com>]

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