I was speaking with jgriffin today about disabling marionette on release builds of B2G (https://github.com/mozilla-b2g/B2G/issues/84). We currently set ENABLE_MARIONETTE in confvars.sh for the b2g product. As I understand it, packaging and enabling Marionette are separate and distinct operations. Let's add configure.in logic to enable marionette and control whether it's packaged. I propose: --enable-marionette: this would set the preference needed for marionette to be enabled to true. Default: disabled --enable-marionette-packaging: this would control whether or not marionette. Default: disabled without --enable-marionette, enabled with --enable-marionette I am guessing enabling the marionette preference without a packaged up copy of marionette is useless. Should it be considered an error condition for marionette to be enabled but packaging disabled?
(In reply to John Ford [:jhford] from comment #0) > > I am guessing enabling the marionette preference without a packaged up copy > of marionette is useless. Should it be considered an error condition for > marionette to be enabled but packaging disabled? Yes. --enable-marionette got changed to ENABLE_MARIONETTE=1 in https://bugzilla.mozilla.org/show_bug.cgi?id=753022#c16, but we may want to reconsider that. As more people get involved in testing with Marionette, there will likely be more people wanting to enable that on their own builds. Adding ted for input.
For the record, I am strongly opposed to your proposal to have separate options for enabling and packaging the code. That is serious overkill. I'm not wild about adding more configure options in general, only because we already have too many, and if we add them then people use them (and get confused by the results). If this is just something you want to flip for release builds, then I don't think it needs a configure option.
So maybe we should just have ENABLE_MARIONETTE=1 both package and enable Marionette, in both B2G and Firefox.
(In reply to Jonathan Griffin (:jgriffin) from comment #3) > So maybe we should just have ENABLE_MARIONETTE=1 both package and enable > Marionette, in both B2G and Firefox. That sounds reasonable to me
(In reply to Ted Mielczarek [:ted] from comment #2) > For the record, I am strongly opposed to your proposal to have separate > options for enabling and packaging the code. That is serious overkill. Cool. I think anything that's not a hardcoded "marionette is enabled for b2g" is a great outcome. (In reply to Jonathan Griffin (:jgriffin) from comment #3) > So maybe we should just have ENABLE_MARIONETTE=1 both package and enable > Marionette, in both B2G and Firefox. In that case, can we set it by default to be disabled and have the engineering builds explicitly enable them? I have a patch to gonk-misc that would make all B2G build system driven engineering builds to set ENABLE_MARIONETTE=1.
(In reply to John Ford [:jhford] from comment #5) > (In reply to Ted Mielczarek [:ted] from comment #2) > > For the record, I am strongly opposed to your proposal to have separate > > options for enabling and packaging the code. That is serious overkill. > > Cool. I think anything that's not a hardcoded "marionette is enabled for > b2g" is a great outcome. > > (In reply to Jonathan Griffin (:jgriffin) from comment #3) > > So maybe we should just have ENABLE_MARIONETTE=1 both package and enable > > Marionette, in both B2G and Firefox. > > In that case, can we set it by default to be disabled and have the > engineering builds explicitly enable them? I have a patch to gonk-misc that > would make all B2G build system driven engineering builds to set > ENABLE_MARIONETTE=1. Yes, that's fine. We'll have to make sure this mechanism is usable by our Jenkins CI too, since it produces its own builds. Worst case I can monkey patch mozconfig in the Jenkins build script.
Created attachment 652163 [details] [diff] [review] Make ENABLE_MARIONETTE consistent According to discussion here and on bug 779919, this patch removes --enable-marionette, and makes ENABLE_MARIONETTE=1 both package and enable marionette, in both Firefox and B2G. This should not land until we update https://github.com/mozilla-b2g/gonk-misc/blob/master/default-gecko-config in order to avoid breaking the existing CI.
Comment on attachment 652163 [details] [diff] [review] Make ENABLE_MARIONETTE consistent This looks good to me, but I don't know if I can do the final review.
Comment on attachment 652163 [details] [diff] [review] Make ENABLE_MARIONETTE consistent Review of attachment 652163 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/app/b2g.js @@ +425,5 @@ > > pref("media.volume.steps", 10); > > //Enable/disable marionette server, set listening port > +#ifdef ENABLE_MARIONETTE Put this above the comment, just so the preprocessed output file doesn't contain the comment when marionette isn't enabled.
Review comments addressed and pushed as https://hg.mozilla.org/integration/mozilla-inbound/rev/98fc3d40910c
Backed out due to leaking crashtest, https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c041240db3. This leak is probably a side effect of the fact that this patch enable Marionette in Firefox (whereas before it was packaged but not enabled). I'll re-land this patch with the previous only-package behavior for desktop Firefox, and file a new bug to investigate to leak.
Created attachment 654356 [details] [diff] [review] Make ENABLE_MARIONETTE consistent v0.2 This version doesn't cause ENABLE_MARIONETTE=1 to enable Marionette in desktop Firefox, it just packages it.
Pushed updated patch to try: https://tbpl.mozilla.org/?tree=Try&rev=856d450d64e7
Along with the leak, there was also a jsreftest test failure and leak, and debug reftests timing out (after a lovely 7200 seconds).
Re-landed as https://hg.mozilla.org/integration/mozilla-inbound/rev/4f008e6bbb9d, minus the change that caused Marionette to be loaded (rather than just packaged and available) on desktop Firefox when ENABLE_MARIONETTE=1.
https://hg.mozilla.org/mozilla-central/rev/4f008e6bbb9d
or
