Re: [dpdk-dev] [PATCHv3] Remove validate-abi.sh from tree

[Ray Kinsella <mdr@ashroe.eu>]

On 17/04/2020 11:20, Thomas Monjalon wrote:
> 17/04/2020 12:11, Ray Kinsella:
>>
>> On 16/04/2020 15:54, Neil Horman wrote:
>>> Since we've moved away from our initial validate-abi.sh script, in
>>> favor of check-abi.sh, which uses libabigail, remove the old script from
>>> the tree, and update the docs accordingly
>>>
>>> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>>> CC: thomas@monjalon.net
>>> CC: david.marchand@redhat.com
>>> CC: mdr@ashroe.eu
>>> ---
>>>  MAINTAINERS                                |   1 -
>>>  devtools/validate-abi.sh                   | 251 ---------------------
>>>  doc/guides/contributing/abi_versioning.rst |  50 ++--
>>>  3 files changed, 18 insertions(+), 284 deletions(-)
>>>  delete mode 100755 devtools/validate-abi.sh
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 4800f6884..84b633431 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -152,7 +152,6 @@ F: devtools/gen-abi.sh
>>>  F: devtools/libabigail.abignore
>>>  F: devtools/update-abi.sh
>>>  F: devtools/update_version_map_abi.py
>>> -F: devtools/validate-abi.sh
>>>  F: buildtools/check-experimental-syms.sh
>>>  F: buildtools/map-list-symbol.sh
>>>  
>>> diff --git a/devtools/validate-abi.sh b/devtools/validate-abi.sh
>>> deleted file mode 100755
>>> index f64e19d38..000000000
>>> --- a/devtools/validate-abi.sh
>>> +++ /dev/null
>>> @@ -1,251 +0,0 @@
>>> -#!/usr/bin/env bash
>>> -# SPDX-License-Identifier: BSD-3-Clause
>>> -# Copyright(c) 2015 Neil Horman. All rights reserved.
>>> -# Copyright(c) 2017 6WIND S.A.
>>> -# All rights reserved
>>> -
>>> -set -e
>>> -
>>> -abicheck=abi-compliance-checker
>>> -abidump=abi-dumper
>>> -default_dst=abi-check
>>> -default_target=x86_64-native-linuxapp-gcc
>>> -
>>> -# trap on error
>>> -err_report() {
>>> -    echo "$0: error at line $1"
>>> -}
>>> -trap 'err_report $LINENO' ERR
>>> -
>>> -print_usage () {
>>> -	cat <<- END_OF_HELP
>>> -	$(basename $0) [options] <rev1> <rev2>
>>> -
>>> -	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
>>> -}
>>> -
>>> -# 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
>>> -}
>>> -
>>> -# 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() {
>>> -	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
>>> -}
>>> -
>>> -# 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
>>> -}
>>> -
>>> -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 [ $# != 2 ]; then
>>> -	print_usage
>>> -	exit 1
>>> -fi
>>> -
>>> -tag1=$1
>>> -tag2=$2
>>> -
>>> -# convert path to absolute
>>> -case "${dst}" in
>>> -	/*) ;;
>>> -	*) dst=${PWD}/${dst} ;;
>>> -esac
>>> -dpdkroot=$(readlink -f $(dirname $0)/..)
>>> -
>>> -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
>>> -
>>> -rm -rf ${dst}
>>> -mkdir -p ${dst}
>>> -set_log_file ${dst}/abi-check.log
>>> -log "INFO" "Logs available in ${dst}/abi-check.log"
>>> -
>>> -command -v ${abicheck} || log "INFO" "Can't find ${abicheck} utility"
>>> -command -v ${abidump} || log "INFO" "Can't find ${abidump} utility"
>>> -
>>> -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)"
>>> -
>>> -if [ "$hash1" = "$hash2" ]; then
>>> -	log "ERROR" "$tag1 and $tag2 are the same revisions"
>>> -	exit 1
>>> -fi
>>> -
>>> -cmd mkdir -p ${dst}
>>> -
>>> -# dump abi for each revision
>>> -gen_abi ${hash1}
>>> -gen_abi ${hash2}
>>> -
>>> -# compare the abi dumps
>>> -cmd cd ${dst}
>>> -ret=0
>>> -list=""
>>> -for i in ${hash2}/*.dump; do
>>> -	name=`basename $i`
>>> -	libname=${name%.dump}
>>> -
>>> -	if [ ! -f ${hash1}/$name ]; then
>>> -		log "INFO" "$NAME does not exist in $tag1. skipping..."
>>> -		continue
>>> -	fi
>>> -
>>> -	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
>>> -
>>> -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
>>> -
>>> -log "INFO" "Logs are in ${dst}/abi-check.log"
>>> -log "INFO" "HTML reports are in ${dst}/compat_reports directory"
>>> -
>>> -exit $ret
>>> diff --git a/doc/guides/contributing/abi_versioning.rst b/doc/guides/contributing/abi_versioning.rst
>>> index a21f4e7a4..219823923 100644
>>> --- a/doc/guides/contributing/abi_versioning.rst
>>> +++ b/doc/guides/contributing/abi_versioning.rst
>>> @@ -482,41 +482,27 @@ Running the ABI Validator
>>>  -------------------------
>>>  
>>>  The ``devtools`` directory in the DPDK source tree contains a utility program,
>>> -``validate-abi.sh``, for validating the DPDK ABI based on the Linux `ABI
>>> -Compliance Checker
>>> -<http://ispras.linuxbase.org/index.php/ABI_compliance_checker>`_.
>>> +``check-abi.sh``, for validating the DPDK ABI based on the libabigail `abidiff
>>> +utility: <https://sourceware.org/libabigail/manual/abidiff.html>`_
>>>  
>>> -This has a dependency on the ``abi-compliance-checker`` and ``and abi-dumper``
>>> -utilities which can be installed via a package manager. For example::
>>> +The syntax of the ``check-abi.sh`` utility is::
>>>  
>>> -   sudo yum install abi-compliance-checker
>>> -   sudo yum install abi-dumper
>>> +   ./devtools/check-abi.sh <refdir> <newdir>
>>>  
>>> -The syntax of the ``validate-abi.sh`` utility is::
>>> +Where <refdir> specifies the directory housing the reference build of dpdk, and
>>> +<newdir> specifies the dpdk build directory to check the abi of
>>>  
>>> -   ./devtools/validate-abi.sh <REV1> <REV2>
>>> +Example:
>>> +To compare your build branch to the ABI of the master branch
>>> +after you have built your branch
>>>  
>>> -Where ``REV1`` and ``REV2`` are valid gitrevisions(7)
>>> -https://www.kernel.org/pub/software/scm/git/docs/gitrevisions.html
>>> -on the local repo.
>>> +#.        $ cd <path to dpdk src tree>
>>> +#.        $ mkdir ~/ref
>>> +#.        $ git clone --local --no-hardlinks --single-branch -b master . ~/ref 
>>> +#.        $ cd ~/ref
>>> +#.        $ cp <path to dpdk src tree from above>/.config ./.config
>>> +#.        $ make defconfig
>>> +#.        $ make
>>> +#.        $ <path to dpdk src tree>/devtools/check-abi.sh ~/ref <path to dpdk src tree>
>>>  
>>> -For example::
>>> -
>>> -   # Check between the previous and latest commit:
>>> -   ./devtools/validate-abi.sh HEAD~1 HEAD
>>> -
>>> -   # Check on a specific compilation target:
>>> -   ./devtools/validate-abi.sh -t x86_64-native-linux-gcc HEAD~1 HEAD
>>> -
>>> -   # Check between two tags:
>>> -   ./devtools/validate-abi.sh v2.0.0 v2.1.0
>>> -
>>> -   # Check between git master and local topic-branch "vhost-hacking":
>>> -   ./devtools/validate-abi.sh master vhost-hacking
>>> -
>>> -After the validation script completes (it can take a while since it need to
>>> -compile both tags) it will create compatibility reports in the
>>> -``./abi-check/compat_report`` directory. Listed incompatibilities can be found
>>> -as follows::
>>> -
>>> -  grep -lr Incompatible abi-check/compat_reports/
>>> +       
>>>
>>
>> check-abi.sh appears to be backward step in terms of usability.
> 
> No, check-abi.sh benefits from a nice integration in build scripts.
> See below.
> 
>> With validate-abi.sh I do can do a "validate-abi.sh HEAD~1 HEAD".
>> And it will do the build, install, dump and comparison for me. 
>> And it picked up my 20.0.2 - > 21.0 changes no problem. 
>>
>> With check-abi on the other hand, I need to the build and install myself.
>> check-abi requires dump files, but I see no reference in the documentation to how these are created.
>> It silently fails when it doesn't find any ...
>>
>> Do I run abi-dumper on the so's myself, or how does it work?
> 
> check-abi.sh is integrated in test-build.sh and test-meson-builds.sh.
> Probably we should document usage in these scripts.
> 

ok in that case, this documentation should reference those scripts instead?