[libcamera-devel] [PATCH v2 1/2] utils: ABI Compatibility checker

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Mon, May 15, 2023 at 10:58:11AM +0100, Kieran Bingham via libcamera-devel wrote:
> Provide support to compare ABI compatibility between any two git commits
> or by a commit and the most recent ancestral tag of that commit.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  utils/abi-compat.sh | 198 ++++++++++++++++++++++++++++++++++++++++++++

The README.rst file should be updated to list the new dependency on
abi-compliance-checker.

>  1 file changed, 198 insertions(+)
>  create mode 100755 utils/abi-compat.sh
> 
> diff --git a/utils/abi-compat.sh b/utils/abi-compat.sh
> new file mode 100755
> index 000000000000..b1944dc7d594
> --- /dev/null
> +++ b/utils/abi-compat.sh
> @@ -0,0 +1,198 @@

Missing SPDX tag.

> +#!/bin/bash
> +
> +NAME=$(basename "$0")

Missing blank line.

> +usage() {
> +cat << EOF

Missing indentation.

> +$NAME: Determine the ABI/API compatibility of two build versions
> +
> +  $NAME [--help] [--abi-dir=<PATH>] [--tmp-dir=<PATH>] ARGS
> +
> +The positional arguments (ARGS) determine the versions that will be compared and
> +take three variants:
> +
> +  - No positional arguments:
> +      $NAME [optional arguments]
> +
> +      It is assumed to compare the current git HEAD against the most recent TAG
> +
> +  - One positional argument:
> +      $NAME [optional aguments] COMMITISH
> +
> +      The given COMMITISH is compared against it's most recent TAG
> +
> +  - Two positional arguments:
> +      $NAME [optional aguments] BASE COMMITISH

If you swapped the arguments, parsing would be easier.

> +
> +      The given COMMITISH is compared against the given BASE.
> +
> +Optional Arguments:
> +  --abi-dir <path> Use <path> for storing (or retrieving existing) ABI data
> +                   files
> +
> +  --tmp-dir <path> Specify temporary build location for building ABI data.
> +                   This could be a tmpfs/RAM disk to save on disk writes.
> +EOF
> +}
> +
> +POSITIONAL=()
> +while [[ $# -gt 0 ]]
> +do
> +case $1 in

while [[ $# -gt 0 ]] ; do
	option=$1
	shift

	case $option in

This will allow you to drop multiple shifts below.

> +    -h|--help)
> +        usage
> +	exit 127;

Why 127 ?

> +        ;;

There's a mix of tabs and spaces for indentation, here and below.

> +
> +    --abi-dir)
> +	ABI_DIR=$2
> +	shift; shift;
> +	;;
> +
> +    --tmp-dir)
> +	TMP=$2
> +	shift; shift;
> +	;;
> +

	-*)
		error
		;;

> +    *)  # Parse unidentified arguments based on position
> +        POSITIONAL+=("$1")
> +        shift # past argument
> +        ;;
> +esac
> +done
> +set -- "${POSITIONAL[@]}" # restore positional parameters

It would be nice to set the from and to variables directly, to group all
argument parsing in one place.

> +
> +ABI_DIR=${ABI_DIR:-abi}
> +TMP=${TMP:-"$ABI_DIR/tmp/"}
> +
> +mkdir -p "$ABI_DIR"
> +mkdir -p "$TMP"
> +
> +dbg () {
> +	echo "$@" >> /dev/stderr

	echo "$@" >&2

Same below.

> +}

Could we move all functions to the top of the file, before the "main"
code ?

> +
> +die () {
> +	echo "$NAME: $*" >> /dev/stderr
> +	exit 128

The usual exit code in case of failure is 1.

> +}
> +
> +prev_release () {
> +	git describe --tags --abbrev=0 "$1"^ \
> +		|| die "Failed to identify previous release tag from $1"
> +}
> +
> +describe () {
> +	git describe --tags "$@" \

Should this be $1 ?

> +		|| die "Failed to describe $1"
> +}
> +
> +# Make sure we exit on errors during argument parsing
> +set -Eeuo pipefail

You've parsed the arguments already :-)

> +
> +# Check HEAD against previous 'release'
> +if test $# -eq 0;

You use [[ above instead of test, let's be consistent.

if [[ $# -eq 0 ]] ; then

same below.

Would the following be more readable ? Up to you.

case $# in
	0)
		FROM=$(prev_release HEAD)
		TO=$(describe HEAD)
		;;
	1)
		FROM=$(prev_release "$1")
		TO=$(describe "$1")
		;;
	2)
		FROM=$(describe "$1")
		TO=$(describe "$2")
		;;
	*)
		error
		;;
