852235 - Let's kill mozb2g!

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Andrew Halberstadt [:ahal]]

I'm asking this question now because it appears that no one is using it (http://mxr.mozilla.org/mozilla-central/search?string=mozb2g) so if we want to get rid of it we should do it now while it is easy to get rid of.

Personally, I don't really think that mozb2g belongs in mozbase. At the moment it is kind of an awkward collection of miscellaneous things that don't really belong together.

I would suggest if there's anything in mozb2g that is super useful to keep, we try to make a home for it in one of the other mozbase modules. If we still have a bunch of random hacks that are very b2g specific we can always make a new module in testing/b2g or similar.

Comment 1

[Jonathan Griffin (:jgriffin)]

I believe Eideticker may be using mozb2g.

Comment 2

[William Lachance (:wlach)]

Eideticker uses mozb2g, it's just outside the tree.

After having used it for four-odd months, I have to agree that the module doesn't quite "feel" right. I strongly believe we *do* want to abstract away some of the details of managing FirefoxOS devices (the b2g process, profile information, the marionette connection, etc.) but where that should live and how it should work is open for discussion.

I'd be in favour of leaving things as they are until we figure out how to replace it.

Comment 3

[Andrew Halberstadt [:ahal]]

(In reply to William Lachance (:wlach) from comment #2)
> I'd be in favour of leaving things as they are until we figure out how to
> replace it.

My only worry here is that someone is going to stumble on it and say "oh cool a library for making my b2g life easier" and then it will just get harder for us to remove it.

What does eideticker actually use? We could always try to refactor only that for a start, nuke the rest, and then if we find we need it later we have wonderful revision history to get it back :)

E.g profile stuff I think could go into mozprofile as a b2g subclass (we already have firefox, thunderbird subclasses there). I know Jeff won't like this ;) but I think it's an improvement.

Comment 4

[Jeff Hammel]

