Last Comment Bug 779984 - --enable-marionette should be useful
: --enable-marionette should be useful
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Jonathan Griffin (:jgriffin)
:
Mentors:
Depends on:
Blocks: 784796
  Show dependency treegraph
 
Reported: 2012-08-02 14:15 PDT by John Ford [:jhford]
Modified: 2012-10-21 14:24 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Make ENABLE_MARIONETTE consistent (11.14 KB, patch)
2012-08-15 11:45 PDT, Jonathan Griffin (:jgriffin)
ted: review+
jhford: feedback+
Details | Diff | Review
Make ENABLE_MARIONETTE consistent v0.2 (9.81 KB, patch)
2012-08-22 13:40 PDT, Jonathan Griffin (:jgriffin)
jgriffin: review+
Details | Diff | Review

Description John Ford [:jhford] 2012-08-02 14:15:14 PDT
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) 2012-08-02 14:43:54 PDT
(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 Ted Mielczarek [:ted.mielczarek] 2012-08-02 17:54:12 PDT
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) 2012-08-02 18:03:48 PDT
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 2012-08-03 08:45:11 PDT
(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] 2012-08-03 11:22:14 PDT
(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) 2012-08-03 11:53:13 PDT
(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) 2012-08-15 11:45:36 PDT
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 8 John Ford [:jhford] 2012-08-15 11:56:07 PDT
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 Ted Mielczarek [:ted.mielczarek] 2012-08-22 06:33:58 PDT
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) 2012-08-22 11:18:18 PDT
Review comments addressed and pushed as https://hg.mozilla.org/integration/mozilla-inbound/rev/98fc3d40910c
Comment 11 Jonathan Griffin (:jgriffin) 2012-08-22 13:15:43 PDT
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) 2012-08-22 13:40:40 PDT
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.
Comment 13 Jonathan Griffin (:jgriffin) 2012-08-22 13:45:50 PDT
Pushed updated patch to try: https://tbpl.mozilla.org/?tree=Try&rev=856d450d64e7
Comment 14 Phil Ringnalda (:philor) 2012-08-22 15:36:12 PDT
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) 2012-08-22 16:27:57 PDT
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] 2012-08-23 03:46:33 PDT
https://hg.mozilla.org/mozilla-central/rev/4f008e6bbb9d

Note You need to log in before you can comment on or make changes to this bug.