753605 - Move emulator code into separate package

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Jonathan Griffin (:jgriffin)]

The emulator handling code which Marionette uses should be moved into a separate package, which could be imported by the marionette test runner.

This would allow it to be used for other purposes, like CI running jlal's gaia framework.

As part of this, wait_for_port should be moved out of emulator and into the testrunner.

Comment 1

[Jonathan Griffin (:jgriffin)]

We discussed this briefly with Clint.  We're going to make a generic android emulator class, and a B2G specific subclass, and add them to mozdevice.

Comment 2

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: new base abstract class Emulator and B2GEmulator extending it

Comment 3

[Mihnea Dobrescu-Balaur (:mihneadb)]

Comment on attachment 641511 [details] [diff] [review]
new base abstract class Emulator and B2GEmulator extending it

wrong file

Comment 4

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: new base abstract class Emulator and B2GEmulator extending it

Comment 5

[Jonathan Griffin (:jgriffin)]

Comment on attachment 641513 [details] [diff] [review]
new base abstract class Emulator and B2GEmulator extending it

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

Good progress, I made some suggestions about how the base class could be made further generic.

::: build/mobile/emulator.py
@@ +38,5 @@
> +    deviceRe = re.compile(r"^emulator-(\d+)(\s*)(.*)$")
> +
> +    def __init__(self, homedir=None, noWindow=False, logcat_dir=None, arch="x86",
> +                 emulatorBinary=None, res='480x800', userdata=None,
> +                 memory='512', partition_size='512'):

homedir is relevant only for B2G; it should only be in the B2G subclass.

@@ +42,5 @@
> +                 memory='512', partition_size='512'):
> +        self.port = None
> +        self._emulator_launched = False
> +        self.proc = None
> +        self.marionette_port = None

We should rename self.marionette_port to self.local_port, since this will be used in contexts outside of Marionette.

@@ +48,5 @@
> +        self._tmp_userdata = None
> +        self._adb_started = False
> +        self.logcat_dir = logcat_dir
> +        self.logcat_proc = None
> +        self.arch = arch

We should move arch to the B2G subclass, since it isn't used in the base class.

@@ +96,5 @@
> +    def _check_for_adb(self):
> +        self.adb = os.path.join(self.homedir,
> +                                'glue/gonk/out/host/linux-x86/bin/adb')
> +        if not os.access(self.adb, os.F_OK):
> +            self.adb = os.path.join(self.homedir, 'bin/adb')

These two relative paths (glue/gonk... and bin/adb) are relevant to B2G only, so this part of this function should be in the B2G subclass.

@@ +178,5 @@
> +        if not self._emulator_launched:
> +            return
> +        self.close()
> +        self.start()
> +        return self.setup_port_forwarding(port)

I think we shouldn't call setup_port_forwarding here, since it's mostly a Marionette-specific use case. (This also means we don't need a port parameter to this method.) Instead, when we update Marionette to use this class, we should call setup_port_forwarding from Marionette where we call restart().

@@ +206,5 @@
> +        self.port = int(list(online)[0])
> +
> +
> +    # must be overridden in extending class
> +    def _check_for_os(self):

I think this might better be named _locate_files, since that's what it's really doing.

@@ +207,5 @@
> +
> +
> +    # must be overridden in extending class
> +    def _check_for_os(self):
> +        pass

For methods like this that must be overridden, they should raise NotImplementedError in the base class, or use the @abstractmethod decorator.

@@ +218,5 @@
> +        qemu_args = self.args[:]
> +        if self.copy_userdata:
> +            # Make a copy of the userdata.img for this instance of the emulator
> +            # to use.
> +            self._tmp_userdata = tempfile.mktemp(prefix='marionette')

We should use a generic 'emulator' prefix here.

Comment 6

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: modified according to feedback

Comment 7

[Jonathan Griffin (:jgriffin)]

Comment on attachment 642026 [details] [diff] [review]
modified according to feedback

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

Looks good!  I think we'll probably want to move B2GEmulator into its own file.

::: build/mobile/emulator.py
@@ +90,5 @@
> +    def _check_for_adb(self):
> +        if not os.access(self.adb, os.F_OK):
> +            adb = subprocess.Popen(['which', 'adb'],
> +                                                   stdout=subprocess.PIPE,
> +                                                   stderr=subprocess.STDOUT)

should line these up with the previous argument

Comment 8

[Jonathan Griffin (:jgriffin)]

Comment on attachment 642026 [details] [diff] [review]
modified according to feedback

Joel, you can provide feedback or not on this as time and interest dictate.

Comment 9

[Jonathan Griffin (:jgriffin)]

Comment on attachment 642026 [details] [diff] [review]
modified according to feedback

Mark, I understand you may be using the android emulator in some autophone stuff, so this class might help you.

Comment 10

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: moved b2gemulator to a separate file; alligned args

Comment 11

[Jonathan Griffin (:jgriffin)]

Comment on attachment 642638 [details] [diff] [review]
moved b2gemulator to a separate file; alligned args

Thanks!

Comment 12

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

Comment on attachment 642638 [details] [diff] [review]
moved b2gemulator to a separate file; alligned args

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

overall this looks pretty good.

::: build/mobile/b2gemulator.py
@@ +13,5 @@
> +import socket
> +import subprocess
> +from telnetlib import Telnet
> +import tempfile
> +import time

you don't use multiprocessing, shutil, socket, subprocess, Telnet, tempfile, time in here

@@ +33,5 @@
> +        if self.homedir is not None:
> +            self.homedir = os.path.expanduser(homedir)
> +
> +    def _check_file(self, filePath):
> +        if not os.access(filePath, os.F_OK):

why not os.exists?

::: build/mobile/emulator.py
@@ +7,5 @@
> +from mozprocess import ProcessHandlerMixin
> +import multiprocessing
> +import os
> +import re
> +import platform

I don't believe you use platform in here.

@@ +262,5 @@
> +        """ Set up TCP port forwarding to the specified port on the device,
> +            using any availble local port, and return the local port.
> +        """
> +
> +        import socket

we import this at the top

Comment 13

[Mihnea Dobrescu-Balaur (:mihneadb)]

I preserved the imports, I figured they weren't used before as well so I thought it was somewhat of an in-house convention.

Will modify. :)

