[dpdk-dev] [PATCH] Remove abi_versioning.sh from tree

[dpdk-dev] [PATCH] Remove abi_versioning.sh from tree

[Neil Horman]

Since we've moved away from our initial abi_versioning.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
---
 MAINTAINERS                                |   1 -
 devtools/validate-abi.sh                   | 251 ---------------------
 doc/guides/contributing/abi_versioning.rst |  18 +-
 3 files changed, 9 insertions(+), 261 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..1c4a3f927 100644
--- a/doc/guides/contributing/abi_versioning.rst
+++ b/doc/guides/contributing/abi_versioning.rst
@@ -482,9 +482,9 @@ 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::
@@ -492,9 +492,9 @@ 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::
+The syntax of the ``check-abi.sh`` utility is::
 
-   ./devtools/validate-abi.sh <REV1> <REV2>
+   ./devtools/check-abi.sh <REV1> <REV2>
 
 Where ``REV1`` and ``REV2`` are valid gitrevisions(7)
 https://www.kernel.org/pub/software/scm/git/docs/gitrevisions.html
@@ -503,16 +503,16 @@ on the local repo.
 For example::
 
    # Check between the previous and latest commit:
-   ./devtools/validate-abi.sh HEAD~1 HEAD
+   ./devtools/check-abi.sh HEAD~1 HEAD
 
    # Check on a specific compilation target:
-   ./devtools/validate-abi.sh -t x86_64-native-linux-gcc HEAD~1 HEAD
+   ./devtools/check-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
+   ./devtools/check-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
+   ./devtools/check-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
-- 
2.25.1

Re: [dpdk-dev] [PATCH] Remove abi_versioning.sh from tree

[David Marchand]

On Mon, Apr 6, 2020 at 9:34 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> Since we've moved away from our initial abi_versioning.sh script, in

abi_versioning.sh does not exist (idem with the patch title).
I suppose you meant validate-abi.sh.

> favor of check_abi.sh, which uses libabigail, remove the old script from

check-abi.sh

> the tree, and update the docs accordingly
>

[snip]

> diff --git a/doc/guides/contributing/abi_versioning.rst b/doc/guides/contributing/abi_versioning.rst
> index a21f4e7a4..1c4a3f927 100644
> --- a/doc/guides/contributing/abi_versioning.rst
> +++ b/doc/guides/contributing/abi_versioning.rst
> @@ -482,9 +482,9 @@ 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::
> @@ -492,9 +492,9 @@ 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::
> +The syntax of the ``check-abi.sh`` utility is::
>
> -   ./devtools/validate-abi.sh <REV1> <REV2>
> +   ./devtools/check-abi.sh <REV1> <REV2>

The new script is not a direct replacement.
It won't take git revisions, but build directories where versions of
dpdk have been compiled.

devtools/test-build.sh and devtools/test-meson-builds.sh illustrate its use.
https://git.dpdk.org/dpdk/tree/devtools/test-meson-builds.sh#n127



-- 
David Marchand

Re: [dpdk-dev] [PATCH] Remove abi_versioning.sh from tree

[Neil Horman]

On Tue, Apr 07, 2020 at 09:36:17AM +0200, David Marchand wrote:
> On Mon, Apr 6, 2020 at 9:34 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > Since we've moved away from our initial abi_versioning.sh script, in
> 
> abi_versioning.sh does not exist (idem with the patch title).
> I suppose you meant validate-abi.sh.
> 
Crud, you're right, I was convoluting terms, sorry.  Shall I repost with a
corrected changelog?

Neil

> > favor of check_abi.sh, which uses libabigail, remove the old script from
> 
> check-abi.sh
> 
> > the tree, and update the docs accordingly
> >
> 
> [snip]
> 
> > diff --git a/doc/guides/contributing/abi_versioning.rst b/doc/guides/contributing/abi_versioning.rst
> > index a21f4e7a4..1c4a3f927 100644
> > --- a/doc/guides/contributing/abi_versioning.rst
> > +++ b/doc/guides/contributing/abi_versioning.rst
> > @@ -482,9 +482,9 @@ 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::
> > @@ -492,9 +492,9 @@ 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::
> > +The syntax of the ``check-abi.sh`` utility is::
> >
> > -   ./devtools/validate-abi.sh <REV1> <REV2>
> > +   ./devtools/check-abi.sh <REV1> <REV2>
> 
> The new script is not a direct replacement.
> It won't take git revisions, but build directories where versions of
> dpdk have been compiled.
> 
> devtools/test-build.sh and devtools/test-meson-builds.sh illustrate its use.
> https://git.dpdk.org/dpdk/tree/devtools/test-meson-builds.sh#n127
> 
> 
> 
> -- 
> David Marchand
> 
> 

