[{"id":27327,"web_url":"https://patchwork.libcamera.org/comment/27327/","msgid":"<60262415-8d39-633f-24e0-6446d4b1c927@ideasonboard.com>","date":"2023-06-13T07:43:25","subject":"Re: [libcamera-devel] [PATCH v2 1/2] utils: ABI Compatibility\n\tchecker","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kieran,\n\nScript looks fine to me for most parts but I'm encountering a few issues \nwhile testing.\n\nOn 5/15/23 3:28 PM, Kieran Bingham via libcamera-devel wrote:\n> Provide support to compare ABI compatibility between any two git commits\n> or by a commit and the most recent ancestral tag of that commit.\n>\n> Signed-off-by: Kieran Bingham \n> ---\n> utils/abi-compat.sh | 198 ++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 198 insertions(+)\n> create mode 100755 utils/abi-compat.sh\n>\n> diff --git a/utils/abi-compat.sh b/utils/abi-compat.sh\n> new file mode 100755\n> index 000000000000..b1944dc7d594\n> --- /dev/null\n> +++ b/utils/abi-compat.sh\n> @@ -0,0 +1,198 @@\n> +#!/bin/bash\n> +\n> +NAME=$(basename \"$0\")\n> +usage() {\n> +cat << EOF\n> +$NAME: Determine the ABI/API compatibility of two build versions\n> +\n> + $NAME [--help] [--abi-dir=] [--tmp-dir=] ARGS\n> +\n> +The positional arguments (ARGS) determine the versions that will be compared and\n> +take three variants:\n> +\n> + - No positional arguments:\n> + $NAME [optional arguments]\n> +\n> + It is assumed to compare the current git HEAD against the most recent TAG\n> +\n> + - One positional argument:\n> + $NAME [optional aguments] COMMITISH\n> +\n> + The given COMMITISH is compared against it's most recent TAG\n> +\n> + - Two positional arguments:\n> + $NAME [optional aguments] BASE COMMITISH\n> +\n> + The given COMMITISH is compared against the given BASE.\n> +\n> +Optional Arguments:\n> + --abi-dir Use for storing (or retrieving existing) ABI data\n> + files\n> +\n> + --tmp-dir Specify temporary build location for building ABI data.\n> + This could be a tmpfs/RAM disk to save on disk writes.\n> +EOF\n> +}\n> +\n> +POSITIONAL=()\n> +while [[ $# -gt 0 ]]\n> +do\n> +case $1 in\n> + -h|--help)\n> + usage\n> +\texit 127;\n> + ;;\n> +\n> + --abi-dir)\n> +\tABI_DIR=$2\n> +\tshift; shift;\n> +\t;;\n> +\n> + --tmp-dir)\n> +\tTMP=$2\n> +\tshift; shift;\n> +\t;;\n> +\n> + *) # Parse unidentified arguments based on position\n> + POSITIONAL+=(\"$1\")\n> + shift # past argument\n> + ;;\n> +esac\n> +done\n> +set -- \"${POSITIONAL[@]}\" # restore positional parameters\n> +\n> +ABI_DIR=${ABI_DIR:-abi}\n> +TMP=${TMP:-\"$ABI_DIR/tmp/\"}\n\nthis seems to generate path with additional slashes:\n\n     Removing Worktree abi/tmp//v0.0.5-51-g7836a118\n     Removing abi/tmp//v0.0.5-51-g7836a118-build\n\n> +\n> +mkdir -p \"$ABI_DIR\"\n> +mkdir -p \"$TMP\"\n> +\n> +dbg () {\n> +\techo \"$@\" >> /dev/stderr\n> +}\n> +\n> +die () {\n> +\techo \"$NAME: $*\" >> /dev/stderr\n> +\texit 128\n> +}\n> +\n> +prev_release () {\n> +\tgit describe --tags --abbrev=0 \"$1\"^ \\\n> +\t\t|| die \"Failed to identify previous release tag from $1\"\n> +}\n> +\n> +describe () {\n> +\tgit describe --tags \"$@\" \\\n> +\t\t|| die \"Failed to describe $1\"\n> +}\n> +\n> +# Make sure we exit on errors during argument parsing\n> +set -Eeuo pipefail\n> +\n> +# Check HEAD against previous 'release'\n> +if test $# -eq 0;\n> +then\n> +\tFROM=$(prev_release HEAD)\n> +\tTO=$(describe HEAD)\n> +fi\n> +\n> +# Check COMMIT against previous release\n> +if test $# -eq 1;\n> +then\n> +\tFROM=$(prev_release \"$1\")\n> +\tTO=$(describe \"$1\")\n> +fi\n> +\n> +# Check ABI between FROM and TO explicitly\n> +if test $# -eq 2;\n> +then\n> +\tFROM=$(describe \"$1\")\n> +\tTO=$(describe \"$2\")\n> +fi\n> +\n> +echo \"Validating ABI compatibility between $FROM and $TO\"\n> +\n> +# Generate an abi-compliance-checker xml description file\n> +# $PREFIX is assumed to be /usr/local/\n> +create_xml() {\n> +\t# $1: Output file\n> +\t# $2: Version\n> +\t# $3: Install root\n> +\n> +\techo \"$2\" > \"$1\"\n> +\techo \"$3/usr/local/include/\" >> \"$1\"\n> +\techo \"$3/usr/local/lib/\" >> \"$1\"\n> +}\n> +\n> +# Check if an ABI dump file exists, and if not create one\n> +# by building a minimal configuration of libcamera at the specified\n> +# version using a clean worktree.\n> +create_abi_dump() {\n> +\tVERSION=\"$1\"\n> +\tABI_FILE=\"$ABI_DIR/$VERSION.abi.dump\"\n> +\tWORKTREE=\"$TMP/$VERSION\"\n> +\tBUILD=\"$TMP/$VERSION-build\"\n> +\n> +\t# Use a fully qualified path when calling ninja -C\n> +\tINSTALL=$(realpath \"$TMP/$VERSION-install\")\n> +\n> +\tif test ! -e \"$ABI_FILE\";\n> +\tthen\n> +\t\tdbg \"Creating ABI dump for $VERSION in $ABI_DIR\"\n> +\t\tgit worktree add \"$WORKTREE\" \"$VERSION\"\n\nI aborted the script in middle, so the worktree was added but did't \nreach the point of removal below. Re-running kept hindered by\n\n     fatal: 'abi/tmp//v0.0.5' is a missing but already registered worktree;\n     use 'add -f' to override, or 'prune' or 'remove' to clear\n\nso I added '--force'. I guess --force will help here permanently (or an \naddition git worktree remove before git worktree add...)\n\n> +\n> +\t\tmeson setup \"$BUILD\" \"$WORKTREE\" \\\n> +\t\t\t-Ddocumentation=disabled \\\n> +\t\t\t-Dcam=disabled \\\n> +\t\t\t-Dqcam=disabled \\\n> +\t\t\t-Dgstreamer=disabled \\\n> +\t\t\t-Dlc-compliance=disabled \\\n> +\t\t\t-Dtracing=disabled \\\n> +\t\t\t-Dpipelines=\n> +\n> +\t\tninja -C \"$BUILD\"\n> +\t\tDESTDIR=\"$INSTALL\" ninja -C \"$BUILD\" install\n> +\n> +\t\t# Create an xml descriptor with parameters to generate the dump file\n> +\t\tcreate_xml \\\n> +\t\t\t\"$INSTALL/libcamera-abi-dump.xml\" \\\n> +\t\t\t\"$VERSION\" \\\n> +\t\t\t\"$INSTALL\"\n> +\n> +\n> +\t\t# We currently have warnings produced that we can ignore\n> +\t\t# While we have pipefail set, finish with || true\n> +\t\tabi-compliance-checker \\\n> +\t\t\t-lib libcamera \\\n> +\t\t\t-v1 \"$VERSION\" \\\n> +\t\t\t-dump \"$INSTALL/libcamera-abi-dump.xml\" \\\n> +\t\t\t-dump-path \"$ABI_FILE\" || true\n> +\n> +\t\tdbg Created \"$ABI_FILE\"\n> +\n> +\t\tdbg Removing Worktree \"$WORKTREE\"\n> +\t\tgit worktree remove -f \"$WORKTREE\"\n> +\n> +\t\tdbg Removing \"$BUILD\"\n> +\t\trm -r \"$BUILD\"\n> +\n> +\t\tdbg Removing \"$INSTALL\"\n> +\t\trm -r \"$INSTALL\"\n> +\tfi\n> +}\n> +\n> +# Create the requested ABI dump files if they don't yet exist\n> +create_abi_dump \"$FROM\"\n> +create_abi_dump \"$TO\"\n\nAt every create_abi_dump, I get the following:\n\n```\nUsing GCC 12 (x86_64-redhat-linux, target: x86_64)\nWARNING: May not work properly with GCC 4.8.[0-2], 6.* and higher due to \nbug #78040 in GCC. Please try other GCC versions with the help of \n--gcc-path=PATH option or create ABI dumps by ABI Dumper tool instead to \navoid using GCC. Test selected GCC version first by -test option.\nERROR: library objects are not found\nCreated abi/v0.0.5.abi.dump\nRemoving Worktree abi/tmp/v0.0.5\nRemoving abi/tmp/v0.0.5-build\nRemoving /home/uajain/src/libcamera/abi/tmp/v0.0.5-install\n```\n\nand it ends with\n```\nERROR: can't access 'abi/v0.0.5.abi.dump\n```\n\nFull Log: https://paste.debian.net/plain/1282822\n\nHave you run into issues like the above?\n> +\n> +# Todo: Future iterations and extensions here could add \"-stdout -xml\" and\n> +# parse the results automatically\n> +abi-compliance-checker -l libcamera \\\n> +\t-old \"$ABI_DIR/$FROM.abi.dump\" \\\n> +\t-new \"$ABI_DIR/$TO.abi.dump\" \\\n> +\t;\n> +\n> +# On (far too many) occasions, the tools keep running leaving a cpu core @ 100%\n> +# CPU usage. Perhaps some subprocess gets launched but never rejoined. Stop\n> +# them all\n> +killall abi-compliance-checker 2>/dev/null","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5A3DBBD78E\n\tfor ;\n\tTue, 13 Jun 2023 07:43:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BB92561E4B;\n\tTue, 13 Jun 2023 09:43:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 968E261E47\n\tfor ;\n\tTue, 13 Jun 2023 09:43:30 +0200 (CEST)","from [192.168.1.108] (unknown [103.86.18.143])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1E7119A8;\n\tTue, 13 Jun 2023 09:42:59 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1686642212;\n\tbh=rEr4Z2AVW6ct5BZ7ljdv0v7NDHn3YDPFSb32G7KtYpY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=eVpIKnKhWClJZKhLLF8i/yxY+SjAiXAm5f3JLP5TvrzptJWHaobHLxFoUqCku81R6\n\tYcl5hYy3PkoNdYAeNyD9epbtWS4jvJ1aZ5CXYq7CtaQKY8iACBlKTFnvIGZ7dtoEu7\n\tHAZ1YfV67rp/HVIGglqeIjf3nFDypVNZl3+Hdh8x8sKhJMLuvVXAQEulOIPlNk3Hup\n\tN9pMrHTYdcFKKcUMgSfiCliKq3CU8q4TPW0Gdv2EmSXIT7nnUuD8mHf/m9TZDr1jdc\n\txCURt3rzfbsNBW5zSMA19IZjyBYOeb98ldohb+oXmBksdcgkuIeuxpalTmaZknFD19\n\t+jHl9WMh8gqMA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1686642180;\n\tbh=rEr4Z2AVW6ct5BZ7ljdv0v7NDHn3YDPFSb32G7KtYpY=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=Zth46mQuQ32n1DbbuicPxMMUrgzOixX4kjYLWkjyDhjlG46ALUb7WiQF0WW5ohIGt\n\tfTmDlEZQZSf6CeMkUgjUdHU8lshSRiL5elMykZPNDCM4gDwtyEmCpLRHnBDKUWFc0G\n\tezwoA3p7dW1/e0TSyF1FDsUp/pNMQPu9pb2GJEVI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Zth46mQu\"; dkim-atps=neutral","Message-ID":"<60262415-8d39-633f-24e0-6446d4b1c927@ideasonboard.com>","Date":"Tue, 13 Jun 2023 13:13:25 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.7.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera devel ","References":"<20230515095812.3409747-1-kieran.bingham@ideasonboard.com>\n\t<20230515095812.3409747-2-kieran.bingham@ideasonboard.com>","In-Reply-To":"<20230515095812.3409747-2-kieran.bingham@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] utils: ABI Compatibility\n\tchecker","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27455,"web_url":"https://patchwork.libcamera.org/comment/27455/","msgid":"<20230704090321.GA4778@pendragon.ideasonboard.com>","date":"2023-07-04T09:03:21","subject":"Re: [libcamera-devel] [PATCH v2 1/2] utils: ABI Compatibility\n\tchecker","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Mon, May 15, 2023 at 10:58:11AM +0100, Kieran Bingham via libcamera-devel wrote:\n> Provide support to compare ABI compatibility between any two git commits\n> or by a commit and the most recent ancestral tag of that commit.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> utils/abi-compat.sh | 198 ++++++++++++++++++++++++++++++++++++++++++++\n\nThe README.rst file should be updated to list the new dependency on\nabi-compliance-checker.\n\n> 1 file changed, 198 insertions(+)\n> create mode 100755 utils/abi-compat.sh\n> \n> diff --git a/utils/abi-compat.sh b/utils/abi-compat.sh\n> new file mode 100755\n> index 000000000000..b1944dc7d594\n> --- /dev/null\n> +++ b/utils/abi-compat.sh\n> @@ -0,0 +1,198 @@\n\nMissing SPDX tag.\n\n> +#!/bin/bash\n> +\n> +NAME=$(basename \"$0\")\n\nMissing blank line.\n\n> +usage() {\n> +cat << EOF\n\nMissing indentation.\n\n> +$NAME: Determine the ABI/API compatibility of two build versions\n> +\n> + $NAME [--help] [--abi-dir=] [--tmp-dir=] ARGS\n> +\n> +The positional arguments (ARGS) determine the versions that will be compared and\n> +take three variants:\n> +\n> + - No positional arguments:\n> + $NAME [optional arguments]\n> +\n> + It is assumed to compare the current git HEAD against the most recent TAG\n> +\n> + - One positional argument:\n> + $NAME [optional aguments] COMMITISH\n> +\n> + The given COMMITISH is compared against it's most recent TAG\n> +\n> + - Two positional arguments:\n> + $NAME [optional aguments] BASE COMMITISH\n\nIf you swapped the arguments, parsing would be easier.\n\n> +\n> + The given COMMITISH is compared against the given BASE.\n> +\n> +Optional Arguments:\n> + --abi-dir Use for storing (or retrieving existing) ABI data\n> + files\n> +\n> + --tmp-dir Specify temporary build location for building ABI data.\n> + This could be a tmpfs/RAM disk to save on disk writes.\n> +EOF\n> +}\n> +\n> +POSITIONAL=()\n> +while [[ $# -gt 0 ]]\n> +do\n> +case $1 in\n\nwhile [[ $# -gt 0 ]] ; do\n\toption=$1\n\tshift\n\n\tcase $option in\n\nThis will allow you to drop multiple shifts below.\n\n> + -h|--help)\n> + usage\n> +\texit 127;\n\nWhy 127 ?\n\n> + ;;\n\nThere's a mix of tabs and spaces for indentation, here and below.\n\n> +\n> + --abi-dir)\n> +\tABI_DIR=$2\n> +\tshift; shift;\n> +\t;;\n> +\n> + --tmp-dir)\n> +\tTMP=$2\n> +\tshift; shift;\n> +\t;;\n> +\n\n\t-*)\n\t\terror\n\t\t;;\n\n> + *) # Parse unidentified arguments based on position\n> + POSITIONAL+=(\"$1\")\n> + shift # past argument\n> + ;;\n> +esac\n> +done\n> +set -- \"${POSITIONAL[@]}\" # restore positional parameters\n\nIt would be nice to set the from and to variables directly, to group all\nargument parsing in one place.\n\n> +\n> +ABI_DIR=${ABI_DIR:-abi}\n> +TMP=${TMP:-\"$ABI_DIR/tmp/\"}\n> +\n> +mkdir -p \"$ABI_DIR\"\n> +mkdir -p \"$TMP\"\n> +\n> +dbg () {\n> +\techo \"$@\" >> /dev/stderr\n\n\techo \"$@\" >&2\n\nSame below.\n\n> +}\n\nCould we move all functions to the top of the file, before the \"main\"\ncode ?\n\n> +\n> +die () {\n> +\techo \"$NAME: $*\" >> /dev/stderr\n> +\texit 128\n\nThe usual exit code in case of failure is 1.\n\n> +}\n> +\n> +prev_release () {\n> +\tgit describe --tags --abbrev=0 \"$1\"^ \\\n> +\t\t|| die \"Failed to identify previous release tag from $1\"\n> +}\n> +\n> +describe () {\n> +\tgit describe --tags \"$@\" \\\n\nShould this be $1 ?\n\n> +\t\t|| die \"Failed to describe $1\"\n> +}\n> +\n> +# Make sure we exit on errors during argument parsing\n> +set -Eeuo pipefail\n\nYou've parsed the arguments already :-)\n\n> +\n> +# Check HEAD against previous 'release'\n> +if test $# -eq 0;\n\nYou use [[ above instead of test, let's be consistent.\n\nif [[ $# -eq 0 ]] ; then\n\nsame below.\n\nWould the following be more readable ? Up to you.\n\ncase $# in\n\t0)\n\t\tFROM=$(prev_release HEAD)\n\t\tTO=$(describe HEAD)\n\t\t;;\n\t1)\n\t\tFROM=$(prev_release \"$1\")\n\t\tTO=$(describe \"$1\")\n\t\t;;\n\t2)\n\t\tFROM=$(describe \"$1\")\n\t\tTO=$(describe \"$2\")\n\t\t;;\n\t*)\n\t\terror\n\t\t;;\nesac\n\n> +then\n> +\tFROM=$(prev_release HEAD)\n> +\tTO=$(describe HEAD)\n> +fi\n> +\n> +# Check COMMIT against previous release\n> +if test $# -eq 1;\n> +then\n> +\tFROM=$(prev_release \"$1\")\n> +\tTO=$(describe \"$1\")\n> +fi\n> +\n> +# Check ABI between FROM and TO explicitly\n> +if test $# -eq 2;\n> +then\n> +\tFROM=$(describe \"$1\")\n> +\tTO=$(describe \"$2\")\n> +fi\n> +\n> +echo \"Validating ABI compatibility between $FROM and $TO\"\n> +\n> +# Generate an abi-compliance-checker xml description file\n\ns/$/./\n\nSame where applicable.\n\n> +# $PREFIX is assumed to be /usr/local/\n\nMaybe set prefix to /usr/local/ explicitly when running meson setup\ninstead of assuming it ?\n\n> +create_xml() {\n> +\t# $1: Output file\n> +\t# $2: Version\n> +\t# $3: Install root\n\nInstead of using comments, I find it more explicit to use local\nvariables:\n\n\tlocal output_file=\"$1\"\n\tlocal version=\"$2\"\n\tlocal install_root=\"$3\"\n\nAnd use the variables below.\n\n> +\n> +\techo \"$2\" > \"$1\"\n> +\techo \"$3/usr/local/include/\" >> \"$1\"\n> +\techo \"$3/usr/local/lib/\" >> \"$1\"\n> +}\n> +\n> +# Check if an ABI dump file exists, and if not create one\n> +# by building a minimal configuration of libcamera at the specified\n> +# version using a clean worktree.\n> +create_abi_dump() {\n> +\tVERSION=\"$1\"\n> +\tABI_FILE=\"$ABI_DIR/$VERSION.abi.dump\"\n> +\tWORKTREE=\"$TMP/$VERSION\"\n> +\tBUILD=\"$TMP/$VERSION-build\"\n\nMake these local variables.\n\n> +\n> +\t# Use a fully qualified path when calling ninja -C\n> +\tINSTALL=$(realpath \"$TMP/$VERSION-install\")\n> +\n> +\tif test ! -e \"$ABI_FILE\";\n> +\tthen\n\n\tif [[ ! -e \"$ABI_FILE\" ]] ; then\n\n> +\t\tdbg \"Creating ABI dump for $VERSION in $ABI_DIR\"\n> +\t\tgit worktree add \"$WORKTREE\" \"$VERSION\"\n> +\n> +\t\tmeson setup \"$BUILD\" \"$WORKTREE\" \\\n> +\t\t\t-Ddocumentation=disabled \\\n> +\t\t\t-Dcam=disabled \\\n> +\t\t\t-Dqcam=disabled \\\n> +\t\t\t-Dgstreamer=disabled \\\n> +\t\t\t-Dlc-compliance=disabled \\\n> +\t\t\t-Dtracing=disabled \\\n> +\t\t\t-Dpipelines=\n> +\n> +\t\tninja -C \"$BUILD\"\n> +\t\tDESTDIR=\"$INSTALL\" ninja -C \"$BUILD\" install\n> +\n> +\t\t# Create an xml descriptor with parameters to generate the dump file\n> +\t\tcreate_xml \\\n> +\t\t\t\"$INSTALL/libcamera-abi-dump.xml\" \\\n> +\t\t\t\"$VERSION\" \\\n> +\t\t\t\"$INSTALL\"\n> +\n> +\n> +\t\t# We currently have warnings produced that we can ignore\n> +\t\t# While we have pipefail set, finish with || true\n\nDo we need pipefail ?\n\n> +\t\tabi-compliance-checker \\\n\nShould we have a check that abi-compliance-checker exists, with a nice\nerror message if it doesn't ?\n\n> +\t\t\t-lib libcamera \\\n> +\t\t\t-v1 \"$VERSION\" \\\n> +\t\t\t-dump \"$INSTALL/libcamera-abi-dump.xml\" \\\n> +\t\t\t-dump-path \"$ABI_FILE\" || true\n> +\n> +\t\tdbg Created \"$ABI_FILE\"\n> +\n> +\t\tdbg Removing Worktree \"$WORKTREE\"\n> +\t\tgit worktree remove -f \"$WORKTREE\"\n> +\n> +\t\tdbg Removing \"$BUILD\"\n> +\t\trm -r \"$BUILD\"\n> +\n> +\t\tdbg Removing \"$INSTALL\"\n> +\t\trm -r \"$INSTALL\"\n> +\tfi\n> +}\n> +\n> +# Create the requested ABI dump files if they don't yet exist\n> +create_abi_dump \"$FROM\"\n> +create_abi_dump \"$TO\"\n> +\n> +# Todo: Future iterations and extensions here could add \"-stdout -xml\" and\n> +# parse the results automatically\n> +abi-compliance-checker -l libcamera \\\n> +\t-old \"$ABI_DIR/$FROM.abi.dump\" \\\n> +\t-new \"$ABI_DIR/$TO.abi.dump\" \\\n> +\t;\n\nIs the semicolon needed ?\n\n> +\n> +# On (far too many) occasions, the tools keep running leaving a cpu core @ 100%\n> +# CPU usage. Perhaps some subprocess gets launched but never rejoined. Stop\n> +# them all\n\nPlease add a TODO comment to remind we should investigate this.\n\n> +killall abi-compliance-checker 2>/dev/null\n\nIt would be nicer to kill only the process that was launched on the\nprevious line, not all instances of abi-compliance-checker.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CD37BBDC71\n\tfor ;\n\tTue, 4 Jul 2023 09:03:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1D2C3628C0;\n\tTue, 4 Jul 2023 11:03:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 85CC261E36\n\tfor ;\n\tTue, 4 Jul 2023 11:03:20 +0200 (CEST)","from pendragon.ideasonboard.com (85-160-42-71.reb.o2.cz\n\t[85.160.42.71])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 52112D4A;\n\tTue, 4 Jul 2023 11:02:36 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1688461402;\n\tbh=w9dLDsgMPJGtt2X7eejm3k8Ht0hDPE9K9WRW6090hHA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=JkCRrnqd9KWQj4sHPMH9i425ehdpXL/6WGZzKNJQynFH/FctVV4jp22zFjK/pEyNN\n\trk1kd0/60yCwSALjswsEreHVvrppjsltvvttY10WwX3nLwDPK5fsFKsDgcSGdXn+R4\n\tSr9ed4UyVz4r/i1zGvF8PtMkSENbM3L78fLRwSaz3VjedpySGcJ5lAUGUFmMj7BFtS\n\tA6Cj0zRl9towddybsm59p8gzI1DLqNY4OmqlS7YEEjF3Y2i/F9jL/Cd9+6AnQiaItc\n\tMCH3MQjYWJVexQodUYHYMu7lcQXZTlsC3/vXnknxirscGV0low+sbI6HCbA3bPhDsQ\n\tencNXN/EraiRg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1688461356;\n\tbh=w9dLDsgMPJGtt2X7eejm3k8Ht0hDPE9K9WRW6090hHA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XaCVa+wcfGde4R/Naeg5j4jJ6z42bU4Zz6yPsEfzYN8b8fWiaeCCLD8yd5NMuNCqx\n\t+W+JTs9ne64PdhjjTbIy1DkZOiFBsxMPUekpstEJKgsxD1k5JFJOQ559AKeq6GzSoi\n\ttAghQzV0gnJeaBF9AwE9uhEjn97VRZIHPg/V/oF0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"XaCVa+wc\"; dkim-atps=neutral","Date":"Tue, 4 Jul 2023 12:03:21 +0300","To":"Kieran Bingham ","Message-ID":"<20230704090321.GA4778@pendragon.ideasonboard.com>","References":"<20230515095812.3409747-1-kieran.bingham@ideasonboard.com>\n\t<20230515095812.3409747-2-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230515095812.3409747-2-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] utils: ABI Compatibility\n\tchecker","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27459,"web_url":"https://patchwork.libcamera.org/comment/27459/","msgid":"<168847585627.3298873.9828340507215161664@Monstersaurus>","date":"2023-07-04T13:04:16","subject":"Re: [libcamera-devel] [PATCH v2 1/2] utils: ABI Compatibility\n\tchecker","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2023-07-04 10:03:21)\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Mon, May 15, 2023 at 10:58:11AM +0100, Kieran Bingham via libcamera-devel wrote:\n> > Provide support to compare ABI compatibility between any two git commits\n> > or by a commit and the most recent ancestral tag of that commit.\n> > \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > utils/abi-compat.sh | 198 ++++++++++++++++++++++++++++++++++++++++++++\n> \n> The README.rst file should be updated to list the new dependency on\n> abi-compliance-checker.\n> \n> > 1 file changed, 198 insertions(+)\n> > create mode 100755 utils/abi-compat.sh\n> > \n> > diff --git a/utils/abi-compat.sh b/utils/abi-compat.sh\n> > new file mode 100755\n> > index 000000000000..b1944dc7d594\n> > --- /dev/null\n> > +++ b/utils/abi-compat.sh\n> > @@ -0,0 +1,198 @@\n> \n> Missing SPDX tag.\n\nAdded:\n# SPDX-License-Identifier: GPL-2.0-or-later\n\n> > +#!/bin/bash\n> > +\n> > +NAME=$(basename \"$0\")\n> \n> Missing blank line.\n> \n> > +usage() {\n> > +cat << EOF\n> \n> Missing indentation.\n> \n> > +$NAME: Determine the ABI/API compatibility of two build versions\n> > +\n> > + $NAME [--help] [--abi-dir=] [--tmp-dir=] ARGS\n> > +\n> > +The positional arguments (ARGS) determine the versions that will be compared and\n> > +take three variants:\n> > +\n> > + - No positional arguments:\n> > + $NAME [optional arguments]\n> > +\n> > + It is assumed to compare the current git HEAD against the most recent TAG\n> > +\n> > + - One positional argument:\n> > + $NAME [optional aguments] COMMITISH\n> > +\n> > + The given COMMITISH is compared against it's most recent TAG\n> > +\n> > + - Two positional arguments:\n> > + $NAME [optional aguments] BASE COMMITISH\n> \n> If you swapped the arguments, parsing would be easier.\n\nParsing might, but I think this is better this way for users.\n\n \"From ... To\" makes more sense that \"To From\".\n\n\n> > +\n> > + The given COMMITISH is compared against the given BASE.\n> > +\n> > +Optional Arguments:\n> > + --abi-dir Use for storing (or retrieving existing) ABI data\n> > + files\n> > +\n> > + --tmp-dir Specify temporary build location for building ABI data.\n> > + This could be a tmpfs/RAM disk to save on disk writes.\n> > +EOF\n> > +}\n> > +\n> > +POSITIONAL=()\n> > +while [[ $# -gt 0 ]]\n> > +do\n> > +case $1 in\n> \n> while [[ $# -gt 0 ]] ; do\n> option=$1\n> shift\n> \n> case $option in\n> \n> This will allow you to drop multiple shifts below.\n\nYes, thanks.\n\n\n> \n> > + -h|--help)\n> > + usage\n> > + exit 127;\n> \n> Why 127 ?\n> \n> > + ;;\n> \n> There's a mix of tabs and spaces for indentation, here and below.\n> \n> > +\n> > + --abi-dir)\n> > + ABI_DIR=$2\n> > + shift; shift;\n> > + ;;\n> > +\n> > + --tmp-dir)\n> > + TMP=$2\n> > + shift; shift;\n> > + ;;\n> > +\n> \n> -*)\n> error\n> ;;\n> \n> > + *) # Parse unidentified arguments based on position\n> > + POSITIONAL+=(\"$1\")\n> > + shift # past argument\n> > + ;;\n> > +esac\n> > +done\n> > +set -- \"${POSITIONAL[@]}\" # restore positional parameters\n> \n> It would be nice to set the from and to variables directly, to group all\n> argument parsing in one place.\n> \n> > +\n> > +ABI_DIR=${ABI_DIR:-abi}\n> > +TMP=${TMP:-\"$ABI_DIR/tmp/\"}\n> > +\n> > +mkdir -p \"$ABI_DIR\"\n> > +mkdir -p \"$TMP\"\n> > +\n> > +dbg () {\n> > + echo \"$@\" >> /dev/stderr\n> \n> echo \"$@\" >&2\n> \n> Same below.\n> \n> > +}\n> \n> Could we move all functions to the top of the file, before the \"main\"\n> code ?\n> \n> > +\n> > +die () {\n> > + echo \"$NAME: $*\" >> /dev/stderr\n> > + exit 128\n> \n> The usual exit code in case of failure is 1.\n> \n> > +}\n> > +\n> > +prev_release () {\n> > + git describe --tags --abbrev=0 \"$1\"^ \\\n> > + || die \"Failed to identify previous release tag from $1\"\n> > +}\n> > +\n> > +describe () {\n> > + git describe --tags \"$@\" \\\n> \n> Should this be $1 ?\n\nIt may as well be - changed.\n\n> \n> > + || die \"Failed to describe $1\"\n> > +}\n> > +\n> > +# Make sure we exit on errors during argument parsing\n> > +set -Eeuo pipefail\n> \n> You've parsed the arguments already :-)\n> \n> > +\n> > +# Check HEAD against previous 'release'\n> > +if test $# -eq 0;\n> \n> You use [[ above instead of test, let's be consistent.\n> \n> if [[ $# -eq 0 ]] ; then\n> \n> same below.\n> \n> Would the following be more readable ? Up to you.\n> \n> case $# in\n> 0)\n> FROM=$(prev_release HEAD)\n> TO=$(describe HEAD)\n> ;;\n> 1)\n> FROM=$(prev_release \"$1\")\n> TO=$(describe \"$1\")\n> ;;\n> 2)\n> FROM=$(describe \"$1\")\n> TO=$(describe \"$2\")\n> ;;\n> *)\n> error\n> ;;\n> esac\n\nYes, a case on $# is way nicer ... not sure why I didn't do it that way\nto start with ...\n\n\n> > +then\n> > + FROM=$(prev_release HEAD)\n> > + TO=$(describe HEAD)\n> > +fi\n> > +\n> > +# Check COMMIT against previous release\n> > +if test $# -eq 1;\n> > +then\n> > + FROM=$(prev_release \"$1\")\n> > + TO=$(describe \"$1\")\n> > +fi\n> > +\n> > +# Check ABI between FROM and TO explicitly\n> > +if test $# -eq 2;\n> > +then\n> > + FROM=$(describe \"$1\")\n> > + TO=$(describe \"$2\")\n> > +fi\n> > +\n> > +echo \"Validating ABI compatibility between $FROM and $TO\"\n> > +\n> > +# Generate an abi-compliance-checker xml description file\n> \n> s/$/./\n> \n> Same where applicable.\n> \n> > +# $PREFIX is assumed to be /usr/local/\n> \n> Maybe set prefix to /usr/local/ explicitly when running meson setup\n> instead of assuming it ?\n> \n> > +create_xml() {\n> > + # $1: Output file\n> > + # $2: Version\n> > + # $3: Install root\n> \n> Instead of using comments, I find it more explicit to use local\n> variables:\n> \n> local output_file=\"$1\"\n> local version=\"$2\"\n> local install_root=\"$3\"\n> \n> And use the variables below.\n> \n> > +\n> > + echo \"$2\" > \"$1\"\n> > + echo \"$3/usr/local/include/\" >> \"$1\"\n> > + echo \"$3/usr/local/lib/\" >> \"$1\"\n> > +}\n> > +\n> > +# Check if an ABI dump file exists, and if not create one\n> > +# by building a minimal configuration of libcamera at the specified\n> > +# version using a clean worktree.\n> > +create_abi_dump() {\n> > + VERSION=\"$1\"\n> > + ABI_FILE=\"$ABI_DIR/$VERSION.abi.dump\"\n> > + WORKTREE=\"$TMP/$VERSION\"\n> > + BUILD=\"$TMP/$VERSION-build\"\n> \n> Make these local variables.\n> \n> > +\n> > + # Use a fully qualified path when calling ninja -C\n> > + INSTALL=$(realpath \"$TMP/$VERSION-install\")\n> > +\n> > + if test ! -e \"$ABI_FILE\";\n> > + then\n> \n> if [[ ! -e \"$ABI_FILE\" ]] ; then\n> \n> > + dbg \"Creating ABI dump for $VERSION in $ABI_DIR\"\n> > + git worktree add \"$WORKTREE\" \"$VERSION\"\n> > +\n> > + meson setup \"$BUILD\" \"$WORKTREE\" \\\n> > + -Ddocumentation=disabled \\\n> > + -Dcam=disabled \\\n> > + -Dqcam=disabled \\\n> > + -Dgstreamer=disabled \\\n> > + -Dlc-compliance=disabled \\\n> > + -Dtracing=disabled \\\n> > + -Dpipelines=\n> > +\n> > + ninja -C \"$BUILD\"\n> > + DESTDIR=\"$INSTALL\" ninja -C \"$BUILD\" install\n> > +\n> > + # Create an xml descriptor with parameters to generate the dump file\n> > + create_xml \\\n> > + \"$INSTALL/libcamera-abi-dump.xml\" \\\n> > + \"$VERSION\" \\\n> > + \"$INSTALL\"\n> > +\n> > +\n> > + # We currently have warnings produced that we can ignore\n> > + # While we have pipefail set, finish with || true\n> \n> Do we need pipefail ?\n> \n\nWithout it, I believe failures in the subprocessses / functions don't\npropogate up. I want to catch errors everywhere except here (which is\nannoying as this is the main command that runs that I want to know\nsucceeds - so this needs further investigation later too.\n\nI think it's because of our use of the private headers that can't be\nparsed by the abi-compliance-checker and upsets it.\n\nAnd so rather than adding a todo to investigate - I've now stopped\ninstalling the private headers from libcamera-base so I should be able\nto remove the || true now.\n\n\n> > + abi-compliance-checker \\\n> \n> Should we have a check that abi-compliance-checker exists, with a nice\n> error message if it doesn't ?\n\nAdded.\n\n> \n> > + -lib libcamera \\\n> > + -v1 \"$VERSION\" \\\n> > + -dump \"$INSTALL/libcamera-abi-dump.xml\" \\\n> > + -dump-path \"$ABI_FILE\" || true\n\nThis || true will be removed now.\n\nIt means the abi-compliance checker can't (without modifying this\nscript) create abi dump files before the fix to the libcamera-base - but\nthat will only affect me for now - so it should be a limitation that\nwill disappear rapidly after the next release.\n\n\n> > +\n> > + dbg Created \"$ABI_FILE\"\n> > +\n> > + dbg Removing Worktree \"$WORKTREE\"\n> > + git worktree remove -f \"$WORKTREE\"\n> > +\n> > + dbg Removing \"$BUILD\"\n> > + rm -r \"$BUILD\"\n> > +\n> > + dbg Removing \"$INSTALL\"\n> > + rm -r \"$INSTALL\"\n> > + fi\n> > +}\n> > +\n> > +# Create the requested ABI dump files if they don't yet exist\n> > +create_abi_dump \"$FROM\"\n> > +create_abi_dump \"$TO\"\n> > +\n> > +# Todo: Future iterations and extensions here could add \"-stdout -xml\" and\n> > +# parse the results automatically\n> > +abi-compliance-checker -l libcamera \\\n> > + -old \"$ABI_DIR/$FROM.abi.dump\" \\\n> > + -new \"$ABI_DIR/$TO.abi.dump\" \\\n> > + ;\n> \n> Is the semicolon needed ?\n> \n\nIt was to help me - but no - removed.\n\n\n> > +\n> > +# On (far too many) occasions, the tools keep running leaving a cpu core @ 100%\n> > +# CPU usage. Perhaps some subprocess gets launched but never rejoined. Stop\n> > +# them all\n> \n> Please add a TODO comment to remind we should investigate this.\n> \n\nAdded\n+# TODO: Investigate this and report upstream\n\n\n> > +killall abi-compliance-checker 2>/dev/null\n> \n> It would be nicer to kill only the process that was launched on the\n> previous line, not all instances of abi-compliance-checker.\n\nI'll keep this as a shotgun approach. I think when I looked at this\nabi-compliance-checker spawns processes that do not exit when the parent\nprocess exits. I believe that would mean the PID of the launched process\nwon't exist to be killed ...\n\n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DBE85BE175\n\tfor ;\n\tTue, 4 Jul 2023 13:04:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1769F628C0;\n\tTue, 4 Jul 2023 15:04:21 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9BD1660384\n\tfor ;\n\tTue, 4 Jul 2023 15:04:19 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4A8939B9;\n\tTue, 4 Jul 2023 15:03:35 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1688475861;\n\tbh=cUs009Zxw6omXlVTEjbIsfrFJF4aY/X7NVZ6WV/Iu4g=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Bc9tStE8da7K6wkz0/XzcUfkfDn9pJQHnwaMUbS8dKJSIwG7TtD//dou/JZL98MRb\n\tM28fQGrI2oi8f6yZ7bC9Yc0zpin8Bfa0sHkUusTiLQYQeutqKb9v5AgCGszkUtaDQI\n\tjlWMscMgBFEMVhknVOzY1hJewjMGupK4wOPyeneV5e/E6UeXWGili4L3UHRf3rIW9r\n\t7S6Srd/MFvB7Gba7pXSFzTaFfJZkmzphz+4LMaE/sL4M5r+hbJaqjVX+wR/yD4bfES\n\taT5AQkx52NgGf5zTNUuTsi7xn6EZ5i/uEpiRyZ+PSH7aGQ707H8utk1zWyspwrM2sT\n\trqq/dcq/I/g8A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1688475815;\n\tbh=cUs009Zxw6omXlVTEjbIsfrFJF4aY/X7NVZ6WV/Iu4g=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=d/LZMtqMbu8S3QDQf9H/AsWpxV4etp3n1VGvC4zufTOmJ+e6bOc76a7XKlI+EtGr4\n\t05OHTBCmI2EImwGJr10vQn1MpZdvTWfDC0tg2IIjA8Csu2g+q+swI89peeZo5EIhWp\n\tHSk/0uKab30BjGuLO0qMIKEmT2K2sbj7jWe5tLb0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"d/LZMtqM\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20230704090321.GA4778@pendragon.ideasonboard.com>","References":"<20230515095812.3409747-1-kieran.bingham@ideasonboard.com>\n\t<20230515095812.3409747-2-kieran.bingham@ideasonboard.com>\n\t<20230704090321.GA4778@pendragon.ideasonboard.com>","To":"Laurent Pinchart ","Date":"Tue, 04 Jul 2023 14:04:16 +0100","Message-ID":"<168847585627.3298873.9828340507215161664@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] utils: ABI Compatibility\n\tchecker","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]