Starting an Android application is complicated. You want to do this with the "am" tool, but the syntax is fairly complicated. In the worst case, you have to specify all of these seperately: * Appname (e.g. org.mozilla.fennec) * Activity name (eg. "App", "BrowserActivity") * Intent (e.g. android.intent.action.VIEW) * Environment variables (e.g. DEBUG=1) * URL (e.g. http://google.com) * Extra command line options (e.g. -profile /path/to/profile/on/device) Other times, you just want to run a simple shell command on the device. The problem is that our current methods (fireProcess and launchProcess) don't really distinguish these cases, so you have special case code all over the place to try to do the above (which only ever works for Fennec!). The also imply that you can get a useful logfile of the output of running these commands, which is rarely the case. We need good, non-confusing abstractions for both of these cases so I wrote some. The motivating use-case for me is to be able to launch different types of browsers (stock Android, Chrome, etc.) for Eideticker, but I think they will ultimately be useful as replacements for fireProcess and launchProcess. To avoid boiling the earth, I've left the latter two in for now (but marked them as deprecated). Eventually I envision we would make Mochitest, Talos, etc. use my new methods and we can remove these methods.

Oh, also one thing about this patch: It kind of makes a joke of devicemanager being a cross-platform abstraction. Honestly I'm not sure what to think/do about that: there are very real differences between how operating systems will stop/start applications and we *need* operating system specific abstractions to be able to deal with that if this is going to be useful for anything other than testing a specific version of mobile Firefox (i.e. assuming that the activity name and intent always stay the same).

Comment on attachment 598285 [details] [diff] [review] Patch to add shell() and launchApplication() methods Review of attachment 598285 [details] [diff] [review]: ----------------------------------------------------------------- this means we could remove some api's from sutagent once we convert existing scripts to use this. r- I would just want to ensure we have an option for blocking/nonblocking calls to launch a process. ::: build/mobile/devicemanagerADB.py @@ +72,5 @@ > + proc.wait() > + if proc.returncode == 0: > + return proc.stdout.read() > + else: > + return None this waits until the process is completed?

So after some discussions with jmaher and geoffbrown, I'm convinced this could use a bit of refinement: * We should have an option/variant of shell() to direct output to a file, to handle cases like bug 705192 (where we might have a very large log file of results). * We should seperate out the Android-specific stuff into a Mixin and create seperate classes to handle that case (going with DroidADB and DroidSUT for now). Re: Blocking/non-blocking * shell's intent is to run a command to completion. I can't really think of a use-case where you wouldn't want it to be synchronous/blocking. * launchApplication is mostly non-blocking in the sense that it exits after the application is successfully launched (but not completely started up). We could in theory provide a variant which didn't wait to see the process before returning, but again, I don't really see the use case there. I worry that it would be just giving people yet another opportunity to shoot themselves in the foot. :P

(In reply to William Lachance (:wlach) from comment #4) > * shell's intent is to run a command to completion. I can't really think of > a use-case where you wouldn't want it to be synchronous/blocking. That seems fine to me. For xpcshell, synchronous/blocking should be good.

It would be helpful (perhaps necessary) for shell() to accept and handle env and cwd arguments also: The SUT "EXEC" command expects environment variables to be specified in a specific format.

...and also deal with character escapes, so that characters like ( are handled properly by ADB/SUT: See https://bugzilla.mozilla.org/show_bug.cgi?id=705192#c12

Here's a revised patch which addresses the following: 1. geoffbrown's use case (running a command on the device and getting the full log file without loading the whole thing into memory). at this point that's only implemented for SUT (it would be more effort to do for ADB and may not be worth it atm given that ADB is debugging-only) 2. shell command now returns return code of command, and only returns actually command output. 3. handle cwd and environment (based on earlier work by geoffbrown) 4. android-specific functionality (launchApplication) moved into a mixin in a file called "droid". Instantiate new classes "DroidADB" and "DroidSUT" to use. To make (1) and (2) work, a fairly invasive refactoring of devicemanagerSUT was required. I am fairly confident that the external behaviour is unchanged, but we'll see after the try run I just scheduled. (I may need to update this patch for bitrot, etc. as things are applied to the build -- just posting this here as an FYI for now)

Try run for 74f3f3d38c90 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=74f3f3d38c90 Results (out of 31 total builds): exception: 2 success: 25 warnings: 4 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/wlachance@mozilla.com-74f3f3d38c90

Updated patch for bitrot.

Comment on attachment 599603 [details] [diff] [review] Updated patch for specifying shell command, launchApplication Review of attachment 599603 [details] [diff] [review]: ----------------------------------------------------------------- just some simple questions, otherwise this is great. ::: build/mobile/devicemanager.py @@ +227,5 @@ > + acmd.extend(["-d", ''.join(['"', url, '"'])]) > + > + self.shell(acmd) > + > + return self.processExist(app) is there a chance we could have a timing issue here? For example launch fennec might take a few seconds? Also when running robocop, we launch the instrumentation but we don't do am start, we do am instrument. ::: build/mobile/devicemanagerSUT.py @@ +265,5 @@ > + # periodically flush data to output file to make sure it doesn't get > + # too big/unwieldly > + if len(data) > 1024: > + outputfile.write(data[0:1024]) > + data = data[1024:] how is this w.r.t performance? we run many instances of dm on a foopie along with webservers, etc...

(In reply to Joel Maher (:jmaher) from comment #11) > Comment on attachment 599603 [details] [diff] [review] > Updated patch for specifying shell command, launchApplication > > Review of attachment 599603 [details] [diff] [review]: > ----------------------------------------------------------------- > > just some simple questions, otherwise this is great. > > ::: build/mobile/devicemanager.py > @@ +227,5 @@ > > + acmd.extend(["-d", ''.join(['"', url, '"'])]) > > + > > + self.shell(acmd) > > + > > + return self.processExist(app) > > is there a chance we could have a timing issue here? For example launch > fennec might take a few seconds? Good point. We should probably spin until the process has actually started here. I'll update the patch (shouldn't affect the try server run I'm doing, as nothing calls this code yet) > Also when running robocop, we launch the instrumentation but we don't do am > start, we do am instrument. Yes, we may want to update this for robocop, which I'm not really very familiar with. How about we handle that in a seperate bug? > ::: build/mobile/devicemanagerSUT.py > @@ +265,5 @@ > > + # periodically flush data to output file to make sure it doesn't get > > + # too big/unwieldly > > + if len(data) > 1024: > > + outputfile.write(data[0:1024]) > > + data = data[1024:] > > how is this w.r.t performance? we run many instances of dm on a foopie > along with webservers, etc... This should actually be better than before in the worst case, where we always processed the entire output from the agent multiple times. Probably only important for edge cases like xpcshell thoguh, where we're processing an entire log file. Parsing a few kilobytes is never going to be very slow.

Try run for d22280b55574 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=d22280b55574 Results (out of 17 total builds): exception: 2 success: 1 warnings: 14 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/wlachance@mozilla.com-d22280b55574

(In reply to William Lachance (:wlach) from comment #13) > (In reply to Joel Maher (:jmaher) from comment #11) > > Comment on attachment 599603 [details] [diff] [review] > > Updated patch for specifying shell command, launchApplication > > > > Review of attachment 599603 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > just some simple questions, otherwise this is great. > > > > ::: build/mobile/devicemanager.py > > @@ +227,5 @@ > > > + acmd.extend(["-d", ''.join(['"', url, '"'])]) > > > + > > > + self.shell(acmd) > > > + > > > + return self.processExist(app) > > > > is there a chance we could have a timing issue here? For example launch > > fennec might take a few seconds? > > Good point. We should probably spin until the process has actually started > here. I'll update the patch (shouldn't affect the try server run I'm doing, > as nothing calls this code yet) Come to think of it, maybe it would be better just to return right away. Just because the process exists it doesn't mean it's ready to do anything. Maybe a better alternative here is to return the following: On success (shell command to start app returned '0'): Return True On failure (shell command to start app didn't return '0'): Return False

Comment on attachment 599641 [details] [diff] [review] Updated patch to correct merge problems Review of attachment 599641 [details] [diff] [review]: ----------------------------------------------------------------- Good stuff -- this should work for xpcshell too. ::: build/mobile/devicemanager.py @@ +648,5 @@ > + > + # truncate off the return code line > + file.truncate(length - bytes_from_end) > + file.seek(0,2) > + file.write(0) I think you intended: file.write('\0') ? As it is, I get: Traceback (most recent call last): File "/home/mozdev/src/config/pythonpath.py", line 52, in <module> main(sys.argv[1:]) File "/home/mozdev/src/config/pythonpath.py", line 44, in main execfile(script, frozenglobals) File "/home/mozdev/src/testing/xpcshell/remotexpcshelltests.py", line 340, in <module> main() File "/home/mozdev/src/testing/xpcshell/remotexpcshelltests.py", line 335, in main **options.__dict__): File "/home/mozdev/src/testing/xpcshell/runxpcshelltests.py", line 516, in runTests stdout=pStdout, stderr=pStderr, env=self.env, cwd=testdir) File "/home/mozdev/src/testing/xpcshell/remotexpcshelltests.py", line 215, in launchProcess rc = self.device.shell(cmd, f, cwd=self.remoteHere, env=env) File "/home/mozdev/src/build/mobile/devicemanagerADB.py", line 96, in shell lastline = _pop_last_line(outputfile) File "/home/mozdev/src/build/mobile/devicemanager.py", line 652, in _pop_last_line file.write(0) TypeError: expected a character buffer object

Try run for c856fc3d065c is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=c856fc3d065c Results (out of 29 total builds): exception: 2 success: 3 warnings: 24 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/wlachance@mozilla.com-c856fc3d065c

(In reply to Geoff Brown [:gbrown] from comment #16) > I think you intended: file.write('\0') ? > > As it is, I get: > > Traceback (most recent call last): > File "/home/mozdev/src/config/pythonpath.py", line 52, in <module> > main(sys.argv[1:]) > File "/home/mozdev/src/config/pythonpath.py", line 44, in main > execfile(script, frozenglobals) > File "/home/mozdev/src/testing/xpcshell/remotexpcshelltests.py", line 340, > in <module> > main() > File "/home/mozdev/src/testing/xpcshell/remotexpcshelltests.py", line 335, > in main > **options.__dict__): > File "/home/mozdev/src/testing/xpcshell/runxpcshelltests.py", line 516, in > runTests > stdout=pStdout, stderr=pStderr, env=self.env, cwd=testdir) > File "/home/mozdev/src/testing/xpcshell/remotexpcshelltests.py", line 215, > in launchProcess > rc = self.device.shell(cmd, f, cwd=self.remoteHere, env=env) > File "/home/mozdev/src/build/mobile/devicemanagerADB.py", line 96, in shell > lastline = _pop_last_line(outputfile) > File "/home/mozdev/src/build/mobile/devicemanager.py", line 652, in > _pop_last_line > file.write(0) > TypeError: expected a character buffer object Hmm, I didn't get that behaviour here, but you are right that was what I intended. Will post a fixed patch soon.

(In reply to Mozilla RelEng Bot from comment #17) > Try run for c856fc3d065c is complete. > Detailed breakdown of the results available here: > https://tbpl.mozilla.org/?tree=Try&rev=c856fc3d065c > Results (out of 29 total builds): > exception: 2 > success: 3 > warnings: 24 > Builds (or logs if builds failed) available at: > http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/wlachance@mozilla. > com-c856fc3d065c Hmm, not sure what's going on here. I'm not seeing this kind of behaviour on my machine. Going to try running against try again.

Try run for 8cd72c53c067 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=8cd72c53c067 Results (out of 28 total builds): success: 25 warnings: 3 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/wlachance@mozilla.com-8cd72c53c067

final patch, addresses issue with appending integer, return value of launchApplication, and removes accidentally left-in launchApplication method in devicemanager

Comment on attachment 599971 [details] [diff] [review] address a few more issues I just realized that the agent changes (actually done by :gbrown) still need to be reviewed. Joel, could you take care of this? They look fine to me.

Comment on attachment 599971 [details] [diff] [review] address a few more issues My mistake, jmaher had already looked over the agent changes.

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