408157 - update verification should fail (not just warn) when binary files diff

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P3)

Description

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

During the FF3.0beta2 release, we discovered that the win32 update verification step was detecting differences between binary files, but only flagging the diff as a warning. This mean that the verify completed "successfully", even though some serious problems had been found.

...
Binary files source/bin/freebl3.chk and target/bin/freebl3.chk differ
Binary files source/bin/softokn3.chk and target/bin/softokn3.chk differ
WARN: check_update returned non-zero exit code for WINNT_x86-msvc downloads/Firefox Setup 3.0 Beta 1.exe vs. downloads/firefox-3.0b2.nl.win32.installer.exe: 1
...

We should always fail (not warn) in this situation. This would cause the verify step to fail, and attract our attention. Alternatively, if we confirm we can ignore the diff between these two files, we should not bother diff-ing them in the first place.

Comment 1

[Robert Helmer [:rhelmer]]

Attachment: save results of diff in order to search for errors

Comment 2

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 292924 [details] [diff] [review]
save results of diff in order to search for errors

I'm pretty sure the removed-files exclusion still applies (it's added by the update), did you check on installed-chrome.txt ?

Also, did you want to duplicate the Binary files output ?

Comment 3

[Robert Helmer [:rhelmer]]

(In reply to comment #2)
> (From update of attachment 292924 [details] [diff] [review])
> I'm pretty sure the removed-files exclusion still applies (it's added by the
> update), did you check on installed-chrome.txt ?

Yeah I should've mentioned this; I wasn't sure if it mattered or not.. I think you're right, but maybe we should just allow the WARN instead of hardcoding exceptions?

> Also, did you want to duplicate the Binary files output ?

You mean where it'll be duped by both the output of results.diff and the grep command, if any are found? Yeah that's annoying; maybe we should do "grep ... > errors.log" and cat errors.log if there are any problems?

Also I think it'd be better to check for the existence of any files we are planning on creating and removing them before starting; that'd be more foolproof than removing them at the bottom (crashing out would leave them lying around, for instance).

How would you feel about a patch with these changes ^

Comment 4

[Robert Helmer [:rhelmer]]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 292924 [details] [diff] [review] [details])
> > I'm pretty sure the removed-files exclusion still applies (it's added by the
> > update), did you check on installed-chrome.txt ?
> 
> Yeah I should've mentioned this; I wasn't sure if it mattered or not.. I think
> you're right, but maybe we should just allow the WARN instead of hardcoding
> exceptions?

Sorry for the bugspam, my thinking here is wrong.. we should exclude these, then WARN would actually be meaningful, and we can turn buildbot orange if that happens..

Comment 5

[Nick Thomas [:nthomas] (UTC+12)]

Checking file existence before writing is definitely a more robust thing to do, but the > redirection should ensure the file is overwritten. I'd be tempted to just dump the grep output to /dev/null, since the list of differences is normally short enough that it's not hard on the eye to scan (ie no need to duplicate to make things more obvious).

Your comments made me think about this a bit more. Currently, if the diff exit-status is non-zero for an update, then the WARN message is printed by mozilla/testing/release/updates/verify.sh and the we continue to test the next locale/version combo. The patch would add an extra FAIL line for binary differences, which is easy to search for, but doesn't change the exit status from verify.sh.

