Closed Bug 573281 Opened 10 years ago Closed 10 years ago

Update DeviceManager.py for android

Categories

(Testing :: General, defect)

ARM
Android
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cmtalbert, Assigned: cmtalbert)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch DeviceManager Update v1 (obsolete) — Splinter Review
This brings devicemanager in the tree up to what I'm currently using.  There are a couple of important changes:

* Fix up the info command so that it is useful devicemanager::getInfo
* Allow devicemanager to install an apk application to the device devicemanager::installApp()
** Install takes a path to the installation bundle on the device.  Each agent will recognize its OS's bundle and will know how to unpack and install on the device.
** One optional enhancement we might want: ability to set destination for unpacked bundle. This is not necessary (or possible) on Android, but other platforms this could be useful.  Or we could simply have the agent return to devicemanager the location of the unpacked bundle and the agent could handle that for us. (I like the second option better).
* Allow devicemanager to uninstall an application from the device - devicemanager::uninstallApp()
** Takes a name of an application to uninstall.  This is what's needed for Android.  I'm not sure if only specifying a name is good enough for other platforms, or if that's something we can reasonably assume an agent can keep track of for us.

Here's the big change.
Pushing a large file on Android was fraught with difficulties.  After doing a lot of work with the agent and devicemanager we settled on a new paradigm for pushFile:
* DM pushes the file to the agent, keeping track of number of bytes sent
* DM compares number of bytes accepted by the agent to number of bytes total in command and throws an error if they are different
* Agent returns the md-5 hash of the file pushed as a response to the push command
* DM compares that md-5 hash with a local hash of the file it just pushed.
* This means the push blocks until all bytes are read by the agent.

Doing this enabled us to run without any sleeps in DM::sendCMD, which is cool.  I've been running all my tests this way for a while now and it's working.  The sleeps are still commented out.  I think for future proofing, we want to leave them in, but set all the sendCMD calls to use sleep=0 (the default).  This way if we need them for debugging later, we don't have to put them back.

I'll attach a patch for your perusal.
Attachment #452503 - Flags: review?(jmaher)
Depends on: 573282
Comment on attachment 452503 [details] [diff] [review]
DeviceManager Update v1

>diff --git a/build/mobile/devicemanager.py b/build/mobile/devicemanager.py
>+      if (ctr == 1):
>+        noQuit = False

Can you explain what ctr is and why if it is 1 we quit?

    
>     # sleep 5 seconds / MB
>     filesize = os.path.getsize(localname)
>     sleepsize = 1024 * 1024
>-    sleepTime = (int(filesize / sleepsize) * 5) + 2
>+    sleepTime = (int(filesize / sleepsize) * 10) + 2

lets adjust the comment at the top of the block to indicate the new value of 10 seconds/MB


>-    retVal = self.sendCMD(['push ' + destname + '\r\n', data], newline = False, sleep = sleepTime)
>+    retVal = self.sendCMD(['push ' + destname + ' ' + str(filesize) + '\r\n', data], newline = False, sleep = sleepTime)
>+    retVal = retVal.split("\n")[0]
>     if (retVal == None):
>       if (self.debug >= 2): print "Error in sendCMD, not validating push"
>       return None

If sendCMD returns None (it has a few instances where this is possible), the retVal.split('\n') will throw an exception.  



>+    if (str(localHash) == str(retVal)):
>+      retVal = True
>+    else:
>+      retVal = False
> 

we are returning False now?  All code to date has been written assuming None is a failure.  That might not be ideal, but we have code in places like Talos and releng setup scripts that use these api's.



>-  def getInfo(self, directive):
>+  # all - all of them - or call it with no parameters to get all the information
>+  def getInfo(self, directive=None):
>     data = None
>+    result = {}
>+    collapseSpaces = re.compile('  +')
>+
>     if (directive in ('os','id','uptime','systime','screen','memory','process',
>                       'disk','power')):
>       data = self.sendCMD(['info ' + directive, 'quit'], sleep = 1)
>+      if (data is None):
>+        return result
>+      data = self.stripPrompt(data)
>+      data = collapseSpaces.sub(' ', data)
>+      result[directive] = data.split('\n')
>     else:
>-      directive = None
>-      data = self.sendCMD(['info', 'quit'], sleep = 1)
>+      directive = ['os', 'id', 'uptime', 'systime', 'screen', 'memory', 'process',
>+                   'disk', 'power']
>+      for d in directive:
>+        data = self.sendCMD(['info ' + d, 'quit'], sleep = 1)
>+        if (data is None):
>+          continue
>+        data = self.stripPrompt(data)
>+        data = collapseSpaces.sub(' ', data)
>+        result[d] = data.split('\n')


