1366761 - Add automatic checks for new ABI changes to automation/buildbot-slave/build.sh

Status: RESOLVED FIXED

(NSS :: Test, enhancement)

Description

[Kai Engert [:KaiE:]]

We should implement a script, that reports the differences between a reference version (branch or version) of NSS, and the current snapshot of NSS.

Because within a minor release of NSS (e.g. 3.30), we don't usually allow any API changes or additions in a patch release (e.g. 3.30.1), the suggestion is to compare between minor releases (e.g. 3.30 to 3.31).

To implement these automatic checks, I suggest that we add a new text file to NSS, which records the reference HG tag that should be used for comparison.

We should do the same for NSPR.

If the ABI comparison script is executed, the following should happen:
- the local NSPR and NSS trees should be cloned into a separate temporary
  directory
- based on the information files, those temporary trees are switched
  to the older branches/releases
- the older versions of nspr/nss are built
- the "abidiff" tool is executed, to compare the binary .so files,
  also providing the public header files to the tool

Some changes reported by the "abidiff" might be permissible.

Known good changes should be suppressed by the script.
New, unverified changes should cause the script to fail.

After a script/test failure, a developer must decide if the ABI change is incorrect and must be reverted.

If the change is permissible, the report should be checked in as "expected", allowing the script to not report failures for this change any longer.

Comment 1

[Kai Engert [:KaiE:]]

I'd like to start with the buildbot/slave environment.

Comment 2

[Kai Engert [:KaiE:]]

Attachment: 1366761-v1.patch

I'd like to check in this patch, which works for me locally.

I want to land with all expectations being empty, although we already have some permissible changes on trunk. The reason is, I want to test if the buildbot infrastructure is correctly reporting the new changes. (I don't have a separate test-drive buildbot environment.)

Franziskus, are you interested to review it?

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8870109 [details] [diff] [review]
1366761-v1.patch

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

This is a great idea.  Can you update your release automation scripts so that they update the previous-nss-release file and flush the expected ABI compat flags.

IMPORTANT: You should take the value for previous-nspr-release from the checked-out baseline copy of NSS (see Bug 1366757).  That might mean uplifting that change onto the immediately preceding version, but it avoids having to maintain an NSPR version in two places.

::: automation/buildbot-slave/build.sh
@@ +218,5 @@
>      print_result "NSS - tests - ${BITS} bits - ${OPT}" ${RET} 0
>      return ${RET}
>  }
>  
> +check_abi()

This is big enough to justify a new script, I think.

