Pull request for bug 792048 #33

Closed
wants to merge 9 commits into
from

Projects

None yet

2 participants

@wlach
Mozilla member

The motivation behind this patch is good, but I think the best solution here is really just to make an installApp method which works as installLocalApp does above. Really there's no good reason why installApp doesn't do the push onto the device, and updating our existing code which uses the interface (really just sut_tools and eideticker) should be easy. I filed a bug about this here:

https://bugzilla.mozilla.org/show_bug.cgi?id=792072

@wlach
Mozilla member

Good catch. Let's do it like this though:

raise DMError("Command must be an array of command arguments, not a string!")

@wlach

Function names should not normally be capitalized. Could we use something like "getDroidByHWID" here?

@wlach
Mozilla member

I am really not crazy about importing Zeroconf files with a different license directly into eideticker, though it looks like pyzeroconf is not currently in pypi so I'm not sure what the alternatives are. I guess we can do that for now.

Other than that the code looks pretty good! I don't have any objections other than the method name above. The debug print statements are a little icky but we don't really have a better logging solution for mozdevice/mozbase right now.

@wlach
Mozilla member

I'd prefer to add a new method to devicemanager for this, rather than just outright discarding the output. I filed a bug for this:

https://bugzilla.mozilla.org/show_bug.cgi?id=792084

(will try to get this done later today)

@wlach
Mozilla member

This looks fine.

@wlach
Mozilla member

I would probably just instantiate a /dev/null file on demand, rather than making it a member variable. I also wonder if we shouldn't turn this on its head, and make all commands quiet unless the user specifies otherwise (maybe with a verbose parameter or class-wide debugging flag like we have for dmSUT). I know I find most of the chatter generated by dmADB to be pretty useless most of the time.

@wlach
Mozilla member

Ok, just talked with gbrown on irc about this feature (he was the original implementor), and I think we're ok to just remove it. A revised patch to do that would be great.

 jmaher: we were wondering if it would be ok to drop the code in devicemanagerADB which tries to restart the device in root mode. It almost never works, except for possibly some early development boards
 (apparently its only effect is to slow things down when instantiating a dmADB object)
 wlach: I cannot remember why we do that, I believe gbrown added that code in for dmADB
 jmaher: I think that code has been around at least as long as I have. let me check
 wlach: iirc we did that so we didn't have to deal with file permission issues
 wlach: I haven't tried it lately, but the root stuff did work on my Galaxy S. hold on a minute and I'll see if it's still useful to me
* rail is now known as rail_away
 gbrown: thanks
 gbrown: I think the default consensus seems to be that devicemanagerADB shouldn't need root permissions for most things. I've certainly had good luck using it to run mochitest and friends
 wlach: that root restart doesn't help me ... I need to physically disconnect / reconnect the phone after the adb root anyway. and I think you are right -- we don't need root for most things. so go ahead and remove that code if you like.
@vvuk

Closing this in favour of a separate commit just for mDNS stuff; a lot of the pieces here have already been fixed elsewhere in the meantime.

@vvuk vvuk closed this Oct 10, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment