UnpackFile doesn't handle tests.zip properly

RESOLVED FIXED

Status

Release Engineering
General
P2
major
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: jhford, Assigned: jhford)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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
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
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.
(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?
Blocks: 549427
(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.
Created attachment 437618 [details] [diff] [review]
UnpackFile changes
Assignee: nobody → jhford
Status: NEW → ASSIGNED
Attachment #437618 - Flags: review?(catlee)
Severity: normal → major
Priority: -- → P2
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.
do we know how many times this error checking has found an error that was a valid error?
Alternately, we should be doing unzip -oq (for quiet) so the error checking doesn't pick up filenames as it currently does.
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.
Attachment #437618 - Flags: review?(catlee) → review+
Comment on attachment 437618 [details] [diff] [review]
UnpackFile changes

http://hg.mozilla.org/build/buildbotcustom/rev/a20f711dc417
Attachment #437618 - Flags: checked-in+
baked overnight in production and looks good.  Closing bug
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Going green now.

Please don't consider bug complete if it hasn't been pushed to the masters.
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.