679602 - Improve robustness of devicemanagerADB: check for adb, remote cp

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Geoff Brown [:gbrown]]

Recent attempts to run xpcshell tests on Android have run into some problems that we can handle better:

 - If adb cannot be executed (SDK not installed or not in PATH), devicemanagerADB can report this issue explicitly.

 - If cp cannot be executed on device (not installed, no permissions for shell and/or fennec user), devicemanagerADB can attempt to push files directly rather than pushing to a temporary location and copying them.

Comment 1

[Geoff Brown [:gbrown]]

Attachment: patch for review

Explicitly check for ability to execute adb and report appropriate error.
Also check for cp on device and avoid run-as trick when cp is not available. 

When run-as and cp are available, devicemanager pushes files to a temp location, then uses run-as cp to copy them to their final destinations. This allows files to be owned by the uid associated with the fennec package, more closely resembling a production install of fennec. 

However, some devices do not have cp installed, or cp is installed but cannot be executed by non-privileged users; in these cases, devicemanager just pushes files to their final destinations, and they are owned by shell (or root). As long as chmod successfully grants full permissions on the files, this is sufficient to run a broad range of tests (the full set of xpcshell tests works for me this way).

Comment 2

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

Comment on attachment 553853 [details] [diff] [review]
patch for review

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

I don't see anything wrong with this patch as it and it helps solve some problems with adb and automation, so r+.


I would like to ensure that we can do a: 'runas {packageName} cp x y'

Maybe we could do a:
 * create tempfile on local host
 * push tempfile to testroot/tmp
 * runas packagename mkdir testroot/sanity
 * runas packagename cp testroot/tmp/tmpfile testroot/sanity/testfile
 * verify file exists
 * runas packagename rmdir testroot/sanity

That would be a bit more comprehensive.  Thoughts?

Comment 3

[Geoff Brown [:gbrown]]

Attachment: patch for review

I agree: A more comprehensive test is warranted. I have implemented just a slight modification on the operations suggested.

Comment 4

[Geoff Brown [:gbrown]]

http://tbpl.mozilla.org/?tree=Try&rev=1afe5356e410

Comment 5

[Geoff Brown [:gbrown]]

From the try run:

Connecting to: 10.250.49.19
reconnecting socket
unable to connect socket
reconnecting socket
unable to connect socket
reconnecting socket
unable to connect socket
reconnecting socket
unable to connect socket
reconnecting socket
unable to connect socket
reconnecting socket
unable to connect socket
devroot None
/builds/tegra-032/test/../error.flg
Remote Device Error: devRoot from devicemanager [None] is not correct
program finished with exit code 1


Did my change cause this?

Comment 6

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

this is a problem that pops up every now and then, if this happens on all runs, you have a problem, but this code that you are running shouldn't even be executing on tinderbox.

Comment 7

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

Comment on attachment 554282 [details] [diff] [review]
patch for review

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

::: build/mobile/devicemanagerADB.py
@@ +548,5 @@
> +      if (not self.dirExists(self.tmpDir)):
> +        self.mkDir(self.tmpDir)
> +      self.checkCmd(["shell", "run-as", packageName, "mkdir", devroot + "/sanity"])
> +      self.checkCmd(["push", os.path.abspath(sys.argv[0]), self.tmpDir + "/tmpfile"])
> +      self.checkCmd(["shell", "run-as", packageName, "cp", self.tmpDir + "/tmpfile", devroot + "/sanity"])

where is '/tmpfile' created at?  otherwise this looks good.

Comment 8

[Geoff Brown [:gbrown]]

os.path.abspath(sys.argv[0]) gives the full path to the .py file that is being executed. So, for example, if remotexpcshelltests.py is using devicemanagerADB, then remotexpcshelltests.py is pushed as the tmpfile. This way we do not risk a local file creation failure.

Comment 9

[Ed Morley [:emorley]]

In my queue, which is being sent to try and then presuming green, I'll land on inbound later today.

Comment 10

[Ed Morley [:emorley]]

http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=e3f0e5b06e77

http://hg.mozilla.org/integration/mozilla-inbound/rev/4bd995181c52

Comment 11

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/4bd995181c52