Closed Bug 1321408 Opened 8 years ago Closed 7 years ago

Move ENABLE_MARIONETTE to python configure

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1321408 - Move ENABLE_MARIONETTE to python configure.
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
Bug 1321408 - Pre: Remove all references to Marionette from b2g.
58 bytes, text/x-review-board-request
ato
: review+
Details 
Just part of the python configure transition.  I also want to take the opportunity to not AC_DEFINE anything globally, so that flipping the flag doesn't trigger (much) recompilation.
chmanchester: if you can tell me how to "depends_if"/"depends_when"/"implies_option" a solution that makes the ENABLE_MARIONETTE default True for non-Android, non-gonk platforms and *doesn't* tie Marionette to the toolkit configuration directly, that would be best.  I thought about including marionette.configure in each application's configuration but couldn't figure out how to set a different default without overriding a user's setting.

maja_zf: can you suggest some try pushes that would verify this actually works?  I don't really know where Marionette tests run.
Flags: needinfo?(mjzffr)
You can use this try syntax to run all relevant Marionette tests:

    -b do -e -p linux64,macosx64,win64,android-api-15 -u xpcshell,marionette,marionette-e10s,luciddream,web-platform-tests,firefox-ui-functional,firefox-ui-functional-e10s -t none

The key part of that is ‘android-api-15’.

Marionette on Fennec/Android is in Tier-3, so you will have to include this in your Treeherder filter.  There are a number of expected errors with the Mn jobs on Fennec, so compare with a recent try push on central.
Flags: needinfo?(mjzffr)
Thanks, ato.

luciddream can be removed; doesn't run anymore. Could also add a few more suites that rely on Marionette for bootstrapping like reftest and mochitest:

-u xpcshell,marionette,marionette-e10s,web-platform-tests,firefox-ui-functional,firefox-ui-functional-e10s,media-tests,media-youtube-tests,reftest,mochitest
Comment on attachment 8815958 [details]
Bug 1321408 - Move ENABLE_MARIONETTE to python configure.

https://reviewboard.mozilla.org/r/96726/#review97290

Re the question in the commit message, SUBSTS are usually visible to Makefiles.

::: b2g/graphene/moz.build:7
(Diff revision 1)
> +for var in ('ENABLE_MARIONETTE'):
> +    if CONFIG[var]:
> +        DEFINES['ENABLE_MARIONETTE'] = 1

Does moz.build actually read this directory? I don't think it would. I sort of doubt any of these builds still work, or marionette would still work with them if they did, we may be able to just remove mentions of Marionette from b2g related things.

::: b2g/installer/package-manifest.in:770
(Diff revision 1)
>  @RESPATH@/chrome/icons/
>  @RESPATH@/chrome/chrome@JAREXT@
>  @RESPATH@/chrome/chrome.manifest
>  @RESPATH@/components/B2GComponents.manifest
>  @BINPATH@/@DLL_PREFIX@omxplugin@DLL_SUFFIX@
> -#if defined(ENABLE_MARIONETTE) || !defined(MOZ_WIDGET_GONK)
> +#if defined(ENABLE_MARIONETTE)

This looks like a stray change, we're not doing anything with `MOZ_WIDGET_GONK` here, are we?

::: build/moz.configure/marionette.configure:7
(Diff revision 1)
> +# Marionette is a Web Driver / Selenium comamnd server and client
> +# automation driver for Mozilla's Gecko engine.  For more, see
> +# https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette.
> +option('--enable-marionette',
> +       help='Enable internal Marionette command server')
> +
> +@depends('--enable-marionette', target, toolkit)
> +def marionette(value, target, toolkit):
> +    if value.origin != 'default':
> +        return bool(value) or None
> +
> +    # Always enable Marionette if not Android or B2G.
> +    if target.os == 'Android':
> +        return False
> +
> +    if toolkit == 'gonk':
> +        return False
> +
> +    return True

Looking at the original definition in old-configure.in, we're basically force-enabling the value for everything that's not Android or gonk, so I think what we want is to use `imply_option` with an argument that returns `True` except when building Android or gonk.

::: toolkit/moz.configure:858
(Diff revision 1)
> +
> +# Marionette isn't really a toolkit feature, it's a Gecko engine feature, but
> +# it's not clear how to set a project- or application-specific default, so the
> +# default is determined in the included file, and we include the file here so
> +# that toolkit is available to it.
> +include('../build/moz.configure/marionette.configure')

