[PATCHv2] Remove validate-abi.sh from tree

  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 |  61 ++---
 3 files changed, 23 insertions(+), 290 deletions(-)
 delete mode 100755 devtools/validate-abi.sh
  

On 08/04/2020 20:56, 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 |  61 ++---
>  3 files changed, 23 insertions(+), 290 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..d32cf440a 100644
> --- a/doc/guides/contributing/abi_versioning.rst
> +++ b/doc/guides/contributing/abi_versioning.rst
> @@ -482,41 +482,26 @@ 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>`_.
> -
> -This has a dependency on the ``abi-compliance-checker`` and ``and abi-dumper``
> -utilities which can be installed via a package manager. For example::
> -
> -   sudo yum install abi-compliance-checker
> -   sudo yum install abi-dumper
> -
> -The syntax of the ``validate-abi.sh`` utility is::
> -
> -   ./devtools/validate-abi.sh <REV1> <REV2>
> -
> -Where ``REV1`` and ``REV2`` are valid gitrevisions(7)
> -https://www.kernel.org/pub/software/scm/git/docs/gitrevisions.html
> -on the local repo.
> -
> -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``, for validating the DPDK ABI based on the libabigail abidiff
> +utility:
> +https://sourceware.org/libabigail/manual/abidiff.html 

(from v1 feedback)
Links should be in the format.
`PEP8 (Style Guide for Python Code) <https://www.python.org/dev/peps/pep-0008/>`_.

> +
> +The syntax of the ``check-abi.sh`` utility is::
> +
> +   ./devtools/check-abi.sh <refdir> <newdir>

(from v1 feedback)
Could we simplify this all greatly, by telling people to use the meson/ninja build,
so they get this checking out of the box, without all the headache below?

> +
> +Where <refdir> specifies the directory housing the reference build of dpdk, and
> +<newdir> specifies the dpdk build directory to check the abi of
> +
> +Example:
> +        # To compare your build branch to the ABI of the master branch
> +        # after you have built your branch
> +        $ 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>
> +        
>
  

On Thu, Apr 09, 2020 at 08:57:34AM +0100, Ray Kinsella wrote:
> 
> 
> On 08/04/2020 20:56, 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 |  61 ++---
> >  3 files changed, 23 insertions(+), 290 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..d32cf440a 100644
> > --- a/doc/guides/contributing/abi_versioning.rst
> > +++ b/doc/guides/contributing/abi_versioning.rst
> > @@ -482,41 +482,26 @@ 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>`_.
> > -
> > -This has a dependency on the ``abi-compliance-checker`` and ``and abi-dumper``
> > -utilities which can be installed via a package manager. For example::
> > -
> > -   sudo yum install abi-compliance-checker
> > -   sudo yum install abi-dumper
> > -
> > -The syntax of the ``validate-abi.sh`` utility is::
> > -
> > -   ./devtools/validate-abi.sh <REV1> <REV2>
> > -
> > -Where ``REV1`` and ``REV2`` are valid gitrevisions(7)
> > -https://www.kernel.org/pub/software/scm/git/docs/gitrevisions.html
> > -on the local repo.
> > -
> > -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``, for validating the DPDK ABI based on the libabigail abidiff
> > +utility:
> > +https://sourceware.org/libabigail/manual/abidiff.html 
> 
> (from v1 feedback)
> Links should be in the format.
> `PEP8 (Style Guide for Python Code) <https://www.python.org/dev/peps/pep-0008/>`_.
> 
copy that, I'll fix it up

> > +
> > +The syntax of the ``check-abi.sh`` utility is::
> > +
> > +   ./devtools/check-abi.sh <refdir> <newdir>
> 
> (from v1 feedback)
> Could we simplify this all greatly, by telling people to use the meson/ninja build,
> so they get this checking out of the box, without all the headache below?
> 
I think bruce noted that was never merged, correct?

> > +
> > +Where <refdir> specifies the directory housing the reference build of dpdk, and
> > +<newdir> specifies the dpdk build directory to check the abi of
> > +
> > +Example:
> > +        # To compare your build branch to the ABI of the master branch
> > +        # after you have built your branch
> > +        $ 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>
> > +        
> > 
>
  

