Closed Bug 408157 Opened 12 years ago Closed 10 years ago

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

Categories

(Release Engineering :: General, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: joduinn, Unassigned)

References

Details

(Whiteboard: needs cygwin support)

Attachments

(2 files, 2 obsolete files)

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.
Assignee: nobody → rhelmer
Status: NEW → ASSIGNED
Attachment #292924 - Flags: review?(nrthomas)
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 ?
(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 ^

Priority: -- → P2
(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..
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 ?
Attachment #292924 - Flags: review?(nrthomas)
(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.
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.
Attachment #292924 - Attachment is obsolete: true
Attachment #294315 - Flags: review?(nrthomas)
Whiteboard: waiting for review
Comment on attachment 294315 [details] [diff] [review]
[checked in] detect binary, non-binary, and other errors from diff

Sorry for the delay, r+.
Attachment #294315 - Flags: review?(nrthomas) → review+
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
Attachment #294315 - Attachment description: detect binary, non-binary, and other errors from diff → [checked in] detect binary, non-binary, and other errors from diff
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch more correct error checking (obsolete) — Splinter Review
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 (!).
Attachment #294589 - Flags: review?(nrthomas)
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.
Attachment #294589 - Flags: review?(nrthomas) → review+
(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!
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
Attachment #294589 - Attachment is obsolete: true
Whiteboard: waiting for review → testing on staging
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)
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Whiteboard: testing on staging
This works fine for everything except for win32 cygwin, the "diff" command does not return whether or not the file is binary.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: needs cygwin support
Priority: P2 → P3
Assignee: rhelmer → nobody
Status: REOPENED → NEW
Attachment #294610 - Attachment is patch: true
Attachment #294610 - Attachment mime type: application/octet-stream → text/plain
Taking for quick investigation.
Assignee: nobody → joduinn
Priority: P3 → P2
lowering to p3, as I dont have time to look at this right now.
Priority: P2 → P3
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.
Assignee: joduinn → nobody
Component: Release Engineering → Release Engineering: Future
QA Contact: build → release
(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.
Status: NEW → RESOLVED
Closed: 12 years ago10 years ago
Resolution: --- → FIXED
Moving closed Future bugs into Release Engineering in preparation for removing the Future component.
Component: Release Engineering: Future → Release Engineering
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.