Re: [dpdk-dev] [PATCH] Remove abi_versioning.sh from tree

[David Marchand]

On Tue, Apr 7, 2020 at 1:33 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Tue, Apr 07, 2020 at 09:36:17AM +0200, David Marchand wrote:
> > On Mon, Apr 6, 2020 at 9:34 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > >
> > > Since we've moved away from our initial abi_versioning.sh script, in
> >
> > abi_versioning.sh does not exist (idem with the patch title).
> > I suppose you meant validate-abi.sh.
> >
> Crud, you're right, I was convoluting terms, sorry.  Shall I repost with a
> corrected changelog?

Yes, and with the other comment I did earlier, addressed too.


> > > favor of check_abi.sh, which uses libabigail, remove the old script from
> >
> > check-abi.sh
> >
> > > the tree, and update the docs accordingly
> > >
> >
> > [snip]
> >
> > > diff --git a/doc/guides/contributing/abi_versioning.rst b/doc/guides/contributing/abi_versioning.rst
> > > index a21f4e7a4..1c4a3f927 100644
> > > --- a/doc/guides/contributing/abi_versioning.rst
> > > +++ b/doc/guides/contributing/abi_versioning.rst
> > > @@ -482,9 +482,9 @@ 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::
> > > @@ -492,9 +492,9 @@ 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::
> > > +The syntax of the ``check-abi.sh`` utility is::
> > >
> > > -   ./devtools/validate-abi.sh <REV1> <REV2>
> > > +   ./devtools/check-abi.sh <REV1> <REV2>
> >
> > The new script is not a direct replacement.
> > It won't take git revisions, but build directories where versions of
> > dpdk have been compiled.
> >
> > devtools/test-build.sh and devtools/test-meson-builds.sh illustrate its use.
> > https://git.dpdk.org/dpdk/tree/devtools/test-meson-builds.sh#n127
> >

Thanks.


-- 
David Marchand

Re: [dpdk-dev] [PATCH] Remove abi_versioning.sh from tree

[Thomas Monjalon]

07/04/2020 13:33, Neil Horman:
> On Tue, Apr 07, 2020 at 09:36:17AM +0200, David Marchand wrote:
> > On Mon, Apr 6, 2020 at 9:34 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > >
> > > Since we've moved away from our initial abi_versioning.sh script, in
> > 
> > abi_versioning.sh does not exist (idem with the patch title).
> > I suppose you meant validate-abi.sh.
> > 
> Crud, you're right, I was convoluting terms, sorry.  Shall I repost with a
> corrected changelog?

Not only the commit log, look below how you did not care about the basic
usage of the new tool.


> > > favor of check_abi.sh, which uses libabigail, remove the old script from
> > 
> > check-abi.sh
> > 
> > > the tree, and update the docs accordingly
[...]
> > > --- a/doc/guides/contributing/abi_versioning.rst
> > > +++ b/doc/guides/contributing/abi_versioning.rst

The maintainer of doc/guides/contributing/abi_*.rst
is Ray Kinsella so I add him as Cc.

[...]
> > > -The syntax of the ``validate-abi.sh`` utility is::
> > > +The syntax of the ``check-abi.sh`` utility is::
> > >
> > > -   ./devtools/validate-abi.sh <REV1> <REV2>
> > > +   ./devtools/check-abi.sh <REV1> <REV2>
> > 
> > The new script is not a direct replacement.
> > It won't take git revisions, but build directories where versions of
> > dpdk have been compiled.
> > 
> > devtools/test-build.sh and devtools/test-meson-builds.sh illustrate its use.
> > https://git.dpdk.org/dpdk/tree/devtools/test-meson-builds.sh#n127

David, I think Neil did not take time to understand what changed in
ABI tooling.
I really wonder who is the real maintainer of ABI tooling and policy.
Neil, Ray, I was expecting a better involvement in this major
policy enforcement.

This is where we are:
	- Neil asked first for ABI compatibility
	- Neil created validate-abi.sh
	- Ray asked for a strict policy
	- Kevin worked on a new tooling
	- David completed the tooling work
	- David integrated ABI checks in Travis

There are many people partly involved.
I think we need one person truly involved in ABI questions,
someone who feels responsible and will take care of details
like the documentation update requested above.

Please don't rely on David and myself, we are already very busy
with making sure every patches are properly reviewed.
We need good help on the ABI topic in general.

Re: [dpdk-dev] [PATCH] Remove abi_versioning.sh from tree

[Neil Horman]