(In reply to Andrew Halberstadt [:ahal] from comment #3)
> (In reply to William Lachance (:wlach) from comment #2)
> > I'd be in favour of leaving things as they are until we figure out how to
> > replace it.

+0.5

> My only worry here is that someone is going to stumble on it and say "oh
> cool a library for making my b2g life easier" and then it will just get
> harder for us to remove it.

I'm not sure if this is a reason in and of itself.  I agree with the sentiment, but can't really support making decisions based on this sort of contingency.  There is a communication/cultural component here: ideally if someone is going to use it they'll touch base wrt the state of the code.  If they don't, and we do decide to deprecate, they can e.g. use the pypi package (which i don't think we actually have) or incorporate our remnants into their code directly -OR- use whatever we replace it with, if applicable.

> What does eideticker actually use? We could always try to refactor only that
> for a start, nuke the rest, and then if we find we need it later we have
> wonderful revision history to get it back :)
> 
> E.g profile stuff I think could go into mozprofile as a b2g subclass (we
> already have firefox, thunderbird subclasses there). I know Jeff won't like
> this ;) but I think it's an improvement.

I don't ;) I was actually not expecting to object, since I was anticipating mozb2g "profile stuff" was e.g. preferences and other stuff related to what is currently in mozprofile.  However, reading the mozb2g code, I don't really feel that what is there now -- taking a string, write to user.js specifically on a b2g device -- is an improvement assuming essentially copy+paste code.

I'm not super against the idea in principle, depends on the implementation. But I also see no need at present.  I do agree that what is there now isn't great, but I could say that about a lot of code.

Comment 5

[Andrew Halberstadt [:ahal]]

(In reply to Jeff Hammel [:jhammel] from comment #4)
> I don't ;) I was actually not expecting to object, since I was anticipating
> mozb2g "profile stuff" was e.g. preferences and other stuff related to what
> is currently in mozprofile.  However, reading the mozb2g code, I don't
> really feel that what is there now -- taking a string, write to user.js
> specifically on a b2g device -- is an improvement assuming essentially
> copy+paste code.

It's looking like most of the b2g profile related stuff is going to be handled by mozrunner once bug 870876 lands. It was decided (via meeting) that we would wait for that to land before evaluating what is left over in mozb2g.

Comment 6

[Jeff Hammel]

General consensus is yes here, IIRC from in person conversations during mozbase work week.

Comment 7

[William Lachance (:wlach)]

(In reply to Jeff Hammel [:jhammel] from comment #6)
> General consensus is yes here, IIRC from in person conversations during
> mozbase work week.

Yes, there are some problems with the mozrunner implementation currently which prevents it from a being a drop-in replacement at the moment (i.e. it doesn't actually work with non-emulated devices), but nothing insurmountable.

Comment 8

[William Lachance (:wlach)]

Bug 949646 eliminates the use of mozb2g in eideticker, after which we can kill this module.

(I basically just rolled the little useful functionality left in mozb2g into eideticker itself -- most of what mozb2g did, gaiatest does better)

Comment 9

[Malini Das [:mdas] - Away, not checking bugmail]

I'm not sure if I should have this discussion here, or in another bug, but we need some gaiatest/marionette independent place for some B2G code to live.

The code in marionette-client and gaiatest have their own assumptions: I made a B2GTestCaseMixin in marionette-client, but it assumes you're using unittest's TestCases, and gaiatest relies on marionette-client, and we shouldn't have to get the full gaiatest or marionette client just to get some code that lets us restart b2g or manage profiles on b2g devices.

This problem is highlighted with speedtests which use GaiaDevice in an object (from gaiatest) for its device management properties, but that object isn't a unittest TestCase. It needs to create a device manager for GaiaDevice to use, and to do so, it can either inherit from B2GTestCaseMixin even though it isn't a TestCase or copy pasted from B2GTestCaseMixin.

We need a place for a B2Gmanager/mixin/whathaveyou to exist that's not in gaiatest or marionette client, and that was the original function of mozb2g (though now it's unused). There's a need for something like this now. If putting this in mozbase is awkward, where should we put something that helps us automate the same thing across multiple concerns?

Comment 10

[William Lachance (:wlach)]

(In reply to Malini Das [:mdas] from comment #9)
> I'm not sure if I should have this discussion here, or in another bug, but
> we need some gaiatest/marionette independent place for some B2G code to live.
> 
> The code in marionette-client and gaiatest have their own assumptions: I
> made a B2GTestCaseMixin in marionette-client, but it assumes you're using
> unittest's TestCases, and gaiatest relies on marionette-client, and we
> shouldn't have to get the full gaiatest or marionette client just to get
> some code that lets us restart b2g or manage profiles on b2g devices.
> 
> This problem is highlighted with speedtests which use GaiaDevice in an
> object (from gaiatest) for its device management properties, but that object
> isn't a unittest TestCase. It needs to create a device manager for
> GaiaDevice to use, and to do so, it can either inherit from B2GTestCaseMixin
> even though it isn't a TestCase or copy pasted from B2GTestCaseMixin.

Can't you just create a device manager in a few lines of code and assign it to the GaiaDevice object?

> We need a place for a B2Gmanager/mixin/whathaveyou to exist that's not in
> gaiatest or marionette client, and that was the original function of mozb2g
> (though now it's unused). There's a need for something like this now. If
> putting this in mozbase is awkward, where should we put something that helps
> us automate the same thing across multiple concerns?

I think it's probably best for ease of understanding if we keep gaia/marionette logic out of mozbase. This doesn't necessarily exclude something which just starts/stops the b2g process and manages its profile, in fact ahal has modified mozrunner to do exactly this (though I think it still needs work to be usable for more than just mochitest/reftest, where it's used now).

Could we add yet another thing (outside of mozbase) to glue mozrunner to gaia/marionette? I am open to that idea. :) This is a conversation probably best had in person with a whiteboard... maybe in Taipei in January?

Comment 11

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to William Lachance (:wlach) from comment #10)
> 
> Can't you just create a device manager in a few lines of code and assign it
> to the GaiaDevice object?
> 
Yes, that works just for GaiaDevice, but I'm saying that the above speedtests object is using GaiaDevice just to use the non-gaiatest and non-marionette specific functions like restartb2g. That should exist in a module on its own so they don't even have to use GaiaDevice, since the real functions of GaiaDevice aren't relevant to this speedtests object.

> Could we add yet another thing (outside of mozbase) to glue mozrunner to
> gaia/marionette? I am open to that idea. :) This is a conversation probably
> best had in person with a whiteboard... maybe in Taipei in January?

It's entirely possible that some consumer of this module will want to connect to an already running instance, and I want a solution that doesn't involve gaiatest/marionette.

We can definitely talk more about it in person.

Comment 12

[Andrew Halberstadt [:ahal]]

What you're describing, is basically the b2g equivalent of automation.py.in. I'm sure automation.py started out with the best of intentions, but if we aren't careful, whatever we come up with will end up just as convoluted. I don't think anyone ever objected to having the mozb2g functionality live in mozbase, I think the objection was just about having it in a utility class that was awkward.

If there's b2g specific code that needs to be shared across modules, try to figure out what category it belongs to. E.g, networking code? -> moznetwork, device manipulation? -> mozdevice, managing b2g process? -> mozrunner, etc.. Once that's figured out, we can create b2g specific subclasses, and add abstractions over that so it just works the same as the desktop equivalent. The important thing is that we have several collections of related code as opposed to one collection of unrelated code.

As an example, mozrunner already works this way. There are Firefox, Thunderbird, and B2G specific implementations that all have the same base API.

Comment 13

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Andrew Halberstadt [:ahal] from comment #12)
> What you're describing, is basically the b2g equivalent of automation.py.in.
> I'm sure automation.py started out with the best of intentions, but if we
> aren't careful, whatever we come up with will end up just as convoluted. I
> don't think anyone ever objected to having the mozb2g functionality live in
> mozbase, I think the objection was just about having it in a utility class
> that was awkward.
> 
> If there's b2g specific code that needs to be shared across modules, try to
> figure out what category it belongs to. E.g, networking code? -> moznetwork,
> device manipulation? -> mozdevice, managing b2g process? -> mozrunner, etc..
> Once that's figured out, we can create b2g specific subclasses, and add
> abstractions over that so it just works the same as the desktop equivalent.
> The important thing is that we have several collections of related code as
> opposed to one collection of unrelated code.
> 
> As an example, mozrunner already works this way. There are Firefox,
> Thunderbird, and B2G specific implementations that all have the same base
> API.

Ah, thanks, I filed this bug to address this: https://bugzilla.mozilla.org/show_bug.cgi?id=951365

Comment 14

[William Lachance (:wlach)]

Attachment: Remove mozb2g;r=ahal

(see bug for rationale)

Comment 15

[Andrew Halberstadt [:ahal]]

Comment on attachment 8356261 [details] [diff] [review]
Remove mozb2g;r=ahal

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

Thanks! There are a few references to it in m-c we'll have to remove i.e testing/mozbase/packages.txt, but I'll take care of that as part of the final m-c sync.

Comment 16

[William Lachance (:wlach)]

Pushed: https://github.com/mozilla/mozbase/commit/f2617edccb9393a2e640385ecb68c72895fa04fe