1257326 - Move "feature flags" to Python configure

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

These are variables handled in old-configure that don't depend on few or no other variables, and few or no other variables depend on. Often they're just set in a convars.sh and AC_SUBST/DEFINED'd.

There's a group (android specific) starting with --enable-android-apz at https://dxr.mozilla.org/mozilla-central/rev/341344bdec8f10bf50646cd6ef2355361435cbf6/old-configure.in#4118

And another starting with MOZ_PLACES at https://dxr.mozilla.org/mozilla-central/rev/341344bdec8f10bf50646cd6ef2355361435cbf6/old-configure.in#7678

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1257326 - Provide reasons for implied options when the caller provides an immediate value.

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

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1257326 - Move MOZ_PLACES to Python configure.

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

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1257326 - Move MOZ_SOCIAL to Python configure.

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

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1257326 - Remove MOZ_SERVICES_COMMON. It is usually set, but never checked.

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

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1257326 - Remove MOZ_SERVICES_CRYPTO. It is usually set, but never checked.

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

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1257326 - Move MOZ_SERVICES_HEALTHREPORT to Python configure.

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

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1257326 - Remove MOZ_SERVICES_NOTIFICATIONS.

It is never set by default, and only triggers installation of a prefs file
that no longer exists if it is set.

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

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1257326 - Move MOZ_SERVICES_SYNC to Python configure.

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

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1257326 - Move MOZ_SERVICES_CLOUDSYNC to Python configure.

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

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8744497 [details]
MozReview Request: Bug 1257326 - Provide reasons for implied options when the caller provides an immediate value.

https://reviewboard.mozilla.org/r/48575/#review45639

::: build/moz.configure/init.configure:731
(Diff revision 1)
> +# Returns a condition that will evaluate to the opposite of
> +# disabled_for.

I'd put enabled_for first.

::: build/moz.configure/init.configure:734
(Diff revision 1)
> +    return condition_implementation
> +
> +# Returns a condition that will evaluate to the opposite of
> +# disabled_for.
> +@template
> +def enabled_for(enabled_projects):

*enabled_projects

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8744498 [details]
MozReview Request: Bug 1257326 - Move MOZ_PLACES to Python configure.

https://reviewboard.mozilla.org/r/48577/#review45641

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8744498 [details]
MozReview Request: Bug 1257326 - Move MOZ_PLACES to Python configure.

https://reviewboard.mozilla.org/r/48577/#review45645

Actually, let me retract this r+. MOZ_PLACES, and at least MOZ_SOCIAL that I've looked at introduce a behavior change that we'd rather avoid: they were exclusively per-application settings, and could not be changed from the command line, environment or mozconfig. Useing env_flag makes it so, and I don't think it's a good thing. We already have too many moving parts, it's not the right time to add even more.

Comment 13

[Mike Hommey [:glandium]]