I don't think this needs its own file.
Attachment #8815958 - Flags: review?(cmanchester)
(In reply to Chris Manchester (:chmanchester) from comment #6)
> Comment on attachment 8815958 [details]
> Bug 1321408 - Move ENABLE_MARIONETTE to python configure.
> 
> https://reviewboard.mozilla.org/r/96726/#review97290
> 
> Re the question in the commit message, SUBSTS are usually visible to
> Makefiles.

Thanks.

> ::: b2g/graphene/moz.build:7
> (Diff revision 1)
> > +for var in ('ENABLE_MARIONETTE'):
> > +    if CONFIG[var]:
> > +        DEFINES['ENABLE_MARIONETTE'] = 1
> 
> Does moz.build actually read this directory? I don't think it would. I sort
> of doubt any of these builds still work, or marionette would still work with
> them if they did, we may be able to just remove mentions of Marionette from
> b2g related things.

The ones where I add moz.build files are toplevel application targets; they have an app.mozbuild.  If moz.build files aren't automatically read from the toplevel, then know, this would be busted.

As for removing b2g related things: I'm fine with that, but not the right person to make that call.  Do you know who could own that?  I wonder if ato knows, I'll NI them.

> ::: b2g/installer/package-manifest.in:770
> (Diff revision 1)
> >  @RESPATH@/chrome/icons/
> >  @RESPATH@/chrome/chrome@JAREXT@
> >  @RESPATH@/chrome/chrome.manifest
> >  @RESPATH@/components/B2GComponents.manifest
> >  @BINPATH@/@DLL_PREFIX@omxplugin@DLL_SUFFIX@
> > -#if defined(ENABLE_MARIONETTE) || !defined(MOZ_WIDGET_GONK)
> > +#if defined(ENABLE_MARIONETTE)
> 
> This looks like a stray change, we're not doing anything with
> `MOZ_WIDGET_GONK` here, are we?

No, see the commit message: I want to unify everything behind the flag, rather than having these ad-hoc things sprinkled around.  That is, if you want Marionette, set ENABLE_MARIONETTE -- don't have a bunch of "if ENABLE_MARIONETTE, or other ad-hoc condition" sprinkled around and subtly different.

> ::: build/moz.configure/marionette.configure:7
> (Diff revision 1)
> > +# Marionette is a Web Driver / Selenium comamnd server and client
> > +# automation driver for Mozilla's Gecko engine.  For more, see
> > +# https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette.
> > +option('--enable-marionette',
> > +       help='Enable internal Marionette command server')
> > +
> > +@depends('--enable-marionette', target, toolkit)
> > +def marionette(value, target, toolkit):
> > +    if value.origin != 'default':
> > +        return bool(value) or None
> > +
> > +    # Always enable Marionette if not Android or B2G.
> > +    if target.os == 'Android':
> > +        return False
> > +
> > +    if toolkit == 'gonk':
> > +        return False
> > +
> > +    return True
> 
> Looking at the original definition in old-configure.in, we're basically
> force-enabling the value for everything that's not Android or gonk, so I
> think what we want is to use `imply_option` with an argument that returns
> `True` except when building Android or gonk.

I will try to arrange this.

> ::: toolkit/moz.configure:858
> (Diff revision 1)
> > +
> > +# Marionette isn't really a toolkit feature, it's a Gecko engine feature, but
> > +# it's not clear how to set a project- or application-specific default, so the
> > +# default is determined in the included file, and we include the file here so
> > +# that toolkit is available to it.
> > +include('../build/moz.configure/marionette.configure')
> 
> I don't think this needs its own file.

OK, I'll fold it into toolkit.  I think you're wrong, though: folding everything into toolkit is how you end up with an unmaintainable configure.in...
> > Looking at the original definition in old-configure.in, we're basically
> > force-enabling the value for everything that's not Android or gonk, so I
> > think what we want is to use `imply_option` with an argument that returns
> > `True` except when building Android or gonk.
> 
> I will try to arrange this.

I spent a few minutes, and I can't figure out how to use imply_option *and* have both --enable and --disable work in mozconfig.  I get "conflicting options" since imply_option is always setting one.  I've just inlined what I had before with the computed default if the user doesn't set the value.
:ato, can you comment on whether marionette on b2g is still supported?
Flags: needinfo?(ato)
(In reply to Nick Alexander :nalexander (leave until January 2017) from comment #8)
> > > Looking at the original definition in old-configure.in, we're basically
> > > force-enabling the value for everything that's not Android or gonk, so I
> > > think what we want is to use `imply_option` with an argument that returns
> > > `True` except when building Android or gonk.
> > 
> > I will try to arrange this.
> 
> I spent a few minutes, and I can't figure out how to use imply_option *and*
> have both --enable and --disable work in mozconfig.  I get "conflicting
> options" since imply_option is always setting one.  I've just inlined what I
> had before with the computed default if the user doesn't set the value.

Today people can un-set ENABLE_MARIONETTE in their mozconfig, but still get a marionette enabled build, I think we should stop supporting this by no longer supporting the --disable case where we were force-enabling the value before.
(In reply to Chris Manchester (:chmanchester) from comment #10)
> :ato, can you comment on whether marionette on b2g is still supported?

No it's not. It has the same rule as platform in that we have support for removing b2g support.
Flags: needinfo?(ato)
(In reply to Chris Manchester (:chmanchester) from comment #11)
> (In reply to Nick Alexander :nalexander (leave until January 2017) from
> comment #8)
> > > > Looking at the original definition in old-configure.in, we're basically
> > > > force-enabling the value for everything that's not Android or gonk, so I
> > > > think what we want is to use `imply_option` with an argument that returns
> > > > `True` except when building Android or gonk.
> > > 
> > > I will try to arrange this.
> > 
> > I spent a few minutes, and I can't figure out how to use imply_option *and*
> > have both --enable and --disable work in mozconfig.  I get "conflicting
> > options" since imply_option is always setting one.  I've just inlined what I
> > had before with the computed default if the user doesn't set the value.
> 
> Today people can un-set ENABLE_MARIONETTE in their mozconfig, but still get
> a marionette enabled build, I think we should stop supporting this by no
> longer supporting the --disable case where we were force-enabling the value
> before.

I think we should stop supporting what was there previously, but why would we *not* let the user make a build without Marionette?

What I've done now defaults as before, but lets the user --enable if it's not there, and --disable if it is there.  I'd like a strong argument for why we wouldn't let the user control the build.
Flags: needinfo?(cmanchester)
On the flip side, what is there to gain from building without marionette? I guess that disables copying some files, but does that make a noticeable difference?
(In reply to Mike Hommey [:glandium] from comment #14)
> On the flip side, what is there to gain from building without marionette? I
> guess that disables copying some files, but does that make a noticeable
> difference?

As usual, it's a non-trivial delta for Android.  From a local omni.ja, I see >300kb uncompressed.  It's not easy for me to check compressed right now.

  -rw-r--r--     12476   1-Jan-2010  00:00:00  chrome/marionette/content/accessibility.js
  -rw-r--r--     13387   1-Jan-2010  00:00:00  chrome/marionette/content/action.js
  -rw-r--r--      2934   1-Jan-2010  00:00:00  chrome/marionette/content/addon.js
  -rw-r--r--      5568   1-Jan-2010  00:00:00  chrome/marionette/content/assert.js
  -rw-r--r--     94168   1-Jan-2010  00:00:00  chrome/marionette/content/atom.js
  -rw-r--r--      9097   1-Jan-2010  00:00:00  chrome/marionette/content/browser.js
  -rw-r--r--      4988   1-Jan-2010  00:00:00  chrome/marionette/content/capture.js
  -rw-r--r--      4333   1-Jan-2010  00:00:00  chrome/marionette/content/cert.js
  -rw-r--r--      3728   1-Jan-2010  00:00:00  chrome/marionette/content/cookies.js
  -rw-r--r--      6342   1-Jan-2010  00:00:00  chrome/marionette/content/dispatcher.js
  -rw-r--r--     94763   1-Jan-2010  00:00:00  chrome/marionette/content/driver.js
  -rw-r--r--     30751   1-Jan-2010  00:00:00  chrome/marionette/content/element.js
  -rw-r--r--     11574   1-Jan-2010  00:00:00  chrome/marionette/content/error.js
  -rw-r--r--     13986   1-Jan-2010  00:00:00  chrome/marionette/content/evaluate.js
  -rw-r--r--     38379   1-Jan-2010  00:00:00  chrome/marionette/content/event.js
  -rw-r--r--     10186   1-Jan-2010  00:00:00  chrome/marionette/content/frame.js
  -rw-r--r--     10407   1-Jan-2010  00:00:00  chrome/marionette/content/interaction.js
  -rw-r--r--      3074   1-Jan-2010  00:00:00  chrome/marionette/content/l10n.js
  -rw-r--r--     13749   1-Jan-2010  00:00:00  chrome/marionette/content/legacyaction.js
  -rw-r--r--     55236   1-Jan-2010  00:00:00  chrome/marionette/content/listener.js
  -rw-r--r--      1712   1-Jan-2010  00:00:00  chrome/marionette/content/logging.js
  -rw-r--r--      7573   1-Jan-2010  00:00:00  chrome/marionette/content/message.js
  -rw-r--r--      2853   1-Jan-2010  00:00:00  chrome/marionette/content/modal.js
  -rw-r--r--      3725   1-Jan-2010  00:00:00  chrome/marionette/content/navigate.js
  -rw-r--r--     11069   1-Jan-2010  00:00:00  chrome/marionette/content/proxy.js
  -rw-r--r--      4389   1-Jan-2010  00:00:00  chrome/marionette/content/server.js
  -rw-r--r--      6074   1-Jan-2010  00:00:00  chrome/marionette/content/simpletest.js
  -rw-r--r--      1147   1-Jan-2010  00:00:00  chrome/marionette/content/test.xul
  -rw-r--r--       667   1-Jan-2010  00:00:00  chrome/marionette/content/test2.xul
  -rw-r--r--      1369   1-Jan-2010  00:00:00  chrome/marionette/content/test_anonymous_content.xul
  -rw-r--r--       481   1-Jan-2010  00:00:00  chrome/marionette/content/test_dialog.xul
  -rw-r--r--       316   1-Jan-2010  00:00:00  chrome/marionette/content/test_nested_iframe.xul
  -rw-r--r--      7816   1-Jan-2010  00:00:00  components/marionette.js
Why are those things not in an addon?
(In reply to Mike Hommey [:glandium] from comment #16)
> Why are those things not in an addon?

History?  Maybe dburns knows more.
Flags: needinfo?(dburns)
History. We started in B2g and we couldnt do addons there. Also, with the addons ecosystem being in this weird place with addon signing and now webextensions, we can't have our core test framework broken by their decisions.

We havent enabled this for release builds for security reasons yet so I wouldnt worry about the size of things as they stand now. We can see about optimising things for a smaller footprint when we go to do a release on android. We have been releasing with desktop since Firefox 24.
Flags: needinfo?(dburns)
(In reply to Nick Alexander :nalexander (leave until January 2017) from comment #13)
> (In reply to Chris Manchester (:chmanchester) from comment #11)
> > (In reply to Nick Alexander :nalexander (leave until January 2017) from
> > comment #8)
> > > > > Looking at the original definition in old-configure.in, we're basically
> > > > > force-enabling the value for everything that's not Android or gonk, so I
> > > > > think what we want is to use `imply_option` with an argument that returns
> > > > > `True` except when building Android or gonk.
> > > > 
> > > > I will try to arrange this.
> > > 
> > > I spent a few minutes, and I can't figure out how to use imply_option *and*
> > > have both --enable and --disable work in mozconfig.  I get "conflicting
> > > options" since imply_option is always setting one.  I've just inlined what I
> > > had before with the computed default if the user doesn't set the value.
> > 
> > Today people can un-set ENABLE_MARIONETTE in their mozconfig, but still get
> > a marionette enabled build, I think we should stop supporting this by no
> > longer supporting the --disable case where we were force-enabling the value
> > before.
> 
> I think we should stop supporting what was there previously, but why would
> we *not* let the user make a build without Marionette?
> 
> What I've done now defaults as before, but lets the user --enable if it's
> not there, and --disable if it is there.  I'd like a strong argument for why
> we wouldn't let the user control the build.

I'm not advocating we force enable it on Android, or anywhere it's not already force-enabled. We can't run mochitests or reftests on desktop without marionette at this point, so it seems like disabling it there will hardly ever be what a developer intends. Without a compelling reason, I don't see why we would start supporting this now. If we do find someone who wants to do this, I would think that's a discussion for a different bug.
Flags: needinfo?(cmanchester)
(In reply to Chris Manchester (:chmanchester) from comment #10)
> :ato, can you comment on whether marionette on b2g is still supported?

It isn’t.  Please remove any B2G related Marionette code.
Comment on attachment 8815958 [details]
Bug 1321408 - Move ENABLE_MARIONETTE to python configure.

https://reviewboard.mozilla.org/r/96726/#review98974

Clearing review until we have a version of the patch that doesn't appear to continue supporting b2g, and doesn't add the ability to disable marionette on desktop.
Attachment #8815958 - Flags: review?(cmanchester)
(In reply to Chris Manchester (:chmanchester) from comment #19)
> (In reply to Nick Alexander :nalexander (leave until January 2017) from
> comment #13)
> > (In reply to Chris Manchester (:chmanchester) from comment #11)
> > > (In reply to Nick Alexander :nalexander (leave until January 2017) from
> > > comment #8)
> > > > > > Looking at the original definition in old-configure.in, we're basically
> > > > > > force-enabling the value for everything that's not Android or gonk, so I
> > > > > > think what we want is to use `imply_option` with an argument that returns
> > > > > > `True` except when building Android or gonk.
> > > > > 
> > > > > I will try to arrange this.
> > > > 
> > > > I spent a few minutes, and I can't figure out how to use imply_option *and*
> > > > have both --enable and --disable work in mozconfig.  I get "conflicting
> > > > options" since imply_option is always setting one.  I've just inlined what I
> > > > had before with the computed default if the user doesn't set the value.
> > > 
> > > Today people can un-set ENABLE_MARIONETTE in their mozconfig, but still get
> > > a marionette enabled build, I think we should stop supporting this by no
> > > longer supporting the --disable case where we were force-enabling the value
> > > before.
> > 
> > I think we should stop supporting what was there previously, but why would
> > we *not* let the user make a build without Marionette?
> > 
> > What I've done now defaults as before, but lets the user --enable if it's
> > not there, and --disable if it is there.  I'd like a strong argument for why
> > we wouldn't let the user control the build.
> 
> I'm not advocating we force enable it on Android, or anywhere it's not
> already force-enabled. We can't run mochitests or reftests on desktop
> without marionette at this point, so it seems like disabling it there will
> hardly ever be what a developer intends. Without a compelling reason, I
> don't see why we would start supporting this now. If we do find someone who
> wants to do this, I would think that's a discussion for a different bug.

chmanchester: OK, I agree with this.  I wasn't aware that Marionette was required for our major testing suites.  I'm all for removing ankle cannons!
Comment on attachment 8818791 [details]
Bug 1321408 - Pre: Remove all references to Marionette from b2g.

https://reviewboard.mozilla.org/r/98740/#review99096
Attachment #8818791 - Flags: review?(ato) → review+
Comment on attachment 8815958 [details]
Bug 1321408 - Move ENABLE_MARIONETTE to python configure.

https://reviewboard.mozilla.org/r/96726/#review99464

::: toolkit/moz.configure:895
(Diff revision 3)
> +imply_option('--enable-marionette', marionette_default,
> +             reason='not Android and not gonk')

We tend to put imply_option before the option definition.
Attachment #8815958 - Flags: review?(cmanchester) → review+
Comment on attachment 8815958 [details]
Bug 1321408 - Move ENABLE_MARIONETTE to python configure.

https://reviewboard.mozilla.org/r/96726/#review97290

> Looking at the original definition in old-configure.in, we're basically force-enabling the value for everything that's not Android or gonk, so I think what we want is to use `imply_option` with an argument that returns `True` except when building Android or gonk.

We discussed in Bugzilla (at some length), and I've come around to chmanchester's position.  The commit pushed won't change the force-enabling behavior.
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/82893c43bbd5
Pre: Remove all references to Marionette from b2g. r=ato
https://hg.mozilla.org/integration/autoland/rev/998a91a71810
Move ENABLE_MARIONETTE to python configure. r=chmanchester
https://hg.mozilla.org/mozilla-central/rev/82893c43bbd5
https://hg.mozilla.org/mozilla-central/rev/998a91a71810
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Would it make sense to uplift this patch to aurora so it can make the next ESR release? Would be good to have the same build configs across all channels for the next year. Alexander, what do you think?
Assignee: nobody → nalexander
Flags: needinfo?(nalexander)
(In reply to Henrik Skupin (:whimboo) from comment #33)
> Would it make sense to uplift this patch to aurora so it can make the next
> ESR release? Would be good to have the same build configs across all
> channels for the next year. Alexander, what do you think?

Sure, I think that's sensible.  Is a very small change.  I don't know how to request uplift using mozreview -- is that possible?
Flags: needinfo?(nalexander)
Uplifts for testonly changes can be requested by putting the [checkin-needed-aurora] entry into the whiteboard. There is no need to request for approval.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: