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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1257104 - Allow options with choices and possibly no given values

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/

Comment 2

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1257104 - Move MOZ_APPLEMEDIA to moz.configure

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/

Comment 3

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1257104 - Move --disable-wmf to moz.configure

Review commit: https://reviewboard.mozilla.org/r/40437/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40437/

Comment 4

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1257104 - Move --disable-ffmpeg to moz.configure

Review commit: https://reviewboard.mozilla.org/r/40439/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40439/

Comment 5

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1257104 - Move --disable-fmp4 to moz.configure

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/

Comment 6

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1257104 - Move --enable-eme to moz.configure

Review commit: https://reviewboard.mozilla.org/r/40443/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40443/

Comment 7

[(not currently active) Ted Mielczarek]

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

Comment 8

[(not currently active) Ted Mielczarek]

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".

Comment 9

[(not currently active) Ted Mielczarek]

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"

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 8731213 [details]
MozReview Request: Bug 1257104 - Move --disable-ffmpeg to moz.configure

https://reviewboard.mozilla.org/r/40439/#review37069

Comment 11

[(not currently active) Ted Mielczarek]

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

[(not currently active) Ted Mielczarek]

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

[(not currently active) Ted Mielczarek]

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 []`?

Comment 14

[Mike Hommey [:glandium]]

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.

Comment 15

[Mike Hommey [:glandium]]

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.

Comment 16

[Mike Hommey [:glandium]]

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

[Pulsebot]

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

Comment 18

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #15)
> Anyways, let's keep that for a possible followup.

Filed bug 1257421

Comment 19

[Carsten Book [:Tomcat]]

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