On Thu, Apr 09, 2020 at 06:39:54AM -0400, Neil Horman wrote:
> On Thu, Apr 09, 2020 at 08:57:34AM +0100, Ray Kinsella wrote:
> > 
> > 
> > On 08/04/2020 20:56, 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 |  61 ++---
> > >  3 files changed, 23 insertions(+), 290 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..d32cf440a 100644
> > > --- a/doc/guides/contributing/abi_versioning.rst
> > > +++ b/doc/guides/contributing/abi_versioning.rst
> > > @@ -482,41 +482,26 @@ 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>`_.
> > > -
> > > -This has a dependency on the ``abi-compliance-checker`` and ``and abi-dumper``
> > > -utilities which can be installed via a package manager. For example::
> > > -
> > > -   sudo yum install abi-compliance-checker
> > > -   sudo yum install abi-dumper
> > > -
> > > -The syntax of the ``validate-abi.sh`` utility is::
> > > -
> > > -   ./devtools/validate-abi.sh <REV1> <REV2>
> > > -
> > > -Where ``REV1`` and ``REV2`` are valid gitrevisions(7)
> > > -https://www.kernel.org/pub/software/scm/git/docs/gitrevisions.html
> > > -on the local repo.
> > > -
> > > -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``, for validating the DPDK ABI based on the libabigail abidiff
> > > +utility:
> > > +https://sourceware.org/libabigail/manual/abidiff.html 
> > 
> > (from v1 feedback)
> > Links should be in the format.
> > `PEP8 (Style Guide for Python Code) <https://www.python.org/dev/peps/pep-0008/>`_.
> > 
> copy that, I'll fix it up
> 
> > > +
> > > +The syntax of the ``check-abi.sh`` utility is::
> > > +
> > > +   ./devtools/check-abi.sh <refdir> <newdir>
> > 
> > (from v1 feedback)
> > Could we simplify this all greatly, by telling people to use the meson/ninja build,
> > so they get this checking out of the box, without all the headache below?
> > 
> I think bruce noted that was never merged, correct?
> 
Yep, correct. :-(
  

On 09/04/2020 11:43, Bruce Richardson wrote:
> On Thu, Apr 09, 2020 at 06:39:54AM -0400, Neil Horman wrote:
>> On Thu, Apr 09, 2020 at 08:57:34AM +0100, Ray Kinsella wrote:
>>>
>>>
>>> On 08/04/2020 20:56, 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 |  61 ++---
>>>>  3 files changed, 23 insertions(+), 290 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..d32cf440a 100644
>>>> --- a/doc/guides/contributing/abi_versioning.rst
>>>> +++ b/doc/guides/contributing/abi_versioning.rst
>>>> @@ -482,41 +482,26 @@ 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>`_.
>>>> -
>>>> -This has a dependency on the ``abi-compliance-checker`` and ``and abi-dumper``
>>>> -utilities which can be installed via a package manager. For example::
>>>> -
>>>> -   sudo yum install abi-compliance-checker
>>>> -   sudo yum install abi-dumper
>>>> -
>>>> -The syntax of the ``validate-abi.sh`` utility is::
>>>> -
>>>> -   ./devtools/validate-abi.sh <REV1> <REV2>
>>>> -
>>>> -Where ``REV1`` and ``REV2`` are valid gitrevisions(7)
>>>> -https://www.kernel.org/pub/software/scm/git/docs/gitrevisions.html
>>>> -on the local repo.
>>>> -
>>>> -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``, for validating the DPDK ABI based on the libabigail abidiff
>>>> +utility:
>>>> +https://sourceware.org/libabigail/manual/abidiff.html 
>>>
>>> (from v1 feedback)
>>> Links should be in the format.
>>> `PEP8 (Style Guide for Python Code) <https://www.python.org/dev/peps/pep-0008/>`_.
>>>
>> copy that, I'll fix it up
>>
>>>> +
>>>> +The syntax of the ``check-abi.sh`` utility is::
>>>> +
>>>> +   ./devtools/check-abi.sh <refdir> <newdir>
>>>
>>> (from v1 feedback)
>>> Could we simplify this all greatly, by telling people to use the meson/ninja build,
>>> so they get this checking out of the box, without all the headache below?
>>>
>> I think bruce noted that was never merged, correct?
>>
> Yep, correct. :-(
> 

apologies, was there a reason?
  

09/04/2020 12:45, Ray Kinsella:
> On 09/04/2020 11:43, Bruce Richardson wrote:
> > On Thu, Apr 09, 2020 at 06:39:54AM -0400, Neil Horman wrote:
> >> On Thu, Apr 09, 2020 at 08:57:34AM +0100, Ray Kinsella wrote:
> >>> On 08/04/2020 20:56, Neil Horman wrote:
> >>>> +The syntax of the ``check-abi.sh`` utility is::
> >>>> +
> >>>> +   ./devtools/check-abi.sh <refdir> <newdir>
> >>>
> >>> (from v1 feedback)
> >>> Could we simplify this all greatly, by telling people to use the meson/ninja build,
> >>> so they get this checking out of the box, without all the headache below?
> >>>
> >> I think bruce noted that was never merged, correct?
> >>
> > Yep, correct. :-(
> 
> apologies, was there a reason?

Because build tool job is building, not checking.
It would be wrong to make (slow) checks mandatory in all builds.

The need is to enforce checking ABI.
The result is already published by Travis in patchwork and in an
email to the author I believe.
Not checking email and patchwork is not a good excuse.

Patchwork must be a mandatory read for everybody for all checks
in general. Let's not give up on general CI workflow.
  

On Thu, Apr 09, 2020 at 12:59:50PM +0200, Thomas Monjalon wrote:
> 09/04/2020 12:45, Ray Kinsella:
> > On 09/04/2020 11:43, Bruce Richardson wrote:
> > > On Thu, Apr 09, 2020 at 06:39:54AM -0400, Neil Horman wrote:
> > >> On Thu, Apr 09, 2020 at 08:57:34AM +0100, Ray Kinsella wrote:
> > >>> On 08/04/2020 20:56, Neil Horman wrote:
> > >>>> +The syntax of the ``check-abi.sh`` utility is::
> > >>>> +
> > >>>> +   ./devtools/check-abi.sh <refdir> <newdir>
> > >>>
> > >>> (from v1 feedback)
> > >>> Could we simplify this all greatly, by telling people to use the meson/ninja build,
> > >>> so they get this checking out of the box, without all the headache below?
> > >>>
> > >> I think bruce noted that was never merged, correct?
> > >>
> > > Yep, correct. :-(
> > 
> > apologies, was there a reason?
> 
> Because build tool job is building, not checking.
> It would be wrong to make (slow) checks mandatory in all builds.
> 
> The need is to enforce checking ABI.
> The result is already published by Travis in patchwork and in an
> email to the author I believe.
> Not checking email and patchwork is not a good excuse.
> 
> Patchwork must be a mandatory read for everybody for all checks
> in general. Let's not give up on general CI workflow.
> 
But it would be helpful to at least codify the commands in the example patch
into a build target, for the convienience of local checking prior to CI
submission, no?
Neil

> 
> 
>
  

09/04/2020 15:02, Neil Horman:
> On Thu, Apr 09, 2020 at 12:59:50PM +0200, Thomas Monjalon wrote:
> > 09/04/2020 12:45, Ray Kinsella:
> > > On 09/04/2020 11:43, Bruce Richardson wrote:
> > > > On Thu, Apr 09, 2020 at 06:39:54AM -0400, Neil Horman wrote:
> > > >> On Thu, Apr 09, 2020 at 08:57:34AM +0100, Ray Kinsella wrote:
> > > >>> On 08/04/2020 20:56, Neil Horman wrote:
> > > >>>> +The syntax of the ``check-abi.sh`` utility is::
> > > >>>> +
> > > >>>> +   ./devtools/check-abi.sh <refdir> <newdir>
> > > >>>
> > > >>> (from v1 feedback)
> > > >>> Could we simplify this all greatly, by telling people to use the meson/ninja build,
> > > >>> so they get this checking out of the box, without all the headache below?
> > > >>>
> > > >> I think bruce noted that was never merged, correct?
> > > >>
> > > > Yep, correct. :-(
> > > 
> > > apologies, was there a reason?
> > 
> > Because build tool job is building, not checking.
> > It would be wrong to make (slow) checks mandatory in all builds.
> > 
> > The need is to enforce checking ABI.
> > The result is already published by Travis in patchwork and in an
> > email to the author I believe.
> > Not checking email and patchwork is not a good excuse.
> > 
> > Patchwork must be a mandatory read for everybody for all checks
> > in general. Let's not give up on general CI workflow.
> > 
> But it would be helpful to at least codify the commands in the example patch
> into a build target, for the convienience of local checking prior to CI
> submission, no?

I don't understand what you mean.
  

On 09/04/2020 11:59, Thomas Monjalon wrote:
> 09/04/2020 12:45, Ray Kinsella:
>> On 09/04/2020 11:43, Bruce Richardson wrote:
>>> On Thu, Apr 09, 2020 at 06:39:54AM -0400, Neil Horman wrote:
>>>> On Thu, Apr 09, 2020 at 08:57:34AM +0100, Ray Kinsella wrote:
>>>>> On 08/04/2020 20:56, Neil Horman wrote:
>>>>>> +The syntax of the ``check-abi.sh`` utility is::
>>>>>> +
>>>>>> +   ./devtools/check-abi.sh <refdir> <newdir>
>>>>>
>>>>> (from v1 feedback)
>>>>> Could we simplify this all greatly, by telling people to use the meson/ninja build,
>>>>> so they get this checking out of the box, without all the headache below?
>>>>>
>>>> I think bruce noted that was never merged, correct?
>>>>
>>> Yep, correct. :-(
>>
>> apologies, was there a reason?
> 
> Because build tool job is building, not checking.
> It would be wrong to make (slow) checks mandatory in all builds.
> 
> The need is to enforce checking ABI.
> The result is already published by Travis in patchwork and in an
> email to the author I believe.
> Not checking email and patchwork is not a good excuse.
> 
> Patchwork must be a mandatory read for everybody for all checks
> in general. Let's not give up on general CI workflow.
> 

Thomas 

You are trying to solve two problems at once; CI tooling and ABI.
Let's try to solve one at a time. 

1. The ABI check, will make the build _marginally_ slower.
You _should_ only need to rebuild the changes between A and B. 

2. The meson/ninja are an order of magnitude faster than GNU Make. 
We can afford this check. 

3. If we want to lessen the ABI burden and send the correct message.
It should be a build blocker, contributors need to hear the message loud and clear. 

Most important people _consuming_ DPDK will never see this message.
Only people _changing_ the ABI will see it - the people we want to hear the message loud and clear. 

Ray K
  

09/04/2020 16:52, Ray Kinsella:
> On 09/04/2020 11:59, Thomas Monjalon wrote:
> > 09/04/2020 12:45, Ray Kinsella:
> >> On 09/04/2020 11:43, Bruce Richardson wrote:
> >>> On Thu, Apr 09, 2020 at 06:39:54AM -0400, Neil Horman wrote:
> >>>> On Thu, Apr 09, 2020 at 08:57:34AM +0100, Ray Kinsella wrote:
> >>>>> On 08/04/2020 20:56, Neil Horman wrote:
> >>>>>> +The syntax of the ``check-abi.sh`` utility is::
> >>>>>> +
> >>>>>> +   ./devtools/check-abi.sh <refdir> <newdir>
> >>>>>
> >>>>> (from v1 feedback)
> >>>>> Could we simplify this all greatly, by telling people to use the meson/ninja build,
> >>>>> so they get this checking out of the box, without all the headache below?
> >>>>>
> >>>> I think bruce noted that was never merged, correct?
> >>>>
> >>> Yep, correct. :-(
> >>
> >> apologies, was there a reason?
> > 
> > Because build tool job is building, not checking.
> > It would be wrong to make (slow) checks mandatory in all builds.
> > 
> > The need is to enforce checking ABI.
> > The result is already published by Travis in patchwork and in an
> > email to the author I believe.
> > Not checking email and patchwork is not a good excuse.
> > 
> > Patchwork must be a mandatory read for everybody for all checks
> > in general. Let's not give up on general CI workflow.
> > 
> 
> Thomas 
> 
> You are trying to solve two problems at once; CI tooling and ABI.
> Let's try to solve one at a time.

No, you want to mix two problems in a single tool :-)


> 1. The ABI check, will make the build _marginally_ slower.
> You _should_ only need to rebuild the changes between A and B.

Not so marginal.
A re-build takes less than a second. A mandatory check takes 10 secs
on my machine.


> 2. The meson/ninja are an order of magnitude faster than GNU Make. 
> We can afford this check.

I am doing such build 10 times (each target) per patch.
But that's not the main issue.


> 3. If we want to lessen the ABI burden and send the correct message.
> It should be a build blocker, contributors need to hear the message loud and clear.

The developer needs to get or build/save the ABI reference.
Making such ABI reference for each target is not so obvious:
  - all symbols must be enabled (dependencies)
  - some fixes may be needed for some compilers


> Most important people _consuming_ DPDK will never see this message.
> Only people _changing_ the ABI will see it - the people we want to hear the message loud and clear.

No, they will have issue in DPDK compilation if something in the check
goes wrong. We should not bother end users with internal checks.


The message is 
a) run the check by
  1) setting DPDK_ABI_REF_VERSION on command line or in devel.config file
  2) running devtools/test-build.sh or devtools/test-meson-builds.sh
b) check what Travis is reporting in
  - email to you
  - patchwork reports

I think Travis report is convenient to use.
The local check is integrated in build scripts
but cannot be run by default because of the reasons above.
  

On 09/04/2020 16:18, Thomas Monjalon wrote:
> 09/04/2020 16:52, Ray Kinsella:
>> On 09/04/2020 11:59, Thomas Monjalon wrote:
>>> 09/04/2020 12:45, Ray Kinsella:
>>>> On 09/04/2020 11:43, Bruce Richardson wrote:
>>>>> On Thu, Apr 09, 2020 at 06:39:54AM -0400, Neil Horman wrote:
>>>>>> On Thu, Apr 09, 2020 at 08:57:34AM +0100, Ray Kinsella wrote:
>>>>>>> On 08/04/2020 20:56, Neil Horman wrote:
>>>>>>>> +The syntax of the ``check-abi.sh`` utility is::
>>>>>>>> +
>>>>>>>> +   ./devtools/check-abi.sh <refdir> <newdir>
>>>>>>>
>>>>>>> (from v1 feedback)
>>>>>>> Could we simplify this all greatly, by telling people to use the meson/ninja build,
>>>>>>> so they get this checking out of the box, without all the headache below?
>>>>>>>
>>>>>> I think bruce noted that was never merged, correct?
>>>>>>
>>>>> Yep, correct. :-(
>>>>
>>>> apologies, was there a reason?
>>>
>>> Because build tool job is building, not checking.
>>> It would be wrong to make (slow) checks mandatory in all builds.
>>>
>>> The need is to enforce checking ABI.
>>> The result is already published by Travis in patchwork and in an
>>> email to the author I believe.
>>> Not checking email and patchwork is not a good excuse.
>>>
>>> Patchwork must be a mandatory read for everybody for all checks
>>> in general. Let's not give up on general CI workflow.
>>>
>>
>> Thomas 
>>
>> You are trying to solve two problems at once; CI tooling and ABI.
>> Let's try to solve one at a time.
> 
> No, you want to mix two problems in a single tool :-)
> 
> 
>> 1. The ABI check, will make the build _marginally_ slower.
>> You _should_ only need to rebuild the changes between A and B.
> 
> Not so marginal.
> A re-build takes less than a second. A mandatory check takes 10 secs
> on my machine.
> 
> 
>> 2. The meson/ninja are an order of magnitude faster than GNU Make. 
>> We can afford this check.
> 
> I am doing such build 10 times (each target) per patch.
> But that's not the main issue.
> 
> 
>> 3. If we want to lessen the ABI burden and send the correct message.
>> It should be a build blocker, contributors need to hear the message loud and clear.
> 
> The developer needs to get or build/save the ABI reference.
> Making such ABI reference for each target is not so obvious:
>   - all symbols must be enabled (dependencies)
>   - some fixes may be needed for some compilers
> 
> 
>> Most important people _consuming_ DPDK will never see this message.
>> Only people _changing_ the ABI will see it - the people we want to hear the message loud and clear.
> 
> No, they will have issue in DPDK compilation if something in the check
> goes wrong. We should not bother end users with internal checks.
> 
> 
> The message is 
> a) run the check by
>   1) setting DPDK_ABI_REF_VERSION on command line or in devel.config file
>   2) running devtools/test-build.sh or devtools/test-meson-builds.sh
> b) check what Travis is reporting in
>   - email to you
>   - patchwork reports
> 
> I think Travis report is convenient to use.
> The local check is integrated in build scripts
> but cannot be run by default because of the reasons above.
> 
> 

