Closed Bug 669549 Opened 8 years ago Closed 8 years ago

Some DeviceManagerADB functions do not work

Categories

(Testing :: General, defect)

All
Android
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gbrown, Assigned: gbrown)

References

Details

Attachments

(3 files, 10 obsolete files)

8.09 KB, patch
Details | Diff | Splinter Review
2.31 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
11.60 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
While developing a remote version of runxpcshelltests.py (bug 668349) I tried using various DeviceManagerADB functions to drive tests on device (Samsung Galaxy S, Android 2.2.1) over USB. I noted these failures:

mkDirs(path) failed for all paths. Note that mkDirs invokes adb shell command "mkdir -p <path>", but the -p option is not available on Android (at least for me); attempting to use mkdir -p <path> on device creates a "-p" directory.

dirExists(path) returns true regardless of the existence of <path>. Note that dirExists invokes adb shell command "ls <path>" and only returns false if this generates an exception. But "ls nonexistentpath" simply generates output:

nonexistentpath: No such file or directory

and there is no cause for an exception.
Blocks: 668349
I notice isDir(path) now -- it seems to work fine. The SUT implementation has different implementations of isDir and dirExists; it is not clear to me if there should be a (subtle?) difference between these functions. Could adb.dirExists just call adb.isDir?
Attached patch patch for discussion (obsolete) — Splinter Review
I stole the SUT version of mkDirs, recursively calling mkDir; it works for me. Should this be moved into the base class (devicemanager)?

I implemented dirExists as a call to isDir. I think they should be equivalent:
                            dirExists    isDir
path specifies a file         false*     false
path specifies a dir          true       true
path does not exist           false      false

but an alternate interpretation is:
                            dirExists    isDir
path specifies a file         true*      false
path specifies a dir          true       true
path does not exist           false      false
Attached patch updated patch for discussion (obsolete) — Splinter Review
I found another problem. pushDir was implemented simply as "adb push <localdir> <remotedir>" and that mostly works...except for symbolic links. If <localdir> contains a symbolic link, the link is pushed, rather than the linked-to file. To get around this, I walk <localdir> and call pushFile for each file encountered.
Attachment #544377 - Attachment is obsolete: true
Attached patch updated patch for discussion (obsolete) — Splinter Review
Another minor change: pushFile(local, remote) calls chmodDir(remote); when remote is a directory, this resulted in adb chmod on every file in the directory. Now, only the newly-pushed file is chmod'ed.
Attachment #544583 - Attachment is obsolete: true
Attached patch patch for review (obsolete) — Splinter Review
Assignee: nobody → gbrown
Attachment #544869 - Attachment is obsolete: true
Attachment #544873 - Flags: review?(jmaher)
Comment on attachment 544873 [details] [diff] [review]
patch for review

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

looks good, lets get this in.  I would only like to consider the self.mkDir vs self.mkDirs comment before checking in.

::: build/mobile/devicemanagerADB.py
@@ +67,1 @@
>  

while it would be nice to refactor this into the base class, I would like to wait until this file is stable for a while.  There is some uncertainty with requirements as to rooting a device and run-as support.

@@ +89,5 @@
> +          if (relRoot!="."):
> +            targetDir = targetDir + relRoot + "/"
> +          targetDir = targetDir + dir
> +          if (not self.dirExists(targetDir)):
> +            self.mkDir(targetDir)

does this make all the parent directories as well?  I would think a self.mkDirs() to be safe

@@ +458,5 @@
>          else:
>            self.checkCmd(["shell", "chmod", "777", remoteDir.strip()])
>            print "chmod " + remoteDir.strip()
>        self.checkCmd(["shell", "chmod", "777", remoteDir])
> +      #print "chmod " + remoteDir

