979650 - ENABLE_MARIONETTE defaults to off in self-built builds

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

This means if someone developing lower level code inadvertently causes a Marionette test on Tbpl to break, if they want to try and fix it, the first thing they're likely to see is:


$ ./mach marionette-test
The marionette-test command requires a Marionette-enabled build.

Add 'ENABLE_MARIONETTE=1' to your mozconfig file and re-build the application.
Your currently active mozconfig is /Users/dmose/r/abr-gecko-dev/.mozconfig.


My guess is that this the result of some tests like:

http://dxr.mozilla.org/mozilla-central/source/browser/app/moz.build#15

Which require marionette to be explicitly enabled, rather than explicitly disabled.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Just FYI, here was the mozconfig I was using on Mac that caused this to be hit:

export PATH="`brew --prefix ccache`/libexec:$PATH"

# Import the stock config for building the browser (Firefox)
. $topsrcdir/browser/config/mozconfig

. $topsrcdir/build/macosx/mozconfig.common

# Define where build files should go. This places them in the directory
# "obj-ff-dbg" under the current source directory
#mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/../objdir/mac-debug

# -s makes builds quieter by default
# -j4 allows 4 tasks to run in parallel. Set the number to be the amount of
# cores in your machine. 4 is a good number.
mk_add_options MOZ_MAKE_FLAGS="-s -j7"

# Enable debug builds
ac_add_options --enable-debug
ac_add_options --enable-optimize

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Interestingly, adding various different things at the top of the file, including

ENABLE_MARIONETTE=1

export ENABLE_MARIONETTE=1

and

mk_add_options ENABLE_MARIONETTE=1


followed by "./mach build" do not fix the problem.  Perhaps a clobber build is required...

Comment 3

[Jonathan Griffin (:jgriffin)]

Yeah, a clobber is required. 

Since ENABLE_MARIONETTE=1 is now the default for official builds, I agree it makes sense to always enable it for desktop Firefox, rather than requiring something in mozconfig.  Adding Ted in case he has a different opinion.

Comment 4

[(not currently active) Ted Mielczarek]

The defaults should always be as close as possible to what we ship. I don't see any reason for this to not be on by default.

Comment 5

[Jonathan Griffin (:jgriffin)]

Looks like we are already doing the right things for desktop Firefox builds (but not B2G desktop builds); we just need to update the mach targets to realize this.

Comment 6

[Jonathan Griffin (:jgriffin)]

Attachment: Always build with Marionette for Firefox, b2g desktop,

Running this on try: https://tbpl.mozilla.org/?tree=Try&rev=f6cd599854f0

Comment 7

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #6)
> Created attachment 8389399 [details] [diff] [review]
> Always build with Marionette for Firefox, b2g desktop,
> 
> Running this on try: https://tbpl.mozilla.org/?tree=Try&rev=f6cd599854f0

Looks like this is a fail for B2G desktop builds.

Comment 8

[Jonathan Griffin (:jgriffin)]

new try run: https://tbpl.mozilla.org/?tree=Try&rev=96a4e9ff8200

Comment 9

[Jonathan Griffin (:jgriffin)]

Attachment: Always build with Marionette for Firefox, b2g desktop,

Finally got a green try run!  https://tbpl.mozilla.org/?tree=Try&rev=b0b5c48cbfeb.  The intent here is to primarily to make it so that B2G desktop builds always get built with Marionette, and also to remove the ENABLE_MARIONETTE checking for desktop Firefox mach commands (since it isn't relevant there).

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 8390722 [details] [diff] [review]
Always build with Marionette for Firefox, b2g desktop,

Review of attachment 8390722 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/app/moz.build
@@ +12,4 @@
>      'nsBrowserApp.cpp',
>  ]
>  
> +DEFINES['ENABLE_MARIONETTE'] = 1

Can you just rip out all the "DEFINES['ENABLE_MARIONETTE'] = 1" bits and move that up to configure.in instead?
if test -n "$ENABLE_MARIONETTE"; then
  AC_DEFINE(ENABLE_MARIONETTE)
fi

Comment 11

[Jonathan Griffin (:jgriffin)]

Attachment: Always build with Marionette for Firefox, b2g desktop,

Move the define() stuff to configure.in:

https://tbpl.mozilla.org/?tree=Try&rev=3093ab7a0d1f
https://tbpl.mozilla.org/?tree=Try&rev=ecdd77a6bcf7

Comment 12

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8393746 [details] [diff] [review]
Always build with Marionette for Firefox, b2g desktop,

carry r+ forward

Comment 13

[Jonathan Griffin (:jgriffin)]

try runs were green.  https://hg.mozilla.org/integration/mozilla-inbound/rev/f36d896befb4

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f36d896befb4