Thomas the reality on this is that people have a tendency to filter this messages into an email folder
and don't always see them. 

My 2c is that this will always be a struggle unless we find a way to make it un-ignore-able.
Hence my build-wiring suggestion. 

Ray K
  

09/04/2020 18:29, Ray Kinsella:
> On 09/04/2020 16:18, Thomas Monjalon wrote:
> > 09/04/2020 16:52, Ray Kinsella:
> >> On 09/04/2020 11:59, Thomas Monjalon wrote:
> >>> 09/04/2020 12:45, Ray Kinsella:
> >>>> On 09/04/2020 11:43, Bruce Richardson wrote:
> >>>>> On Thu, Apr 09, 2020 at 06:39:54AM -0400, Neil Horman wrote:
> >>>>>> On Thu, Apr 09, 2020 at 08:57:34AM +0100, Ray Kinsella wrote:
> >>>>>>> On 08/04/2020 20:56, Neil Horman wrote:
> >>>>>>>> +The syntax of the ``check-abi.sh`` utility is::
> >>>>>>>> +
> >>>>>>>> +   ./devtools/check-abi.sh <refdir> <newdir>
> >>>>>>>
> >>>>>>> (from v1 feedback)
> >>>>>>> Could we simplify this all greatly, by telling people to use the meson/ninja build,
> >>>>>>> so they get this checking out of the box, without all the headache below?
> >>>>>>>
> >>>>>> I think bruce noted that was never merged, correct?
> >>>>>>
> >>>>> Yep, correct. :-(
> >>>>
> >>>> apologies, was there a reason?
> >>>
> >>> Because build tool job is building, not checking.
> >>> It would be wrong to make (slow) checks mandatory in all builds.
> >>>
> >>> The need is to enforce checking ABI.
> >>> The result is already published by Travis in patchwork and in an
> >>> email to the author I believe.
> >>> Not checking email and patchwork is not a good excuse.
> >>>
> >>> Patchwork must be a mandatory read for everybody for all checks
> >>> in general. Let's not give up on general CI workflow.
> >>>
> >>
> >> Thomas 
> >>
> >> You are trying to solve two problems at once; CI tooling and ABI.
> >> Let's try to solve one at a time.
> > 
> > No, you want to mix two problems in a single tool :-)
> > 
> > 
> >> 1. The ABI check, will make the build _marginally_ slower.
> >> You _should_ only need to rebuild the changes between A and B.
> > 
> > Not so marginal.
> > A re-build takes less than a second. A mandatory check takes 10 secs
> > on my machine.
> > 
> > 
> >> 2. The meson/ninja are an order of magnitude faster than GNU Make. 
> >> We can afford this check.
> > 
> > I am doing such build 10 times (each target) per patch.
> > But that's not the main issue.
> > 
> > 
> >> 3. If we want to lessen the ABI burden and send the correct message.
> >> It should be a build blocker, contributors need to hear the message loud and clear.
> > 
> > The developer needs to get or build/save the ABI reference.
> > Making such ABI reference for each target is not so obvious:
> >   - all symbols must be enabled (dependencies)
> >   - some fixes may be needed for some compilers
> > 
> > 
> >> Most important people _consuming_ DPDK will never see this message.
> >> Only people _changing_ the ABI will see it - the people we want to hear the message loud and clear.
> > 
> > No, they will have issue in DPDK compilation if something in the check
> > goes wrong. We should not bother end users with internal checks.
> > 
> > 
> > The message is 
> > a) run the check by
> >   1) setting DPDK_ABI_REF_VERSION on command line or in devel.config file
> >   2) running devtools/test-build.sh or devtools/test-meson-builds.sh
> > b) check what Travis is reporting in
> >   - email to you
> >   - patchwork reports
> > 
> > I think Travis report is convenient to use.
> > The local check is integrated in build scripts
> > but cannot be run by default because of the reasons above.
> 
> Thomas the reality on this is that people have a tendency to filter
> this messages into an email folder and don't always see them. 
> 
> My 2c is that this will always be a struggle unless we find a way
> to make it un-ignore-able.
> Hence my build-wiring suggestion. 

