1491419 - move a bunch of configure options to moz.configure

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Nathan Froyd [:froydnj]]

Picking off easy stuff while waiting for other things to happen.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: move --enable-maintenance-service to moz.configure

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: move --enable-bundled-fonts to moz.configure

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: move --enable-verify-mar to moz.configure

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: move --enable-updater to moz.configure

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: remove --enable-gamepad from old_configure_options

Apparently we made gamepad support mandatory and ditched the option.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: move MOZ_{SCTP,SRTP} to moz.configure

These settings are just dependent on WebRTC being enabled.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: move --enable-raw to moz.configure

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: move --enable-hardware-aec-ns to moz.configure

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: move --enable-tasktracer to moz.configure

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: move --enable-reflow-perf to moz.configure

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 9009232 [details] [diff] [review]
move --enable-bundled-fonts to moz.configure

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

r- just to circle back.

::: toolkit/moz.configure
@@ +1095,5 @@
> +
> +# Bundled fonts on desktop platform
> +# ==============================================================
> +
> +option('--enable-bundled-fonts',

Can we actually make this be Desktop-only, perhaps by testing application?

Also, I see https://searchfox.org/mozilla-central/source/browser/confvars.sh#11-14 -- can we get this out of confvars.sh and move the defaulting into moz.configure?

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 9009234 [details] [diff] [review]
move --enable-updater to moz.configure

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

::: toolkit/moz.configure
@@ +1111,5 @@
>             depends_if('--enable-verify-mar')(lambda _: True))
>  set_config('MOZ_VERIFY_MAR_SIGNATURE',
>             depends_if('--enable-verify-mar')(lambda _: True))
> +
> +# Verify MAR signatures

This comment looks wrong.

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 9009238 [details] [diff] [review]
move --enable-hardware-aec-ns to moz.configure

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

