Closed Bug 1491419 Opened Last year Closed 11 months ago

move a bunch of configure options to moz.configure

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(10 files, 3 obsolete files)

3.43 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
3.39 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
929 bytes, patch
nalexander
: review+
Details | Diff | Splinter Review
3.53 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
4.86 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
2.74 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
4.05 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
4.22 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
3.94 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
3.52 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
Picking off easy stuff while waiting for other things to happen.
Attachment #9009231 - Flags: review?(core-build-config-reviews)
Attachment #9009232 - Flags: review?(core-build-config-reviews)
Attachment #9009233 - Flags: review?(core-build-config-reviews)
Attachment #9009234 - Flags: review?(core-build-config-reviews)
Apparently we made gamepad support mandatory and ditched the option.
Attachment #9009235 - Flags: review?(core-build-config-reviews)
These settings are just dependent on WebRTC being enabled.
Attachment #9009236 - Flags: review?(core-build-config-reviews)
Attachment #9009237 - Flags: review?(core-build-config-reviews)
Attachment #9009238 - Flags: review?(core-build-config-reviews)
Attachment #9009239 - Flags: review?(core-build-config-reviews)
Attachment #9009240 - Flags: review?(core-build-config-reviews)
Attachment #9009231 - Flags: review?(core-build-config-reviews) → review+
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?
Attachment #9009232 - Flags: review?(core-build-config-reviews) → review-
Attachment #9009233 - Flags: review?(core-build-config-reviews) → review+
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.
Attachment #9009234 - Flags: review?(core-build-config-reviews) → review+
Attachment #9009235 - Flags: review?(core-build-config-reviews) → review+
Attachment #9009236 - Flags: review?(core-build-config-reviews) → review+
Attachment #9009237 - Flags: review?(core-build-config-reviews) → review+
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?
Attachment #9009238 - Flags: review?(core-build-config-reviews) → review+
Attachment #9009239 - Flags: review?(core-build-config-reviews) → review+
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?
Attachment #9009240 - Flags: review?(core-build-config-reviews) → review+
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.
Flags: needinfo?(nalexander)
(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.
Flags: needinfo?(nalexander)
(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.
Second round, now with also noticing that confvars.h default-enabled this on
Windows.
Attachment #9009679 - Flags: review?(nalexander)
Attachment #9009231 - Attachment is obsolete: true
Now with target restrictions and correct defaulting, instead of confvars.sh.
Attachment #9009680 - Flags: review?(nalexander)
Attachment #9009232 - Attachment is obsolete: true
Now with enabling only when webrtc is enabled.
Attachment #9009684 - Flags: review?(nalexander)
Attachment #9009238 - Attachment is obsolete: true
Attachment #9009679 - Flags: review?(nalexander) → review+
Attachment #9009680 - Flags: review?(nalexander) → review+
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!
Attachment #9009684 - Flags: review?(nalexander) → review+
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.
Pushed by nfroyd@mozilla.com:
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
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/cc2807a09902
Port bug 1491419 - Changes to confvars.sh (maintenance service and bundled fonts). r=jorgk DONTBUILD
You need to log in before you can comment on or make changes to this bug.