Note: There are a few cases of duplicates in user autocompletion which are being worked on.

--enable-marionette should be useful

RESOLVED FIXED in mozilla17

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jhford, Assigned: jgriffin)

Tracking

unspecified
mozilla17
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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?
(Assignee)

Comment 1

5 years ago
(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.
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
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.
Attachment #652163 - Flags: review?(jhford)
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.
Attachment #652163 - Flags: review?(jhford) → feedback+
(Assignee)

Updated

5 years ago
Attachment #652163 - Flags: review?(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.
Attachment #652163 - Flags: review?(ted.mielczarek) → review+
Assignee: nobody → jgriffin
(Assignee)

Comment 10

5 years ago
Review comments addressed and pushed as https://hg.mozilla.org/integration/mozilla-inbound/rev/98fc3d40910c
Target Milestone: --- → mozilla17
(Assignee)

Comment 11

5 years ago
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.
(Assignee)

Comment 12

5 years ago
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.
Attachment #652163 - Attachment is obsolete: true
Attachment #654356 - Flags: review+
(Assignee)

Comment 13

5 years ago
Pushed updated patch to try: https://tbpl.mozilla.org/?tree=Try&rev=856d450d64e7
(Assignee)

Updated

5 years ago
Blocks: 784796
Along with the leak, there was also a jsreftest test failure and leak, and debug reftests timing out (after a lovely 7200 seconds).
(Assignee)

Comment 15

5 years ago
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.