Move android "feature flags" to Python configure

RESOLVED FIXED in Firefox 48

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: cmanchester, Assigned: cmanchester)

Tracking

(Blocks 1 bug)

unspecified
mozilla48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(11 attachments, 8 obsolete attachments)

58 bytes, text/x-review-board-request
glandium
: review+
Details
58 bytes, text/x-review-board-request
nalexander
: review+
Details
58 bytes, text/x-review-board-request
nalexander
: review+
Details
58 bytes, text/x-review-board-request
nalexander
: review+
Details
58 bytes, text/x-review-board-request
nalexander
: review+
Details
58 bytes, text/x-review-board-request
nalexander
: review+
Details
58 bytes, text/x-review-board-request
nalexander
: review+
Details
58 bytes, text/x-review-board-request
nalexander
: review+
Details
58 bytes, text/x-review-board-request
nalexander
: review+
Details
58 bytes, text/x-review-board-request
nalexander
: review+
Details
58 bytes, text/x-review-board-request
nalexander
: review+
Details
There are several, and I'm going to keep nalexander in the loop on these, so splitting from bug 1257326.
I found 6 variables along the way that are only checked in preprocessor directives, and are therefore not necessary to include in substs, so they are not included there as a result of this series. They are:

MOZ_ANDROID_APZ
MOZ_ANDROID_BEAM
MOZ_ANDROID_DOWNLOADS_INTEGRATION
MOZ_ANDROID_DOWNLOAD_CONTENT_SERVICE
MOZ_LOCALE_SWITCHER
MOZ_SWITCHBOARD
And do not port the subst, because its value is not used.

Review commit: https://reviewboard.mozilla.org/r/41121/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41121/
Attachment #8732352 - Flags: review?(mh+mozilla)
This does not port the subst, because its value is not used,
and removes MOZ_SWITCHBOARD from AppConstants.jsm, because its
value is not used.

