Closed Bug 741479 Opened 8 years ago Closed 8 years ago

DeviceManagerADB contains a lot of Fennec-isms

Categories

(Testing :: Mozbase, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla14

People

(Reporter: jgriffin, Assigned: jgriffin)

Details

Attachments

(1 file, 3 obsolete files)

The current implementation of DeviceManagerADB contains a lot of code which is specific to Fennec.  We'd like to use this class for B2G, but it doesn't work in that environment as-is.

We could do one of two things:

1 - subclass this and override every method that does something specific to Fennec
2 - write a generic base class, and then provide Fennec- and B2G- specific subclasses

Option 2 is cleaner IMO.  We could even call the Fennec subclass "DeviceManagerADB" so that no changes would be needed by consumers of this class; the generic base class could be called something like "DeviceManagerBaseADB", and the B2G subclass "DeviceManagerB2GADB".

If this approach is OK, I can work on refactoring the code as appropriate.
Attached patch devicemanager patch (obsolete) — Splinter Review
This patch creates a base class for the ADB-based device manager (devicemanagerbaseADB.py), and a Fennec-specific subclass (devicemanagerADB.py).  Existing consumers of devicemanagerADB.py can use it unchanged, except for the fact that devicemanagerbaseADB.py will need to be in the same directory.  I've adjusted the reftest and mochitest makefiles accordingly.

I was able to keep most of the functionality in the base class, by modifying the base class in a couple of minor ways:
- allow passing of the adb path (in B2G automation, adb is not on the path, but is instead produced as part of the B2G build)
- allow passing of a device name, which if specified, is used in all calls to adb (as 'adb -s $deviceName args...').  This allows the devicemanager to operate on systems where adb manages more than one device simultaneously (as is the case with B2G CI)

The only things that I've moved to the Fennec sublcass are:
- the construction of a Fennec-specific packageName (in B2G, there are no package names that work as targets for run-as)
- the particular logic around getDeviceRoot (the Fennec code doesn't work on B2G, because /mnt/sdcard is read-only on b2G)

I think this structure will make it easy to adapt the code in the future, should B2G and Fennec diverge more wrt to adb usage.
Attachment #611634 - Flags: review?(wlachance)
Attachment #611634 - Flags: review?(jmaher)
Attached patch devicemanager patch, v0.2 (obsolete) — Splinter Review
Updated patch in which both classes are in the same file.
Attachment #611634 - Attachment is obsolete: true
Attachment #611634 - Flags: review?(wlachance)
Attachment #611634 - Flags: review?(jmaher)
Attachment #611639 - Flags: review?(wlachance)
Attachment #611639 - Flags: review?(jmaher)
Comment on attachment 611639 [details] [diff] [review]
devicemanager patch, v0.2

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

I see 3 things here:
1) change the test root
2) change the package name
3) add in '-s' support for >1 device

I really don't see why we need to subclass for this.  I thought we had a patch already for -s support, although it doesn't seem to be checked in.  

As for the test root, we try to use /data/local/tests and fall back to /mnt/sdcard/tests.  We really should modify our tools to verify that the testroot is writeable during initialization.  That would solve your problem.

Regarding the packagename, Will has done a lot of great work on refactoring things.  In the current devicemanagerADB if you pass in the packagename to the init, we use it.  

Right now I am not seeing a need for another subclass.  Maybe there are other things I am not seeing?
Attachment #611639 - Flags: review?(jmaher) → review-
Comment on attachment 611639 [details] [diff] [review]
devicemanager patch, v0.2

I agree with Joel's comments about checking for the device path.

The serial number / deviceName stuff is really a separate bug, though it's a small enough change that I think we can roll it into this bug. I don't know if someone has done it before, but it's certainly something we absolutely want.

I am not a great fan of inheritance in general, but honestly the use of it here does not seem that bad to me. I don't really see a better way of getting the behavior that we want (which I guess after jmaher's suggestion of checking to see if deviceRoot is writable, is just about not defaulting to using org.mozilla.fennec as the package name)