Comment 14

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: removed unused imports, switched to os.path.exists, fixed some whitespace

Comment 15

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

Comment on attachment 643216 [details] [diff] [review]
removed unused imports, switched to os.path.exists, fixed some whitespace

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

::: build/mobile/b2gemulator.py
@@ +29,5 @@
> +                             'B2G_HOME correctly?') % filePath)
> +
> +    def _check_for_adb(self):
> +        self.adb = os.path.join(self.homedir,
> +                                'glue/gonk/out/host/linux-x86/bin/adb')

my adb doesn't live in a place like this, I assume that is where the adb for the emulator lives?

@@ +30,5 @@
> +
> +    def _check_for_adb(self):
> +        self.adb = os.path.join(self.homedir,
> +                                'glue/gonk/out/host/linux-x86/bin/adb')
> +        if not os.access(self.adb, os.F_OK):

another os.access here, can we replace all instances with os.path.exists?

::: build/mobile/emulator.py
@@ +86,5 @@
> +        else:
> +            return self.port is not None
> +
> +    def _check_for_adb(self):
> +        if not os.access(self.adb, os.F_OK):

os.path.exists?

@@ +234,5 @@
> +        """ Rotate a logfile, by recursively rotating logs further in the sequence,
> +            deleting the last file if necessary.
> +        """
> +        destlog = os.path.join(self.logcat_dir, 'emulator-%d.%d.log' % (self.port, index))
> +        if os.access(destlog, os.F_OK):

os.path.exists?

Comment 16

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Joel Maher (:jmaher) from comment #15)
> Comment on attachment 643216 [details] [diff] [review]
> removed unused imports, switched to os.path.exists, fixed some whitespace
> 
> Review of attachment 643216 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/mobile/b2gemulator.py
> @@ +29,5 @@
> > +                             'B2G_HOME correctly?') % filePath)
> > +
> > +    def _check_for_adb(self):
> > +        self.adb = os.path.join(self.homedir,
> > +                                'glue/gonk/out/host/linux-x86/bin/adb')
> 
> my adb doesn't live in a place like this, I assume that is where the adb for
> the emulator lives?

I think so, I'm not sure. This was there before and I did not interfere with it. It is only used when doing connect(), which I don't use when I run the tests via Marionette. Maybe jgriffin can tell us more about it.

Comment 17

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: switched to os.path.exists all over

Comment 18

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

Comment on attachment 643244 [details] [diff] [review]
switched to os.path.exists all over

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

when I give you an r+ with a comment to change something, feel free to carry over my r+ unless you feel that your changes are significant.

Comment 19

[Mihnea Dobrescu-Balaur (:mihneadb)]

Sorry, I didn't know. Thanks!

Comment 20

[Jonathan Griffin (:jgriffin)]

(In reply to Mihnea Dobrescu-Balaur from comment #16)
> (In reply to Joel Maher (:jmaher) from comment #15)
> > Comment on attachment 643216 [details] [diff] [review]
> > removed unused imports, switched to os.path.exists, fixed some whitespace
> > 
> > Review of attachment 643216 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: build/mobile/b2gemulator.py
> > @@ +29,5 @@
> > > +                             'B2G_HOME correctly?') % filePath)
> > > +
> > > +    def _check_for_adb(self):
> > > +        self.adb = os.path.join(self.homedir,
> > > +                                'glue/gonk/out/host/linux-x86/bin/adb')
> > 
> > my adb doesn't live in a place like this, I assume that is where the adb for
> > the emulator lives?
> 
> I think so, I'm not sure. This was there before and I did not interfere with
> it. It is only used when doing connect(), which I don't use when I run the
> tests via Marionette. Maybe jgriffin can tell us more about it.

Yes, this is path of adb relative to the B2G directory.  Note that this only true on linux; we should add code similar to http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/emulator.py#77 so that we select the right relative path on Mac as well.  Is isn't possible to run B2G emulators on windows, so no need to worry about that.

Comment 21

[Jonathan Griffin (:jgriffin)]

Comment on attachment 643244 [details] [diff] [review]
switched to os.path.exists all over

r+ with the change suggested in comment #20

Comment 22

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: cleaned the adb finding logic, fixed the paths

carrying forward r+ from :jmaher :jgriffin

Comment 23

[Jonathan Griffin (:jgriffin)]

Thanks Mihnea.  Can you update this patch one last time with the changes from bug 775308, which just landed?  I can then land this one.

Comment 24

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: add -gpu on

carrying forward r+ from :jmaher :jgriffin

Comment 25

[Jonathan Griffin (:jgriffin)]

http://hg.mozilla.org/mozilla-central/rev/2585b74429d0

Will file a separate github pull request for merging this into mozbase.