Closed Bug 1257104 Opened 4 years ago Closed 4 years ago

Move --enable-eme and related to moz.configure

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

No description provided.
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)
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)
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)
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 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 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 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+
Attachment #8731214 - Flags: review?(ted) → review+
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.
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 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+
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.
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.
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
(In reply to Mike Hommey [:glandium] from comment #15)
> Anyways, let's keep that for a possible followup.

Filed bug 1257421
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.