@@ +223,5 @@
> +{
> +    print_log "######## NSS ABI CHECK - ${BITS} bits - ${OPT} ########"
> +    rm -rf ${HGDIR}/baseline
> +    mkdir ${HGDIR}/baseline
> +    hg clone ${HGDIR}/nspr ${HGDIR}/baseline/nspr

I would clone with the -u flag:

hg clone -u "$BASE_NSPR" "$HGDIR/nspr" "$HGDIR/baseline/nspr"

It makes the code here a lot shorter and you can avoid all the messing around with changing directories that you do.  I checked and the command fails if the tag doesn't exist.

@@ +226,5 @@
> +    mkdir ${HGDIR}/baseline
> +    hg clone ${HGDIR}/nspr ${HGDIR}/baseline/nspr
> +    hg clone ${HGDIR}/nss ${HGDIR}/baseline/nss
> +    BASE_PR=`cat ${HGDIR}/nss/automation/abi-check/previous-nspr-release`
> +    BASE_S=`cat ${HGDIR}/nss/automation/abi-check/previous-nss-release`

s/BASE_PR/BASE_NSPR;s/BASE_S/BASE_NSS/

@@ +253,5 @@
> +    pushd ${HGDIR}/baseline/nss
> +
> +    print_log "$ ${MAKE} ${NSS_BUILD_TARGET}"
> +    #${MAKE} ${NSS_BUILD_TARGET} 2>&1 | tee -a ${LOG_ALL} | grep ${GREP_BUFFER} "^${MAKE}"
> +    ${MAKE} ${NSS_BUILD_TARGET} 2>&1 | tee -a ${LOG_ALL}

Why not use build.sh?  It's faster.

@@ +280,5 @@
> +            > nss/automation/abi-check/new-report-$SO.txt
> +        diff -u nss/automation/abi-check/expected-report-$SO.txt \
> +                nss/automation/abi-check/new-report-$SO.txt >> ${ABI_REPORT}
> +    done
> +    

trailing ws

Comment 4

[Kai Engert [:KaiE:]]

Some comments addressed:
https://hg.mozilla.org/projects/nss/graph


(In reply to Martin Thomson [:mt:] from comment #3)
> > +check_abi()
> 
> This is big enough to justify a new script, I think.

We can use a new script for the taskcluster environment, which would be a next step.

The current buildbot environment uses a single script to drive all variations.


> I would clone with the -u flag:
> hg clone -u "$BASE_NSPR" "$HGDIR/nspr" "$HGDIR/baseline/nspr"

thanks, agreed


> @@ +253,5 @@
> > +    ${MAKE} ${NSS_BUILD_TARGET} 2>&1 | tee -a ${LOG_ALL}
> 
> Why not use build.sh?  It's faster.

There are many buildbot slaves, and I didn't have the time yet to reconfigure them to have gyp available. It must work on old systems like RHEL 5.

Comment 5

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/projects/nss/rev/0c3800b6eaba

Comment 6

[Martin Thomson [:mt:]]

Kai, I really think that you should get BASE_NSPR from the file that is in the baseline, as I mentioned.  I marked this as IMPORTANT in my comments.

Comment 7

[Kai Engert [:KaiE:]]

I'll work on that

Comment 8

[Martin Thomson [:mt:]]

Kai, is this the right bug to attribute to https://hg.mozilla.org/projects/nss/rev/cecaa42fccf5 and https://hg.mozilla.org/projects/nss/rev/713b15751ad2 - those don't have a bug number on them.

Comment 9

[Martin Thomson [:mt:]]

I also suggested that your release helper scripts reset the baseline and clear the expected diffs, probably along with setting the beta flag.  This is something that will be easy to forget to get right.

Comment 10

[Kai Engert [:KaiE:]]

(In reply to Martin Thomson [:mt:] from comment #6)
> Kai, I really think that you should get BASE_NSPR from the file that is in
> the baseline, as I mentioned.  I marked this as IMPORTANT in my comments.

I don't understand how this would work.

If I have the latest version checked out, I know the current NSPR version requirement.

I cannot checkout the baseline version, without knowing the baseline version number.

I don't know the previous version, without doing math. Are you suggesting I do math and subtract 1 from the current version? I'm not sure this will always be correct.

Comment 11

[Kai Engert [:KaiE:]]

(In reply to Martin Thomson [:mt:] from comment #9)
> I also suggested that your release helper scripts reset the baseline and
> clear the expected diffs, probably along with setting the beta flag.  This
> is something that will be easy to forget to get right.

Yes, I already had this planned, I will do this separately.

Comment 12

[Kai Engert [:KaiE:]]

(In reply to Martin Thomson [:mt:] from comment #8)
> Kai, is this the right bug to attribute to
> https://hg.mozilla.org/projects/nss/rev/cecaa42fccf5 and
> https://hg.mozilla.org/projects/nss/rev/713b15751ad2 - those don't have a
> bug number on them.

I don't understand your comment.

For each of those links, the displayed content displays bug numbers.

I was doing it for the past actions on NSS 3.31 version, as I'd expect it for future changes.

While we'll develop NSS 3.32, it will start with empty "expected-report" files. Then we'll potentially discover permissible API changes. Then updated "expected-report" files will have to be checked in, to hide the known OK reports, until 3.32 was over. I think such future commits should be attributed to those bugs, that introduce the reported ABI changes. That's why I attributed the changes to the "expected-report" files to those bugs, not to this bug.

Comment 13

[Martin Thomson [:mt:]]

(In reply to Kai Engert (:kaie:) from comment #10)
> (In reply to Martin Thomson [:mt:] from comment #6)
> > Kai, I really think that you should get BASE_NSPR from the file that is in
> > the baseline, as I mentioned.  I marked this as IMPORTANT in my comments.
> 
> I don't understand how this would work.

Checkout BASE_NSS then find the nspr-version.txt file from that directory, not the current one and use that to find the baseline version.  As I said, it means uplifting nspr-version.txt to the baseline version for 3.31, but thereafter it should work fine.

In summary:

hg clone -u $(cat $root/nss/.../baseline-nss) $root/nss $root/baseline/nss
hg clone -u $(head -1 $root/baseline/nss/.../nspr-version.txt) $root/nspr $root/baseline/nspr

Comment 14

[Martin Thomson [:mt:]]

(In reply to Kai Engert (:kaie:) from comment #12)
> (In reply to Martin Thomson [:mt:] from comment #8)
> > Kai, is this the right bug to attribute to
> > https://hg.mozilla.org/projects/nss/rev/cecaa42fccf5 and
> > https://hg.mozilla.org/projects/nss/rev/713b15751ad2 - those don't have a
> > bug number on them.
> 
> I don't understand your comment.
> 
> For each of those links, the displayed content displays bug numbers.

I assume that the bug numbers are those that introduced the ABI change, not this bug, which is where you are adding this feature, that's all.

Comment 15

[Kai Engert [:KaiE:]]

(In reply to Martin Thomson [:mt:] from comment #13)
> 
> Checkout BASE_NSS then find the nspr-version.txt file from that directory,
> not the current one and use that to find the baseline version.

Now I get it, thanks.


(In reply to Martin Thomson [:mt:] from comment #14)
> I assume that the bug numbers are those that introduced the ABI change, not
> this bug, which is where you are adding this feature, that's all.

Yes, I think that's what I did.

Comment 16

[Kai Engert [:KaiE:]]

Added NSPR version information to NSS 3.30 branch:
https://hg.mozilla.org/projects/nss/rev/7f7a8c1f2c26e27c772493255a2d116f773f5ba5

Comment 17

[Kai Engert [:KaiE:]]

Martin, you made one more assumption in your suggestion, which isn't straightforward.

The "current NSPR version required" is recorded as the human readable number, which we use for the source archives.

The "previous-nspr-release" is required as a HG tag or branch name.

In order for this to work automatically, we'd have to convert the human readable number into an HG tag.

The conversion logic is doable, but it requires that we'll never change the style of our HG tags.
So far we don't have any automatic expectations of tag naming recording anywhere, so this would be the start of doing so.

Comment 18

[Kai Engert [:KaiE:]]

But ok, I'll do it. I'll use awk to print the major and minor versions, and ignore any third level patch number.
I'll use those numbers to derive the NSPR_$MAJOR_$MINOR_BRANCH name

Comment 19

[Kai Engert [:KaiE:]]

Attachment: 1366761-part3.patch

Comment 20

[Martin Thomson [:mt:]]

Comment on attachment 8870375 [details] [diff] [review]
1366761-part3.patch

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

::: automation/buildbot-slave/build.sh
@@ +235,5 @@
>  
> +    BASE_VERSION=`head -1 ${HGDIR}/baseline/nss/automation/release/nspr-version.txt`
> +    BASE_MAJOR=`echo $BASE_VERSION | awk -F '.' '{print $1}'`
> +    BASE_MINOR=`echo $BASE_VERSION | awk -F '.' '{print $2}'`
> +    BASE_NSPR="NSPR_${BASE_MAJOR}_${BASE_MINOR}_BRANCH"

One line:

BASE_NSPR=NSPR_$(head -1 .../nspr-version.txt | cut -d . -f 1-2 | tr . _)_BRANCH

@@ +248,1 @@
>      print_log "$ pushd ${HGDIR}/baseline/nss"

I don't think that you need this log line.

Comment 21

[Kai Engert [:KaiE:]]

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

Comment 22

[Kai Engert [:KaiE:]]

(In reply to Martin Thomson [:mt:] from comment #3)
> 
> Can you update your release automation scripts so
> that they update the previous-nss-release file and flush the expected ABI
> compat flags.

filed as bug 1367435.

Closing this bug.

Comment 23

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8870109 [details] [diff] [review]
1366761-v1.patch

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

This was resolved.
Though it doesn't seem to work. See https://bot.nss-crypto.org:8011/builders/fedora-x32-DBG

Comment 24

[Kai Engert [:KaiE:]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #23)
> Though it doesn't seem to work. See
> https://bot.nss-crypto.org:8011/builders/fedora-x32-DBG

That's strange.

After the command "hg update -u NSS_3_30_BRANCH source-nss destination-nss" is used, the destination directory doesn't contain file nss/automation/release/nspr-version.txt

Although "hg summary" says it's on the 3.30 branch, "hg log" shows entries from trunk, not the branch.

The other x64-DBG build on the same machine is working, so it cannot be related to the tools.

Maybe that local HG source is corrupted. I've clobbered it manually to get a fresh clone, and we'll see on the next run if that fixed it.

Comment 25

[Kai Engert [:KaiE:]]

Clobbering the tree fixed the issue.