[dpdk-dev,v3] devtools: rework abi checker script

Message ID 20170911084635.11707-1-olivier.matz@6wind.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Olivier Matz Sept. 11, 2017, 8:46 a.m. UTC
  The initial version of the script had some limitations:
- cannot work on a non-clean workspace
- environment variables are not documented
- no compilation log in case of failure
- return success even it abi is incompatible

This patch addresses these issues and rework the code.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---

v2->v3:
- fix when not launched from dpdk root dir
- use "-Og -Wno-error" instead of "-O0"
- fix typo in commit log

v1->v2:
- use /usr/bin/env to find bash (which is required)
- fix displayed path to html reports
- reword help for -f option

 devtools/validate-abi.sh | 392 ++++++++++++++++++++++++-----------------------
 1 file changed, 200 insertions(+), 192 deletions(-)
  

Comments

Neil Horman Sept. 13, 2017, 3 p.m. UTC | #1
On Mon, Sep 11, 2017 at 10:46:35AM +0200, Olivier Matz wrote:
> The initial version of the script had some limitations:
> - cannot work on a non-clean workspace
> - environment variables are not documented
> - no compilation log in case of failure
> - return success even it abi is incompatible
> 
> This patch addresses these issues and rework the code.
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
> 
> v2->v3:
> - fix when not launched from dpdk root dir
> - use "-Og -Wno-error" instead of "-O0"
> - fix typo in commit log
> 
> v1->v2:
> - use /usr/bin/env to find bash (which is required)
> - fix displayed path to html reports
> - reword help for -f option
> 
>  devtools/validate-abi.sh | 392 ++++++++++++++++++++++++-----------------------
>  1 file changed, 200 insertions(+), 192 deletions(-)
> 

This is alot closer, I think theres just a little wierdness left.  When running
this checker script, the end of the log shows this:

CMD: abi-compliance-checker -l librte_vhost.so -old /home/nhorman/git/dpdk/devtools/abi-check/222555480/librte_vhost.so.dump -new /home/nhorman/git/dpdk/devtools/abi-check/02657b4ad/librte_vhost.so.dump
NOTICE: At least one call to abi-compliance-checker returned an error.
NOTICE: ABI may be incompatible, please check logs for details.
NOTICE: Incompatible list:  librte_cryptodev.so librte_eal.so librte_efd.so librte_ethdev.so librte_eventdev.so librte_hash.so librte_pdump.so librte_pmd_crypto_scheduler.so librte_pmd_ring.so librte_ring.so

Which I think is something of a false positive.  The line:
NOTICE: At least one call to abi-compliance-checker returned an error.

I think is emitted simply because abi-compilance-checker returns non-zero and
reports that error if it finds any incompatibilities.  I'm not sure we want to
flag that as an error per se.  It gives the impression something has gone wrong,
rather than correctly identifying that there are incompatibilities.

Though on the other hand, maybe we do want to set that red flag so people look
at what the incompatibilities are.  As I say it out loud, perhaps that
preferable.

Thoughts
Neil
  
Olivier Matz Sept. 19, 2017, 9:15 a.m. UTC | #2
Hi Neil,

On Wed, Sep 13, 2017 at 11:00:13AM -0400, Neil Horman wrote:
> On Mon, Sep 11, 2017 at 10:46:35AM +0200, Olivier Matz wrote:
> > The initial version of the script had some limitations:
> > - cannot work on a non-clean workspace
> > - environment variables are not documented
> > - no compilation log in case of failure
> > - return success even it abi is incompatible
> > 
> > This patch addresses these issues and rework the code.
> > 
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > ---
> > 
> > v2->v3:
> > - fix when not launched from dpdk root dir
> > - use "-Og -Wno-error" instead of "-O0"
> > - fix typo in commit log
> > 
> > v1->v2:
> > - use /usr/bin/env to find bash (which is required)
> > - fix displayed path to html reports
> > - reword help for -f option
> > 
> >  devtools/validate-abi.sh | 392 ++++++++++++++++++++++++-----------------------
> >  1 file changed, 200 insertions(+), 192 deletions(-)
> > 
> 
> This is alot closer, I think theres just a little wierdness left.  When running
> this checker script, the end of the log shows this:
> 
> CMD: abi-compliance-checker -l librte_vhost.so -old /home/nhorman/git/dpdk/devtools/abi-check/222555480/librte_vhost.so.dump -new /home/nhorman/git/dpdk/devtools/abi-check/02657b4ad/librte_vhost.so.dump
> NOTICE: At least one call to abi-compliance-checker returned an error.
> NOTICE: ABI may be incompatible, please check logs for details.
> NOTICE: Incompatible list:  librte_cryptodev.so librte_eal.so librte_efd.so librte_ethdev.so librte_eventdev.so librte_hash.so librte_pdump.so librte_pmd_crypto_scheduler.so librte_pmd_ring.so librte_ring.so
> 
> Which I think is something of a false positive.  The line:
> NOTICE: At least one call to abi-compliance-checker returned an error.
>
> I think is emitted simply because abi-compilance-checker returns non-zero and
> reports that error if it finds any incompatibilities.  I'm not sure we want to
> flag that as an error per se.  It gives the impression something has gone wrong,
> rather than correctly identifying that there are incompatibilities.
> 
> Though on the other hand, maybe we do want to set that red flag so people look
> at what the incompatibilities are.  As I say it out loud, perhaps that
> preferable.

