Closed
Bug 1257104
Opened 8 years ago
Closed 8 years ago
Move --enable-eme and related to moz.configure
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox48 fixed)
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: glandium, Assigned: glandium)
References
(Blocks 1 open bug)
Details
Attachments
(6 files)
58 bytes,
text/x-review-board-request
|
ted
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
ted
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
ted
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
ted
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
ted
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
ted
:
review+
|
Details |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
While rare, this is something we support for e.g. --enable-eme, where it can be either given with no values, "adobe", some future other EME GMP adapter names, or a combination of them. Review commit: https://reviewboard.mozilla.org/r/40433/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40433/
Attachment #8731210 -
Flags: review?(ted)
Assignee | ||
Comment 2•8 years ago
|
||
Note the AC_DEFINE used to be in a COMPILE_ENVIRONMENT block, but the define is actually used in Gecko preferences, so it's actually better that the define is always set when MOZ_APPLEMEDIA is enabled. Review commit: https://reviewboard.mozilla.org/r/40435/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40435/
Attachment #8731211 -
Flags: review?(ted)
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40437/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40437/
Attachment #8731212 -
Flags: review?(ted)
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40439/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40439/
Attachment #8731213 -
Flags: review?(ted)
Assignee | ||
Comment 5•8 years ago
|
||
Note the imply_option from --enable-ffmpeg replaces the explicit check from old-configure.in and triggers an error in the same circumstances. Review commit: https://reviewboard.mozilla.org/r/40441/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40441/
Attachment #8731214 -
Flags: review?(ted)
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40443/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40443/
Attachment #8731215 -
Flags: review?(ted)
Comment 7•8 years ago
|
||
Comment on attachment 8731210 [details] MozReview Request: Bug 1257104 - Allow options with choices and possibly no given values https://reviewboard.mozilla.org/r/40433/#review37057
Attachment #8731210 -
Flags: review?(ted) → review+
Comment 8•8 years ago
|
||
Comment on attachment 8731211 [details] MozReview Request: Bug 1257104 - Move MOZ_APPLEMEDIA to moz.configure https://reviewboard.mozilla.org/r/40435/#review37059 ::: toolkit/moz.configure:154 (Diff revision 1) > +# Apple platform decoder support > +# ============================================================== > +@depends(toolkit) > +def applemedia(toolkit): > + if toolkit in ('cocoa', 'uikit'): > + set_config('MOZ_APPLEMEDIA', '1') Do you have any plans to migrate variables to be actual booleans at some point? That seems like it'd be nicer than "1" or "unset".
Attachment #8731211 -
Flags: review?(ted) → review+
Comment 9•8 years ago
|
||
Comment on attachment 8731212 [details] MozReview Request: Bug 1257104 - Move --disable-wmf to moz.configure https://reviewboard.mozilla.org/r/40437/#review37067 ::: toolkit/moz.configure:172 (Diff revision 1) > +def wmf(value, target): > + if value.origin == 'default': > + # Enable Windows Media Foundation support by default. > + # Note our minimum SDK version is Windows 7 SDK, so we are (currently) > + # guaranteed to have a recent-enough SDK to build WMF. > + value = target.os == 'WINNT' Reusing the variable in this way seems weird. ::: toolkit/moz.configure:173 (Diff revision 1) > + if value.origin == 'default': > + # Enable Windows Media Foundation support by default. > + # Note our minimum SDK version is Windows 7 SDK, so we are (currently) > + # guaranteed to have a recent-enough SDK to build WMF. > + value = target.os == 'WINNT' > + if value and target.os != 'WINNT': I wonder if this pattern is common enough to give special-handling to? "Option N is only valid for configuration X"
Attachment #8731212 -
Flags: review?(ted) → review+
Comment 10•8 years ago
|
||
Comment on attachment 8731213 [details] MozReview Request: Bug 1257104 - Move --disable-ffmpeg to moz.configure https://reviewboard.mozilla.org/r/40439/#review37069
Attachment #8731213 -
Flags: review?(ted) → review+
Updated•8 years ago
|
Attachment #8731214 -
Flags: review?(ted) → review+
Comment 11•8 years ago
|
||
Comment on attachment 8731214 [details] MozReview Request: Bug 1257104 - Move --disable-fmp4 to moz.configure https://reviewboard.mozilla.org/r/40441/#review37073 ::: toolkit/moz.configure:206 (Diff revision 1) > + help='Disable support for in built Fragmented MP4 parsing') > + > +@depends('--disable-fmp4', target, wmf, applemedia) > +def fmp4(value, target, wmf, applemedia): > + if value.origin == 'default': > + # target.os == 'Android' includes all B2G versions I can't tell if this changes the exact logic from configure, but I'm also not that concerned about breaking weird B2G versions. ::: toolkit/moz.configure:210 (Diff revision 1) > + if value.origin == 'default': > + # target.os == 'Android' includes all B2G versions > + value = wmf or applemedia or target.os == 'Android' > + if value: > + set_config('MOZ_FMP4', '1') > + set_define('MOZ_FMP4', '1') Feels like we should grow a combo function that does both of these since we use it all over the place.
Comment 12•8 years ago
|
||
https://reviewboard.mozilla.org/r/40441/#review37073 ::: toolkit/moz.configure:206 (Diff revision 1) > + help='Disable support for in built Fragmented MP4 parsing') > + > +@depends('--disable-fmp4', target, wmf, applemedia) > +def fmp4(value, target, wmf, applemedia): > + if value.origin == 'default': > + # target.os == 'Android' includes all B2G versions I can't tell if this changes the exact logic from configure, but I'm also not that concerned about breaking weird B2G versions. ::: toolkit/moz.configure:210 (Diff revision 1) > + if value.origin == 'default': > + # target.os == 'Android' includes all B2G versions > + value = wmf or applemedia or target.os == 'Android' > + if value: > + set_config('MOZ_FMP4', '1') > + set_define('MOZ_FMP4', '1') Feels like we should grow a combo function that does both of these since we use it all over the place.
Comment 13•8 years ago
|
||
Comment on attachment 8731215 [details] MozReview Request: Bug 1257104 - Move --enable-eme to moz.configure https://reviewboard.mozilla.org/r/40443/#review37113 ::: toolkit/moz.configure:233 (Diff revision 1) > + set_config('MOZ_EME', '1') > + set_define('MOZ_EME', '1') > + # Theoretically, we could pass `value` directly when it is a > + # PositiveOptionValue, but somehow, the JSON serialization in configure.py > + # outputs inconsistent data in some cases when we do # (a closing bracket > + # without an opening one). Should you file a bug on this? (Also you have an extraneous # in this comment. ::: toolkit/moz.configure:234 (Diff revision 1) > + set_define('MOZ_EME', '1') > + # Theoretically, we could pass `value` directly when it is a > + # PositiveOptionValue, but somehow, the JSON serialization in configure.py > + # outputs inconsistent data in some cases when we do # (a closing bracket > + # without an opening one). > + set_config('MOZ_EME_MODULES', [] if value in (True, False) else list(value)) This definitely reads strangely. Is there an `isinstance` or something else you could use here, like `list(value) if isinstance(value, X) else []`?
Attachment #8731215 -
Flags: review?(ted) → review+
Assignee | ||
Comment 14•8 years ago
|
||
https://reviewboard.mozilla.org/r/40437/#review37067 > I wonder if this pattern is common enough to give special-handling to? "Option N is only valid for configuration X" It's already possible, albeit cumbersome (so we'd need a template for it). The problem here is that target is not available to do that. But I'm thinking about lifting that restriction somehow.
Assignee | ||
Comment 15•8 years ago
|
||
https://reviewboard.mozilla.org/r/40441/#review37073 > I can't tell if this changes the exact logic from configure, but I'm also not that concerned about breaking weird B2G versions. MOZ_FMP4 was set in all the cases of ANDROID_VERSION. > Feels like we should grow a combo function that does both of these since we use it all over the place. That's actually tricky. set_define is a template defined in util.configure, and set_config is a function that is only available to @depends functions, and that is bound to a different instance of DependsOutput for each such function, so it can't be called from a template without being passed down to that template. A random thought of something ugly that would work, though, is something like: set_define('FOO', 'bar', also=set_config) which, despite the ugliness, actually has the advantage of still responding to grep set_define and grep set_config... Anyways, let's keep that for a possible followup.
Assignee | ||
Comment 16•8 years ago
|
||
https://reviewboard.mozilla.org/r/40443/#review37113 > Should you file a bug on this? (Also you have an extraneous # in this comment. Filed bug 1257347
Comment 17•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9689998f67d4 https://hg.mozilla.org/integration/mozilla-inbound/rev/732831f18167 https://hg.mozilla.org/integration/mozilla-inbound/rev/05815430d44b https://hg.mozilla.org/integration/mozilla-inbound/rev/a49ead1a3eee https://hg.mozilla.org/integration/mozilla-inbound/rev/9496c850df7f https://hg.mozilla.org/integration/mozilla-inbound/rev/3e0a7ab70f87
Assignee | ||
Comment 18•8 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #15) > Anyways, let's keep that for a possible followup. Filed bug 1257421
Comment 19•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9689998f67d4 https://hg.mozilla.org/mozilla-central/rev/732831f18167 https://hg.mozilla.org/mozilla-central/rev/05815430d44b https://hg.mozilla.org/mozilla-central/rev/a49ead1a3eee https://hg.mozilla.org/mozilla-central/rev/9496c850df7f https://hg.mozilla.org/mozilla-central/rev/3e0a7ab70f87
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•