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

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

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

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

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

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

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

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/

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

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

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 on attachment 8744498 [details] MozReview Request: Bug 1257326 - Move MOZ_PLACES to Python configure. https://reviewboard.mozilla.org/r/48577/#review45641

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.

And I now realize bug 1257958 did that too :(

(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).

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

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

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/

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

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

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

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

(In reply to Mike Hommey [:glandium] from comment #48) > So... I'm conflicted. This might deserve a thread on dev-builds.

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

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.

(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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 on attachment 8744499 [details] MozReview Request: Bug 1257326 - Move MOZ_SOCIAL to Python configure. https://reviewboard.mozilla.org/r/48579/#review48633

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 on attachment 8744502 [details] MozReview Request: Bug 1257326 - Move MOZ_SERVICES_HEALTHREPORT to Python configure. https://reviewboard.mozilla.org/r/48585/#review48637

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 on attachment 8744505 [details] MozReview Request: Bug 1257326 - Move MOZ_SERVICES_CLOUDSYNC to Python configure. https://reviewboard.mozilla.org/r/48591/#review48641

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

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

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

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

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

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

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.

(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).