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

18627 diff mbox series

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@ideasonboard.com>
---
 utils/abi-compat.sh | 198 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 198 insertions(+)
 create mode 100755 utils/abi-compat.sh

Hi Kieran,

Script looks fine to me for most parts but I'm encountering a few issues 
while testing.

On 5/15/23 3:28 PM, 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@ideasonboard.com>
> ---
>   utils/abi-compat.sh | 198 ++++++++++++++++++++++++++++++++++++++++++++
>   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 @@
> +#!/bin/bash
> +
> +NAME=$(basename "$0")
> +usage() {
> +cat << EOF
> +$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
> +
> +      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
> +    -h|--help)
> +        usage
> +	exit 127;
> +        ;;
> +
> +    --abi-dir)
> +	ABI_DIR=$2
> +	shift; shift;
> +	;;
> +
> +    --tmp-dir)
> +	TMP=$2
> +	shift; shift;
> +	;;
> +
> +    *)  # Parse unidentified arguments based on position
> +        POSITIONAL+=("$1")
> +        shift # past argument
> +        ;;
> +esac
> +done
> +set -- "${POSITIONAL[@]}" # restore positional parameters
> +
> +ABI_DIR=${ABI_DIR:-abi}
> +TMP=${TMP:-"$ABI_DIR/tmp/"}

this seems to generate path with additional slashes:

     Removing Worktree abi/tmp//v0.0.5-51-g7836a118
     Removing abi/tmp//v0.0.5-51-g7836a118-build

> +
> +mkdir -p "$ABI_DIR"
> +mkdir -p "$TMP"
> +
> +dbg () {
> +	echo "$@" >> /dev/stderr
> +}
> +
> +die () {
> +	echo "$NAME: $*" >> /dev/stderr
> +	exit 128
> +}
> +
> +prev_release () {
> +	git describe --tags --abbrev=0 "$1"^ \
> +		|| die "Failed to identify previous release tag from $1"
> +}
> +
> +describe () {
> +	git describe --tags "$@" \
> +		|| die "Failed to describe $1"
> +}
> +
> +# Make sure we exit on errors during argument parsing
> +set -Eeuo pipefail
> +
> +# Check HEAD against previous 'release'
> +if test $# -eq 0;
> +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
> +# $PREFIX is assumed to be /usr/local/
> +create_xml() {
> +	# $1: Output file
> +	# $2: Version
> +	# $3: Install root
> +
> +	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"
> +
> +	# Use a fully qualified path when calling ninja -C
> +	INSTALL=$(realpath "$TMP/$VERSION-install")
> +
> +	if test ! -e "$ABI_FILE";
> +	then
> +		dbg "Creating ABI dump for $VERSION in $ABI_DIR"
> +		git worktree add "$WORKTREE" "$VERSION"

I aborted the script in middle, so the worktree was added but did't 
reach the point of removal below. Re-running kept hindered by

     fatal: 'abi/tmp//v0.0.5' is a missing but already registered worktree;
     use 'add -f' to override, or 'prune' or 'remove' to clear

so I added '--force'. I guess --force will help here permanently (or an 
addition git worktree remove before git worktree add...)

> +
> +		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
> +		abi-compliance-checker \
> +			-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"

At every create_abi_dump, I get the following:

```
Using GCC 12 (x86_64-redhat-linux, target: x86_64)
WARNING: May not work properly with GCC 4.8.[0-2], 6.* and higher due to 
bug #78040 in GCC. Please try other GCC versions with the help of 
--gcc-path=PATH option or create ABI dumps by ABI Dumper tool instead to 
avoid using GCC. Test selected GCC version first by -test option.
ERROR: library objects are not found
Created abi/v0.0.5.abi.dump
Removing Worktree abi/tmp/v0.0.5
Removing abi/tmp/v0.0.5-build
Removing /home/uajain/src/libcamera/abi/tmp/v0.0.5-install
```

and it ends with
```
ERROR: can't access 'abi/v0.0.5.abi.dump
```

Full Log: https://paste.debian.net/plain/1282822

Have you run into issues like the above?
> +
> +# 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" \
> +	;
> +
> +# 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
> +killall abi-compliance-checker 2>/dev/null

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@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.

Quoting Laurent Pinchart (2023-07-04 10:03:21)
> 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@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.

Added:
# SPDX-License-Identifier: GPL-2.0-or-later

> > +#!/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.

Parsing might, but I think this is better this way for users.

 "From ... To" makes more sense that "To From".


> > +
> > +      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.

Yes, thanks.


> 
> > +    -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 ?

It may as well be - changed.

> 
> > +             || 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

Yes, a case on $# is way nicer ... not sure why I didn't do it that way
to start with ...


> > +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 ?
> 

Without it, I believe failures in the subprocessses / functions don't
propogate up. I want to catch errors everywhere except here (which is
annoying as this is the main command that runs that I want to know
succeeds - so this needs further investigation later too.

I think it's because of our use of the private headers that can't be
parsed by the abi-compliance-checker and upsets it.

And so rather than adding a todo to investigate - I've now stopped
installing the private headers from libcamera-base so I should be able
to remove the || true now.


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

Added.

> 
> > +                     -lib libcamera \
> > +                     -v1 "$VERSION" \
> > +                     -dump "$INSTALL/libcamera-abi-dump.xml" \
> > +                     -dump-path "$ABI_FILE" || true

This || true will be removed now.

It means the abi-compliance checker can't (without modifying this
script) create abi dump files before the fix to the libcamera-base - but
that will only affect me for now - so it should be a limitation that
will disappear rapidly after the next release.


> > +
> > +             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 ?
> 

It was to help me - but no - removed.


> > +
> > +# 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.
> 

Added
+# TODO: Investigate this and report upstream


> > +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.

I'll keep this as a shotgun approach. I think when I looked at this
abi-compliance-checker spawns processes that do not exit when the parent
process exits. I believe that would mean the PID of the launched process
won't exist to be killed ...

> -- 
> Regards,
> 
> Laurent Pinchart