1423145 - NSS buildbot ABI check report a failure, because we don't look at the individual bits of the abidiff result code

Status: RESOLVED FIXED

(NSS :: Test, enhancement)

Description

[Kai Engert (:KaiE:)]

The buildbot ABI checks currently fail with:
  TB [2017-12-04 21:55:48] FAILED: failure executing the ABI checks

The log files don't say what's happening, and I cannot figure it out by inspecting the build slave environment.

We should add better logging of the failed scenario.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: 1423145-v1.patch

Comment 2

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8934461 [details] [diff] [review]
1423145-v1.patch

Review of attachment 8934461 [details] [diff] [review]:
-----------------------------------------------------------------

let's see what this tells us

Comment 3

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/e9b09c7136cf

Comment 4

[Kai Engert (:KaiE:)]

We get nonzero return values from abidiff execution on the nspr4 library (for which currently a (whitelisted) difference is reported).

According to the manual page, nonzero return values aren't necessarily failures. The return value must be checked for individual bits to know what the return values means. Currently, we just treat any nonzero return as a failure.

We must update the build script to look at the individual bits, and don't fail on bits that indicate just a reported difference.

Comment 5

[Kai Engert (:KaiE:)]

Here's documentation for the return values that will help the reviewer:
https://sourceware.org/git/gitweb.cgi?p=libabigail.git;a=blob;f=doc/manuals/abidiff.rst

(search for section "Return values")

Comment 6

[Kai Engert (:KaiE:)]

Attachment: 1423145-v2.patch

Comment 7

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8936795 [details] [diff] [review]
1423145-v2.patch

Review of attachment 8936795 [details] [diff] [review]:
-----------------------------------------------------------------

::: automation/buildbot-slave/build.sh
@@ +275,5 @@
> +        ABIDIFF_ABI_CHANGE=$((($RET & 0x04) != 0))
> +        ABIDIFF_ABI_INCOMPATIBLE_CHANGE=$((($RET & 0x08) != 0))
> +        ABIDIFF_UNKNOWN_BIT_SET=$((($RET & 0xf0) != 0))
> +
> +        # If abidiff reports an error, or a usage error, or if it sets an result

nit: a result

@@ +282,5 @@
> +        # result report with our whitelist. This allows us to silence changes
> +        # that we're already aware of and have been declared acceptable.
> +
> +        REPORT_RET_AS_FAILURE=0
> +        if [ $ABIDIFF_ERROR -ne 0 ]; then

This should probably be a switch statement.

@@ +292,5 @@
> +        if [ $ABIDIFF_UNKNOWN_BIT_SET -ne 0 ]; then
> +            REPORT_RET_AS_FAILURE=1
> +        fi
> +
> +        if [ $REPORT_RET_AS_FAILURE -ne 0 ]; then

Maybe add some logging for the cases that we don't consider an error (then we'd have logging in every case).

Comment 8

[Kai Engert (:KaiE:)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #7)
> @@ +282,5 @@
> > +        # result report with our whitelist. This allows us to silence changes
> > +        # that we're already aware of and have been declared acceptable.
> > +
> > +        REPORT_RET_AS_FAILURE=0
> > +        if [ $ABIDIFF_ERROR -ne 0 ]; then
> 
> This should probably be a switch statement.

I cannot follow your thought, can you describe how this would work?

Did you notice I'm checking three different variables?

Because we're checking for a bitmask, I don't see how a switch statement could efficiently check for all possible values.


> @@ +292,5 @@
> > +        if [ $ABIDIFF_UNKNOWN_BIT_SET -ne 0 ]; then
> > +            REPORT_RET_AS_FAILURE=1
> > +        fi
> > +
> > +        if [ $REPORT_RET_AS_FAILURE -ne 0 ]; then
> 
> Maybe add some logging for the cases that we don't consider an error (then
> we'd have logging in every case).

Very good idea, I'll add logging for each individual bit.

Comment 9

[Kai Engert (:KaiE:)]

Attachment: 1423145-v3.patch

Comment 10

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8937407 [details] [diff] [review]
1423145-v3.patch

Review of attachment 8937407 [details] [diff] [review]:
-----------------------------------------------------------------

Oops forgot about this, sorry. Get it landed!

Comment 11

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/db32ef3be38e