800655 - devicemanagerADB's pushDir/pushFile/getTempDir somewhat broken

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[William Lachance (:wlach)]

There are currently some issues with dmADB's pushDir and pushFile and getTempDir methods, probably at least partially introduced by bug 795456.

This is definitely part of the problem in bug 799863 and possibly partly 800566.

Problem 1:

If we're using run-as and the directory in which the file should be contained in already exists, we incorrectly try to copy over it as opposed to putting it inside the directory. Unfortunately we don't see that it failed, it's all totally silent. :(

Problem 2:

Because of some bad indentation, we silently fail to run pushDir if the directory we're pushing to already exists.

Problem 3:

We attempt to create the temporary directory and return a value from it if it doesn't already exists, but mkDir now returns None. So we always fail in that case.

Comment 1

[William Lachance (:wlach)]

Attachment: Make dmADB's pushFile work more like dmSUT's

From patch:

    dmSUT's pushFile will throw an exception if you attempt to push a file
    to a directory (e.g. pushing the file "foo" to "/mnt/sdcard"). With dmADB,
    we put the file in the directory if not using run-as, and would silently fa
    if we *were* using run-as. This patch attempts to make dmADB do what dmSUT
    does, which is throw an exception if we attempt to directly use pushFile wi
    a directory as a destination.

Comment 2

[William Lachance (:wlach)]

Attachment: dmADB's pushDir fails if directory already exists

This one's pretty self-explanatory

Comment 3

[William Lachance (:wlach)]

Attachment: dmADB's getTempDir method returns None the first time

Likewise, pretty self-explanatory.

Comment 4

[William Lachance (:wlach)]

Attachment: Add option of setting package name when using dm utility

I don't personally like the run-as behaviour in dmADB, but if we're going to include it, we might as well be able to test it.

This option is silently ignored if you're using the dm utility with sut agent. I didn't think it was worth putting in any extra error checking.

Comment 5

[Mark Côté [:mcote]]

Comment on attachment 670621 [details] [diff] [review]
Make dmADB's pushFile work more like dmSUT's

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

Looks good.

Comment 6

[Mark Côté [:mcote]]

Comment on attachment 670622 [details] [diff] [review]
dmADB's pushDir fails if directory already exists

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

This is good, but I was thinking this might be a good opportunity to clean up some non-Pythonic stuff, if you feel like it, as noted below.

::: mozdevice/mozdevice/devicemanagerADB.py
@@ +197,2 @@
>          # limitation
>          if (not self.dirExists(remoteDir)):

Might as well get rid of those superfluous parentheses.

@@ +197,4 @@
>          # limitation
>          if (not self.dirExists(remoteDir)):
>              self.mkDirs(remoteDir+"/x")
> +        if (self.useZip):

Ditto.

@@ +198,5 @@
>          if (not self.dirExists(remoteDir)):
>              self.mkDirs(remoteDir+"/x")
> +        if (self.useZip):
> +            try:
> +                localZip = tempfile.mktemp()+".zip"

Add spaces around +.

@@ +205,5 @@
> +                self.pushFile(localZip, remoteZip)
> +                os.remove(localZip)
> +                data = self._runCmdAs(["shell", "unzip", "-o", remoteZip, "-d", remoteDir]).stdout.read()
> +                self._checkCmdAs(["shell", "rm", remoteZip])
> +                if (re.search("unzip: exiting", data) or re.search("Operation not permitted", data)):

Superfluous parentheses.

@@ +217,5 @@
> +                relRoot = os.path.relpath(root, localDir)
> +                for f in files:
> +                    localFile = os.path.join(root, f)
> +                    remoteFile = remoteDir + "/"
> +                    if (relRoot!="."):

Ditto.

@@ +223,5 @@
> +                    remoteFile = remoteFile + f
> +                    self.pushFile(localFile, remoteFile)
> +                for d in dirs:
> +                    targetDir = remoteDir + "/"
> +                    if (relRoot!="."):

Ditto.

Comment 7

[Mark Côté [:mcote]]

Comment on attachment 670623 [details] [diff] [review]
dmADB's getTempDir method returns None the first time

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

Great, just a couple small comments again.

::: mozdevice/mozdevice/devicemanagerADB.py
@@ +553,4 @@
>          """
>          # Cache result to speed up operations depending
>          # on the temporary directory.
> +        if self.tempDir is None:

Would this be better as "if not self.tempDir", just so that we don't rely on "None" specifically? '' should have the same meaning, I'm thinking. No magic values. :) But not too important.

@@ +557,1 @@
>              self.tempDir = self.getDeviceRoot() + "/tmp"

Forgot to mention this in the previous reviews, but I'm a bigger fan of the posixpath module for constructing paths, but again not too important.

Comment 8

[Mark Côté [:mcote]]

Comment on attachment 670625 [details] [diff] [review]
Add option of setting package name when using dm utility

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

I have one concern here so I'm r-ing... if I'm confused lemme know.

::: mozdevice/mozdevice/dmcli.py
@@ +147,5 @@
>          parser.add_option("-d", "--hwid", action="store",
>                            type="string", dest="hwid",
>                            help="HWID", default=None)
> +        parser.add_option("--package-name", action="store",
> +                          type = "string", dest="packagename",

Remove spaces around =. Feel free to clean up the other options as well. :)

@@ +149,5 @@
>                            help="HWID", default=None)
> +        parser.add_option("--package-name", action="store",
> +                          type = "string", dest="packagename",
> +                          help="Packagename (if using DeviceManagerADB)",
> +                          default=None)

Does this not override the DeviceManagerADB default packageName value of 'fennec'?

Comment 9

[Mark Côté [:mcote]]

Comment on attachment 670625 [details] [diff] [review]
Add option of setting package name when using dm utility

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

r+ing after explanation in IRC. Defaulting to None should be fine for the purposes of dmcli.

Comment 10

[William Lachance (:wlach)]

Pushed with the suggested changes, except as noted below:

https://github.com/mozilla/mozbase/compare/8cb2fb038c...1de5f8de4e

I accidentally forgot to address your issues about the spacing in the dmcli patch before pushing. Fixed that as a followup:

https://github.com/mozilla/mozbase/commit/8cd3153b4f51505c7f18eba99ecb392c4bfa6384