Do we want to FAIL on all differences ? Could probably do that fairly easily in verify.sh with s/WARN/FAIL/ and have another boolean to track overall exit status from the check_updates calls (and probably elsewhere). My guess is that it's useful to continue to do the whole run before failing out, rather than immediately. The problem with that is we have some known issues that are usually ignored, eg
Fx2/win32: Only in source/bin: .autoreg
(could probably fix the mar packaging, don't think this file is used anymore because the app detects changes in itself better)
Tb1.5/win32: Files source/bin/regxpcom.exe and target/bin/regxpcom.exe differ
(EOL now though, bug 376675)
Fx3/win32: a whole bunch of stuff needs to be added to removed-files.in (mex files in res\fonts, spellcheck.xpt on mac ?)

We could just FAIL on binary differences, by encoding more information into the exit status returned from check_updates to verify.sh. Is that what you mean in comment #4 ? FAIL/red on binary diffs and WARN/orange on other diffs ?

Comment 6

[Robert Helmer [:rhelmer]]

(In reply to comment #5)
> Checking file existence before writing is definitely a more robust thing to do,
> but the > redirection should ensure the file is overwritten. I'd be tempted to
> just dump the grep output to /dev/null, since the list of differences is
> normally short enough that it's not hard on the eye to scan (ie no need to
> duplicate to make things more obvious).

Ok.

> Your comments made me think about this a bit more. Currently, if the diff
> exit-status is non-zero for an update, then the WARN message is printed by
> mozilla/testing/release/updates/verify.sh and the we continue to test the next
> locale/version combo. The patch would add an extra FAIL line for binary
> differences, which is easy to search for, but doesn't change the exit status
> from verify.sh.

Hm, yeah not sure about this. I was thinking that we'd return 0 if the verify script didn't encounter any problems (e.g. diff missing, file system full etc) and log FAIL or WARN for a logscraper to pick up.

> Do we want to FAIL on all differences ? Could probably do that fairly easily in
> verify.sh with s/WARN/FAIL/ and have another boolean to track overall exit
> status from the check_updates calls (and probably elsewhere). My guess is that
> it's useful to continue to do the whole run before failing out, rather than
> immediately. The problem with that is we have some known issues that are
> usually ignored, eg
> Fx2/win32: Only in source/bin: .autoreg
> (could probably fix the mar packaging, don't think this file is used anymore
> because the app detects changes in itself better)
> Tb1.5/win32: Files source/bin/regxpcom.exe and target/bin/regxpcom.exe differ
> (EOL now though, bug 376675)
> Fx3/win32: a whole bunch of stuff needs to be added to removed-files.in (mex
> files in res\fonts, spellcheck.xpt on mac ?)

I think we should start WARNing on these, and work on fixing them. It's error-prone to have "known good" error/warning messages. Once we actually fix them, I'd be open to reporting FAIL for any diff.

> We could just FAIL on binary differences, by encoding more information into the
> exit status returned from check_updates to verify.sh. Is that what you mean in
> comment #4 ? FAIL/red on binary diffs and WARN/orange on other diffs ?

Right, I'm thinking FAIL on binary diffs and WARN on others, at least for now. I'm not sure about the exit code thing; it'd be nice to differentiate the test code failing versus the test failing, and it's easy enough to parse the log with bootstrap or buildbot.

Comment 7

[Robert Helmer [:rhelmer]]

Attachment: [checked in] detect binary, non-binary, and other errors from diff

This annoyingly redundant, but I think it gives us all the info we need, if we were to search the log file for both FAIL and WARN.

Comment 8

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 294315 [details] [diff] [review]
[checked in] detect binary, non-binary, and other errors from diff

Sorry for the delay, r+.

Comment 9

[Robert Helmer [:rhelmer]]

Comment on attachment 294315 [details] [diff] [review]
[checked in] detect binary, non-binary, and other errors from diff

Checking in common/check_updates.sh;
/cvsroot/mozilla/testing/release/common/check_updates.sh,v  <--  check_updates.sh
new revision: 1.8; previous revision: 1.7
done
Checking in updates/verify.sh;
/cvsroot/mozilla/testing/release/updates/verify.sh,v  <--  verify.sh
new revision: 1.12; previous revision: 1.11
done

Comment 10

[Robert Helmer [:rhelmer]]

Sorry, there's a bug in here; need to check both the exit code of diff and grep, this will do the wrong thing as-is. Testing a fix now.

Comment 11

[Robert Helmer [:rhelmer]]

Attachment: more correct error checking

Ok, tested this in both the "binary diffs" and "all ok" scenarios.

It looks like we get no warnings for Firefox currently; I see .autoreg in both source and target, etc (!).

Comment 12

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 294589 [details] [diff] [review]
more correct error checking

Looks as good as shell ever does :-)

I think you need "return 3" in the two unknown error cases - otherwise the else block you added to verify.sh earlier is superfluous.

Comment 13

[Robert Helmer [:rhelmer]]

(In reply to comment #12)
> (From update of attachment 294589 [details] [diff] [review])
> Looks as good as shell ever does :-)
> 
> I think you need "return 3" in the two unknown error cases - otherwise the else
> block you added to verify.sh earlier is superfluous. 

Oh right, thanks; yeah the one in verify.sh is nice because it tells you exactly which OS and locale failed on it's own FAIL line, so you can quickly scan the results.

I'll add that and land, thanks!

Comment 14

[Robert Helmer [:rhelmer]]

Attachment: [checked in] more correct error checking [as landed]

Checking in common/check_updates.sh;
/cvsroot/mozilla/testing/release/common/check_updates.sh,v  <--  check_updates.sh
new revision: 1.9; previous revision: 1.8
done

Comment 15

[Robert Helmer [:rhelmer]]

Works on staging. I see non-binary diffs for win32 (WARN), and I faked up a scenario for the binary diff case (FAIL), and also the normal "desirable" case works as expected (no WARN, no FAIL)

Comment 16

[Robert Helmer [:rhelmer]]

This works fine for everything except for win32 cygwin, the "diff" command does not return whether or not the file is binary.

Comment 17

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Taking for quick investigation.

Comment 18

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

lowering to p3, as I dont have time to look at this right now.

Comment 19

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

While I still think this is something we should fix, I've not had time to look at this in months. Pushing to Future until I (or anyone else) gets time for this.

Comment 20

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to comment #16)
> This works fine for everything except for win32 cygwin, the "diff" command does
> not return whether or not the file is binary.

Given that we only have TB2 still using Cygwin for update-verify I think we should close this FIXED.

Comment 21

[Chris Cooper [:coop] (he/him)]

Moving closed Future bugs into Release Engineering in preparation for removing the Future component.