Awesome that we are doing this.  I think we can clean this code up a bit to make it simpler:
    directives = ['os', 'id', 'uptime', 'systime', 'screen', 'memory', 'process',
                  'disk', 'power']

    if (directive in directives):
      directives = [directive]

    for d in directives:
      data = self.sendCMD(['info ' + d, 'quit'], sleep = 1)
      if (data is None):
        continue
      data = self.stripPrompt(data)
      data = collapseSpaces.sub(' ', data)
      result[d] = data.split('\n')


>+
>+  """
>+  Installs the application onto the device
>+  Application bundle - path to the application bundle on the device
>+  Returns True or False depending on what we get back
>+  TODO: we need a real way to know if this works or not
>+  """
>+  def installApp(self, appBundlePath):
>+    data = self.sendCMD(['inst ' + appBundlePath, 'quit'], sleep = 1)
>+    if (data is None):
>+      return False
>+    else:
>+      return True
>+
>+  """
>+  Uninstalls the named application from device and causes a reboot.
>+  Returns True, but it doesn't mean anything other than the command was sent,
>+  the reboot happens and we don't know if this succeeds or not.
>+  """
>+  def uninstallApp(self, appName):
>+    self.sendCMD(['uninst ' + appName, 'quit'])
>+    return True
>+

I notice you are adding in a double space between functions throughout the file, but only a single space between the new functions here at the end?



Another note if we are cleaning stuff is, I would like to remove all the 'quit' commands that we use.  That was originally designed for when we were not caching the telnet session, so they are sort of useless now.  


r- for the error around return values, the rest are more nits.
Attachment #452503 - Flags: review?(jmaher) → review-
This addresses all review comments and then goes a little bit further as we discussed on IRC today.
* It removes all the 'quit' statements
* It re-works the logic for the sendCMD loop to make the noQuit = False noQuit = True be a little easier to follow since we were using that for two very different things.

There is now a "should this command close the socket" list of commands that must close the socket once they complete.  This was the original intention of the noQuit directive.

There is now a "does this command expect a response" list that controls whether or not the command should ping for a response from the agent.  This was the secondary behavior of the "noQuit" directive.

There is also the notion that the agent can throw an error back to us and that error is defined as a line beginning with '##AGENT-ERROR##' we use that as well as a loop guard to make sure we don't fall into any infinite loops.  The agent-error string needs to be added to the agent, as it isn't currently implemented yet.  

I've tested this using all the tests on the remote-testing/tests repo and everything is working. I've also ran a bunch of manual tests against it.  

Let me know what you think.
Assignee: nobody → ctalbert
Attachment #452503 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #457743 - Flags: review?(jmaher)
Comment on attachment 457743 [details] [diff] [review]
Device Manager Update Round 2

>diff --git a/build/mobile/devicemanager.py b/build/mobile/devicemanager.py
>+import pdb

can we remove this from production code?

>+    #pdb.set_trace()

same here

>   def sendCMD(self, cmdline, newline = True, sleep = 0):
>     for cmd in cmdline:
>       if (cmd == 'quit'): break

do we need the break on quit anymore?


>-        self._sock.send(cmd)
>+        numbytes = self._sock.send(cmd)
>+        if (numbytes != len(cmd)):
>+          print "ERROR: our cmd was " + str(len(cmd)) + " bytes and we only sent " + str(numbytes)
>+          return None

nice!

>@@ -180,28 +242,42 @@
> 
>     if (self.debug >= 2): print "sending: push " + destname
>     
>-    # sleep 5 seconds / MB
>+    # sleep 10 seconds / MB
>     filesize = os.path.getsize(localname)
>     sleepsize = 1024 * 1024
>-    sleepTime = (int(filesize / sleepsize) * 5) + 2
>+    sleepTime = (int(filesize / sleepsize) * 10) + 2
>     f = open(localname, 'rb')
>     data = f.read()
>     f.close()
>-    retVal = self.sendCMD(['push ' + destname + '\r\n', data], newline = False, sleep = sleepTime)
>-    if (retVal == None):
>-      if (self.debug >= 2): print "Error in sendCMD, not validating push"
>+    retVal = self.sendCMD(['push ' + destname + ' ' + str(filesize) + '\r\n', data], newline = False, sleep = sleepTime)

a lot of reference to sleep here, which we don't use anymore.  Can we maybe clean this up a bit more?


>+      retline = retVal.split("\n")[0]

why not use stripPrompt() here?


> 
>+  """
>+  Installs the application onto the device
>+  Application bundle - path to the application bundle on the device
>+  Returns True or False depending on what we get back
>+  TODO: we need a real way to know if this works or not
>+  """
>+  def installApp(self, appBundlePath):
>+    data = self.sendCMD(['inst ' + appBundlePath], sleep = 1)
>+    if (data is None):
>+      return False
>+    else:
>+      return True
>+

I like this, but how does this work for maemo (under the hood)?  A nice usability error here is that installApp can easily be called assuming a local file to the host computer.
Also why do we have a sleep = 1 here?  we don't use sleep and a 1 second sleep seems sort of useless, especially for something like install.