And I now realize bug 1257958 did that too :(

Comment 14

[Chris Manchester (limited bugmail, email directly)]

(In reply to Mike Hommey [:glandium] from comment #12)
> Comment on attachment 8744498 [details]
> MozReview Request: Bug 1257326 - Move MOZ_PLACES to Python configure.
> 
> https://reviewboard.mozilla.org/r/48577/#review45645
> 
> Actually, let me retract this r+. MOZ_PLACES, and at least MOZ_SOCIAL that
> I've looked at introduce a behavior change that we'd rather avoid: they were
> exclusively per-application settings, and could not be changed from the
> command line, environment or mozconfig. Useing env_flag makes it so, and I
> don't think it's a good thing. We already have too many moving parts, it's
> not the right time to add even more.

That is a good point. I'll add a project_flag template that doesn't expose this (and convert those already moved in bug 1257958).

Comment 15

[Mike Hommey [:glandium]]

(In reply to Chris Manchester (:chmanchester) from comment #14)
> That is a good point. I'll add a project_flag template that doesn't expose
> this (and convert those already moved in bug 1257958).

I'm wondering if we shouldn't keep something similar-ish to confvars.sh, where we have a single place where all such project-specific settings would be grouped together.

A random thought is to add something like:

def project_flags():
    return namespace(
        MOZ_PLACES=True,
        MOZ_SOCIAL=True,
        ...
    )

to $project/moz.configure (maybe with a @depends('--help')), add an empty project_flags() in init.configure as a fallback, and have the right set_config/set_defines based on that, like, set_config('MOZ_PLACES', delayed_getattr(project_flags, 'MOZ_PLACES'), which then could be templated.

Comment 16

[Chris Manchester (limited bugmail, email directly)]

(In reply to Mike Hommey [:glandium] from comment #15)
> (In reply to Chris Manchester (:chmanchester) from comment #14)
> > That is a good point. I'll add a project_flag template that doesn't expose
> > this (and convert those already moved in bug 1257958).
> 
> I'm wondering if we shouldn't keep something similar-ish to confvars.sh,
> where we have a single place where all such project-specific settings would
> be grouped together.
> 
> A random thought is to add something like:
> 
> def project_flags():
>     return namespace(
>         MOZ_PLACES=True,
>         MOZ_SOCIAL=True,
>         ...
>     )
> 
> to $project/moz.configure (maybe with a @depends('--help')), add an empty
> project_flags() in init.configure as a fallback, and have the right
> set_config/set_defines based on that, like, set_config('MOZ_PLACES',
> delayed_getattr(project_flags, 'MOZ_PLACES'), which then could be templated.

I started with something like that in my queue, but got to the {enabled/disabled}_for templates because they make everything about any individual flag get set in a single place.

I have a simple patch that converts these to something that can't be set from a mozconfig or environment variable that otherwise preserves behavior. I'll post that and we can see if something similar to confvars.sh seems preferable.

Comment 17

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1257326 - Respect origins set by any caller of CommandLineHelper.add.

Origins will be set for any caller of CommandLineHelper.add, but will only
be propagated if args are added to extra_args. This results in an incorrect
origin recorded for mozconfig injected arguments.

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

Comment 18

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1257326 - Replace the env_flag configure template with a template that does not permit setting values directly from the environment or a mozconfig.

For most cases, this replaces a value that was set in a way that would ignore
an environment variable, so this restores behavior for values that were
set in confvars.sh.

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

Comment 19

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744497 [details]
MozReview Request: Bug 1257326 - Provide reasons for implied options when the caller provides an immediate value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48575/diff/1-2/

Comment 20

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744498 [details]
MozReview Request: Bug 1257326 - Move MOZ_PLACES to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48577/diff/1-2/

Comment 21

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744499 [details]
MozReview Request: Bug 1257326 - Move MOZ_SOCIAL to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48579/diff/1-2/

Comment 22

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744500 [details]
MozReview Request: Bug 1257326 - Remove MOZ_SERVICES_COMMON. It is usually set, but never checked.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48581/diff/1-2/

Comment 23

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744501 [details]
MozReview Request: Bug 1257326 - Remove MOZ_SERVICES_CRYPTO. It is usually set, but never checked.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48583/diff/1-2/

Comment 24

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744502 [details]
MozReview Request: Bug 1257326 - Move MOZ_SERVICES_HEALTHREPORT to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48585/diff/1-2/

Comment 25

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744503 [details]
MozReview Request: Bug 1257326 - Remove MOZ_SERVICES_NOTIFICATIONS.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48587/diff/1-2/

Comment 26

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744504 [details]
MozReview Request: Bug 1257326 - Move MOZ_SERVICES_SYNC to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48589/diff/1-2/

Comment 27

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744505 [details]
MozReview Request: Bug 1257326 - Move MOZ_SERVICES_CLOUDSYNC to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48591/diff/1-2/

Comment 28

[Mike Hommey [:glandium]]

Comment on attachment 8745651 [details]
MozReview Request: Bug 1257326 - Respect origins set by any caller of CommandLineHelper.add.

https://reviewboard.mozilla.org/r/49065/#review46177

Comment 29

[Mike Hommey [:glandium]]

Comment on attachment 8745652 [details]
MozReview Request: Bug 1257326 - Replace the env_flag configure template with a template that does not permit setting values directly from the environment or a mozconfig.

https://reviewboard.mozilla.org/r/49067/#review46179

::: build/moz.configure/init.configure:706
(Diff revision 1)
>  
>      opt = option(env=env, **kwargs)
>  
>      @depends(opt.option)
>      def option_implementation(value):
> +        if value.origin in ('environment', 'mozconfig'):

The rule of thumb is that if you want something that the user can't change, you don't use an option(). What you're testing here is not actually catching all cases where the users would set the option themselves (for instance, it's legit to pass "env" values through the command-line/ac_add_options (and will be the recommended way in the future)). Also, all option()s are listed in configure --help, and this is not desirable. Finally, the configure_error is confusing. What is project_option?

Stepping back a little, I can see confvars.sh-like using imply_option(), so it would still be desirable to have an option() for each of those. How about adding an argument to option() for a set of  allowed origins, which for those would be 'implied'? Then we can make configure --help hide the options that can only be implied.

Comment 30

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1257326 - Add a parameter to restrict accepted origins for an option in python configure.

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

Comment 31

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1257326 - Treat MOZ_ANDROID_APZ as a regular option rather than a flag.

A subsequent commit will replace env_flag and make it impossible to pass
--disable-android-apz, so this converts it to a reqular option for compatibility.

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

Comment 32

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8745651 [details]
MozReview Request: Bug 1257326 - Respect origins set by any caller of CommandLineHelper.add.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49065/diff/1-2/

Comment 33

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8745652 [details]
MozReview Request: Bug 1257326 - Replace the env_flag configure template with a template that does not permit setting values directly from the environment or a mozconfig.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49067/diff/1-2/

Comment 34

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744497 [details]
MozReview Request: Bug 1257326 - Provide reasons for implied options when the caller provides an immediate value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48575/diff/2-3/

Comment 35

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744498 [details]
MozReview Request: Bug 1257326 - Move MOZ_PLACES to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48577/diff/2-3/

Comment 36

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744499 [details]
MozReview Request: Bug 1257326 - Move MOZ_SOCIAL to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48579/diff/2-3/

Comment 37

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744500 [details]
MozReview Request: Bug 1257326 - Remove MOZ_SERVICES_COMMON. It is usually set, but never checked.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48581/diff/2-3/

Comment 38

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744501 [details]
MozReview Request: Bug 1257326 - Remove MOZ_SERVICES_CRYPTO. It is usually set, but never checked.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48583/diff/2-3/

Comment 39

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744502 [details]
MozReview Request: Bug 1257326 - Move MOZ_SERVICES_HEALTHREPORT to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48585/diff/2-3/

Comment 40

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744503 [details]
MozReview Request: Bug 1257326 - Remove MOZ_SERVICES_NOTIFICATIONS.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48587/diff/2-3/

Comment 41

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744504 [details]
MozReview Request: Bug 1257326 - Move MOZ_SERVICES_SYNC to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48589/diff/2-3/

Comment 42

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744505 [details]
MozReview Request: Bug 1257326 - Move MOZ_SERVICES_CLOUDSYNC to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48591/diff/2-3/

Comment 43

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/49065/#review46889

Comment 44

[Mike Hommey [:glandium]]

Comment on attachment 8747340 [details]
MozReview Request: Bug 1257326 - Add a parameter to restrict accepted origins for an option in python configure.

https://reviewboard.mozilla.org/r/49845/#review46891

::: python/mozbuild/mozbuild/configure/options.py:311
(Diff revision 1)
>              return self.default
>  
> +        if self.possible_origins and origin not in self.possible_origins:
> +            raise InvalidOptionError(
> +                '%s can not be set by %s. Values are accepted from: %s' %
> +                (option, origin, self.possible_origins))

you'll want something like ', '.join(self.possible_origins)

::: python/mozbuild/mozbuild/test/configure/test_options.py:164
(Diff revision 1)
>          self.assertEquals(option.nargs, '?')
>  
>          option = Option(env='FOO', default=('a', 'b'))
>          self.assertEquals(option.nargs, '*')
>  
> +

No need for an extra line here.

Comment 45

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/49845/#review46893

::: python/mozbuild/mozbuild/configure/options.py:119
(Diff revision 1)
>      - `default` can be used to give a default value to the option. When the
>        `name` of the option starts with '--enable-' or '--with-', the implied
>        default is an empty PositiveOptionValue. When it starts with '--disable-'
>        or '--without-', the implied default is a NegativeOptionValue.
>      - `choices` restricts the set of values that can be given to the option.
>      - `help` is the option description for use in the --help output.

Please add documentation for `possible_origins`.

Comment 46

[Mike Hommey [:glandium]]

Comment on attachment 8747341 [details]
MozReview Request: Bug 1257326 - Treat MOZ_ANDROID_APZ as a regular option rather than a flag.

https://reviewboard.mozilla.org/r/49847/#review46895

::: mobile/android/moz.configure:47
(Diff revision 1)
>  # usage of the framework.
>  env_flag('MOZ_SWITCHBOARD',
>           help='Include Switchboard A/B framework on Android',
>           default=True)
>  
> -env_flag('MOZ_ANDROID_APZ',
> +option(name='--disable-android-apz',

We normally don't explicitly put the argument name for `name`.

To make the change idempotent, you also want to add env='MOZ_ANDROID_APZ'.

::: mobile/android/moz.configure:50
(Diff revision 1)
> -         set_as_define=True,
> -         name='--enable-android-apz')
> +@depends('--disable-android-apz')
> +def android_apz(value):
> +    if value:
> +        return True
> +

you /could/ do:
android_apz = depends_if('--disable-android-apz')(lambda x: True)

Comment 47

[Mike Hommey [:glandium]]

Comment on attachment 8745652 [details]
MozReview Request: Bug 1257326 - Replace the env_flag configure template with a template that does not permit setting values directly from the environment or a mozconfig.

https://reviewboard.mozilla.org/r/49067/#review46897

::: build/moz.configure/init.configure:694
(Diff revision 2)
>  # 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.
>  @template
> -def env_flag(env=None, set_for_old_configure=False, set_as_define=False,
> +def project_flag(env=None, condition=None, set_for_old_configure=False,

Note, we'll want a way to list all those project_flags somehow. Would you mind filing a followup bug?

::: build/moz.configure/init.configure:703
(Diff revision 2)
> -        configure_error("An env_flag must be passed a variable name to set.")
> +        configure_error("A project_flag must be passed a variable name to set.")
> +    if not condition:
> +        configure_error("A project_flag must be passed a condition to derive a value.")
>  
> -    opt = option(env=env, **kwargs)
> +    opt = option(env=env, possible_origins=('implied',), **kwargs)
> +    imply_option(env, condition, reason='project_flag condition')

Mmmm I was more thinking about something like:
- you define project_flags in, say, toolkit/moz.configure.
- in $project/moz.configure, you add imply_option(flag, True)

You then end up with a block of imply_option() calls in $project/moz.configure which have the same kind of purpose as confvars.sh.

imply_option() will reject the form without a reason, but it probably shouldn't for immediate values.

Although... for the android-specific ones, it makes sense to do everything at once. Less so for things like MOZ_PLACES.

What do you think?

Comment 48

[Mike Hommey [:glandium]]

Comment on attachment 8744498 [details]
MozReview Request: Bug 1257326 - Move MOZ_PLACES to Python configure.

https://reviewboard.mozilla.org/r/48577/#review46899

::: toolkit/moz.configure:394
(Diff revision 3)
> +project_flag('MOZ_PLACES',
> +             help='Build Places if required',
> +             set_for_old_configure=True,
> +             set_as_define=True,
> +             condition=disabled_for('mobile/android', 'b2g'))

... although, this has a nice feel to it too: you look where MOZ_PLACES is defined, and you know exactly for what m-c projects it's enabled... and if I'm not mistaken, with the current implementation, third party projects (think c-c) can still use the imply_option('MOZ_PLACES', True) form in their own moz.configure (modulo the fact they'd need a reason at the moment)

So... I'm conflicted.

Comment 49

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #48)
> So... I'm conflicted.

This might deserve a thread on dev-builds.

Comment 50

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/49067/#review46901

::: build/moz.configure/init.configure:703
(Diff revision 2)
> -        configure_error("An env_flag must be passed a variable name to set.")
> +        configure_error("A project_flag must be passed a variable name to set.")
> +    if not condition:
> +        configure_error("A project_flag must be passed a condition to derive a value.")
>  
> -    opt = option(env=env, **kwargs)
> +    opt = option(env=env, possible_origins=('implied',), **kwargs)
> +    imply_option(env, condition, reason='project_flag condition')

BTW, does this really work? I'd expect imply_option to fail when it happens after the option. At least it's what it's supposed to do, but then, with the recent execution model changes, I might have broken some corner cases.

Comment 51

[Mike Hommey [:glandium]]

Comment on attachment 8744500 [details]
MozReview Request: Bug 1257326 - Remove MOZ_SERVICES_COMMON. It is usually set, but never checked.

https://reviewboard.mozilla.org/r/48581/#review46903

Comment 52

[Mike Hommey [:glandium]]

Comment on attachment 8744501 [details]
MozReview Request: Bug 1257326 - Remove MOZ_SERVICES_CRYPTO. It is usually set, but never checked.

https://reviewboard.mozilla.org/r/48583/#review46905

Comment 53

[Mike Hommey [:glandium]]

Comment on attachment 8744503 [details]
MozReview Request: Bug 1257326 - Remove MOZ_SERVICES_NOTIFICATIONS.

https://reviewboard.mozilla.org/r/48587/#review46907

::: browser/installer/package-manifest.in
(Diff revision 3)
>  ; gre location for now.
>  @RESPATH@/defaults/pref/channel-prefs.js
>  
>  ; Services (gre) prefs
> -#ifdef MOZ_SERVICES_NOTIFICATIONS
> -@RESPATH@/defaults/pref/services-notifications.js

heh, the file is not even in the tree anymore.

Comment 54

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/49067/#review46901

> BTW, does this really work? I'd expect imply_option to fail when it happens after the option. At least it's what it's supposed to do, but then, with the recent execution model changes, I might have broken some corner cases.

It does. It looks like the list of implied options is populated as soon as we execute the file, so they'll be available when we call value_for.

Comment 55

[Chris Manchester (limited bugmail, email directly)]

(In reply to Mike Hommey [:glandium] from comment #48)
> Comment on attachment 8744498 [details]
> MozReview Request: Bug 1257326 - Move MOZ_PLACES to Python configure.
> 
> https://reviewboard.mozilla.org/r/48577/#review46899
> 
> ::: toolkit/moz.configure:394
> (Diff revision 3)
> > +project_flag('MOZ_PLACES',
> > +             help='Build Places if required',
> > +             set_for_old_configure=True,
> > +             set_as_define=True,
> > +             condition=disabled_for('mobile/android', 'b2g'))
> 
> ... although, this has a nice feel to it too: you look where MOZ_PLACES is
> defined, and you know exactly for what m-c projects it's enabled... and if
> I'm not mistaken, with the current implementation, third party projects
> (think c-c) can still use the imply_option('MOZ_PLACES', True) form in their
> own moz.configure (modulo the fact they'd need a reason at the moment)
> 
> So... I'm conflicted.

I'm in favor of this, for the reason mentioned, that you only have to look in one place to understand most things about the option. imply_option from a moz.configure doesn't quite work with this as is, but with a small change it will. This could make the distinction less important, because both setting something in an individual moz.configure or setting it centrally with defaults per project will be possible.

Various people will probably be tweaking these over time, and it changes a long-standing behavior, so I'll post a quick rfc to dev-builds.

Comment 56

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8745651 [details]
MozReview Request: Bug 1257326 - Respect origins set by any caller of CommandLineHelper.add.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49065/diff/2-3/

Comment 57

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8747340 [details]
MozReview Request: Bug 1257326 - Add a parameter to restrict accepted origins for an option in python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49845/diff/1-2/

Comment 58

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8747341 [details]
MozReview Request: Bug 1257326 - Treat MOZ_ANDROID_APZ as a regular option rather than a flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49847/diff/1-2/

Comment 59

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8745652 [details]
MozReview Request: Bug 1257326 - Replace the env_flag configure template with a template that does not permit setting values directly from the environment or a mozconfig.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49067/diff/2-3/

Comment 60

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744497 [details]
MozReview Request: Bug 1257326 - Provide reasons for implied options when the caller provides an immediate value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48575/diff/3-4/

Comment 61

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744498 [details]
MozReview Request: Bug 1257326 - Move MOZ_PLACES to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48577/diff/3-4/

Comment 62

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744499 [details]
MozReview Request: Bug 1257326 - Move MOZ_SOCIAL to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48579/diff/3-4/

Comment 63

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744500 [details]
MozReview Request: Bug 1257326 - Remove MOZ_SERVICES_COMMON. It is usually set, but never checked.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48581/diff/3-4/

Comment 64

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744501 [details]
MozReview Request: Bug 1257326 - Remove MOZ_SERVICES_CRYPTO. It is usually set, but never checked.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48583/diff/3-4/

Comment 65

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744502 [details]
MozReview Request: Bug 1257326 - Move MOZ_SERVICES_HEALTHREPORT to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48585/diff/3-4/

Comment 66

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744503 [details]
MozReview Request: Bug 1257326 - Remove MOZ_SERVICES_NOTIFICATIONS.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48587/diff/3-4/

Comment 67

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744504 [details]
MozReview Request: Bug 1257326 - Move MOZ_SERVICES_SYNC to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48589/diff/3-4/

Comment 68

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744505 [details]
MozReview Request: Bug 1257326 - Move MOZ_SERVICES_CLOUDSYNC to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48591/diff/3-4/

Comment 69

[Mike Hommey [:glandium]]

Comment on attachment 8745652 [details]
MozReview Request: Bug 1257326 - Replace the env_flag configure template with a template that does not permit setting values directly from the environment or a mozconfig.

https://reviewboard.mozilla.org/r/49067/#review48625

::: mobile/android/moz.configure:7
(Diff revision 3)
> -env_flag('MOZ_ANDROID_EXCLUDE_FONTS',
> +project_flag('MOZ_ANDROID_EXCLUDE_FONTS',
> -         help='Whether to exclude font files from the build',
> +             help='Whether to exclude font files from the build',
> -         default=True)
> +             default=True)

The weird thing about the ones in this file is that you could just as well do

# Whether to exclude font files from the build
set_config('MOZ_ANDROID_EXCLUDE_FONTS', True)

# Enable runtime locale switching
set_config('MOZ_LOCALE_SWITCHER', True)

# Enable GCM registration on Nightly builds only
set_config('MOZ_ANDROID_GCM', enabled_in_nightly)
add_old_configure_assignment('MOZ_ANDROID_GCM', enabled_in_nightly)

and so on...

Comment 70

[Mike Hommey [:glandium]]

Comment on attachment 8744497 [details]
MozReview Request: Bug 1257326 - Provide reasons for implied options when the caller provides an immediate value.

https://reviewboard.mozilla.org/r/48575/#review48627

::: python/mozbuild/mozbuild/configure/__init__.py:348
(Diff revision 4)
>  
>          try:
>              value, option_string = self._helper.handle(option)
>          except ConflictingOptionError as e:
>              reason = implied[e.arg].reason
> +            if not isinstance(reason, types.StringTypes):

You could just as well do if isinstance(reason, Option)

::: python/mozbuild/mozbuild/test/configure/test_configure.py:595
(Diff revision 4)
> +
> +        config = get_config([])
> +        self.assertEquals(config, {})
> +
> +        with self.assertRaisesRegexp(InvalidOptionError,
> +            "--enable-foo' implied by 'imply_option at .+imm.configure:7'"

The .+imm.configure looks weird.

Comment 71

[Mike Hommey [:glandium]]

Comment on attachment 8744498 [details]
MozReview Request: Bug 1257326 - Move MOZ_PLACES to Python configure.

https://reviewboard.mozilla.org/r/48577/#review48631

::: toolkit/moz.configure:398
(Diff revision 4)
> +project_flag('MOZ_PLACES',
> +             help='Build Places if required',
> +             set_for_old_configure=True,
> +             set_as_define=True)

If you move MOZ_ANDROID_HISTORY to mobile/android/moz.configure, you can get rid of the set_for_old_configure :)

Comment 72

[Mike Hommey [:glandium]]

Comment on attachment 8744499 [details]
MozReview Request: Bug 1257326 - Move MOZ_SOCIAL to Python configure.

https://reviewboard.mozilla.org/r/48579/#review48633

Comment 73

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/48579/#review48635

::: toolkit/moz.configure:405
(Diff revision 4)
>               set_for_old_configure=True,
>               set_as_define=True)
>  
> +project_flag('MOZ_SOCIAL',
> +             help='Build SocialAPI if required',
> +             default=True)

Come to think of it, it would be worth asking if the default is really meant to be enabled. It's something in toolkit with a bugzilla component under Firefox... that smells fishy. I bet nothing besides Firefox (maybe b2g) actually wants that enabled. Followup?

Comment 74

[Mike Hommey [:glandium]]

Comment on attachment 8744502 [details]
MozReview Request: Bug 1257326 - Move MOZ_SERVICES_HEALTHREPORT to Python configure.

https://reviewboard.mozilla.org/r/48585/#review48637

Comment 75

[Mike Hommey [:glandium]]

Comment on attachment 8744504 [details]
MozReview Request: Bug 1257326 - Move MOZ_SERVICES_SYNC to Python configure.

https://reviewboard.mozilla.org/r/48589/#review48639

::: toolkit/moz.configure:413
(Diff revision 4)
>               help='Build Firefox Health Reporter Service',
>               set_for_old_configure=True,
>               set_as_define=True)
>  
> +project_flag('MOZ_SERVICES_SYNC',
> +             help='Build Sync Services if required')

We can remove the #ifdef MOZ_SERVICES_SYNC sections in b2g/installer/package-manifest.in. AFAICS, MOZ_SERVICES_SYNC has never been enabled on b2g, and they were cargo cult from browser/installer/package-manifest.in.

Comment 76

[Mike Hommey [:glandium]]

Comment on attachment 8744505 [details]
MozReview Request: Bug 1257326 - Move MOZ_SERVICES_CLOUDSYNC to Python configure.

https://reviewboard.mozilla.org/r/48591/#review48641

Comment 77

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/49067/#review46897

> Note, we'll want a way to list all those project_flags somehow. Would you mind filing a followup bug?

Filed bug 1272081

Comment 78

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/48579/#review48635

> Come to think of it, it would be worth asking if the default is really meant to be enabled. It's something in toolkit with a bugzilla component under Firefox... that smells fishy. I bet nothing besides Firefox (maybe b2g) actually wants that enabled. Followup?

Filed bug 1272086

Comment 79

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a409c224d21e
https://hg.mozilla.org/integration/mozilla-inbound/rev/65337ddcc2b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6790ff59d49
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4dc16a12cb1
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5251e65d5f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/82e8c5f0da49
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b16825bf53c
https://hg.mozilla.org/integration/mozilla-inbound/rev/86c89e604e9b
https://hg.mozilla.org/integration/mozilla-inbound/rev/67abfc6b74a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b74c0699042
https://hg.mozilla.org/integration/mozilla-inbound/rev/282619c208b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d4213512f1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/d09963518405

Comment 80

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a409c224d21e
https://hg.mozilla.org/mozilla-central/rev/65337ddcc2b4
https://hg.mozilla.org/mozilla-central/rev/f6790ff59d49
https://hg.mozilla.org/mozilla-central/rev/d4dc16a12cb1
https://hg.mozilla.org/mozilla-central/rev/e5251e65d5f0
https://hg.mozilla.org/mozilla-central/rev/82e8c5f0da49
https://hg.mozilla.org/mozilla-central/rev/9b16825bf53c
https://hg.mozilla.org/mozilla-central/rev/86c89e604e9b
https://hg.mozilla.org/mozilla-central/rev/67abfc6b74a2
https://hg.mozilla.org/mozilla-central/rev/5b74c0699042
https://hg.mozilla.org/mozilla-central/rev/282619c208b6
https://hg.mozilla.org/mozilla-central/rev/5d4213512f1a
https://hg.mozilla.org/mozilla-central/rev/d09963518405

Comment 81

[aleth [:aleth]]

Just checking: Did you intend to change the default value for MOZ_PLACES to false?

Comment 82

[Chris Manchester (limited bugmail, email directly)]

(In reply to aleth [:aleth] from comment #81)
> Just checking: Did you intend to change the default value for MOZ_PLACES to
> false?

Yes. Turning something on by default but off in several other places seems like a pattern we want to move away from.

Comment 83

[Frank-Rainer Grahl (:frg)]

Ist it possible that mozilla\toolkit\moz.configure needs an additonal fix?

These defines are missing from mozilla-config.h in a suite build after Philip Chee ported the changes to comm-central:

>> #define MOZ_SERVICES_CLOUDSYNC 1
>> #define MOZ_SERVICES_SYNC 1
>> #define MOZ_SOCIAL 1

Setting 
>> set_for_old_configure=True, 
>> set_as_define=True,

for each of them seems to make it work again. I didn't try to build Firefox yet.

Comment 84

[Chris Manchester (limited bugmail, email directly)]

(In reply to Frank-Rainer Grahl from comment #83)
> Ist it possible that mozilla\toolkit\moz.configure needs an additonal fix?
> 
> These defines are missing from mozilla-config.h in a suite build after
> Philip Chee ported the changes to comm-central:
> 
> >> #define MOZ_SERVICES_CLOUDSYNC 1
> >> #define MOZ_SERVICES_SYNC 1
> >> #define MOZ_SOCIAL 1
> 
> Setting 
> >> set_for_old_configure=True, 
> >> set_as_define=True,
> 
> for each of them seems to make it work again. I didn't try to build Firefox
> yet.

This is intentional, because these aren't being checked any place in the tree that is impacted by the #define (except in the case of MOZ_SERVICES_CLOUDSYNC, where we were able to move the define to toolkit/modules/moz.build).