Review commit: https://reviewboard.mozilla.org/r/41139/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41139/
Attachment #8732361 - Flags: review?(mh+mozilla)
Nick, can you glance over this patch series and confirm I've done something understandable and reasonable to work with for these flags? Thank you.
Flags: needinfo?(nalexander)
(In reply to Chris Manchester (:chmanchester) from comment #13)
> Nick, can you glance over this patch series and confirm I've done something
> understandable and reasonable to work with for these flags? Thank you.

Chris and I talked over Video.  From a scratch etherpad:  """My desired API looks like:

feature('MOZ_ANDROID_FOO', conditions.is_nightly || !conditions.is_release)

I don't care if we ever declare --enable-android-foo, we can do this all with mozconfig env var declarations if we choose to.

I want to subst everywhere, globally define nowhere (since changing global defines recompiles C code and is expensive), and locally add to DEFINES."""

Chris told me there was a temporary restriction that templates can't set_config, and that the restriction would likely be addressed within a working week.  We agreed to revisit this when we can get a shorter form for each feature flag definition, surfacing the milestone flags more explicitly.
Flags: needinfo?(nalexander)
For reference, bug 1257823 is the proposal that looks promising, and would allow set_config to happen from a template. I'll cancel review here based on our discussion and comment 14.
Attachment #8732352 - Flags: review?(mh+mozilla)
Attachment #8732353 - Flags: review?(mh+mozilla)
Attachment #8732354 - Flags: review?(mh+mozilla)
Attachment #8732355 - Flags: review?(mh+mozilla)
Attachment #8732356 - Flags: review?(mh+mozilla)
Attachment #8732357 - Flags: review?(mh+mozilla)
Attachment #8732358 - Flags: review?(mh+mozilla)
Attachment #8732359 - Flags: review?(mh+mozilla)
Attachment #8732360 - Flags: review?(mh+mozilla)
Attachment #8732361 - Flags: review?(mh+mozilla)
Attachment #8732362 - Flags: review?(mh+mozilla)
(In reply to Nick Alexander :nalexander from comment #14)
> (In reply to Chris Manchester (:chmanchester) from comment #13)
> > Nick, can you glance over this patch series and confirm I've done something
> > understandable and reasonable to work with for these flags? Thank you.
> 
> Chris and I talked over Video.  From a scratch etherpad:  """My desired API
> looks like:
> 
> feature('MOZ_ANDROID_FOO', conditions.is_nightly || !conditions.is_release)

Note this form cannot work.

What could work is something like:

feature('MOZ_ANDROID_FOO', milestone, lambda m: m.is_nightly || !m.is_release)

With some hack it /might/ be possible to do:

feature('MOZ_ANDROID_FOO', lambda milestone: milestone.is_nightly || !milestone.is_release)

But definitely not without a function being involved.
(In reply to Mike Hommey [:glandium] from comment #16)
> (In reply to Nick Alexander :nalexander from comment #14)
> > (In reply to Chris Manchester (:chmanchester) from comment #13)
> > > Nick, can you glance over this patch series and confirm I've done something
> > > understandable and reasonable to work with for these flags? Thank you.
> > 
> > Chris and I talked over Video.  From a scratch etherpad:  """My desired API
> > looks like:
> > 
> > feature('MOZ_ANDROID_FOO', conditions.is_nightly || !conditions.is_release)
> 
> Note this form cannot work.
> 
> What could work is something like:
> 
> feature('MOZ_ANDROID_FOO', milestone, lambda m: m.is_nightly ||
> !m.is_release)
> 
> With some hack it /might/ be possible to do:
> 
> feature('MOZ_ANDROID_FOO', lambda milestone: milestone.is_nightly ||
> !milestone.is_release)
> 
> But definitely not without a function being involved.

One can do things with conditions being a custom type that collects the expression tree before evaluation, but I agree, lambda is simpler.
https://reviewboard.mozilla.org/r/41141/#review37831

::: mobile/android/moz.configure:11
(Diff revision 1)
>  
> +option(env='MOZ_ANDROID_GCM',
> +       help='Enable GCM on Android.')
> +
> +@depends('MOZ_ANDROID_GCM', milestone)
> +def android_gsm(value, milestone):

gsm?
Attachment #8732353 - Attachment description: MozReview Request: Bug 1257958 - Add a template for defines that are unconditionally enabled. → MozReview Request: Bug 1257958 - Add a template for feature flags that depend on the current milestone.
Comment on attachment 8732353 [details]
MozReview Request: Bug 1257958 - Add a template to construct an environment-set option that sets the corresponding variable. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41123/diff/1-2/
Comment on attachment 8732359 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_EXCLUDE_FONTS to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41135/diff/1-2/
Comment on attachment 8732354 [details]
MozReview Request: Bug 1257958 - Move MOZ_LOCALE_SWITCHER to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41125/diff/1-2/
Attachment #8732352 - Attachment is obsolete: true
Attachment #8732355 - Attachment is obsolete: true
Attachment #8732356 - Attachment is obsolete: true
Attachment #8732357 - Attachment is obsolete: true
Attachment #8732358 - Attachment is obsolete: true
Attachment #8732360 - Attachment is obsolete: true
Attachment #8732361 - Attachment is obsolete: true
Attachment #8732362 - Attachment is obsolete: true
Nick, I have a new approach based on our discussion in the three commits starting with comment 19, can you sign off on this before we proceed? Thanks.
Flags: needinfo?(nalexander)
(In reply to Chris Manchester (:chmanchester) from comment #22)
> Nick, I have a new approach based on our discussion in the three commits
> starting with comment 19, can you sign off on this before we proceed? Thanks.

Yeah, this looks much better.  My only request is that we use more descriptive names, like:

feature_enabled_nowhere(...)
feature_enabled_everywhere(...)
feature_enabled_nightly_only(...)
feature_enabled_after_beta(...)

Otherwise, this looks good.  BTW, no need to break this into a bunch of small patches if it's work to do so.  A pre or two and then the meat works for me.
Flags: needinfo?(nalexander)
(In reply to Nick Alexander :nalexander from comment #23)
> (In reply to Chris Manchester (:chmanchester) from comment #22)
> > Nick, I have a new approach based on our discussion in the three commits
> > starting with comment 19, can you sign off on this before we proceed? Thanks.
> 
> Yeah, this looks much better.  My only request is that we use more
> descriptive names, like:
> 
> feature_enabled_nowhere(...)
> feature_enabled_everywhere(...)
> feature_enabled_nightly_only(...)
> feature_enabled_after_beta(...)

seems like it would be better to have predefined callbacks for the condition, and just call
flag('FOO', 'This is foo', nightly_only).

flag is not very descriptive, though.
(In reply to Mike Hommey [:glandium] from comment #24)
> (In reply to Nick Alexander :nalexander from comment #23)
> > (In reply to Chris Manchester (:chmanchester) from comment #22)
> > > Nick, I have a new approach based on our discussion in the three commits
> > > starting with comment 19, can you sign off on this before we proceed? Thanks.
> > 
> > Yeah, this looks much better.  My only request is that we use more
> > descriptive names, like:
> > 
> > feature_enabled_nowhere(...)
> > feature_enabled_everywhere(...)
> > feature_enabled_nightly_only(...)
> > feature_enabled_after_beta(...)
> 
> seems like it would be better to have predefined callbacks for the
> condition, and just call
> flag('FOO', 'This is foo', nightly_only).
> 
> flag is not very descriptive, though.

Fine by me.  I was trying to avoid things that *should* look like "nightly | aurora", given earlier discussion.
Attachment #8732353 - Attachment description: MozReview Request: Bug 1257958 - Add a template for feature flags that depend on the current milestone. → MozReview Request: Bug 1257958 - Add a template for feature flags that depend on the current milestone. r=glandium
Attachment #8732353 - Flags: review?(mh+mozilla)
Comment on attachment 8732353 [details]
MozReview Request: Bug 1257958 - Add a template to construct an environment-set option that sets the corresponding variable. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41123/diff/2-3/
Attachment #8732359 - Attachment description: MozReview Request: Bug 1257958 - Move MOZ_ANDROID_EXCLUDE_FONTS to Python configure. → MozReview Request: Bug 1257958 - Move MOZ_ANDROID_EXCLUDE_FONTS to Python configure. r=nalexander
Attachment #8732359 - Flags: review?(nalexander)
Comment on attachment 8732359 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_EXCLUDE_FONTS to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41135/diff/2-3/
Attachment #8732354 - Attachment description: MozReview Request: Bug 1257958 - Move MOZ_LOCALE_SWITCHER to Python configure. → MozReview Request: Bug 1257958 - Move MOZ_LOCALE_SWITCHER to Python configure. r=nalexander
Attachment #8732354 - Flags: review?(nalexander)
Comment on attachment 8732354 [details]
MozReview Request: Bug 1257958 - Move MOZ_LOCALE_SWITCHER to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41125/diff/2-3/
Attachment #8732359 - Flags: review?(nalexander) → review+
Comment on attachment 8732359 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_EXCLUDE_FONTS to Python configure. r=nalexander

https://reviewboard.mozilla.org/r/41135/#review38029

Very nice.

::: mobile/android/moz.configure:8
(Diff revision 3)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +conditional_feature('MOZ_ANDROID_EXCLUDE_FONTS',
> +                    'Whether to exclude font files in the build',

nit: ... font files *from* the build.
Comment on attachment 8732353 [details]
MozReview Request: Bug 1257958 - Add a template to construct an environment-set option that sets the corresponding variable. r=glandium

https://reviewboard.mozilla.org/r/41123/#review38031

wfm but glandium should have the final say.

::: build/moz.configure/init.configure:620
(Diff revision 3)
> +@template
> +def enabled_everywhere(_):
> +    return True
> +
> +@template
> +def enabled_in_nightly(milestone):

Let's define the major ones, and comment on how they map to the existing variables.
Attachment #8732353 - Flags: review+
Comment on attachment 8732354 [details]
MozReview Request: Bug 1257958 - Move MOZ_LOCALE_SWITCHER to Python configure. r=nalexander

https://reviewboard.mozilla.org/r/41125/#review38033

Looks good.

::: mobile/android/moz.configure:12
(Diff revision 3)
>  conditional_feature('MOZ_ANDROID_EXCLUDE_FONTS',
>                      'Whether to exclude font files in the build',
>                      enabled_in_nightly)
>  
> +conditional_feature('MOZ_LOCALE_SWITCHER',
> +                    'Enable runtime locale switching.',

nit: either drop this period, or add one above.  They should be consistent, and they should reflect whatever is the --help convention.
Attachment #8732354 - Flags: review?(nalexander) → review+
Comment on attachment 8732353 [details]
MozReview Request: Bug 1257958 - Add a template to construct an environment-set option that sets the corresponding variable. r=glandium

https://reviewboard.mozilla.org/r/41123/#review38411

::: build/moz.configure/init.configure:601
(Diff revision 3)
>                       is_nightly=is_nightly,
>                       is_release=is_release)
>  
> +# A template to add a "flag" (environment variable) whose default
> +# will be determined by the given condition. A condition is a callable
> +# that will receive the value of the current milestone as its only

Being limited to a milestone test seems fairly limited.

Instead, you could define enabled_in_nightly as:

@depends(milestone)
def enabled_in_nightly(milestone):
    return milestone.is_nightly

and @depends(condition) in the conditional_feature template.

That being said, set_config changes in bug 1257823  too (I'm waiting for try results to push it to mozreview)...
Attachment #8732353 - Flags: review?(mh+mozilla)
https://reviewboard.mozilla.org/r/41123/#review38411

> Being limited to a milestone test seems fairly limited.
> 
> Instead, you could define enabled_in_nightly as:
> 
> @depends(milestone)
> def enabled_in_nightly(milestone):
>     return milestone.is_nightly
> 
> and @depends(condition) in the conditional_feature template.
> 
> That being said, set_config changes in bug 1257823  too (I'm waiting for try results to push it to mozreview)...

Bug 1257823 is going to help this a lot, I'll re-work this once that lands.
(In reply to Mike Hommey [:glandium] from comment #32)
> @depends(milestone)
> def enabled_in_nightly(milestone):
>     return milestone.is_nightly

BTW, one-offs can also be done in one-liners:

template_name(..., depends(milestone)(lambda m: m.is_nightly))
(In reply to Chris Manchester (:chmanchester) from comment #33)
> https://reviewboard.mozilla.org/r/41123/#review38411
> 
> > Being limited to a milestone test seems fairly limited.
> > 
> > Instead, you could define enabled_in_nightly as:
> > 
> > @depends(milestone)
> > def enabled_in_nightly(milestone):
> >     return milestone.is_nightly
> > 
> > and @depends(condition) in the conditional_feature template.
> > 
> > That being said, set_config changes in bug 1257823  too (I'm waiting for try results to push it to mozreview)...
> 
> Bug 1257823 is going to help this a lot, I'll re-work this once that lands.

Actually I don't see a lot to change other than to address this issue. I was thinking I could just set_config('MOZ_ANDROID...', delayed_getattr( milestone, 'is_nightly')) and not even define an option for these, but that would give up the ability to set things with env variables.
Comment on attachment 8732353 [details]
MozReview Request: Bug 1257958 - Add a template to construct an environment-set option that sets the corresponding variable. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41123/diff/3-4/
Attachment #8732353 - Flags: review?(mh+mozilla)
Comment on attachment 8732359 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_EXCLUDE_FONTS to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41135/diff/3-4/
Comment on attachment 8732354 [details]
MozReview Request: Bug 1257958 - Move MOZ_LOCALE_SWITCHER to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41125/diff/3-4/
Comment on attachment 8732353 [details]
MozReview Request: Bug 1257958 - Add a template to construct an environment-set option that sets the corresponding variable. r=glandium

Unflagging myself, as the patch is outdated.
Attachment #8732353 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #39)
> Comment on attachment 8732353 [details]
> MozReview Request: Bug 1257958 - Add a template for feature flags that
> depend on the current milestone. r=glandium
> 
> Unflagging myself, as the patch is outdated.

Which part is outdated? I've been working with this on top of central, it applies cleanly and works as expected. The subsequent patches are out of date with respect to how they get defines to package-manifest.in, I'm updating that now.
Flags: needinfo?(mh+mozilla)
I see, the patches that just hit inbound outdate this.
Flags: needinfo?(mh+mozilla)
Bug 1257823 did, but bug 1260066 that just landed enforces it.
Review commit: https://reviewboard.mozilla.org/r/43251/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43251/
Attachment #8736466 - Flags: review?(nalexander)
Attachment #8736467 - Flags: review?(nalexander)
Attachment #8736468 - Flags: review?(nalexander)
Attachment #8736469 - Flags: review?(nalexander)
Attachment #8736470 - Flags: review?(nalexander)
Attachment #8736471 - Flags: review?(nalexander)
Attachment #8736472 - Flags: review?(nalexander)
Attachment #8736473 - Flags: review?(nalexander)
Attachment #8732353 - Flags: review?(mh+mozilla)
Comment on attachment 8732353 [details]
MozReview Request: Bug 1257958 - Add a template to construct an environment-set option that sets the corresponding variable. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41123/diff/4-5/
Comment on attachment 8732359 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_EXCLUDE_FONTS to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41135/diff/4-5/
Comment on attachment 8732354 [details]
MozReview Request: Bug 1257958 - Move MOZ_LOCALE_SWITCHER to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41125/diff/4-5/
Attachment #8732354 - Flags: review+ → review?(nalexander)
Attachment #8732359 - Flags: review+ → review?(nalexander)
Comment on attachment 8732353 [details]
MozReview Request: Bug 1257958 - Add a template to construct an environment-set option that sets the corresponding variable. r=glandium

https://reviewboard.mozilla.org/r/41123/#review40427

::: build/moz.configure/init.configure:689
(Diff revision 5)
>  set_config('RELEASE_BUILD', delayed_getattr(milestone, 'is_release'))
>  set_define('RELEASE_BUILD', delayed_getattr(milestone, 'is_release'))
>  add_old_configure_assignment('RELEASE_BUILD',
>                               delayed_getattr(milestone, 'is_release'))
>  
> +# A template to add a "flag" (environment variable) whose default

I think it's very important to highlight that this is `set_config` only.  It might be worth adding a note on how to use it in `moz.build`, and how to make it a define (since that's often wanted).  Even just a reference to another file will help.
Attachment #8732353 - Flags: review+
Comment on attachment 8736466 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_GCM to Python configure. r=nalexander

https://reviewboard.mozilla.org/r/43251/#review40429

::: mobile/android/installer/Makefile.in:44
(Diff revision 1)
>  
>  ifdef MOZ_ANDROID_EXCLUDE_FONTS
>  DEFINES += -DMOZ_ANDROID_EXCLUDE_FONTS=1
>  endif
>  
> +ifdef MOZ_ANDROID_GCM

Wish there was a way to make this happen automatically.

::: mobile/android/installer/Makefile.in:44
(Diff revision 1)
>  
>  ifdef MOZ_ANDROID_EXCLUDE_FONTS
>  DEFINES += -DMOZ_ANDROID_EXCLUDE_FONTS=1
>  endif
>  
> +ifdef MOZ_ANDROID_GCM

Wish there was a way to make this happen automatically.
Attachment #8736466 - Flags: review?(nalexander) → review+
Comment on attachment 8736467 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_DOWNLOADS_INTEGRATION to Python configure. r=nalexander

https://reviewboard.mozilla.org/r/43253/#review40431
Attachment #8736467 - Flags: review?(nalexander) → review+
Comment on attachment 8736468 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_BEAM to Python configure. r=nalexander

https://reviewboard.mozilla.org/r/43255/#review40433
Attachment #8736468 - Flags: review?(nalexander) → review+
Comment on attachment 8736469 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_SEARCH_ACTIVITY to Python configure. r=nalexander

https://reviewboard.mozilla.org/r/43257/#review40435
Attachment #8736469 - Flags: review?(nalexander) → review+
Comment on attachment 8736470 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_MLS_STUMBLER to Python configure. r=nalexander

https://reviewboard.mozilla.org/r/43259/#review40437
Attachment #8736470 - Flags: review?(nalexander) → review+
Comment on attachment 8736471 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_DOWNLOAD_CONTENT_SERVICE to Python configure. r=nalexander

https://reviewboard.mozilla.org/r/43261/#review40439
Attachment #8736471 - Flags: review?(nalexander) → review+
Comment on attachment 8736472 [details]
MozReview Request: Bug 1257958 - Move MOZ_SWITCHBOARD to Python configure. r=nalexander

https://reviewboard.mozilla.org/r/43263/#review40441

::: toolkit/modules/AppConstants.jsm
(Diff revision 1)
>    true,
>  #else
>    false,
>  #endif
>  
> -  MOZ_SWITCHBOARD:

I'm not against removing this, but long term I want `AppConstants.jsm` and `AppConstants.java` to be generated automatically (and include everything), so I'm not seeing the win.

Your call.
Attachment #8736472 - Flags: review?(nalexander) → review+
Attachment #8736473 - Flags: review?(nalexander) → review+
Comment on attachment 8736473 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_APZ to Python configure. r=nalexander

https://reviewboard.mozilla.org/r/43265/#review40443
Comment on attachment 8732359 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_EXCLUDE_FONTS to Python configure. r=nalexander

https://reviewboard.mozilla.org/r/41135/#review40445
Attachment #8732359 - Flags: review?(nalexander) → review+
Comment on attachment 8732354 [details]
MozReview Request: Bug 1257958 - Move MOZ_LOCALE_SWITCHER to Python configure. r=nalexander

https://reviewboard.mozilla.org/r/41125/#review40447
Attachment #8732354 - Flags: review?(nalexander) → review+
chmanchester -- nice series!  Sorry to make you wait on my reviews.
Status: NEW → ASSIGNED
Let's plan to email dev-builds and mobile-firefox-dev.  I can do this if you prefer.
https://reviewboard.mozilla.org/r/43251/#review40429

> Wish there was a way to make this happen automatically.

Right :/ According to glandium this is really the way to do this for now.
https://reviewboard.mozilla.org/r/43263/#review40441

> I'm not against removing this, but long term I want `AppConstants.jsm` and `AppConstants.java` to be generated automatically (and include everything), so I'm not seeing the win.
> 
> Your call.

Ok, makes sense. If this is remotely likely to be checked from js in the future I'm creating a footgun by removing this, so I'll leave it for now.
Comment on attachment 8732353 [details]
MozReview Request: Bug 1257958 - Add a template to construct an environment-set option that sets the corresponding variable. r=glandium

https://reviewboard.mozilla.org/r/41123/#review40535

::: build/moz.configure/init.configure:694
(Diff revision 5)
> +def conditional_feature(name, help, condition):
> +    opt = option(env=name, help=help)
> +
> +    @depends(opt.option, condition)
> +    def flag_implementation(value, condition):
> +        return value or (value.origin == 'default' and
> +                         condition)
> +
> +    set_config(name, flag_implementation)

You'd have more flexibility if you made the template return flag_implementation and let the caller do the set_config. That would make it two lines, but would allow to use set_define or add_old_configure_assignment as well.

The template itself should go in util.configure. The conditions, since they depend on milestone, need to go after milestone, unfortunately.

Also, the template can be used with conditions that have nothing to do with the milestone, so it feels restrictive to only comment about that.

::: build/moz.configure/init.configure:704
(Diff revision 5)
> +        return value or (value.origin == 'default' and
> +                         condition)
> +
> +    set_config(name, flag_implementation)
> +
> +@depends(milestone)

This could just depend on '--help', since it returns True unconditionally.
Attachment #8732353 - Flags: review?(mh+mozilla)
https://reviewboard.mozilla.org/r/41123/#review40535

> You'd have more flexibility if you made the template return flag_implementation and let the caller do the set_config. That would make it two lines, but would allow to use set_define or add_old_configure_assignment as well.
> 
> The template itself should go in util.configure. The conditions, since they depend on milestone, need to go after milestone, unfortunately.
> 
> Also, the template can be used with conditions that have nothing to do with the milestone, so it feels restrictive to only comment about that.

The other thing I had in my queue at one point was "set_as_define" and "set_for_old_configure" keyword args to conditional_feature, which is verbose but effective. I'm not really going to take a stand on keeping these down to one line, but maybe Nick can chime in on this.
(In reply to Chris Manchester (:chmanchester) from comment #70)
> https://reviewboard.mozilla.org/r/41123/#review40535
> 
> > You'd have more flexibility if you made the template return flag_implementation and let the caller do the set_config. That would make it two lines, but would allow to use set_define or add_old_configure_assignment as well.
> > 
> > The template itself should go in util.configure. The conditions, since they depend on milestone, need to go after milestone, unfortunately.
> > 
> > Also, the template can be used with conditions that have nothing to do with the milestone, so it feels restrictive to only comment about that.
> 
> The other thing I had in my queue at one point was "set_as_define" and
> "set_for_old_configure" keyword args to conditional_feature, which is
> verbose but effective. I'm not really going to take a stand on keeping these
> down to one line, but maybe Nick can chime in on this.

I don't care either way.  I think we should land something simple like this and re-jig it if we need to later.
(In reply to Nick Alexander :nalexander from comment #71)
> (In reply to Chris Manchester (:chmanchester) from comment #70)
> > https://reviewboard.mozilla.org/r/41123/#review40535
> > 
> > > You'd have more flexibility if you made the template return flag_implementation and let the caller do the set_config. That would make it two lines, but would allow to use set_define or add_old_configure_assignment as well.
> > > 
> > > The template itself should go in util.configure. The conditions, since they depend on milestone, need to go after milestone, unfortunately.
> > > 
> > > Also, the template can be used with conditions that have nothing to do with the milestone, so it feels restrictive to only comment about that.
> > 
> > The other thing I had in my queue at one point was "set_as_define" and
> > "set_for_old_configure" keyword args to conditional_feature, which is
> > verbose but effective. I'm not really going to take a stand on keeping these
> > down to one line, but maybe Nick can chime in on this.
> 
> I don't care either way.  I think we should land something simple like this
> and re-jig it if we need to later.

Ok, then I will implement Mike's suggestion.
Hm... so I can't:

exclude_fonts = conditional_feature('MOZ_ANDROID_EXCLUDE_FONTS',
                                    'Whether to exclude font files from the build',
                                    enabled_everywhere)

set_config(*exclude_fonts)

because:

 0:00.57 KeyError: u'Cannot assign `exclude_fonts` because it is neither a @depends nor a @template'
Comment on attachment 8732353 [details]
MozReview Request: Bug 1257958 - Add a template to construct an environment-set option that sets the corresponding variable. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41123/diff/5-6/
Attachment #8732353 - Attachment description: MozReview Request: Bug 1257958 - Add a template for feature flags that depend on the current milestone. r=glandium → MozReview Request: Bug 1257958 - Add a template to construct an environment-set option that sets the corresponding variable. r=glandium
Attachment #8732353 - Flags: review?(mh+mozilla)
Comment on attachment 8732359 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_EXCLUDE_FONTS to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41135/diff/5-6/
Comment on attachment 8732354 [details]
MozReview Request: Bug 1257958 - Move MOZ_LOCALE_SWITCHER to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41125/diff/5-6/
Comment on attachment 8736466 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_GCM to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43251/diff/1-2/
Comment on attachment 8736467 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_DOWNLOADS_INTEGRATION to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43253/diff/1-2/
Comment on attachment 8736468 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_BEAM to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43255/diff/1-2/
Comment on attachment 8736469 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_SEARCH_ACTIVITY to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43257/diff/1-2/
Comment on attachment 8736470 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_MLS_STUMBLER to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43259/diff/1-2/
Comment on attachment 8736471 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_DOWNLOAD_CONTENT_SERVICE to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43261/diff/1-2/
Comment on attachment 8736472 [details]
MozReview Request: Bug 1257958 - Move MOZ_SWITCHBOARD to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43263/diff/1-2/
Comment on attachment 8736473 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_APZ to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43265/diff/1-2/
Comment on attachment 8732353 [details]
MozReview Request: Bug 1257958 - Add a template to construct an environment-set option that sets the corresponding variable. r=glandium

https://reviewboard.mozilla.org/r/41123/#review44827

::: build/moz.configure/init.configure:689
(Diff revision 6)
> +# the corresponding variable in set_config, where the value of that variable
> +# passed to set_config only depends on the provided value and default.
> +# If required, the set_as_define and set_for_old_configure arguments
> +# will additionally cause the variable to be set using set_define and
> +# add_old_configure_assignment. util.configure would be an appropriate place for
> +# this, but it uses add_old_configure_assignment, which is defined in this file.

huh? add_old_configure_assignment is declared (and used) in init.configure.

::: build/moz.configure/init.configure:701
(Diff revision 6)
> +        if value:
> +            if len(value):
> +                return value

one of the upsides of NegativeOptionValue being mostly the same type as PositiveOptionValue (a tuple), and NegativeOptionValues always being empty, you can just write:
if len(value):
    return value

::: build/moz.configure/init.configure:714
(Diff revision 6)
> +@depends('--help')
> +def enabled_everywhere(_):
> +    return True

Why not just pass default=True?

::: build/moz.configure/init.configure:718
(Diff revision 6)
> +# milestone.is_nightly corresponds to cases NIGHTLY_BUILD is set.
> +@depends(milestone, '--help')
> +def enabled_in_nightly(milestone, _):
> +    return milestone.is_nightly

why not pass default=delayed_getattr(milestone, 'is_nightly') ? mmmm I can answer this one: that wouldn't work because of the missing required dependency on --help... On the long term, I think delayed_getattr() would be better than many ad-hoc functions, but because of the --help dependency, this will have to wait (I'm planning to lift the requirement for most --help dependencies, stay tuned)
Attachment #8732353 - Flags: review?(mh+mozilla)
https://reviewboard.mozilla.org/r/41123/#review44827

> huh? add_old_configure_assignment is declared (and used) in init.configure.

Err, I thought the comment was in old.configure. Forget it.
https://reviewboard.mozilla.org/r/41123/#review44827

> one of the upsides of NegativeOptionValue being mostly the same type as PositiveOptionValue (a tuple), and NegativeOptionValues always being empty, you can just write:
> if len(value):
>     return value

That didn't quite work, because PositiveOptionValue has a len of 0.
Comment on attachment 8732353 [details]
MozReview Request: Bug 1257958 - Add a template to construct an environment-set option that sets the corresponding variable. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41123/diff/6-7/
Attachment #8732353 - Flags: review?(mh+mozilla)
Comment on attachment 8732359 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_EXCLUDE_FONTS to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41135/diff/6-7/
Comment on attachment 8732354 [details]
MozReview Request: Bug 1257958 - Move MOZ_LOCALE_SWITCHER to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41125/diff/6-7/
Comment on attachment 8736466 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_GCM to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43251/diff/2-3/
Comment on attachment 8736467 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_DOWNLOADS_INTEGRATION to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43253/diff/2-3/
Comment on attachment 8736468 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_BEAM to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43255/diff/2-3/
Comment on attachment 8736469 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_SEARCH_ACTIVITY to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43257/diff/2-3/
Comment on attachment 8736470 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_MLS_STUMBLER to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43259/diff/2-3/
Comment on attachment 8736471 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_DOWNLOAD_CONTENT_SERVICE to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43261/diff/2-3/
Comment on attachment 8736472 [details]
MozReview Request: Bug 1257958 - Move MOZ_SWITCHBOARD to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43263/diff/2-3/
Comment on attachment 8736473 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_APZ to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43265/diff/2-3/
(In reply to Chris Manchester (:chmanchester) from comment #87)
> https://reviewboard.mozilla.org/r/41123/#review44827
> 
> > one of the upsides of NegativeOptionValue being mostly the same type as PositiveOptionValue (a tuple), and NegativeOptionValues always being empty, you can just write:
> > if len(value):
> >     return value
> 
> That didn't quite work, because PositiveOptionValue has a len of 0.

That's covered by the alternative path, that does return bool(value)
(In reply to Mike Hommey [:glandium] from comment #99)
> (In reply to Chris Manchester (:chmanchester) from comment #87)
> > https://reviewboard.mozilla.org/r/41123/#review44827
> > 
> > > one of the upsides of NegativeOptionValue being mostly the same type as PositiveOptionValue (a tuple), and NegativeOptionValues always being empty, you can just write:
> > > if len(value):
> > >     return value
> > 
> > That didn't quite work, because PositiveOptionValue has a len of 0.
> 
> That's covered by the alternative path, that does return bool(value)

Ah, I misunderstood the suggestion. But still, for a NegativeOptionValue we need to return None, not False, for set_define.
Oh, I missed the indentation on the last line.
Comment on attachment 8732353 [details]
MozReview Request: Bug 1257958 - Add a template to construct an environment-set option that sets the corresponding variable. r=glandium

https://reviewboard.mozilla.org/r/41123/#review45003
Attachment #8732353 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8732353 [details]
MozReview Request: Bug 1257958 - Add a template to construct an environment-set option that sets the corresponding variable. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41123/diff/7-8/
Attachment #8736466 - Attachment description: MozReview Request: Bug 1257958 - Move MOZ_ANDROID_GCM to Python configure. → MozReview Request: Bug 1257958 - Move MOZ_ANDROID_GCM to Python configure. r=nalexander
Attachment #8736467 - Attachment description: MozReview Request: Bug 1257958 - Move MOZ_ANDROID_DOWNLOADS_INTEGRATION to Python configure. → MozReview Request: Bug 1257958 - Move MOZ_ANDROID_DOWNLOADS_INTEGRATION to Python configure. r=nalexander
Attachment #8736468 - Attachment description: MozReview Request: Bug 1257958 - Move MOZ_ANDROID_BEAM to Python configure. → MozReview Request: Bug 1257958 - Move MOZ_ANDROID_BEAM to Python configure. r=nalexander
Attachment #8736469 - Attachment description: MozReview Request: Bug 1257958 - Move MOZ_ANDROID_SEARCH_ACTIVITY to Python configure. → MozReview Request: Bug 1257958 - Move MOZ_ANDROID_SEARCH_ACTIVITY to Python configure. r=nalexander
Attachment #8736470 - Attachment description: MozReview Request: Bug 1257958 - Move MOZ_ANDROID_MLS_STUMBLER to Python configure. → MozReview Request: Bug 1257958 - Move MOZ_ANDROID_MLS_STUMBLER to Python configure. r=nalexander
Attachment #8736471 - Attachment description: MozReview Request: Bug 1257958 - Move MOZ_ANDROID_DOWNLOAD_CONTENT_SERVICE to moz.build. → MozReview Request: Bug 1257958 - Move MOZ_ANDROID_DOWNLOAD_CONTENT_SERVICE to Python configure. r=nalexander
Attachment #8736472 - Attachment description: MozReview Request: Bug 1257958 - Move MOZ_SWITCHBOARD to Python configure. → MozReview Request: Bug 1257958 - Move MOZ_SWITCHBOARD to Python configure. r=nalexander
Attachment #8736473 - Attachment description: MozReview Request: Bug 1257958 - Move MOZ_ANDROID_APZ to Python configure. → MozReview Request: Bug 1257958 - Move MOZ_ANDROID_APZ to Python configure. r=nalexander
Comment on attachment 8732359 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_EXCLUDE_FONTS to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41135/diff/7-8/
Comment on attachment 8732354 [details]
MozReview Request: Bug 1257958 - Move MOZ_LOCALE_SWITCHER to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41125/diff/7-8/
Comment on attachment 8736466 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_GCM to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43251/diff/3-4/
Comment on attachment 8736467 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_DOWNLOADS_INTEGRATION to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43253/diff/3-4/
Comment on attachment 8736468 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_BEAM to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43255/diff/3-4/
Comment on attachment 8736469 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_SEARCH_ACTIVITY to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43257/diff/3-4/
Comment on attachment 8736470 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_MLS_STUMBLER to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43259/diff/3-4/
Comment on attachment 8736471 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_DOWNLOAD_CONTENT_SERVICE to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43261/diff/3-4/
Comment on attachment 8736472 [details]
MozReview Request: Bug 1257958 - Move MOZ_SWITCHBOARD to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43263/diff/3-4/
Comment on attachment 8736473 [details]
MozReview Request: Bug 1257958 - Move MOZ_ANDROID_APZ to Python configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43265/diff/3-4/
I'm going to carry forward the r+'s from Nick. The changes since them are cosmetic, with the exception of MOZ_ANDROID_APZ, whose default recently went from enabled in nightly to always on in the underlying confvars.sh.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.