Re: [PATCH v4 1/2] check-uapi: Introduce check-uapi.sh

[John Moon]

On 4/7/2023 12:19 PM, Masahiro Yamada wrote:
> On Tue, Mar 28, 2023 at 2:42 AM John Moon <quic_johmoo@xxxxxxxxxxx> 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 commit 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 commit 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."
> >
> > The script also includes the ability to check the compatibility of
> > all UAPI headers across commits. This allows developers to inspect
> > the stability of the UAPIs over time.
> >
> > Signed-off-by: John Moon <quic_johmoo@xxxxxxxxxxx>
> > ---
> >      - Refactored to exclusively check headers installed by make
> >        headers_install. This simplified the code dramatically and removed
> >        the need to perform complex git diffs.
> >      - Removed the "-m" flag. Since we're checking all installed headers
> >        every time, a flag to check only modified files didn't make sense.
> >      - Added info message when usr/include/Makefile is not present that
> >        it's likely because that file was only introduced in v5.3.
> >      - Changed default behavior of log file. Now, the script will not
> >        create a log file unless you pass "-l <file>".
> >      - Simplified exit handler.
> >      - Added -j $MAX_THREADS to make headers_install to improve speed.
> >      - Cleaned up variable references.
> >
> >   scripts/check-uapi.sh | 488 ++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 488 insertions(+)
> >   create mode 100755 scripts/check-uapi.sh
>
> > +
> > +# Save the current git tree state, stashing if needed
> > +save_tree_state() {
> > +       printf "Saving current tree state... "
> > +       current_ref="$(git rev-parse HEAD)"
> > +       readonly current_ref
> > +       if tree_is_dirty; then
> > +               unstash="true"
> > +               git stash push --quiet
> > +       fi
> > +       printf "OK\n"
> > +}
> > +
> > +# Restore the git tree state, unstashing if needed
> > +restore_tree_state() {
> > +       if [ -z "$current_ref" ]; then
> > +               return 0
> > +       fi
> > +
> > +       printf "Restoring current tree state... "
> > +       git checkout --quiet "$current_ref"
>
>
> This does not restore the original state.
>
> I was on a branch before running this script.
> After everything is finished, I am on a detached commit
> because $current_ref is not a branch.

Good point. I think doing this should address:

current_ref="$(git symbolic-ref --short HEAD 2> /dev/null || git rev 
parse HEAD)"

Will fix in v5.

> > +       if ! do_compile "$(get_header_tree "$past_ref")/include" "$past_header" "${past_header}.bin" 2> "$log"; then
> > +               eprintf "error - couldn't compile version of UAPI header %s at %s\n" "$file" "$past_ref"
> > +               cat "$log" >&2
> > +               exit "$FAIL_COMPILE"
> > +       fi
> > +
> > +       "$ABIDIFF" --non-reachable-types "${past_header}.bin" "${base_header}.bin" > "$log" && ret="$?" || ret="$?"
>
>
> [bikeshed] I might want to write like this:
>
>     ret=0
>     "$ABIDIFF" --non-reachable-types "${past_header}.bin"
> "${base_header}.bin" > "$log" || ret="$?"

Sure, will do.

> > +
> > +
> > +       if [ "$quiet" = "true" ]; then
> > +               run "$base_ref" "$past_ref" "$abi_error_log" "$@" > /dev/null
> > +       else
> > +               run "$base_ref" "$past_ref" "$abi_error_log" "$@"
> > +       fi
>
>
>
>      if [ "$quiet" = "true" ]; then
>              exec > /dev/null
>      fi
>
>      run "$base_ref" "$past_ref" "$abi_error_log" "$@"
>
>
>
> is more elegant because this is the last line of main()
> and exit_handler() does not print anything.

Agreed, will do.

>