From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id C98E51B1A7 for ; Thu, 21 Sep 2017 17:40:51 +0200 (CEST) Received: from [107.15.66.59] (helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.63) (envelope-from ) id 1dv3aj-0006zH-OK; Thu, 21 Sep 2017 11:40:48 -0400 Date: Thu, 21 Sep 2017 11:40:35 -0400 From: Neil Horman To: Olivier Matz Cc: dev@dpdk.org, bruce.richardson@intel.com Message-ID: <20170921154035.GB3389@hmswarspite.think-freely.org> References: <20170911084635.11707-1-olivier.matz@6wind.com> <20170920091253.15794-1-olivier.matz@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170920091253.15794-1-olivier.matz@6wind.com> User-Agent: Mutt/1.8.3 (2017-05-23) X-Spam-Score: -2.9 (--) X-Spam-Status: No Subject: Re: [dpdk-dev] [PATCH v4] devtools: rework abi checker script X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Sep 2017 15:40:52 -0000 On Wed, Sep 20, 2017 at 11:12:53AM +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 > --- > > v3->v4: > - clarify logs on incompatible abi > - log when an error returned an error > - [really] fix the report path > - log the output of make config in the proper file > > 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 | 397 ++++++++++++++++++++++++----------------------- > 1 file changed, 205 insertions(+), 192 deletions(-) > This looks better, thank you for the iterations. One last note: The abi dumper utility errors out with error code of 12 if a given object has no exported symbols, and I see a few of those. You may want to consider catching that error, logging an appropriate message and skipping the error emit. That can be handled later though, as its a corner case. I'd go with this patch, and then do a incremental improvement later Acked-by: Neil Horman > diff --git a/devtools/validate-abi.sh b/devtools/validate-abi.sh > index 0accc99b1..8caf43e83 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,248 @@ > # (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 " > -} > +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] > > - 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. > + > + and are git commit id or tags. > + > + Options: > + -h show this help > + -j enable parallel compilation with threads > + -v show compilation logs on the console > + -d change working directory (default is ${default_dst}) > + -t 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 ret > + 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 > + > + ret=0 > + log "CMD" "$s" > + "$@" || ret=$? > + if [ "$ret" != "0" ]; then > + log "CMD" "previous command returned $ret" > + fi > + > + return $ret > +} > > -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 > + > + cmd git clone ${dpdkroot} ${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 > + > + # 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 > +} > > -#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 > +dpdkroot=$(readlink -e $(dirname $0)/..) > > -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 ), 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 > +cmd cd ${dst} > +ret=0 > +list="" > +for i in ${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 ${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 ${hash1}/$name -new ${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" "ABI may be incompatible, check reports/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}/compat_reports directory" > > -git reset --hard > -log "INFO" "ABI CHECK COMPLETE. REPORTS ARE IN compat_report directory" > -cleanup_and_exit 0 > +exit $ret > -- > 2.11.0 > >