Yes, I can remove this line
"At least one call to abi-compliance-checker returned an error."
which does not give a lot of info anyway.

While reproducing the issue, I also realized that the report path
was not preoperly fixed. So I'm sending a v4 with these fixes.

Thanks,
Olivier
  

Patch

diff --git a/devtools/validate-abi.sh b/devtools/validate-abi.sh
index 0accc99b1..8a512aa3b 100755
--- a/devtools/validate-abi.sh
+++ b/devtools/validate-abi.sh
@@ -1,7 +1,8 @@ 
-#!/bin/sh
+#!/usr/bin/env bash
 #   BSD LICENSE
 #
 #   Copyright(c) 2015 Neil Horman. All rights reserved.
+#   Copyright(c) 2017 6WIND S.A.
 #   All rights reserved.
 #
 #   Redistribution and use in source and binary forms, with or without
@@ -27,236 +28,243 @@ 
 #   (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`
+set -e
 
-usage() {
-	echo "$0 <REV1> <REV2> <TARGET>"
-}
+abicheck=abi-compliance-checker
+abidump=abi-dumper
+default_dst=abi-check
+default_target=x86_64-native-linuxapp-gcc
 
-log() {
-	local level=$1
-	shift
-	echo "$*"
+# trap on error
+err_report() {
+    echo "$0: error at line $1"
 }
+trap 'err_report $LINENO' ERR
 
-validate_tags() {
+print_usage () {
+	cat <<- END_OF_HELP
+	$(basename $0) [options] <rev1> <rev2>
 
-	if [ -z "$HASH1" ]
-	then
-		echo "invalid revision: $TAG1"
-		return
-	fi
-	if [ -z "$HASH2" ]
-	then
-		echo "invalid revision: $TAG2"
-		return
-	fi
+	This script compares the ABI of 2 git revisions of the current
+	workspace. The output is a html report and a compilation log.
+
+	The objective is to make sure that applications built against
+	DSOs from the first revision can still run when executed using
+	the DSOs built from the second revision.
+
+	<rev1> and <rev2> are git commit id or tags.
+
+	Options:
+	  -h		show this help
+	  -j <num>	enable parallel compilation with <num> threads
+	  -v		show compilation logs on the console
+	  -d <dir>	change working directory (default is ${default_dst})
+	  -t <target>	the dpdk target to use (default is ${default_target})
+	  -f		overwrite existing files in destination directory
+
+	The script returns 0 on success, or the value of last failing
+	call of ${abicheck} (incompatible abi or the tool has run with errors).
+	The errors returned by ${abidump} are ignored.
+
+	END_OF_HELP
 }
 
-validate_args() {
-	if [ -z "$TAG1" ]
-	then
-		echo "Must Specify REV1"
-		return
-	fi
-	if [ -z "$TAG2" ]
-	then
-		echo "Must Specify REV2"
-		return
-	fi
-	if [ -z "$TARGET" ]
-	then
-		echo "Must Specify a build target"
+# log in the file, and on stdout if verbose
+# $1: level string
+# $2: string to be logged
+log() {
+	echo "$1: $2"
+	if [ "${verbose}" != "true" ]; then
+		echo "$1: $2" >&3
 	fi
 }
 
+# launch a command and log it, taking care of surrounding spaces with quotes
+cmd() {
+	local i s whitespace
+	s=""
+	whitespace="[[:space:]]"
+	for i in "$@"; do
+		if [[ $i =~ $whitespace ]]; then
+			i=\"$i\"
+		fi
+		if [ -z "$s" ]; then
+			s="$i"
+		else
+			s="$s $i"
+		fi
+	done
+
+	log "CMD" "$s"
+	"$@"
+}
 
-cleanup_and_exit() {
-	rm -rf $ABI_DIR
-	git checkout $CURRENT_BRANCH
-	exit $1
+# redirect or copy stderr/stdout to a file
+# the syntax is unfamiliar, but it makes the rest of the
+# code easier to read, avoiding the use of pipes
+set_log_file() {
+	# save original stdout and stderr in fd 3 and 4
+	exec 3>&1
+	exec 4>&2
+	# create a new fd 5 that send to a file
+	exec 5> >(cat > $1)
+	# send stdout and stderr to fd 5
+	if [ "${verbose}" = "true" ]; then
+		exec 1> >(tee /dev/fd/5 >&3)
+		exec 2> >(tee /dev/fd/5 >&4)
+	else
+		exec 1>&5
+		exec 2>&5
+	fi
 }
 
 # Make sure we configure SHARED libraries
 # Also turn off IGB and KNI as those require kernel headers to build
 fixup_config() {
-	sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" config/defconfig_$TARGET
-	sed -i -e"$ a\CONFIG_RTE_NEXT_ABI=n" 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
-	sed -i -e"$ a\CONFIG_RTE_KNI_KMOD=n" config/defconfig_$TARGET
+	local conf=config/defconfig_$target
+	cmd sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" $conf
+	cmd sed -i -e"$ a\CONFIG_RTE_NEXT_ABI=n" $conf
+	cmd sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" $conf
+	cmd sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" $conf
+	cmd sed -i -e"$ a\CONFIG_RTE_KNI_KMOD=n" $conf
 }
 
-###########################################
-#START
-############################################
+# build dpdk for the given tag and dump abi
+# $1: hash of the revision
+gen_abi() {
+	local i oldpwd
+
+	oldpwd=${PWD}
+	cmd git clone $(dirname $0)/.. ${dst}/${1}
+	cmd cd ${dst}/${1}
+
+	log "INFO" "Checking out version ${1} of the dpdk"
+	# Move to the old version of the tree
+	cmd git checkout ${1}
+
+	fixup_config
+
+	# Now configure the build
+	log "INFO" "Configuring DPDK ${1}"
+	cmd make config T=$target O=$target > ${dst}/log 2>&1
+
+	# Checking abi compliance relies on using the dwarf information in
+	# the shared objects. Build with -g to include them.
+	log "INFO" "Building DPDK ${1}. This might take a moment"
+	cmd make -j$parallel O=$target V=1 EXTRA_CFLAGS="-g -Og -Wno-error" \
+	    EXTRA_LDFLAGS="-g" || log "INFO" "The build failed"
+
+	# Move to the lib directory
+	cmd cd ${PWD}/$target/lib
+	log "INFO" "Collecting ABI information for ${1}"
+	for i in *.so; do
+		[ -e "$i" ] || break
+		cmd $abidump ${i} -o $dst/${1}/${i}.dump -lver ${1} || true
+		# hack to ignore empty SymbolsInfo section (no public ABI)
+		if grep -q "'SymbolInfo' => {}," $dst/${1}/${i}.dump \
+				2> /dev/null; then
+			log "INFO" "${i} has no public ABI, remove dump file"
+			cmd rm -f $dst/${1}/${i}.dump
+		fi
+	done
+	cmd cd ${oldpwd}
+}
 
-#trap on ctrl-c to clean up
-trap cleanup_and_exit SIGINT
+verbose=false
+parallel=1
+dst=${default_dst}
+target=${default_target}
+force=0
+while getopts j:vd:t:fh ARG ; do
+	case $ARG in
+		j ) parallel=$OPTARG ;;
+		v ) verbose=true ;;
+		d ) dst=$OPTARG ;;
+		t ) target=$OPTARG ;;
+		f ) force=1 ;;
+		h ) print_usage ; exit 0 ;;
+		? ) print_usage ; exit 1 ;;
+	esac
+done
+shift $(($OPTIND - 1))
 