::: toolkit/moz.configure
@@ +982,5 @@
>  # opt/production builds (via MOZ_CRASH())
>  set_config('MOZ_WEBRTC_ASSERT_ALWAYS', webrtc)
>  set_define('MOZ_WEBRTC_ASSERT_ALWAYS', webrtc)
>  
> +option('--enable-hardware-aec-ns',

Shouldn't this depend on WebRTC being enabled?

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 9009240 [details] [diff] [review]
move --enable-reflow-perf to moz.configure

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

::: toolkit/moz.configure
@@ +1171,5 @@
> +
> +option('--enable-reflow-perf', help='Enable reflow performance tracing',
> +       default=reflow_perf)
> +
> +# The difference in conditions here is deliberate.

Can you explain what it achieves, for us newbs?

Comment 15

[Nathan Froyd [:froydnj]]

Thanks for the reviews!

(In reply to Nick Alexander :nalexander [he/him] from comment #13)
> ::: toolkit/moz.configure
> @@ +982,5 @@
> >  # opt/production builds (via MOZ_CRASH())
> >  set_config('MOZ_WEBRTC_ASSERT_ALWAYS', webrtc)
> >  set_define('MOZ_WEBRTC_ASSERT_ALWAYS', webrtc)
> >  
> > +option('--enable-hardware-aec-ns',
> 
> Shouldn't this depend on WebRTC being enabled?

You would think that, but this is the way old-configure.in had it, so I opted to leave it that way.  Would you prefer I make its availability depend on WebRTC being enabled?

(In reply to Nick Alexander :nalexander [he/him] from comment #14)
> ::: toolkit/moz.configure
> @@ +1171,5 @@
> > +
> > +option('--enable-reflow-perf', help='Enable reflow performance tracing',
> > +       default=reflow_perf)
> > +
> > +# The difference in conditions here is deliberate.
> 
> Can you explain what it achieves, for us newbs?

I was trying to keep the same behavior as old-configure.in.  For whatever reason, MOZ_REFLOW_PERF_DSP is always enabled in DEBUG, whereas MOZ_REFLOW_PERF can be toggled off by the command-line option.  I don't know the rationale for this.

(In reply to Nick Alexander :nalexander [he/him] from comment #11)
> ::: toolkit/moz.configure
> @@ +1095,5 @@
> > +
> > +# Bundled fonts on desktop platform
> > +# ==============================================================
> > +
> > +option('--enable-bundled-fonts',
> 
> Can we actually make this be Desktop-only, perhaps by testing application?

We can do that, sure.

> Also, I see
> https://searchfox.org/mozilla-central/source/browser/confvars.sh#11-14 --
> can we get this out of confvars.sh and move the defaulting into
> moz.configure?

Good catch, I will do that.

Comment 16

[Nick Alexander :nalexander [he/him]]

(In reply to Nathan Froyd [:froydnj] from comment #15)
> Thanks for the reviews!
> 
> (In reply to Nick Alexander :nalexander [he/him] from comment #13)
> > ::: toolkit/moz.configure
> > @@ +982,5 @@
> > >  # opt/production builds (via MOZ_CRASH())
> > >  set_config('MOZ_WEBRTC_ASSERT_ALWAYS', webrtc)
> > >  set_define('MOZ_WEBRTC_ASSERT_ALWAYS', webrtc)
> > >  
> > > +option('--enable-hardware-aec-ns',
> > 
> > Shouldn't this depend on WebRTC being enabled?
> 
> You would think that, but this is the way old-configure.in had it, so I
> opted to leave it that way.  Would you prefer I make its availability depend
> on WebRTC being enabled?

I would, because I think we should try to make moz.configure tell you when you're aiming the cannon towards your own foot, but I don't feel that strongly about it.

> (In reply to Nick Alexander :nalexander [he/him] from comment #14)
> > ::: toolkit/moz.configure
> > @@ +1171,5 @@
> > > +
> > > +option('--enable-reflow-perf', help='Enable reflow performance tracing',
> > > +       default=reflow_perf)
> > > +
> > > +# The difference in conditions here is deliberate.
> > 
> > Can you explain what it achieves, for us newbs?
> 
> I was trying to keep the same behavior as old-configure.in.  For whatever
> reason, MOZ_REFLOW_PERF_DSP is always enabled in DEBUG, whereas
> MOZ_REFLOW_PERF can be toggled off by the command-line option.  I don't know
> the rationale for this.

I don't feel strongly, but I'd prefer to explain the difference -- like you do here -- rather than say there is a difference but give no guidance.  I don't care if you make the behaviour more uniform; the next person in the subject area can do that.

Comment 17

[(not currently active) Ted Mielczarek]

(In reply to Nathan Froyd [:froydnj] from comment #5)
> Created attachment 9009235 [details] [diff] [review]
> remove --enable-gamepad from old_configure_options
> 
> Apparently we made gamepad support mandatory and ditched the option.

That was bug 1315896, for the record. It just made us use a stub implementation when a real one isn't available for the platform.

Comment 18

[Nathan Froyd [:froydnj]]

Attachment: move --enable-maintenance-service to moz.configure

Second round, now with also noticing that confvars.h default-enabled this on
Windows.

Comment 19

[Nathan Froyd [:froydnj]]

Attachment: move --enable-bundled-fonts to moz.configure

Now with target restrictions and correct defaulting, instead of confvars.sh.

Comment 20

[Nathan Froyd [:froydnj]]

Attachment: move --enable-hardware-aec-ns to moz.configure

Now with enabling only when webrtc is enabled.

Comment 21

[Nick Alexander :nalexander [he/him]]

Comment on attachment 9009684 [details] [diff] [review]
move --enable-hardware-aec-ns to moz.configure

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

Nice.  Setting the phasers to stun rather than obliterate!

Comment 22

[Nathan Froyd [:froydnj]]

I'm going to drop the --enable-updater bit for now; these patches collectively change a lot of stuff, the MOZ_UPDATER bits are a little more intertwined than I'm willing to get into at the moment, and these patches carry some risk of breaking things already.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a985dd40eba
move --enable-maintenance-service to moz.configure; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/61ab6941b463
move --enable-bundled-fonts to moz.configure; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a6d50bdd491
move --enable-verify-mar to moz.configure; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e51843db3f2
remove --enable-gamepad from old_configure_options; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7ba9ef59cce
move MOZ_{SCTP,SRTP} to moz.configure; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/362984e9cc33
move --enable-raw to moz.configure; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/2806d89b9234
move --enable-hardware-aec-ns to moz.configure; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/b257f734506b
move --enable-tasktracer to moz.configure; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6e9cfc12ec5
move --enable-reflow-perf to moz.configure; r=nalexander

Comment 24

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/2a985dd40eba
https://hg.mozilla.org/mozilla-central/rev/61ab6941b463
https://hg.mozilla.org/mozilla-central/rev/4a6d50bdd491
https://hg.mozilla.org/mozilla-central/rev/7e51843db3f2
https://hg.mozilla.org/mozilla-central/rev/d7ba9ef59cce
https://hg.mozilla.org/mozilla-central/rev/362984e9cc33
https://hg.mozilla.org/mozilla-central/rev/2806d89b9234
https://hg.mozilla.org/mozilla-central/rev/b257f734506b
https://hg.mozilla.org/mozilla-central/rev/c6e9cfc12ec5

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cc2807a09902
Port bug 1491419 - Changes to confvars.sh (maintenance service and bundled fonts). r=jorgk DONTBUILD