This gets an ok from me, with nits + the directory checking issues addressed.

>diff -r c6813085fd71 build/mobile/devicemanagerADB.py
>--- a/build/mobile/devicemanagerADB.py	Thu Mar 29 17:52:45 2012 -0700
>+++ b/build/mobile/devicemanagerADB.py	Mon Apr 02 16:08:00 2012 -0700

>+    # The name of the device in adb, used in cases where multiple devices
>+    # are managed by the same adb instance.
>+    self.deviceName = deviceName

Technically this isn't the "name", it's the serial number. Let's call this variable "deviceSerial" (or something similar) and update comments to match.

>+    # the path to adb, or just 'adb' to assume it's on the PATH
>+    self.adb = adb

I think "adbPath" would be a better name here (more descriptive).

>     # all output should be in stdout
>-    proc = subprocess.Popen(["adb", "shell", cmdline],
>+    args = [self.adb] 
>+    if deviceName:
>+      args.extend(['-s', self.deviceName])
>+    args.extend(["shell", cmdline])
>+    proc = subprocess.Popen(args,
>                             stdout=subprocess.PIPE, stderr=subprocess.PIPE)

See above. ^^^

>   def runCmdAs(self, args):
>     if self.useRunAs:
>@@ -686,11 +680,13 @@
>   def checkCmd(self, args):
>     # If we are not root but have run-as, and we're trying to execute 
>     # a shell command then using run-as is the best we can do
>+    finalArgs = [self.adb]
>+    if self.deviceName:
>+      finalArgs.extend(['-s', self.deviceName])
>     if (not self.haveRoot and self.useRunAs and args[0] == "shell" and args[1] != "run-as"):
>-      args.insert(1, "run-as")
>-      args.insert(2, self.packageName)
>-    args.insert(0, "adb")
>-    return subprocess.check_call(args)
>+      finalArgs.extend(['run-as', self.packageName])
>+    finalArgs.extend(args)
>+    return subprocess.check_call(finalArgs)

See above ^^^
Attachment #611639 - Flags: review?(wlachance) → review+
(In reply to Joel Maher (:jmaher) from comment #3)
> Comment on attachment 611639 [details] [diff] [review]
> devicemanager patch, v0.2
> 
> Review of attachment 611639 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I see 3 things here:
> 1) change the test root
> 2) change the package name
> 3) add in '-s' support for >1 device
> 
> I really don't see why we need to subclass for this.  I thought we had a
> patch already for -s support, although it doesn't seem to be checked in.  
> 
> As for the test root, we try to use /data/local/tests and fall back to
> /mnt/sdcard/tests.  We really should modify our tools to verify that the
> testroot is writeable during initialization.  That would solve your problem.
> 
> Regarding the packagename, Will has done a lot of great work on refactoring
> things.  In the current devicemanagerADB if you pass in the packagename to
> the init, we use it.  
> 
> Right now I am not seeing a need for another subclass.  Maybe there are
> other things I am not seeing?

For B2G, we don't want any packageName at all, since there aren't any android-style packages.  The current code makes it hard to provide a mechanism that lets packageName be None, since currently None causes us to generate a fennec default, and this is done in the constructor.  The only way that I can think to handle this without subclassing is to default the packageName argument to something like "fennec" instead of None, and then do something like:

    if packageName == 'fennec':
      if os.getenv('USER'):
        packageName = 'org.mozilla.fennec_' + os.getenv('USER')
      else:
        packageName = 'org.mozilla.fennec_'

I'm not a big fan of magic values like this; IMO subclassing is cleaner, and it doesn't cost us anything.  But it is a workable alternative to the subclass.  Let me know if you'd like me to submit a patch with this approach.