why no .strip() here?
Attachment #544873 - Flags: review?(jmaher) → review+
(In reply to comment #6)
> while it would be nice to refactor this into the base class, I would like to
> wait until this file is stable for a while.  There is some uncertainty with
> requirements as to rooting a device and run-as support.

That sounds sensible - let's revisit later.
 
> @@ +89,5 @@
> > +          if (relRoot!="."):
> > +            targetDir = targetDir + relRoot + "/"
> > +          targetDir = targetDir + dir
> > +          if (not self.dirExists(targetDir)):
> > +            self.mkDir(targetDir)
> 
> does this make all the parent directories as well?  I would think a
> self.mkDirs() to be safe

It works as-is, because this code is nested in the for ... os.walk. I tested by pushing a directory containing directories containing directories (and files) - everything was pushed.
 
> @@ +458,5 @@
> >          else:
> >            self.checkCmd(["shell", "chmod", "777", remoteDir.strip()])
> >            print "chmod " + remoteDir.strip()
> >        self.checkCmd(["shell", "chmod", "777", remoteDir])
> > +      #print "chmod " + remoteDir
> 
> why no .strip() here?

I am not sure, but have not had trouble with it yet, so left it alone!
Keywords: checkin-needed
Comment on attachment 544873 [details] [diff] [review]
patch for review

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

something about this patch is leaving files in the profile with rw-rw-rw- permissions, which breaks running reftests because the profile can't be locked.
Attachment #544873 - Flags: review+ → review-
isDir() had a bug too! isDir(file) would return true if file existed and was a file, because trailing whitespace (\r\n) was invalidating a string comparison. My change to avoid over-chmod'ing relied on isDir to detect whether a destination file or directory was specified during a push and concatenated the filename when only the destination directory was specified. Interaction of these features created a bad filename for many chmod requests (eg ".../user.js/user.js") I fixed isDir and re-tested pushDir.
Attachment #544873 - Attachment is obsolete: true
Attachment #544910 - Flags: review?(blassey.bugs)
Attached patch patch for review (obsolete) — Splinter Review
This version fixes a problem with permissions on the top-level directory of a pushDir.
Attachment #544910 - Attachment is obsolete: true
Attachment #544935 - Flags: review?(blassey.bugs)
Attachment #544910 - Flags: review?(blassey.bugs)
if we can change /data/local inside of getDeviceRoot to /mnt/sdcard, I saw mochitest and reftest work just fine on my tegra using the ADB agent.
devicemanagerSUT's getDeviceRoot uses the SD card as a default, if it exists; otherwise, android.content.Context.getFilesDir(). I changed my ADB getDeviceRoot to use /mnt/sdcard if it exists, else /data/local. For me, this caused a mkDirs failure on /mnt; I updated mkDirs to skip directory components that already exist -- problem solved.

Still, on my device, I see minor permission problems using /mnt/sdcard: I cannot chmod or chown /mnt/sdcard/tests or any sub-directory, except as root. My /mnt/sdcard has u=system g=sdcard_rw perms 775. Running as u=shell, g=shell (sdcard_rw), I can create/read/write in /mnt/sdcard, but chmod returns "Operation not permitted".

I have not had any problems running in /data/local/tests, as shell/shell.
I see the operation no permitted messages as well.  Also, we need to change the listFiles command to be ls -a as we can have .files.
with this patch I can run talos, reftest, mochitest unittests on a nexus S unrooted virgin phone.  I just wanted to get what I had working posted.  This patch has a lot of permissions type errors for the fileIO (/mnt/sdcard stuff), but things do run just fine.
Attached patch updated patch (obsolete) — Splinter Review
This patch includes most of the changes in jmaher's patch (comment 14) and also attempts to chown <fennec-uid> each directory created. I find that chown always fails with "Operation not permitted", unless run with root permissions...so this may not be a useful feature.

Note that this version of getDeviceRoot:
 - returns /data/local/tests IF that directory exists, else
    - returns /mnt/sdcard/tests IF /mnt/sdcard exists, else
       - returns /data/local/tests
The intention is that /mnt/sdcard/tests is the default, unless there is no sdcard, or unless over-ridden by the creation of /data/local/tests (xpcshell tests will likely use this, since it looks like they cannot run on /sdcard).
Attachment #544935 - Attachment is obsolete: true
Attachment #544935 - Flags: review?(blassey.bugs)
Interesting chown/chgrp behavior...

$ cd /tmp
$ echo hello > zz
$ ls -l zz
-rw-rw-rw- shell    shell           6 2011-07-11 18:57 zz
$ run-as org.mozilla.fennec_unofficial id
uid=10083(app_83) gid=10083(app_83) groups=1003(graphics),1004(input),1007(log),1009(mount),1011(adb),1015(sdcard_rw),3001(net_bt_admin),3002(net_bt),3003(inet)
$ chgrp 10083 zz
chgrp: zz: Operation not permitted
$ chgrp app_83 zz
chgrp: zz: Operation not permitted
$ chgrp 1003 zz
$ ls -l zz
-rw-rw-rw- shell    graphics        6 2011-07-11 18:57 zz
$ chgrp log zz
$ ls -l zz
-rw-rw-rw- shell    log             6 2011-07-11 18:57 zz

...but this does not help us.

Maybe devicemanagerADB should "run-as org.mozilla.fennec_unofficial mkdir ..."? But how does devicemanagerADB come up with the package name?
in devicemanagerSUT, we look for an appdir in /data/data/org.mozilla.*;  This can be deterministic of the package name.  The problem is we would need a rooted device.  

Another solution is to require the package name at some point during initialization or before we do fileIO.  This might break existing scripts, but all the automation that uses this to date (talos, reftest, mochitest, some [maybe all] of the releng tools) pass in a package name (i.e. org.mozilla.fennec).  We would need to update the scripts that instantiate devicemanagerADB() and have them setup the package name.
(In reply to comment #16)
> Maybe devicemanagerADB should "run-as org.mozilla.fennec_unofficial mkdir
> ..."? 
That makes sense 

> But how does devicemanagerADB come up with the package name?
Seems reasonable to pass that into the initialization. devicemanagerADB should probably default to org.mozilla.fennec_unofficial, but the run_*_remote.py scripts should specify it.
Thanks for the tips. This version of the patch no longer attempts chown; instead, mkDir executes "adb shell run-as <package-name> mkdir <dir>". The default package-name is org.mozilla.fennec_unofficial, and can be modified in the devicemanagerADB ctor.

Note that this change does not affect pushFile, so files are still created with shell ownership.
Attachment #545289 - Attachment is obsolete: true
This update pushes files to a temporary location, then uses "run-as <package> cp ..." to create files with the correct ownership. It works great for me and I have no other updates in mind, so would appreciate another review.
Attachment #545429 - Attachment is obsolete: true
Attachment #545521 - Flags: review?(jmaher)
additional changes needed to existing tools so we can run out of the box.
Comment on attachment 545521 [details] [diff] [review]
updated patch - use run-as for file and directory creation

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

this works great.  I added another patch to this bug which fixes the runtestsremote.py and remotereftests.py to work as well.  If you can land both patches together that will help!

::: build/mobile/devicemanagerADB.py
@@ +489,2 @@
>    def chmodDir(self, remoteDir):
> +    #print "called chmodDir"

can we remove this comment?
Attachment #545521 - Flags: review?(jmaher) → review+
Comment on attachment 545662 [details] [diff] [review]
updates to mochitest and reftest harness to work with packageName (1.0)

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

::: layout/tools/reftest/remotereftest.py
@@ +390,5 @@
>          sys.exit(1)
>  
>      if (options.dm_trans == "adb"):
>          if (options.deviceIP):
> +            dm = devicemanagerADB.DeviceManagerADB(options.deviceIP, options.devicePort, options.app)

The 3rd parameter in the DeviceManagerADB initializer is actually retrylimit, not packageName. Your tests probably work because retrylimit is unused, and packageName has a useful default, but you won't be able to set the package name this way.

::: testing/mochitest/runtestsremote.py
@@ +312,5 @@
>      parser = RemoteOptions(auto, scriptdir)
>      options, args = parser.parse_args()
>      if (options.dm_trans == "adb"):
>          if (options.deviceIP):
> +            dm = devicemanagerADB.DeviceManagerADB(options.deviceIP, options.devicePort, options.app)

Ditto.
Attachment #545662 - Flags: review?(gbrown) → review-
odd why it worked for me since I was using org.mozilla.fennec and had never installed a org.mozilla.fennec_unofficial on the device I have been using for test.  This is updated and should be more accurate.
Attachment #545662 - Attachment is obsolete: true
Attachment #545702 - Flags: review?(gbrown)
Comment on attachment 545702 [details] [diff] [review]
updates to mochitest and reftest harness to work with packageName (1.1)

Review of attachment 545702 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #545702 - Flags: review?(gbrown) → review+
These were landed on m-i, and backed out because they broke android tests.
Geoff, here's the results from m-i so you can diagnose the problem http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=abe8569eca76
======== BuildStep started ========
'python mochitest/runtestsremote.py ...' warnings
=== Output ===
python mochitest/runtestsremote.py --deviceIP 10.250.49.44 --xre-path ../hostutils/xre --utility-path ../hostutils/bin --certificate-path certs --app org.mozilla.fennec --console-level INFO --http-port 30057 --ssl-port 31057 --pidfile /builds/tegra-057/test/../runtestsremote.pid --browser-chrome --test-path mobile
 in dir /builds/tegra-057/test/build/tests (timeout 2400 secs)
 watching logfiles {}
 argv: ['python', 'mochitest/runtestsremote.py', '--deviceIP', '10.250.49.44', '--xre-path', '../hostutils/xre', '--utility-path', '../hostutils/bin', '--certificate-path', 'certs', '--app', 'org.mozilla.fennec', '--console-level', 'INFO', '--http-port', '30057', '--ssl-port', '31057', '--pidfile', '/builds/tegra-057/test/../runtestsremote.pid', '--browser-chrome', '--test-path', 'mobile']
 environment:
  PATH=/opt/local/bin:/opt/local/sbin:/opt/local/Library/Frameworks/Python.framework/Versions/2.6/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/usr/X11/bin
  PWD=/builds/tegra-057/test/build/tests
  SUT_IP=10.250.49.44
  SUT_NAME=tegra-057
  __CF_USER_TEXT_ENCODING=0x1F6:0:0
 closing stdin
 using PTY: False
Traceback (most recent call last):
  File "mochitest/runtestsremote.py", line 352, in <module>
    main()
  File "mochitest/runtestsremote.py", line 310, in main
    dm_none = devicemanagerADB.DeviceManagerADB()
  File "/builds/tegra-057/test/build/tests/mochitest/devicemanagerADB.py", line 12, in __init__
    self.verifyPackage(packageName)
  File "/builds/tegra-057/test/build/tests/mochitest/devicemanagerADB.py", line 511, in verifyPackage
    data = self.runCmd(["shell", "run-as", packageName, "pwd"]).stdout.read()
  File "/builds/tegra-057/test/build/tests/mochitest/devicemanagerADB.py", line 477, in runCmd
    return subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/subprocess.py", line 623, in __init__
    errread, errwrite)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/subprocess.py", line 1141, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory
program finished with exit code 1
elapsedTime=0.625149
TinderboxPrint: mochitest-browser-chrome<br/>
Unknown Error: command finished with exit code: 1
=== Output ended ===
======== BuildStep ended ========
so when we instantiate devicemanagerADB(), it tries to do a verifyPackage().  This needs to be optional.  My bad for not catching this earlier.  The automation system doesn't have ADB in the path, nor anything connected to it, when I tested the patch with SUT, it verified the package on the ADB device I had connected.

We could even put this in a try/except block.
Updated devicemanagerADB to allow for initialization even when adb is missing:  guarded with try / except.
Attachment #545521 - Attachment is obsolete: true
Attachment #545817 - Flags: review?(jmaher)
Comment on attachment 545817 [details] [diff] [review]
updated devicemanagerADB patch addressing comments 26-29

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

looks good.  This works on a SUT agent with no adb available or device connected.  I had a lot of trouble getting this to work locally via ADB and found that removing the packageName so we do not do a 'runas org.mozilla.fennec' for the mkdir/pushFile commands my tests would run.

So lets not check in the other patch to modify the harnesses until we can figure out why runas isn't working.
Attachment #545817 - Flags: review?(jmaher) → review+
I would like to understand the run-as failure before checking in.

I tested this morning and did not experience problems. 

However, I noted some possible sources of trouble:
 - When we run-as <a.b.c>, the cwd is set to /data/data/<a.b.c>; a script that uses relative paths or otherwise assumes a cwd will experience trouble.
 - When we run-as <a.b.c>, we may have different file access permissions as compared to shell. In particular, on my device, shell can create directories under /data/local, but org.mozilla.fennec_unofficial cannot. I do not see any comparable problems on /mnt/sdcard.
It seems that jmaher's problem with run-as is a known limitation for official builds. I think we can go ahead with check-in.
Keywords: checkin-needed
This (along with most things committed on Friday afternoon) was backed out of mozilla-inbound in order to clear up orange.
Please clarify what needs to land here.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/1627d7efe035
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.