1016041 - Eideticker should work on unrooted devices

Status: RESOLVED FIXED

(Testing Graveyard :: Eideticker, defect)

Description

[William Lachance (:wlach)]

Right now Eideticker depends on using a rooted devices for really no good reason. It would be nice to fix this as keeping devices up to date with rooted software is a bit of a hassle. Also, I really don't want to root my Nexus 5.

Comment 1

[William Lachance (:wlach)]

One aspect of this is using am force-stop to stop applications instead of killing them (because the adb user doesn't have permission to do that). We need a new version of mozdevice incorporating bug 907427 for that.

Comment 2

[William Lachance (:wlach)]

Attachment: Search for orangutan, ntpclient in /data/local/tmp as well

So one aspect of this is allowing the executables that eideticker uses (orangutan, ntpclient) to be in /data/local/tmp/, as that's the only place that some devices (e.g. the Nexus 5) can write to when unrooted.

Comment 3

[William Lachance (:wlach)]

Actually it looks like the stopApplication logic in mozdevice isn't quite adequate as-is, since it doesn't support old versions of Android. Filed bug 1016073 to try to get that addressed.

Comment 4

[Bob Clary [:bc] (inactive)]

Comment on attachment 8428875 [details] [diff] [review]
Search for orangutan, ntpclient in  /data/local/tmp as well

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

r+ with the KeyError fixed. The other comments I leave to your discretion.

::: src/eideticker/eideticker/device.py
@@ +118,5 @@
>                  self.model, self.type))
>          self.deviceProperties = DEVICE_PROPERTIES[self.type][self.model]
>  
> +        for (name, execname) in [("orangutan", "orng"),
> +                                 ("ntpdate", "ntpclient")]:

Since we use 'orangutan' and 'ntpdate' as keys in several places, do you think it would be worthwhile to define them as constants and then when using them as keys use the constant variable name instead of the literal string? Mark has suggested this to me in the past.

@@ +125,5 @@
> +                if self.fileExists(fullpath):
> +                    self.exec_locations[name] = fullpath
> +
> +            if not self.exec_locations[name]:
> +                raise mozdevice.DMError("%s not on device! Please "

the self.exec_locations[name] will raise a KeyError if it is not set and you will never raise the corresponding DMError.

@@ +132,1 @@
>  

Do you think it would be worth while to check both files before raising the DMError and report on them both if neither is installed?

Comment 5

[William Lachance (:wlach)]

(In reply to Bob Clary [:bc:] from comment #4)
> ::: src/eideticker/eideticker/device.py
> @@ +118,5 @@
> >                  self.model, self.type))
> >          self.deviceProperties = DEVICE_PROPERTIES[self.type][self.model]
> >  
> > +        for (name, execname) in [("orangutan", "orng"),
> > +                                 ("ntpdate", "ntpclient")]:
> 
> Since we use 'orangutan' and 'ntpdate' as keys in several places, do you
> think it would be worthwhile to define them as constants and then when using
> them as keys use the constant variable name instead of the literal string?
> Mark has suggested this to me in the past.

Sure, good idea.

> @@ +125,5 @@
> > +                if self.fileExists(fullpath):
> > +                    self.exec_locations[name] = fullpath
> > +
> > +            if not self.exec_locations[name]:
> > +                raise mozdevice.DMError("%s not on device! Please "
> 
> the self.exec_locations[name] will raise a KeyError if it is not set and you
> will never raise the corresponding DMError.

Oops. Will fix.
 
> Do you think it would be worth while to check both files before raising the
> DMError and report on them both if neither is installed?

I don't think it's really necessary. We already tell people to build + install both in the documentation.

Comment 6

[William Lachance (:wlach)]

Pushed with suggested fixes and a minor doc update:

https://github.com/mozilla/eideticker/commit/2d1e5796ffd878a7c88ae099d17a8527dcfeee7b

Comment 7

[William Lachance (:wlach)]

Oops, this is actually not quite fixed, since we still have problems with the stop/start behaviour.

Comment 8

[William Lachance (:wlach)]

Attachment: Use stopApplication instead of killing fennec manually

Comment 9

[William Lachance (:wlach)]

Attachment: Use stopApplication, take 2

First take of the patch had some problems, oops.

Comment 10

[Bob Clary [:bc] (inactive)]

Comment on attachment 8431075 [details] [diff] [review]
Use stopApplication, take 2

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

Looks good.

Comment 11

[William Lachance (:wlach)]

https://github.com/mozilla/eideticker/commit/d3893cd471f8de099fc345869211ae6b9652e436