779984 - --enable-marionette should be useful

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[John Ford [:jhford] CET/CEST Berlin Time]

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?

Comment 1

[Jonathan Griffin (:jgriffin)]

(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.

Comment 2

[(not currently active) Ted Mielczarek]

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.

Comment 3

[Jonathan Griffin (:jgriffin)]

So maybe we should just have ENABLE_MARIONETTE=1 both package and enable Marionette, in both B2G and Firefox.

Comment 4

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

(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

Comment 5

[John Ford [:jhford] CET/CEST Berlin Time]

(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.

Comment 6

[Jonathan Griffin (:jgriffin)]

(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.

Comment 7

[Jonathan Griffin (:jgriffin)]

Attachment: 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 8

[John Ford [:jhford] CET/CEST Berlin Time]

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 9

[(not currently active) Ted Mielczarek]

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.

Comment 10

[Jonathan Griffin (:jgriffin)]

Review comments addressed and pushed as https://hg.mozilla.org/integration/mozilla-inbound/rev/98fc3d40910c

Comment 11

[Jonathan Griffin (:jgriffin)]

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.

Comment 12

[Jonathan Griffin (:jgriffin)]

Attachment: Make ENABLE_MARIONETTE consistent v0.2

This version doesn't cause ENABLE_MARIONETTE=1 to enable Marionette in desktop Firefox, it just packages it.

Comment 13

[Jonathan Griffin (:jgriffin)]

Pushed updated patch to try: https://tbpl.mozilla.org/?tree=Try&rev=856d450d64e7

Comment 14

[Phil Ringnalda (:philor)]

Along with the leak, there was also a jsreftest test failure and leak, and debug reftests timing out (after a lovely 7200 seconds).

Comment 15

[Jonathan Griffin (:jgriffin)]

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.

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4f008e6bbb9d