On Tue, Apr 07, 2020 at 01:58:57PM +0200, Thomas Monjalon wrote:
> 07/04/2020 13:33, Neil Horman:
> > On Tue, Apr 07, 2020 at 09:36:17AM +0200, David Marchand wrote:
> > > On Mon, Apr 6, 2020 at 9:34 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > >
> > > > Since we've moved away from our initial abi_versioning.sh script, in
> > > 
> > > abi_versioning.sh does not exist (idem with the patch title).
> > > I suppose you meant validate-abi.sh.
> > > 
> > Crud, you're right, I was convoluting terms, sorry.  Shall I repost with a
> > corrected changelog?
> 
> Not only the commit log, look below how you did not care about the basic
> usage of the new tool.
> 
> 
> > > > favor of check_abi.sh, which uses libabigail, remove the old script from
> > > 
> > > check-abi.sh
> > > 
> > > > the tree, and update the docs accordingly
> [...]
> > > > --- a/doc/guides/contributing/abi_versioning.rst
> > > > +++ b/doc/guides/contributing/abi_versioning.rst
> 
> The maintainer of doc/guides/contributing/abi_*.rst
> is Ray Kinsella so I add him as Cc.
> 
> [...]
> > > > -The syntax of the ``validate-abi.sh`` utility is::
> > > > +The syntax of the ``check-abi.sh`` utility is::
> > > >
> > > > -   ./devtools/validate-abi.sh <REV1> <REV2>
> > > > +   ./devtools/check-abi.sh <REV1> <REV2>
> > > 
> > > The new script is not a direct replacement.
> > > It won't take git revisions, but build directories where versions of
> > > dpdk have been compiled.
> > > 
> > > devtools/test-build.sh and devtools/test-meson-builds.sh illustrate its use.
> > > https://git.dpdk.org/dpdk/tree/devtools/test-meson-builds.sh#n127
> 
> David, I think Neil did not take time to understand what changed in
> ABI tooling.
> I really wonder who is the real maintainer of ABI tooling and policy.
> Neil, Ray, I was expecting a better involvement in this major
> policy enforcement.
> 
> This is where we are:
> 	- Neil asked first for ABI compatibility
> 	- Neil created validate-abi.sh
> 	- Ray asked for a strict policy
> 	- Kevin worked on a new tooling
> 	- David completed the tooling work
> 	- David integrated ABI checks in Travis
> 
> There are many people partly involved.
> I think we need one person truly involved in ABI questions,
> someone who feels responsible and will take care of details
> like the documentation update requested above.
> 
> Please don't rely on David and myself, we are already very busy
> with making sure every patches are properly reviewed.
> We need good help on the ABI topic in general.
> 
We're all very busy Thomas, theres no need to get bent out of shape.  You're
right, I thought this was easier than it was, thinking that check-abi.sh was
interface compatible, apologies. I'm going through this more carefully, and will
repost soon.

FWIW, I don't think I'll be able to fill this role full time.  I'm not sure if
anyone else is either, i'm afraid.
Neil

> 
> 

Re: [dpdk-dev] [PATCH] Remove abi_versioning.sh from tree

[Ray Kinsella]

On 07/04/2020 12:58, Thomas Monjalon wrote:
> 07/04/2020 13:33, Neil Horman:
>> On Tue, Apr 07, 2020 at 09:36:17AM +0200, David Marchand wrote:
>>> On Mon, Apr 6, 2020 at 9:34 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>>>>
>>>> Since we've moved away from our initial abi_versioning.sh script, in
>>>
>>> abi_versioning.sh does not exist (idem with the patch title).
>>> I suppose you meant validate-abi.sh.
>>>
>> Crud, you're right, I was convoluting terms, sorry.  Shall I repost with a
>> corrected changelog?
> 
> Not only the commit log, look below how you did not care about the basic
> usage of the new tool.
> 
> 
>>>> favor of check_abi.sh, which uses libabigail, remove the old script from
>>>
>>> check-abi.sh
>>>
>>>> the tree, and update the docs accordingly
> [...]
>>>> --- a/doc/guides/contributing/abi_versioning.rst
>>>> +++ b/doc/guides/contributing/abi_versioning.rst
> 
> The maintainer of doc/guides/contributing/abi_*.rst
> is Ray Kinsella so I add him as Cc.

Thanks for that ... 