>+  """
>+  Uninstalls the named application from device and causes a reboot.
>+  Returns True, but it doesn't mean anything other than the command was sent,
>+  the reboot happens and we don't know if this succeeds or not.
>+  """
>+  def uninstallApp(self, appName):
>+    self.sendCMD(['uninst ' + appName])
>+    return True
>+

how does this work on maemo?  wouldn't this need to take a path?  This should be uninstallAppAndReboot().



r=me with the above nits addressed.
Attachment #457743 - Flags: review?(jmaher) → review+
(In reply to comment #3)
> Comment on attachment 457743 [details] [diff] [review]
> Device Manager Update Round 2
> 
> >diff --git a/build/mobile/devicemanager.py b/build/mobile/devicemanager.py
> >+import pdb
> 
> can we remove this from production code?
> 
> >+    #pdb.set_trace()
> 
> same here
Yeah, thought I'd removed those.

> 
> >   def sendCMD(self, cmdline, newline = True, sleep = 0):
> >     for cmd in cmdline:
> >       if (cmd == 'quit'): break
> 
> do we need the break on quit anymore?
No, probably not.
> 
> 
> >-        self._sock.send(cmd)
> >+        numbytes = self._sock.send(cmd)
> >+        if (numbytes != len(cmd)):
> >+          print "ERROR: our cmd was " + str(len(cmd)) + " bytes and we only sent " + str(numbytes)
> >+          return None
> 
> nice!
> 
:)
> >@@ -180,28 +242,42 @@
> > 
> >     if (self.debug >= 2): print "sending: push " + destname
> >     
> >-    # sleep 5 seconds / MB
> >+    # sleep 10 seconds / MB
> >     filesize = os.path.getsize(localname)
> >     sleepsize = 1024 * 1024
> >-    sleepTime = (int(filesize / sleepsize) * 5) + 2
> >+    sleepTime = (int(filesize / sleepsize) * 10) + 2
> >     f = open(localname, 'rb')
> >     data = f.read()
> >     f.close()
> >-    retVal = self.sendCMD(['push ' + destname + '\r\n', data], newline = False, sleep = sleepTime)
> >-    if (retVal == None):
> >-      if (self.debug >= 2): print "Error in sendCMD, not validating push"
> >+    retVal = self.sendCMD(['push ' + destname + ' ' + str(filesize) + '\r\n', data], newline = False, sleep = sleepTime)
> 
> a lot of reference to sleep here, which we don't use anymore.  Can we maybe
> clean this up a bit more?
> 
My thought was that if we ended up needing it for a future device, we'd already have it in place, but I guess I can remove it now and hope that we won't need it in the future.
> 
> >+      retline = retVal.split("\n")[0]
> 
> why not use stripPrompt() here?
> 
yeah I dunno.  I'll do that.
> 
> > 
> >+  """
> >+  Installs the application onto the device
> >+  Application bundle - path to the application bundle on the device
> >+  Returns True or False depending on what we get back
> >+  TODO: we need a real way to know if this works or not
> >+  """
> >+  def installApp(self, appBundlePath):
> >+    data = self.sendCMD(['inst ' + appBundlePath], sleep = 1)
> >+    if (data is None):
> >+      return False
> >+    else:
> >+      return True
> >+
> 
> I like this, but how does this work for maemo (under the hood)?  A nice
> usability error here is that installApp can easily be called assuming a local
> file to the host computer.
> Also why do we have a sleep = 1 here?  we don't use sleep and a 1 second sleep
> seems sort of useless, especially for something like install.
> 
We have a lot of those sleep = 1's all over the code that are now implicitly ignored.  I can remove them all if you want.  I was kind of uncertain what to do about that.

On maemo, this should probably take another argument, a destination path.  For maemo and windows this would have used a zip file (or a self-extracting executable).  You would call:
installApp(self, appBundlePath, destPath)
where destPath could be None
and the agent would accept this command:
inst appbundle dest
where dest could be ""

> >+  """
> >+  Uninstalls the named application from device and causes a reboot.
> >+  Returns True, but it doesn't mean anything other than the command was sent,
> >+  the reboot happens and we don't know if this succeeds or not.
> >+  """
> >+  def uninstallApp(self, appName):
> >+    self.sendCMD(['uninst ' + appName])
> >+    return True
> >+
> 
> how does this work on maemo?  wouldn't this need to take a path?  This should
> be uninstallAppAndReboot().
> 
yeah, this should also take a dest path that could also potentially be None on devicemanaer and "" on agent.

I'll change the name too.
> 
> 
> r=me with the above nits addressed.
Blocks: 579566
http://hg.mozilla.org/mozilla-central/rev/ee164ea7d150 --> FIXED
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Component: New Frameworks → General
You need to log in before you can comment on or make changes to this bug.