Re testRoot, yes we could adjust the existing code such that it checks write permissions on /mnt/sdcard.  I was also thinking about future-proofing the code; are we sure that one piece of logic for determining the device root will be appropriate for all future device classes?  Maybe it will, or if not, we could handle it at the time.
> 3) add in '-s' support for >1 device

This is moved to bug 741994

>>+    # the path to adb, or just 'adb' to assume it's on the PATH
>>+    self.adb = adb
>
>I think "adbPath" would be a better name here (more descriptive).

This is moved to bug 741989
Attached patch devicemanager patch v0.3 (obsolete) — Splinter Review
This version of the patch omits the adbPath and deviceSerial changes that have been moved to other bugs.  It also avoids subclassing by checking for failure of mkDir on /mnt/sdcard, and by defaulting the packageName to the magic value 'fennec', which causes the fennec-specific logic to applied.  This will allow us to pass None for the B2G case without it generating a fennec value.
Attachment #611639 - Attachment is obsolete: true
Attachment #612054 - Flags: review?(jmaher)
Comment on attachment 612054 [details] [diff] [review]
devicemanager patch v0.3

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

if we can fix the mkdir failed test I would be happy!

::: build/mobile/devicemanagerADB.py
@@ +20,4 @@
>      self.packageName = None
>      self.tempDir = None
>  
> +    if packageName == 'fennec':

I am not sure if we can assume the packageName is going to default to fennec.  For this patch, I would be fine leaving that in there as all our existing tinderbox style automation that relies on this leaves it as the default.

@@ +149,5 @@
>    #  failure: None
>    def mkDir(self, name):
>      try:
> +      result = self.runCmdAs(["shell", "mkdir", name]).stdout.read()
> +      if 'mkdir failed' in result:

if the file exists mkdir can fail :(
Attachment #612054 - Flags: review?(jmaher) → review+
Attached patch patch v0.4Splinter Review
> @@ +149,5 @@
>    #  failure: None
>    def mkDir(self, name):
>      try:
> +      result = self.runCmdAs(["shell", "mkdir", name]).stdout.read()
> +      if 'mkdir failed' in result:
>
> if the file exists mkdir can fail :(

Oops, good catch, this patch fixes that.

> @@ +20,4 @@
>      self.packageName = None
>      self.tempDir = None
>  
> +    if packageName == 'fennec':
>
> I am not sure if we can assume the packageName is going to default to 
> fennec.  For this patch, I would be fine leaving that in there as all our 
> existing tinderbox style automation that relies on this leaves it as the 
> default.

'fennec' is just a magic value; it could be anything (except None).  Maybe it would make more sense to use an empty string '' for this, to denote that we should automatically generate a packageName?
Attachment #612054 - Attachment is obsolete: true
Attachment #612266 - Flags: review?(jmaher)
(In reply to Jonathan Griffin (:jgriffin) from comment #9)

> > I am not sure if we can assume the packageName is going to default to 
> > fennec.  For this patch, I would be fine leaving that in there as all our 
> > existing tinderbox style automation that relies on this leaves it as the 
> > default.
> 
> 'fennec' is just a magic value; it could be anything (except None).  Maybe
> it would make more sense to use an empty string '' for this, to denote that
> we should automatically generate a packageName?

Do we actually have any tinderbox automation that uses ADB? I was under the impression that we didn't.

An alternative to all of this would be to have a useRunAs argument that we pass into the constructor (True by default). If this is set to False, don't generate a value for packageName OR try to use run as at all.
(In reply to William Lachance (:wlach) from comment #10)

> 
> An alternative to all of this would be to have a useRunAs argument that we
> pass into the constructor (True by default). If this is set to False, don't
> generate a value for packageName OR try to use run as at all.

Or a generatePackageName argument (True by default).  I think for future-proofing the code, we would want to avoid the assumption that the packageName will only ever be used for runAs.
Attachment #612266 - Flags: review?(jmaher) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/21086fa592c1
Assignee: nobody → jgriffin
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/21086fa592c1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.