Closed
Bug 779984
Opened 11 years ago
Closed 11 years ago
--enable-marionette should be useful
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla17
People
(Reporter: jhford, Assigned: jgriffin)
References
Details
Attachments
(1 file, 1 obsolete file)
9.81 KB,
patch
|
jgriffin
:
review+
|
Details | Diff | Splinter Review |
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•11 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.
Comment 2•11 years ago
|
||
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•11 years ago
|
||
So maybe we should just have ENABLE_MARIONETTE=1 both package and enable Marionette, in both B2G and Firefox.
Comment 4•11 years ago
|
||
(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
Reporter | ||
Comment 5•11 years ago
|
||
(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•11 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•11 years ago
|
||
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)
Reporter | ||
Comment 8•11 years ago
|
||
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•11 years ago
|
Attachment #652163 -
Flags: review?(ted.mielczarek)
Comment 9•11 years ago
|
||
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+
Updated•11 years ago
|
Assignee: nobody → jgriffin
Assignee | ||
Comment 10•11 years ago
|
||
Review comments addressed and pushed as https://hg.mozilla.org/integration/mozilla-inbound/rev/98fc3d40910c
Target Milestone: --- → mozilla17
Assignee | ||
Comment 11•11 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•11 years ago
|
||
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•11 years ago
|
||
Pushed updated patch to try: https://tbpl.mozilla.org/?tree=Try&rev=856d450d64e7
Comment 14•11 years ago
|
||
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•11 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.
Comment 16•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4f008e6bbb9d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•