[libcamera-devel] [PATCH v2 1/2] utils: ABI Compatibility checker
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jul 4 15:04:16 CEST 2023
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 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.
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
More information about the libcamera-devel
mailing list