> 
> [...]
>>>> -The syntax of the ``validate-abi.sh`` utility is::
>>>> +The syntax of the ``check-abi.sh`` utility is::
>>>>
>>>> -   ./devtools/validate-abi.sh <REV1> <REV2>
>>>> +   ./devtools/check-abi.sh <REV1> <REV2>
>>>
>>> The new script is not a direct replacement.
>>> It won't take git revisions, but build directories where versions of
>>> dpdk have been compiled.
>>>
>>> devtools/test-build.sh and devtools/test-meson-builds.sh illustrate its use.
>>> https://git.dpdk.org/dpdk/tree/devtools/test-meson-builds.sh#n127
> 
> David, I think Neil did not take time to understand what changed in
> ABI tooling.
> I really wonder who is the real maintainer of ABI tooling and policy.
> Neil, Ray, I was expecting a better involvement in this major
> policy enforcement.

I missed it on the ML, was check-maintainers.sh run?
I am just getting FYI'ed now, right?

> 
> This is where we are:
> 	- Neil asked first for ABI compatibility
> 	- Neil created validate-abi.sh
> 	- Ray asked for a strict policy

Feedback on that separately. 

> 	- Kevin worked on a new tooling
> 	- David completed the tooling work
> 	- David integrated ABI checks in Travis
> 
> There are many people partly involved.
> I think we need one person truly involved in ABI questions,
> someone who feels responsible and will take care of details
> like the documentation update requested above.
> 
> Please don't rely on David and myself, we are already very busy
> with making sure every patches are properly reviewed.
> We need good help on the ABI topic in general.

Always happy to help. 



 

Re: [dpdk-dev] [PATCH] Remove abi_versioning.sh from tree

[Ray Kinsella]

On 06/04/2020 20:34, Neil Horman wrote:
> Since we've moved away from our initial abi_versioning.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
> ---
>  MAINTAINERS                                |   1 -
>  devtools/validate-abi.sh                   | 251 ---------------------
>  doc/guides/contributing/abi_versioning.rst |  18 +-
>  3 files changed, 9 insertions(+), 261 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..1c4a3f927 100644
> --- a/doc/guides/contributing/abi_versioning.rst
> +++ b/doc/guides/contributing/abi_versioning.rst
> @@ -482,9 +482,9 @@ Running the ABI Validator
>  -------------------------
>

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?

>  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 

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

>  
>  This has a dependency on the ``abi-compliance-checker`` and ``and abi-dumper``
>  utilities which can be installed via a package manager. For example::
> @@ -492,9 +492,9 @@ 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::
> +The syntax of the ``check-abi.sh`` utility is::
>  
> -   ./devtools/validate-abi.sh <REV1> <REV2>
> +   ./devtools/check-abi.sh <REV1> <REV2>
>  
>  Where ``REV1`` and ``REV2`` are valid gitrevisions(7)
>  https://www.kernel.org/pub/software/scm/git/docs/gitrevisions.html
> @@ -503,16 +503,16 @@ on the local repo.
>  For example::
>  
>     # Check between the previous and latest commit:
> -   ./devtools/validate-abi.sh HEAD~1 HEAD
> +   ./devtools/check-abi.sh HEAD~1 HEAD
>  
>     # Check on a specific compilation target:
> -   ./devtools/validate-abi.sh -t x86_64-native-linux-gcc HEAD~1 HEAD
> +   ./devtools/check-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
> +   ./devtools/check-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
> +   ./devtools/check-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
> 

Re: [dpdk-dev] [PATCH] Remove abi_versioning.sh from tree

[Ray Kinsella]

On 07/04/2020 08:36, David Marchand wrote:
> On Mon, Apr 6, 2020 at 9:34 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>>
>> Since we've moved away from our initial abi_versioning.sh script, in
> 
> abi_versioning.sh does not exist (idem with the patch title).
> I suppose you meant validate-abi.sh.
> 
>> favor of check_abi.sh, which uses libabigail, remove the old script from
> 
> check-abi.sh
> 
>> the tree, and update the docs accordingly
>>
> 
> [snip]
> 
>> diff --git a/doc/guides/contributing/abi_versioning.rst b/doc/guides/contributing/abi_versioning.rst
>> index a21f4e7a4..1c4a3f927 100644
>> --- a/doc/guides/contributing/abi_versioning.rst
>> +++ b/doc/guides/contributing/abi_versioning.rst
>> @@ -482,9 +482,9 @@ 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::
>> @@ -492,9 +492,9 @@ 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::
>> +The syntax of the ``check-abi.sh`` utility is::
>>
>> -   ./devtools/validate-abi.sh <REV1> <REV2>
>> +   ./devtools/check-abi.sh <REV1> <REV2>
> 
> The new script is not a direct replacement.
> It won't take git revisions, but build directories where versions of
> dpdk have been compiled.
> 
> devtools/test-build.sh and devtools/test-meson-builds.sh illustrate its use.
> https://git.dpdk.org/dpdk/tree/devtools/test-meson-builds.sh#n127
> 
> 
As described in my other - should we just direct contributors to use the meson/ninja build?

