Message-ID: <32029b6b-4427-d695-63ea-dfdbf6590a6a@quicinc.com>
Date:   Wed, 22 Feb 2023 12:38:52 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH RESEND 1/1] check-uapi: Introduce check-uapi.sh
Content-Language: en-US
To:     Masahiro Yamada <masahiroy@kernel.org>
CC:     Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Nicolas Schier <nicolas@fjasle.eu>,
        <linux-kbuild@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>,
        "Trilok Soni" <quic_tsoni@quicinc.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Bjorn Andersson <andersson@kernel.org>,
        "Todd Kjos" <tkjos@google.com>,
        Matthias Maennich <maennich@google.com>,
        "Giuliano Procida" <gprocida@google.com>,
        <kernel-team@android.com>, Jordan Crouse <jorcrous@amazon.com>
References: <20230217202234.32260-1-quic_johmoo@quicinc.com>
 <20230217202234.32260-2-quic_johmoo@quicinc.com>
 <CAK7LNASf_RQGM6QYwvUN+FLq-v3jK66Pk5jW9M-vM1V4K0xH8g@mail.gmail.com>
From:   John Moon <quic_johmoo@quicinc.com>
In-Reply-To: <CAK7LNASf_RQGM6QYwvUN+FLq-v3jK66Pk5jW9M-vM1V4K0xH8g@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/18/2023 9:25 PM, Masahiro Yamada wrote:
> On Sat, Feb 18, 2023 at 5:23 AM John Moon <quic_johmoo@quicinc.com> wrote:
>>
>> While the kernel community has been good at maintaining backwards
>> compatibility with kernel UAPIs, it would be helpful to have a tool
>> to check if a patch introduces changes that break backwards
>> compatibility.
>>
>> To that end, introduce check-uapi.sh: a simple shell script that
>> checks for changes to UAPI headers using libabigail.
>>
>> libabigail is "a framework which aims at helping developers and
>> software distributors to spot some ABI-related issues like interface
>> incompatibility in ELF shared libraries by performing a static
>> analysis of the ELF binaries at hand."
>>
>> The script uses one of libabigail's tools, "abidiff", to compile the
>> changed header before and after the patch to detect any changes.
>>
>> abidiff "compares the ABI of two shared libraries in ELF format. It
>> emits a meaningful report describing the differences between the two
>> ABIs."
>>
>> Signed-off-by: John Moon <quic_johmoo@quicinc.com>
> 
> 
> 
> 
> BTW, the patch is prefixed with 'RESEND', but it contains
> several diff lines against the previous submission [1].
> 
> People would submit this as v2 with changes mentioned under '---'.
> 

Sorry about that, I'm still learning the etiquette. :)

I will mark the patch rev as v2 after addressing your comments.

> 
> [1]: https://patchwork.kernel.org/project/linux-kbuild/patch/20230217202234.32260-2-quic_johmoo@quicinc.com/
> 
> 
> 
> 
> 
> 
> diff --git a/scripts/check-uapi.sh b/scripts/check-uapi.sh
> index 209c6793a4d0..b9cd3a2d7805 100755
> --- a/scripts/check-uapi.sh
> +++ b/scripts/check-uapi.sh
> @@ -3,6 +3,7 @@
> 
>   # Script to check a patch for UAPI stability
>   set -o errexit
> +set -o pipefail
> 
>   print_usage() {
>          name=$(basename "$0")
> @@ -34,7 +35,7 @@ get_header() {
> 
>          if [ ! -x "${KERNEL_SRC}/scripts/unifdef" ]; then
>                  if ! make -C "${KERNEL_SRC}/scripts" unifdef; then
> -                       echo 'error - failed to build required
> dependency "scripts/unifdef"'
> +                       errlog 'error - failed to build required
> dependency "scripts/unifdef"'
>                          exit 1
>                  fi
>          fi
> @@ -111,7 +112,7 @@ check_changed_files() {
> 
>          total=$((passed + failed))
>          if [ "$total" -eq 0 ]; then
> -               errlog "No changes to UAPI headers detected"
> +               errlog "No changes to UAPI headers detected in most
> recent commit"
>          else
>                  errlog "${passed}/${total} UAPI header file changes
> are backwards compatible"
>          fi
> @@ -190,16 +191,15 @@ check_deps() {
>                  errlog "error - abidiff not found!"
>                  errlog "Please install abigail-tools (version 1.7 or greater)"
>                  errlog "See:
> https://sourceware.org/libabigail/manual/libabigail-overview.html"
> -               exit 2
> +               exit 1
>          fi
> 
> -       local -r abidiff_maj=$("$ABIDIFF" --version | cut -d ' ' -f 2
> | cut -d '.' -f 1)
> -       local -r abidiff_min=$("$ABIDIFF" --version | cut -d ' ' -f 2
> | cut -d '.' -f 1)
> +       read -r abidiff_maj abidiff_min _ < <("$ABIDIFF" --version |
> cut -d ' ' -f 2 | tr '.' ' ')
>          if [ "$abidiff_maj" -lt 1 ] || ([ "$abidiff_maj" -eq 1 ] && [
> "$abidiff_min" -lt 7 ]); then
> -               errlog "error - installed abidiff version too old:
> $("$ABIDIFF" --version)"
> +               errlog "error - abidiff version too old: $("$ABIDIFF"
> --version)"
>                  errlog "Please install abigail-tools (version 1.7 or greater)"
>                  errlog "See:
> https://sourceware.org/libabigail/manual/libabigail-overview.html"
> -               exit 2
> +               exit 1
>          fi
>   }
> 
> @@ -220,13 +220,18 @@ main() {
>          check_deps
> 
>          tmp_dir=$(mktemp -d)
> -       #trap 'rm -rf $tmp_dir' EXIT
> +       trap 'rm -rf $tmp_dir' EXIT
> 
>          if [ -z "$KERNEL_SRC" ]; then
>                  KERNEL_SRC="$(realpath "$(dirname "$0")"/..)"
>          fi
>          export KERNEL_SRC
> 
> +       if ! (cd "$KERNEL_SRC" && git rev-parse --is-inside-work-tree
>> /dev/null 2>&1); then
> +               errlog "error - this script requires the kernel tree
> to be initialized with Git"
> +               exit 1
> +       fi
> +
>          export ARCH
>          export CC
>          export CROSS_COMPILE
> 
> 
> 
> 
> 
>