My other concern is that we will have the same issue with all checks
done in a CI.
I think the right approach is to enforce people checking CI results.
They will be used to check CI in patchwork because the patches will
be blocked.
  

On 09/04/2020 17:51, Thomas Monjalon wrote:
> 09/04/2020 18:29, Ray Kinsella:
>> On 09/04/2020 16:18, Thomas Monjalon wrote:
>>> 09/04/2020 16:52, Ray Kinsella:
>>>> On 09/04/2020 11:59, Thomas Monjalon wrote:
>>>>> 09/04/2020 12:45, Ray Kinsella:
>>>>>> On 09/04/2020 11:43, Bruce Richardson wrote:
>>>>>>> On Thu, Apr 09, 2020 at 06:39:54AM -0400, Neil Horman wrote:
>>>>>>>> On Thu, Apr 09, 2020 at 08:57:34AM +0100, Ray Kinsella wrote:
>>>>>>>>> On 08/04/2020 20:56, Neil Horman wrote:
>>>>>>>>>> +The syntax of the ``check-abi.sh`` utility is::
>>>>>>>>>> +
>>>>>>>>>> +   ./devtools/check-abi.sh <refdir> <newdir>
>>>>>>>>>
>>>>>>>>> (from v1 feedback)
>>>>>>>>> Could we simplify this all greatly, by telling people to use the meson/ninja build,
>>>>>>>>> so they get this checking out of the box, without all the headache below?
>>>>>>>>>
>>>>>>>> I think bruce noted that was never merged, correct?
>>>>>>>>
>>>>>>> Yep, correct. :-(
>>>>>>
>>>>>> apologies, was there a reason?
>>>>>
>>>>> Because build tool job is building, not checking.
>>>>> It would be wrong to make (slow) checks mandatory in all builds.
>>>>>
>>>>> The need is to enforce checking ABI.
>>>>> The result is already published by Travis in patchwork and in an
>>>>> email to the author I believe.
>>>>> Not checking email and patchwork is not a good excuse.
>>>>>
>>>>> Patchwork must be a mandatory read for everybody for all checks
>>>>> in general. Let's not give up on general CI workflow.
>>>>>
>>>>
>>>> Thomas 
>>>>
>>>> You are trying to solve two problems at once; CI tooling and ABI.
>>>> Let's try to solve one at a time.
>>>
>>> No, you want to mix two problems in a single tool :-)
>>>
>>>
>>>> 1. The ABI check, will make the build _marginally_ slower.
>>>> You _should_ only need to rebuild the changes between A and B.
>>>
>>> Not so marginal.
>>> A re-build takes less than a second. A mandatory check takes 10 secs
>>> on my machine.
>>>
>>>
>>>> 2. The meson/ninja are an order of magnitude faster than GNU Make. 
>>>> We can afford this check.
>>>
>>> I am doing such build 10 times (each target) per patch.
>>> But that's not the main issue.
>>>
>>>
>>>> 3. If we want to lessen the ABI burden and send the correct message.
>>>> It should be a build blocker, contributors need to hear the message loud and clear.
>>>
>>> The developer needs to get or build/save the ABI reference.
>>> Making such ABI reference for each target is not so obvious:
>>>   - all symbols must be enabled (dependencies)
>>>   - some fixes may be needed for some compilers
>>>
>>>
>>>> Most important people _consuming_ DPDK will never see this message.
>>>> Only people _changing_ the ABI will see it - the people we want to hear the message loud and clear.
>>>
>>> No, they will have issue in DPDK compilation if something in the check
>>> goes wrong. We should not bother end users with internal checks.
>>>
>>>
>>> The message is 
>>> a) run the check by
>>>   1) setting DPDK_ABI_REF_VERSION on command line or in devel.config file
>>>   2) running devtools/test-build.sh or devtools/test-meson-builds.sh
>>> b) check what Travis is reporting in
>>>   - email to you
>>>   - patchwork reports
>>>
>>> I think Travis report is convenient to use.
>>> The local check is integrated in build scripts
>>> but cannot be run by default because of the reasons above.
>>
>> Thomas the reality on this is that people have a tendency to filter
>> this messages into an email folder and don't always see them. 
>>
>> My 2c is that this will always be a struggle unless we find a way
>> to make it un-ignore-able.
>> Hence my build-wiring suggestion. 
> 
> My other concern is that we will have the same issue with all checks
> done in a CI.
> I think the right approach is to enforce people checking CI results.