esac

> +then
> +	FROM=$(prev_release HEAD)
> +	TO=$(describe HEAD)
> +fi
> +
> +# Check COMMIT against previous release
> +if test $# -eq 1;
> +then
> +	FROM=$(prev_release "$1")
> +	TO=$(describe "$1")
> +fi
> +
> +# Check ABI between FROM and TO explicitly
> +if test $# -eq 2;
> +then
> +	FROM=$(describe "$1")
> +	TO=$(describe "$2")
> +fi
> +
> +echo "Validating ABI compatibility between $FROM and $TO"
> +
> +# Generate an abi-compliance-checker xml description file

s/$/./

Same where applicable.

> +# $PREFIX is assumed to be /usr/local/

Maybe set prefix to /usr/local/ explicitly when running meson setup
instead of assuming it ?

> +create_xml() {
> +	# $1: Output file
> +	# $2: Version
> +	# $3: Install root

Instead of using comments, I find it more explicit to use local
variables:

	local output_file="$1"
	local version="$2"
	local install_root="$3"

And use the variables below.

> +
> +	echo "<version>$2</version>" > "$1"
> +	echo "<headers>$3/usr/local/include/</headers>" >> "$1"
> +	echo "<libs>$3/usr/local/lib/</libs>" >> "$1"
> +}
> +
> +# Check if an ABI dump file exists, and if not create one
> +# by building a minimal configuration of libcamera at the specified
> +# version using a clean worktree.
> +create_abi_dump() {
> +	VERSION="$1"
> +	ABI_FILE="$ABI_DIR/$VERSION.abi.dump"
> +	WORKTREE="$TMP/$VERSION"
> +	BUILD="$TMP/$VERSION-build"

Make these local variables.

> +
> +	# Use a fully qualified path when calling ninja -C
> +	INSTALL=$(realpath "$TMP/$VERSION-install")
> +
> +	if test ! -e "$ABI_FILE";
> +	then

	if [[ ! -e "$ABI_FILE" ]] ; then

> +		dbg "Creating ABI dump for $VERSION in $ABI_DIR"
> +		git worktree add "$WORKTREE" "$VERSION"
> +
> +		meson setup "$BUILD" "$WORKTREE" \
> +			-Ddocumentation=disabled \
> +			-Dcam=disabled \
> +			-Dqcam=disabled \
> +			-Dgstreamer=disabled \
> +			-Dlc-compliance=disabled \
> +			-Dtracing=disabled \
> +			-Dpipelines=
> +
> +		ninja -C "$BUILD"
> +		DESTDIR="$INSTALL" ninja -C "$BUILD" install
> +
> +		# Create an xml descriptor with parameters to generate the dump file
> +		create_xml \
> +			"$INSTALL/libcamera-abi-dump.xml" \
> +			"$VERSION" \
> +			"$INSTALL"
> +
> +
> +		# We currently have warnings produced that we can ignore
> +		# While we have pipefail set, finish with || true

Do we need pipefail ?

> +		abi-compliance-checker \

Should we have a check that abi-compliance-checker exists, with a nice
error message if it doesn't ?

> +			-lib libcamera \
> +			-v1 "$VERSION" \
> +			-dump "$INSTALL/libcamera-abi-dump.xml" \
> +			-dump-path "$ABI_FILE" || true
> +
> +		dbg Created "$ABI_FILE"
> +
> +		dbg Removing Worktree "$WORKTREE"
> +		git worktree remove -f "$WORKTREE"
> +
> +		dbg Removing "$BUILD"
> +		rm -r "$BUILD"
> +
> +		dbg Removing "$INSTALL"
> +		rm -r "$INSTALL"
> +	fi
> +}
> +
> +# Create the requested ABI dump files if they don't yet exist
> +create_abi_dump "$FROM"
> +create_abi_dump "$TO"
> +
> +# Todo: Future iterations and extensions here could add "-stdout -xml" and
> +# parse the results automatically
> +abi-compliance-checker -l libcamera \
> +	-old "$ABI_DIR/$FROM.abi.dump" \
> +	-new "$ABI_DIR/$TO.abi.dump" \
> +	;

Is the semicolon needed ?

> +
> +# On (far too many) occasions, the tools keep running leaving a cpu core @ 100%
> +# CPU usage. Perhaps some subprocess gets launched but never rejoined. Stop
> +# them all

Please add a TODO comment to remind we should investigate this.

> +killall abi-compliance-checker 2>/dev/null

It would be nicer to kill only the process that was launched on the
previous line, not all instances of abi-compliance-checker.

-- 
Regards,

Laurent Pinchart