Closed
Bug 1491419
Opened 6 years ago
Closed 6 years ago
move a bunch of configure options to moz.configure
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
Tracking
(firefox64 fixed)
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
References
(Blocks 1 open bug)
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.
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #9009231 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #9009232 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #9009233 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Comment 4•6 years ago
|
||
Attachment #9009234 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Comment 5•6 years ago
|
||
Apparently we made gamepad support mandatory and ditched the option.
Attachment #9009235 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Comment 6•6 years ago
|
||
These settings are just dependent on WebRTC being enabled.
Attachment #9009236 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Comment 7•6 years ago
|
||
Attachment #9009237 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Comment 8•6 years ago
|
||
Attachment #9009238 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Comment 9•6 years ago
|
||
Attachment #9009239 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Comment 10•6 years ago
|
||
Attachment #9009240 -
Flags: review?(core-build-config-reviews)
Updated•6 years ago
|
Attachment #9009231 -
Flags: review?(core-build-config-reviews) → review+
Comment 11•6 years ago
|
||
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-
Updated•6 years ago
|
Attachment #9009233 -
Flags: review?(core-build-config-reviews) → review+
Comment 12•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #9009235 -
Flags: review?(core-build-config-reviews) → review+
Updated•6 years ago
|
Attachment #9009236 -
Flags: review?(core-build-config-reviews) → review+
Updated•6 years ago
|
Attachment #9009237 -
Flags: review?(core-build-config-reviews) → review+
Comment 13•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #9009239 -
Flags: review?(core-build-config-reviews) → review+
Comment 14•6 years ago
|
||
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+
Assignee | ||
Comment 15•6 years ago
|
||
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)
Comment 16•6 years ago
|
||
(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)
Comment 17•6 years ago
|
||
(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.
Assignee | ||
Comment 18•6 years ago
|
||
Second round, now with also noticing that confvars.h default-enabled this on Windows.
Attachment #9009679 -
Flags: review?(nalexander)
Assignee | ||
Updated•6 years ago
|
Attachment #9009231 -
Attachment is obsolete: true
Assignee | ||
Comment 19•6 years ago
|
||
Now with target restrictions and correct defaulting, instead of confvars.sh.
Attachment #9009680 -
Flags: review?(nalexander)
Assignee | ||
Updated•6 years ago
|
Attachment #9009232 -
Attachment is obsolete: true
Assignee | ||
Comment 20•6 years ago
|
||
Now with enabling only when webrtc is enabled.
Attachment #9009684 -
Flags: review?(nalexander)
Assignee | ||
Updated•6 years ago
|
Attachment #9009238 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #9009679 -
Flags: review?(nalexander) → review+
Updated•6 years ago
|
Attachment #9009680 -
Flags: review?(nalexander) → review+
Comment 21•6 years ago
|
||
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+
Assignee | ||
Comment 22•6 years ago
|
||
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•6 years ago
|
||
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
Comment 24•6 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment 25•6 years ago
|
||
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
Updated•2 years ago
|
Blocks: pyconfigure
Severity: normal → --
You need to log in
before you can comment on or make changes to this bug.
Description
•