715060 - devicemanagerADB pushDir fails on non-rooted device when unzipping to sdcard

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Geoff Brown [:gbrown]]

Bug 701038 introduced the use of zip/unzip to devicemanagerADB.pushDir, to speed up pushes of large directories. If pushDir is used with zip/unzip on a device without privileged/root access, and the destination directory is on /sdcard, then unzip is likely to fail and abort because it cannot change the permissions on the destination files. 

For example, when pushing to /mnt/sdcard/tests/profile (for a mochitest-robotium test run), I see:

Archive:  /mnt/sdcard/tests/profile/adbdmtmp.zip
   creating: chrome/
unzip: can't set permissions of directory 'chrome/': Operation not permitted
unzip: exiting

The remaining files in the directory are not created.

I can find no way to override this behavior in the BusyBox unzip.

Since permissions are of no consequence to most tests, this failure is undesirable. pushDir without zip/unzip will also fail to set file permissions in this scenario, but all the files will still be pushed.

Comment 1

[Geoff Brown [:gbrown]]

Attachment: patch to ADB pushDir

This patch detects the unzip failure and then falls back to the normal pushDir behavior automatically: If unzip fails, devicemanager retries the pushDir without the zip/unzip.

Comment 2

[Geoff Brown [:gbrown]]

:kats reported trouble running mochitest-robotium on an unrooted phone -- I suspect this patch will help him.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 585647 [details] [diff] [review]
patch to ADB pushDir

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

r=me, just a couple nits below.

::: build/mobile/devicemanagerADB.py
@@ +120,5 @@
>          subprocess.check_output(["zip", "-r", localZip, '.'], cwd=localDir)
>          self.pushFile(localZip, remoteZip)
>          os.remove(localZip)
> +        data = self.runCmdAs(["shell", "unzip", "-o", remoteZip, "-d", remoteDir]).stdout.read()
> +        print data

do we need the 'print data' here?

@@ +125,4 @@
>          self.checkCmdAs(["shell", "rm", remoteZip])
> +        if (re.search("unzip: exiting", data) or re.search("Operation not permitted", data)):
> +          print "zip/unzip failure: falling back to normal push"
> +          self.useZip = False

do we need to set self.useZip here?  It is referenced above this.

Comment 4

[Geoff Brown [:gbrown]]

(In reply to Joel Maher (:jmaher) from comment #3)
> do we need the 'print data' here?

No - just being verbose. I will remove it.

> do we need to set self.useZip here?  It is referenced above this.

Yes - so that the recursive pushDir call does not use zip/unzip. Also, I want to not use zip/unzip on any additional pushDir's in the same instance -- if unzip fails once, I figure it's not worth checking again.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

sounds good.  Upload a new patch and I can land it if you need me to.

Comment 6

[Geoff Brown [:gbrown]]

Attachment: patch to ADB pushDir

Just removed extra print call. Carrying forward r+=jmaher.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f35778a02a8

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/1f35778a02a8