557854 - UnpackFile doesn't handle tests.zip properly

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[John Ford [:jhford] CET/CEST Berlin Time]

We need this step to properly deal with a tests.zip tarball.

Similar to:
http://stage.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux/1270660515/firefox-3.7a4pre.en-US.linux-i686.tests.zip

Comment 1

[John Ford [:jhford] CET/CEST Berlin Time]

436     def evaluateCommand(self, cmd):
437         superResult = ShellCommand.evaluateCommand(self, cmd)
438         if superResult != SUCCESS:
439             return superResult
440 
441         if self.filename.endswith(".zip"):
442             if None != re.search('ERROR', cmd.logs['stdio'].getText()):
443                 return FAILURE
444         if None != re.search('^Usage:', cmd.logs['stdio'].getText()):
445             return FAILURE
446 
447         return SUCCESS

It looks like 441 is a broken assumption on the file

  inflating: xpcshell/tests/test_necko/test/data/name-scheme/folder^^/ERROR_IF_SEE_THIS.txt^  

The ERROR pattern is not matched in the symbols zip but does in the tests zip.  I am goign to reproduce an unzip error and see how we can make the pattern more specific

Comment 2

[John Ford [:jhford] CET/CEST Berlin Time]

From the OS X unzip man page:

DIAGNOSTICS
       The exit status (or error level) approximates the exit codes defined by
       PKWARE and takes on the following values, except under VMS:

              0      normal; no errors or warnings detected.
<snip>

We probably don't need this check.  If we still want it, we should change to using "^Archive:" and "^unzip:" as the error cases.  Both of those error cases have non-zero exit status.

Comment 3

[Chris AtLee [:catlee]]

(In reply to comment #2)
> From the OS X unzip man page:
> 
> DIAGNOSTICS
>        The exit status (or error level) approximates the exit codes defined by
>        PKWARE and takes on the following values, except under VMS:
> 
>               0      normal; no errors or warnings detected.
> <snip>
> 
> We probably don't need this check.  If we still want it, we should change to
> using "^Archive:" and "^unzip:" as the error cases.  Both of those error cases
> have non-zero exit status.

How about on Windows?  What happens if unpacking a corrupt archive, or the disk is full, or there are permission problems?

Comment 4

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to comment #3)
> How about on Windows?  What happens if unpacking a corrupt archive, or the disk
> is full, or there are permission problems?

According to the man page for info-zip (what we use), those case are covered with non-zero error codes.  As the windows port is in the upstream package, I think we should rely on unzip to do proper error handling.

Comment 5

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: UnpackFile changes

Comment 6

[John Ford [:jhford] CET/CEST Berlin Time]

looking at the unzip sources (ftp://ftp.info-zip.org/pub/infozip/src/unzip552.zip)

find . -type f -exec grep 'ERROR' {} \;

I was only able to find 'ERROR' in a string in the unix makefile |make check| target, and it was |"#####  ERROR:  .*$|.  That is not a valid pattern to match on as we don't run the unzip unittests.

Comment 7

[John Ford [:jhford] CET/CEST Berlin Time]

do we know how many times this error checking has found an error that was a valid error?

Comment 8

[John Ford [:jhford] CET/CEST Berlin Time]

Alternately, we should be doing unzip -oq (for quiet) so the error checking doesn't pick up filenames as it currently does.

Comment 9

[Chris AtLee [:catlee]]

Comment on attachment 437618 [details] [diff] [review]
UnpackFile changes

We don't know why this check is in there, but it hasn't caught any failures since October.

Comment 10

[John Ford [:jhford] CET/CEST Berlin Time]

Comment on attachment 437618 [details] [diff] [review]
UnpackFile changes

http://hg.mozilla.org/build/buildbotcustom/rev/a20f711dc417

Comment 11

[John Ford [:jhford] CET/CEST Berlin Time]

baked overnight in production and looks good.  Closing bug

Comment 12

[alice nodelman [:alice] [:anode]]

This never actually got applied on talos-master - which has been red since the zip file change landed.

Did a buildbot custom update and then reconfig of the master, will see if that clears things up.

Comment 13

[alice nodelman [:alice] [:anode]]

Going green now.

Please don't consider bug complete if it hasn't been pushed to the masters.