Beyond asking maintainers to check, how would we enforce?

> They will be used to check CI in patchwork because the patches will
> be blocked.
>
  

10/04/2020 08:26, Ray Kinsella:
> On 09/04/2020 17:51, Thomas Monjalon wrote:
> > 09/04/2020 18:29, Ray Kinsella:
> >> On 09/04/2020 16:18, Thomas Monjalon wrote:
> >>> 09/04/2020 16:52, Ray Kinsella:
> >>>> On 09/04/2020 11:59, Thomas Monjalon wrote:
> >>>>> 09/04/2020 12:45, Ray Kinsella:
> >>>>>> On 09/04/2020 11:43, Bruce Richardson wrote:
> >>>>>>> On Thu, Apr 09, 2020 at 06:39:54AM -0400, Neil Horman wrote:
> >>>>>>>> On Thu, Apr 09, 2020 at 08:57:34AM +0100, Ray Kinsella wrote:
> >>>>>>>>> On 08/04/2020 20:56, Neil Horman wrote:
> >>>>>>>>>> +The syntax of the ``check-abi.sh`` utility is::
> >>>>>>>>>> +
> >>>>>>>>>> +   ./devtools/check-abi.sh <refdir> <newdir>
> >>>>>>>>>
> >>>>>>>>> (from v1 feedback)
> >>>>>>>>> Could we simplify this all greatly, by telling people to use the meson/ninja build,
> >>>>>>>>> so they get this checking out of the box, without all the headache below?
> >>>>>>>>>
> >>>>>>>> I think bruce noted that was never merged, correct?
> >>>>>>>>
> >>>>>>> Yep, correct. :-(
> >>>>>>
> >>>>>> apologies, was there a reason?
> >>>>>
> >>>>> Because build tool job is building, not checking.
> >>>>> It would be wrong to make (slow) checks mandatory in all builds.
> >>>>>
> >>>>> The need is to enforce checking ABI.
> >>>>> The result is already published by Travis in patchwork and in an
> >>>>> email to the author I believe.
> >>>>> Not checking email and patchwork is not a good excuse.
> >>>>>
> >>>>> Patchwork must be a mandatory read for everybody for all checks
> >>>>> in general. Let's not give up on general CI workflow.
> >>>>>
> >>>>
> >>>> Thomas 
> >>>>
> >>>> You are trying to solve two problems at once; CI tooling and ABI.
> >>>> Let's try to solve one at a time.
> >>>
> >>> No, you want to mix two problems in a single tool :-)
> >>>
> >>>
> >>>> 1. The ABI check, will make the build _marginally_ slower.
> >>>> You _should_ only need to rebuild the changes between A and B.
> >>>
> >>> Not so marginal.
> >>> A re-build takes less than a second. A mandatory check takes 10 secs
> >>> on my machine.
> >>>
> >>>
> >>>> 2. The meson/ninja are an order of magnitude faster than GNU Make. 
> >>>> We can afford this check.
> >>>
> >>> I am doing such build 10 times (each target) per patch.
> >>> But that's not the main issue.
> >>>
> >>>
> >>>> 3. If we want to lessen the ABI burden and send the correct message.
> >>>> It should be a build blocker, contributors need to hear the message loud and clear.
> >>>
> >>> The developer needs to get or build/save the ABI reference.
> >>> Making such ABI reference for each target is not so obvious:
> >>>   - all symbols must be enabled (dependencies)
> >>>   - some fixes may be needed for some compilers
> >>>
> >>>
> >>>> Most important people _consuming_ DPDK will never see this message.
> >>>> Only people _changing_ the ABI will see it - the people we want to hear the message loud and clear.
> >>>
> >>> No, they will have issue in DPDK compilation if something in the check
> >>> goes wrong. We should not bother end users with internal checks.
> >>>
> >>>
> >>> The message is 
> >>> a) run the check by
> >>>   1) setting DPDK_ABI_REF_VERSION on command line or in devel.config file
> >>>   2) running devtools/test-build.sh or devtools/test-meson-builds.sh
> >>> b) check what Travis is reporting in
> >>>   - email to you
> >>>   - patchwork reports
> >>>
> >>> I think Travis report is convenient to use.
> >>> The local check is integrated in build scripts
> >>> but cannot be run by default because of the reasons above.
> >>
> >> Thomas the reality on this is that people have a tendency to filter
> >> this messages into an email folder and don't always see them. 
> >>
> >> My 2c is that this will always be a struggle unless we find a way
> >> to make it un-ignore-able.
> >> Hence my build-wiring suggestion. 
> > 
> > My other concern is that we will have the same issue with all checks
> > done in a CI.
> > I think the right approach is to enforce people checking CI results.
> 
> Beyond asking maintainers to check, how would we enforce?

Because of the reason below...

> > They will be used to check CI in patchwork because the patches will
> > be blocked.