-if [ -z "$DPDK_MAKE_JOBS" ]
-then
-	# This counts the number of cpus on the system
-	if [ -e /usr/bin/lscpu ]
-	then
-		DPDK_MAKE_JOBS=`lscpu -p=cpu | grep -v "#" | wc -l`
-	else
-		DPDK_MAKE_JOBS=1
-	fi
+if [ $# != 2 ]; then
+	print_usage
+	exit 1
 fi
 
-#Save the current branch
-CURRENT_BRANCH=`git branch | grep \* | cut -d' ' -f2`
+tag1=$1
+tag2=$2
 
-if [ -z "$CURRENT_BRANCH" ]
-then
-	CURRENT_BRANCH=`git log --pretty=format:%H HEAD~1..HEAD`
-fi
+# convert path to absolute
+case "${dst}" in
+	/*) ;;
+	*) dst=${PWD}/${dst} ;;
+esac
 
-if [ -n "$VERBOSE" ]
-then
-	export VERBOSE=/dev/stdout
-else
-	export VERBOSE=/dev/null
+if [ -e "${dst}" -a "$force" = 0 ]; then
+	echo "The ${dst} directory is not empty. Remove it, use another"
+	echo "one (-d <dir>), or force overriding (-f)"
+	exit 1
 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
+rm -rf ${dst}
+mkdir -p ${dst}
+set_log_file ${dst}/abi-check.log
+log "INFO" "Logs available in ${dst}/abi-check.log"
 
-HASH1=$(git show -s --format=%H "$TAG1" -- 2> /dev/null | tail -1)
-HASH2=$(git show -s --format=%H "$TAG2" -- 2> /dev/null | tail -1)
+command -v ${abicheck} || log "INFO" "Can't find ${abicheck} utility"
+command -v ${abidump} || log "INFO" "Can't find ${abidump} utility"
 
-# Make sure our tags exist
-res=$(validate_tags)
-if [ -n "$res" ]
-then
-	echo $res
-	cleanup_and_exit 1
-fi
+hash1=$(git show -s --format=%h "$tag1" -- 2> /dev/null | tail -1)
+hash2=$(git show -s --format=%h "$tag2" -- 2> /dev/null | tail -1)
 
 # Make hashes available in output for non-local reference
-TAG1="$TAG1 ($HASH1)"
-TAG2="$TAG2 ($HASH2)"
-
-ABICHECK=`which abi-compliance-checker 2>/dev/null`
-if [ $? -ne 0 ]
-then
-	log "INFO" "Can't find abi-compliance-checker utility"
-	cleanup_and_exit 1
-fi
-
-ABIDUMP=`which abi-dumper 2>/dev/null`
-if [ $? -ne 0 ]
-then
-	log "INFO" "Can't find abi-dumper utility"
-	cleanup_and_exit 1
-fi
+tag1="$tag1 ($hash1)"
+tag2="$tag2 ($hash2)"
 
-log "INFO" "We're going to check and make sure that applications built"
-log "INFO" "against DPDK DSOs from version $TAG1 will still run when executed"
-log "INFO" "against DPDK DSOs built from version $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
+if [ "$hash1" = "$hash2" ]; then
+	log "ERROR" "$tag1 and $tag2 are the same revisions"
+	exit 1
 fi
 
-# Move to the root of the git tree
-cd $(dirname $0)/..
+cmd mkdir -p ${dst}
 
-log "INFO" "Checking out version $TAG1 of the dpdk"
-# Move to the old version of the tree
-git checkout $HASH1
+# dump abi for each revision
+gen_abi ${hash1}
+gen_abi ${hash2}
 
-fixup_config
+# compare the abi dumps
+ret=0
+list=""
+for i in ${dst}/${hash2}/*.dump; do
+	name=`basename $i`
+	libname=${name%.dump}
 
-# Checking abi compliance relies on using the dwarf information in
-# The shared objects.  Thats only included in the DSO's if we build
-# with -g
-export EXTRA_CFLAGS="$EXTRA_CFLAGS -g -O0"
-export EXTRA_LDFLAGS="$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 -j$DPDK_MAKE_JOBS O=$TARGET > $VERBOSE 2>&1
-
-if [ $? -ne 0 ]
-then
-	log "INFO" "THE BUILD FAILED.  ABORTING"
-	cleanup_and_exit 1
-fi
+	if [ ! -f $dst/${hash1}/$name ]; then
+		log "INFO" "$NAME does not exist in $tag1. skipping..."
+		continue
+	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 $HASH1
+	local_ret=0
+	cmd $abicheck -l $libname \
+	    -old $dst/${hash1}/$name -new $dst/${hash2}/$name || local_ret=$?
+	if [ $local_ret != 0 ]; then
+		log "NOTICE" "$abicheck returned $local_ret"
+		ret=$local_ret
+		list="$list $libname"
+	fi
 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 $HASH2
-
-fixup_config
-
-# 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 -j$DPDK_MAKE_JOBS O=$TARGET > $VERBOSE 2>&1
 
-if [ $? -ne 0 ]
-then
-	log "INFO" "THE BUILD FAILED.  ABORTING"
-	cleanup_and_exit 1
+if [ $ret != 0 ]; then
+	log "NOTICE" "At least one call to $abicheck returned an error."
+	log "NOTICE" "ABI may be incompatible, please check logs for details."
+	log "NOTICE" "Incompatible list: $list"
+else
+	log "NOTICE" "No error detected, ABI is compatible."
 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 $HASH2
-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
+log "INFO" "Logs are in ${dst}/abi-check.log"
+log "INFO" "HTML reports are in ${dst}/${hash2}/compat_reports directory"
 
-git reset --hard
-log "INFO" "ABI CHECK COMPLETE.  REPORTS ARE IN compat_report directory"
-cleanup_and_exit 0
+exit $ret