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 D242B106B for ; Wed, 4 Mar 2015 12:49:56 +0100 (CET) Received: from hmsreliant.think-freely.org ([2001:470:8:a08:7aac:c0ff:fec2:933b] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1YT7oG-00010A-39; Wed, 04 Mar 2015 06:49:55 -0500 Date: Wed, 4 Mar 2015 06:49:51 -0500 From: Neil Horman To: Thomas Monjalon Message-ID: <20150304114951.GB5808@hmsreliant.think-freely.org> References: <1422652596-12777-1-git-send-email-nhorman@tuxdriver.com> <1422901106-20734-1-git-send-email-nhorman@tuxdriver.com> <5425830.nAiNf2ERY3@xps13> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5425830.nAiNf2ERY3@xps13> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Score: -2.9 (--) X-Spam-Status: No Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v2] ABI: Add abi checking utility X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 04 Mar 2015 11:49:57 -0000 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. > > > + > > +usage() { > > + echo "$0 " > > +} > > + > > +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. > > > + 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 >