Re: [dpdk-dev] [PATCH v2 03/10] buildtools: add ABI update shell script

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Oct 16, 2019 at 01:43:18PM +0100, Anatoly Burakov wrote:
> In order to facilitate mass updating of version files, add a shell
> script that recurses into lib/ and drivers/ directories and calls
> the ABI version update script.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> 
> Notes:
>     v2:
>     - Add this patch to split the shell script from previous commit
>     - Fixup miscellaneous bugs
> 
>  buildtools/update-abi.sh | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100755 buildtools/update-abi.sh
> 
> diff --git a/buildtools/update-abi.sh b/buildtools/update-abi.sh
> new file mode 100755
> index 0000000000..a6f916a437
> --- /dev/null
> +++ b/buildtools/update-abi.sh
> @@ -0,0 +1,36 @@
> +#!/bin/bash

Does this actually need to be bash? Most of our scripts use plain "sh".
Also on FreeBSD bash is generally in /usr/local/bin not /bin.

> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2019 Intel Corporation
> +
> +abi_version=""
> +abi_version_file="./config/ABI_VERSION"
> +update_path="lib drivers"
> +
> +if [ -z "$1" ]
> +then

While there are a few scripts in DPDK putting the "then" on the next line
most scripts put it on the same line as the "if", after a ";".

> +      # output to stderr
> +      >&2 echo "provide ABI version"
> +      exit 1
> +fi
> +
> +abi_version=$1

I think you can just do this assignment at the top when you define
abi_version in the first place. Using $1 when it doesn't exist isn't a
problem.

> +
> +if [ -n "$2" ]
> +then
> +      abi_version_file=$2
> +fi
> +
> +if [ -n "$3" ]
> +then
> +      update_path=${@:3}

I think this might be a bash-ism, right? If so, I think using "shift" and
then directly using $@ should work instead to make it sh-compatible..

> +fi
> +
> +echo "New ABI version:" $abi_version
> +echo "ABI_VERSION path:" $abi_version_file
> +echo "Path to update:" $update_path
> +
> +echo $abi_version > $abi_version_file
Do we need to check the abi_version provided is in the correct format?
Should it have both major and minor components, or just major. I think the
former, so we can do minor bumps which keeping major compatibility.

> +
> +find $update_path -name  \*version.map -exec \
> +      ./buildtools/update_version_map_abi.py {} \
> +      $abi_version \; -print
> -- 
> 2.17.1