Re: [dpdk-dev] [PATCH] Remove abi_versioning.sh from tree

[Thomas Monjalon]

08/04/2020 16:34, Ray Kinsella:
> On 07/04/2020 12:58, Thomas Monjalon wrote:
> > I really wonder who is the real maintainer of ABI tooling and policy.
> > Neil, Ray, I was expecting a better involvement in this major
> > policy enforcement.
> 
> I missed it on the ML, was check-maintainers.sh run?
> I am just getting FYI'ed now, right?

Sorry I was probably not clear.
I am talking about all ABI discussions we have in general
on the mailing list.
Sometimes we need help with the tools, sometimes with the doc,
more often we need helping those having ABI issues or questions.
If we don't timely reply to all concerns, ABI enforcement fails.

And let's be even clearer:
I feel too many ABI concerns are managed by David and myself.


> > This is where we are:
> > 	- Neil asked first for ABI compatibility
> > 	- Neil created validate-abi.sh
> > 	- Ray asked for a strict policy
> 
> Feedback on that separately. 
> 
> > 	- Kevin worked on a new tooling
> > 	- David completed the tooling work
> > 	- David integrated ABI checks in Travis
> > 
> > There are many people partly involved.
> > I think we need one person truly involved in ABI questions,
> > someone who feels responsible and will take care of details
> > like the documentation update requested above.
> > 
> > Please don't rely on David and myself, we are already very busy
> > with making sure every patches are properly reviewed.
> > We need good help on the ABI topic in general.
> 
> Always happy to help. 

Please help in emails about ABI issues, including rte_internal marker:
	http://inbox.dpdk.org/dev/?q=ABI
Thanks

Re: [dpdk-dev] [PATCH] Remove abi_versioning.sh from tree

[Bruce Richardson]

On Wed, Apr 08, 2020 at 03:49:49PM +0100, Ray Kinsella wrote:
> 
> 
> On 06/04/2020 20:34, Neil Horman wrote:
> > Since we've moved away from our initial abi_versioning.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
> > ---
> >  MAINTAINERS                                |   1 -
> >  devtools/validate-abi.sh                   | 251 ---------------------
> >  doc/guides/contributing/abi_versioning.rst |  18 +-
> >  3 files changed, 9 insertions(+), 261 deletions(-)
> >  delete mode 100755 devtools/validate-abi.sh
> > 
<snip>
> diff --git a/doc/guides/contributing/abi_versioning.rst b/doc/guides/contributing/abi_versioning.rst
> > index a21f4e7a4..1c4a3f927 100644
> > --- a/doc/guides/contributing/abi_versioning.rst
> > +++ b/doc/guides/contributing/abi_versioning.rst
> > @@ -482,9 +482,9 @@ Running the ABI Validator
> >  -------------------------
> >
> 
> 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?
> 
The abi checks are not merged into the meson build - the idea was proposed
and prototyped but never merged.

Regards,
/Bruce

Re: [dpdk-dev] [PATCH] Remove abi_versioning.sh from tree

[Ray Kinsella]

On 09/04/2020 10:26, Bruce Richardson wrote:
> On Wed, Apr 08, 2020 at 03:49:49PM +0100, Ray Kinsella wrote:
>>
>>
>> On 06/04/2020 20:34, Neil Horman wrote:
>>> Since we've moved away from our initial abi_versioning.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
>>> ---
>>>  MAINTAINERS                                |   1 -
>>>  devtools/validate-abi.sh                   | 251 ---------------------
>>>  doc/guides/contributing/abi_versioning.rst |  18 +-
>>>  3 files changed, 9 insertions(+), 261 deletions(-)
>>>  delete mode 100755 devtools/validate-abi.sh
>>>
> <snip>
>> diff --git a/doc/guides/contributing/abi_versioning.rst b/doc/guides/contributing/abi_versioning.rst
>>> index a21f4e7a4..1c4a3f927 100644
>>> --- a/doc/guides/contributing/abi_versioning.rst
>>> +++ b/doc/guides/contributing/abi_versioning.rst
>>> @@ -482,9 +482,9 @@ Running the ABI Validator
>>>  -------------------------
>>>
>>
>> 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?
>>
> The abi checks are not merged into the meson build - the idea was proposed
> and prototyped but never merged.

ah my mistake ... that is a shame
we need these kind of self check tooling.

> 
> Regards,
> /Bruce
>