1257823 - Move set_config, set_define and imply_option to the global scope

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

I just had this idea, and I think it has advantages over the current approach: Move set_config, set_define and imply_option to the global scope.

In practice, this is what it would look like for set_config:

diff --git a/toolkit/moz.configure b/toolkit/moz.configure
index fa61720..63cddfa 100644
--- a/toolkit/moz.configure
+++ b/toolkit/moz.configure
@@ -261,20 +261,20 @@ option('--disable-ffmpeg',
 
 @depends('--disable-ffmpeg', target)
 def ffmpeg(value, target):
     enabled = bool(value)
     if value.origin == 'default':
         enabled = target.os not in ('Android', 'WINNT')
     if enabled:
         set_define('MOZ_FFMPEG', '1')
-        set_config('MOZ_FFMPEG', '1')
         imply_option('--enable-fmp4', '--enable-ffmpeg')
     return enabled
 
+set_config('MOZ_FFMPEG', ffmpeg)
 
 # Built-in fragmented MP4 support.
 # ==============================================================
 option('--disable-fmp4', env='MOZ_FMP4',
        help='Disable support for in built Fragmented MP4 parsing')
 
 @depends('--disable-fmp4', target, wmf, applemedia)
 def fmp4(value, target, wmf, applemedia):

The result from the ffmpeg function would be fed to the config in this case. Well, since we currently don't support booleans in the config, we'd have to normalize False to an empty string and True to '1' as a beginning.

Implemented as sandboxed code, this would looks like this:

@template
def set_config(name, func):
    @depends(func)
    def wrapper(value):
        # Until booleans are supported
        if value is True:
            set_config(name, '1')
        elif value is False:
            set_config(name, '')
        elif value:
            set_config(name, value)

(allows to test the above patch)

This would, however, *not* be implemented as sandboxed code, but would be directly part of the sandbox and would remove set_config availability from within functions.

The main advantage is that this would allow to know all the configs without looking at the AST (modulo things that already affect options, cf. bug 1256571). It would also be possible to tell which set of options are involved in setting those configs (for the MOZ_FFMPEG case, we'd be able to tell that --enable/disable-ffmpeg, --target, --host and MOZILLABUILD have an influence on it (although for MOZILLABUILD, it's a sad implementation detail))

The same could be done for set_define and imply_option and have similar advantages for each.

This would also remove the special case where a @depends function returns nothing (or None) where we currently return a dict of all the set_config used in the function, which can be considered weird (I wouldn't disagree).

It would also make bug 1257421 easier to implement because both set_config and set_define would be at the same level, and we could trivially add a util function grouping both.

I'm tempted to also change the syntax to be CONFIG['FOO'] = foo (and DEFINES[...]) instead of set_config('FOO', foo) (and set_define(...)), but with foo being a function, that kind of feels weirder; the advantage is that this would make things clearer wrt moz.build.

Thoughts?

Comment 1

[(not currently active) Ted Mielczarek]

I actually really like this idea. We should sort out some specifics, but it has some nice qualities:
1) We can easily introspect all of them
2) We can hang documentation for config variables off of it
3) Having these at the top-level makes it easier to make the ergonomics nicer, like the set_config_and_define idea.

Comment 2

[Chris Manchester (limited bugmail, email directly)]

As I understand it this unblocks putting set_config in a template, which would be very helpful. It's a little weird we end up with a syntax with three elements at the top level (the option(<name>, ...), the @depends(<name>, ...) function, and the set_config), so if I were coming to this for the first time I'd have some mundane questions that might bother me, like whether order matters, how many newlines to put between things, and why I need to repeat the name in the case it's the same for all three.

So it would be helpful to tame some of that with templates. A shorthand that means if you have a option(--{enable,disable}-<name>, ...), it's always responsible for setting/unsetting MOZ_<NAME>, and nothing else, would cover a lot.

Comment 3

[Mike Hommey [:glandium]]

As part of doing this, I actually figured a way to make things work in /some/ way close to what Nick wanted in bug 1257958, but I'm not entirely sure if it's going to be more confusing than not.

So, what this would look like is something like:

set_config('GRE_MILESTONE', milestone.version)

(and yes, this would be properly delayed)

Under the hood, what happens is that the DummyFunction that milestone is (from being a @depends function) has a __getattr__ method that essentially returns this:

  @depends(milestone)
  @advanced  # for getattr
  def result(value):
    return getattr(value, key)


Initially, I implemented a template doing that, but I figured just using milestone.version instead of some_func(milestone, 'version') would be more straightforward (and originally, I had named some_func getattr... with a fallback to the real getattr if the given object was not a DummyFunction)

Now, there are caveats to this:
- it can lead people to believe that's an actual value there, when it's not, so things like `if milestone.version == 'foo'` would not do what people think it would (although the same could probably be said from some_func(milestone, 'version')).
- it doesn't work for more complex things (like milestone.is_release and not milestone.is_nightly), although it might be possible to make it work by adding even more __functions__ to DummyFunction.

So the quesiton here is whether we should go with this or not. It sure is convenient. I wonder if it's not going to be too confusing.

Comment 4

[Nick Alexander :nalexander [he/him]]

> So the quesiton here is whether we should go with this or not. It sure is
> convenient. I wonder if it's not going to be too confusing.

It's nice syntax, but I agree that the evaluation model is hard to describe.  I'd like to see us convert more configure.in to moz.configure before we bake in this sugar.  Perhaps we won't need it; perhaps we'll find something a little different, a little simpler.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Typing while Nick responded...

For now it seems too confusing, I would be very tempted to try to use that as an actual value. A function as the second argument to set_config, while more verbose, is much easier to explain.

Comment 6

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1257823 - Move set_define() to the sandbox

In order to make the transition to global set_define easier, move its
current definition from inside the sandbox to the sandbox itself.

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

Comment 7

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1257823 - Split ConfigureSandbox._db

Currently, ConfigureSandbox._db stores two different kind of
information. This split those in two different instance variables
instead, making things clearer.

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

Comment 8

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1257823 - Move set_config() to the global scope

The way set_config is set currently makes it difficult to introspect
moz.configure files to know what configuration items are being set,
because they're hidden in the control flow of functions.

This makes some of the moz.configure more convoluted, but this is why
there are templates, and we can improve the recurring cases afterwards.

is not how this would land. This would land folded with the next two
commits. This was split to make the review easier.
In this whole series, we can see some emerging patterns, I'm ignoring
them on purpose here. We can deal with templating them in followups, cf.
paragraph above.

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

Comment 9

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1257823 - Move set_config() from @depends to global scope

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

Comment 10

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1257823 - Remove set_config implementation for @depends functions

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

Comment 11

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1257823 - Move set_define() to the global scope

For the same reasons as set_config is being moved to the global scope,
we're moving set_define to the global scope here. An additional change
is that set_define is now part of the sandbox itself instead of being
defined within the sandbox, which makes it share the implementation
details with set_config.

first.

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

Comment 12

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1257823 - Move set_define() from @depends to global scope

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

Comment 13

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1257823 - Remove set_define implementation for @depends functions

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

Comment 14

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1257823 - Keep track of the dependencies of @depends functions

@depends functions are declared like the following:

  @depends('--option', other_function, '--other-option', ...)
  def ...

To simplify some of the processing related to those arguments it's
passed, keep a tuple of Option and DummyFunction objects corresponding
to those arguments.

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

Comment 15

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1257823 - Move imply_option() to the global scope

Like set_config and set_define, we move imply_option to the global
scope.

2, the intermediate steps still work, in this case, it's possible the
intermediate steps are semi broken, with error handling not working as
expected. The unit tests pass, so I'd say it's fine, considering the
code that I think is semi broken goes away in 2 commits. Plus, the next
2 commits will be folded with this one...

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

Comment 16

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1257823 - Move imply_option() from @depends to global scope

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

Comment 17

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1257823 - Remove imply_option implementation for @depends functions

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

Comment 18

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1257823 - Move add_old_configure_assignment() to the global scope

Similar to set_config, set_define and imply_option, but this is a
sandboxed function that stays sandboxed.

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

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 8733800 [details]
MozReview Request: Bug 1257823 - Move set_define() to the sandbox

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

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 8733801 [details]
MozReview Request: Bug 1257823 - Split ConfigureSandbox._db

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

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8733802 [details]
MozReview Request: Bug 1257823 - Move set_config() to the global scope

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

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 8733803 [details]
MozReview Request: Bug 1257823 - Move set_config() from @depends to global scope

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

Comment 23

[Mike Hommey [:glandium]]

Comment on attachment 8733804 [details]
MozReview Request: Bug 1257823 - Remove set_config implementation for @depends functions

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

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8733805 [details]
MozReview Request: Bug 1257823 - Move set_define() to the global scope

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

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 8733806 [details]
MozReview Request: Bug 1257823 - Move set_define() from @depends to global scope

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

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 8733807 [details]
MozReview Request: Bug 1257823 - Remove set_define implementation for @depends functions

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

Comment 27

[Mike Hommey [:glandium]]

Comment on attachment 8733808 [details]
MozReview Request: Bug 1257823 - Keep track of the dependencies of @depends functions

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

Comment 28

[Mike Hommey [:glandium]]

Comment on attachment 8733809 [details]
MozReview Request: Bug 1257823 - Move imply_option() to the global scope

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

Comment 29

[Mike Hommey [:glandium]]

Comment on attachment 8733810 [details]
MozReview Request: Bug 1257823 - Move imply_option() from @depends to global scope

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

Comment 30

[Mike Hommey [:glandium]]

Comment on attachment 8733811 [details]
MozReview Request: Bug 1257823 - Remove imply_option implementation for @depends functions

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

Comment 31

[Mike Hommey [:glandium]]

Comment on attachment 8733812 [details]
MozReview Request: Bug 1257823 - Move add_old_configure_assignment() to the global scope

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

Comment 32

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8733800 [details]
MozReview Request: Bug 1257823 - Move set_define() to the sandbox

https://reviewboard.mozilla.org/r/41953/#review38529

Nice to see how easily something passes the barrier into the sandbox.

Comment 33

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8733801 [details]
MozReview Request: Bug 1257823 - Split ConfigureSandbox._db

https://reviewboard.mozilla.org/r/41955/#review38531

::: python/mozbuild/mozbuild/configure/__init__.py:128
(Diff revision 2)
>          self._options = OrderedDict()
>          # Store the raw values returned by @depends functions
>          self._results = {}
> -        # Store several kind of information:
> -        # - value for each Option, as per returned by Option.get_value
> -        # - raw option (as per command line or environment) for each value
> +        # Store values for each Option, as per returned by Option.get_value
> +        self._option_values = {}
> +        # Store raw option (as per command line or environment) for each Option

Is there a reason you key by `Option` rather than by value?  This seems to just add a little processing at retrieval time.

Perhaps this becomes clearer with additional use.

Comment 34

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8733802 [details]
MozReview Request: Bug 1257823 - Move set_config() to the global scope

https://reviewboard.mozilla.org/r/41957/#review38533

::: python/mozbuild/mozbuild/configure/__init__.py:434
(Diff revision 2)
>          glob.update(__builtins__=__builtins__)
>          return func
>  
> +    def set_config_impl(self, name, value):
> +        '''Implementation of set_config().
> +        Sets a configuration items with the given name to the given value.

nit: *Set* a configuration item.

Consider "Set *the* ...".

::: python/mozbuild/mozbuild/configure/__init__.py:437
(Diff revision 2)
> +    def set_config_impl(self, name, value):
> +        '''Implementation of set_config().
> +        Sets a configuration items with the given name to the given value.
> +        Both `name` and `value` can be references to @depends functions,
> +        in which case the result from these functions is used. If the result
> +        of such functions is None, the configuration item is not set.

nit: of *either* function ...

When would the `name` be `None`?

::: python/mozbuild/mozbuild/configure/__init__.py:437
(Diff revision 2)
> +    def set_config_impl(self, name, value):
> +        '''Implementation of set_config().
> +        Sets a configuration items with the given name to the given value.
> +        Both `name` and `value` can be references to @depends functions,
> +        in which case the result from these functions is used. If the result
> +        of such functions is None, the configuration item is not set.

nit: of *either* function ...

When would the `name` be `None`?

::: python/mozbuild/mozbuild/test/configure/test_configure.py:30
(Diff revision 2)
>  test_data_path = mozpath.abspath(mozpath.dirname(__file__))
>  test_data_path = mozpath.join(test_data_path, 'data')
>  
>  
>  class TestConfigure(unittest.TestCase):
> -    def get_result(self, args=[], environ={}, prog='/bin/configure'):
> +    def get_result(self, args=[], environ={}, configure='moz.configure',

Hey, you snuck in testing multiple files!

::: python/mozbuild/mozbuild/test/configure/test_configure.py:310
(Diff revision 2)
>          config = self.get_config(['--enable-advanced-template'])
>          self.assertIn('PLATFORM', config)
>          self.assertEquals(config['PLATFORM'], sys.platform)
>  
> +    def test_set_config(self):
> +        config, out = self.get_result(['--help'],

Consider a local `def` to not repeat the `configure=` parameter.

Comment 35

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8733803 [details]
MozReview Request: Bug 1257823 - Move set_config() from @depends to global scope

https://reviewboard.mozilla.org/r/41959/#review38535

Stampy Mcstampface.  I checked a sample of these to see unusual cases; one comment below.

::: build/moz.configure/util.configure:111
(Diff revision 2)
>  def namespace(**kwargs):
>      from mozbuild.util import ReadOnlyNamespace
>      return ReadOnlyNamespace(**kwargs)
> +
> +
> +# Some @depends function return namespaces, and one could want to use one

Good comment.  Is there a reason it's not a docstring?

::: mobile/android/gradle.configure:17
(Diff revision 2)
>         help='Enable building mobile/android with Gradle '
>              '(argument: location of binary or wrapper (gradle/gradlew))')
>  
> -@depends('--with-gradle', check_build_environment)
> -def gradle(value, build_env):
> +@depends('--with-gradle')
> +def with_gradle(value):
>      if value:

Hmm.  It occurs that `return bool(value)` is different from what you have, since `None` is different than `False`.  I bet this trips us up at least once, but I don't know how to do better.

Comment 36

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8733804 [details]
MozReview Request: Bug 1257823 - Remove set_config implementation for @depends functions

https://reviewboard.mozilla.org/r/41961/#review38537

Splitting this commit helped the review.  Thanks!

Comment 37

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8733805 [details]
MozReview Request: Bug 1257823 - Move set_define() to the global scope

https://reviewboard.mozilla.org/r/41963/#review38541

::: python/mozbuild/mozbuild/test/configure/test_configure.py:345
(Diff revision 2)
>              # set_config('FOO'...)
>              self.get_config(['--set-foo', '--set-name=FOO'],
>                              configure='set_config.configure')
>  
> +    def test_set_define(self):
> +        config, out = self.get_result(['--help'],

Again, consider lifting a local `set_define`, possibly also encapsulating the `config['DEFINES']`.

Comment 38

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8733806 [details]
MozReview Request: Bug 1257823 - Move set_define() from @depends to global scope

https://reviewboard.mozilla.org/r/41965/#review38543

It's already better to see `{set_config,set_define}(NAME, SAME-VALUE)`.  That'll even get better with a combined helper!

::: build/moz.configure/old.configure:430
(Diff revision 2)
>  # old_configure, so we cheat.
>  @template
>  def set_old_configure_config(name, value):
>      set_config(name, value)
>  
> +# same as above, for set_define

nit: full sentence, and reference `set_old_configure_config` so that it shows up in DXR searches.

Comment 39

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8733807 [details]
MozReview Request: Bug 1257823 - Remove set_define implementation for @depends functions

https://reviewboard.mozilla.org/r/41967/#review38549

Comment 40

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8733808 [details]
MozReview Request: Bug 1257823 - Keep track of the dependencies of @depends functions

https://reviewboard.mozilla.org/r/41969/#review38553

::: python/mozbuild/mozbuild/configure/__init__.py:341
(Diff revision 2)
>              result = DependsOutput()
>              glob.update(
>                  imply_option=result.imply_option,
>              )
>              dummy = wraps(func)(DummyFunction())
> -            self._depends[dummy] = func
> +            self._depends[dummy] = func, dependencies

Consider noting the change to `_depends` around https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/configure/__init__.py#120.

::: python/mozbuild/mozbuild/configure/__init__.py:346
(Diff revision 2)
> -            self._depends[dummy] = func
> -            func.with_help = with_help
> +            self._depends[dummy] = func, dependencies
> +            with_help = self._help_option in dependencies
>              if with_help:
>                  for arg in args:
>                      if (isinstance(arg, DummyFunction) and
> -                            not self._depends[arg].with_help):
> +                            self._help_option not in self._depends[arg][1]):

I think it's worth teasing this apart: you're grabbing the dependencies of `self._depends[arg]`, but it's not all that obvious.

::: python/mozbuild/mozbuild/configure/__init__.py:367
(Diff revision 2)
> -                        if not isinstance(value, OptionValue):
> +                        if not isinstance(arg, Option):
>                              raise ConfigureError(
>                                  "Cannot infer what implied '%s'" % option)
> -                        if name == '--help':
> +                        if arg == self._help_option:
>                              continue
> -                        prefix, opt, values = Option.split_option(name)
> +                        deps.append(self._raw_options.get(arg) or

Ah, I see now why you wanted to key by `Option` and not value.

Comment 41

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8733809 [details]
MozReview Request: Bug 1257823 - Move imply_option() to the global scope

https://reviewboard.mozilla.org/r/41971/#review38561

I have significant questions about the inference, but I don't care to block a large patch series on such concerns.  If my concerns are grounded, consider filing follow-up.

::: python/mozbuild/mozbuild/configure/__init__.py:56
(Diff revision 2)
>          self.implied_options = []
>  
>      def imply_option(self, option, reason=None):
>          if not isinstance(option, types.StringTypes):
>              raise TypeError('imply_option must be given a string')
> -        self.implied_options.append((option, reason))
> +        self.implied_options.append((option, inspect.stack()[1], reason))

These magic constants make me sad.  Consider destructuring to name them.

::: python/mozbuild/mozbuild/configure/__init__.py:466
(Diff revision 2)
> +        '''Implementation of imply_option().
> +        Injects additional options as if they had been passed on the command
> +        line. The `option` argument is a string as in option()'s `name` or
> +        `env`. The option must be declared after `imply_option` references it.
> +        The `value` argument indicates the value to pass to the option.
> +        It can be True (the positive option is injected*), False (the negative

nit: the * is ambiguous -- consider "see Note below" and then "Note: " or similar.

Consider making this whole thing a list of cases, and folding the elaboration into each case.

::: python/mozbuild/mozbuild/configure/__init__.py:492
(Diff revision 2)
> +          are both equivalent to not passing any flag on the command line.
> +
> +        It is recommended to use the positive form ('--enable' or '--with') for
> +        `option`.
> +        '''
> +        if not reason and isinstance(value, DummyFunction):

I don't claim to really understand this, but any such inference process should recurse, right?  That is, we want to ensure there is a single "root" reason.  Or do you want folks using `imply_option` to explicitly break ties?

::: python/mozbuild/mozbuild/configure/__init__.py:500
(Diff revision 2)
> +            if len(possible_reasons) == 1:
> +                if isinstance(possible_reasons[0], Option):
> +                    reason = (self._raw_options.get(possible_reasons[0]) or
> +                              possible_reasons[0].option)
> +
> +        if not reason or not isinstance(value, DummyFunction):

Consider distinguishing no reasons from many reasons.  (Not clear that many reasons can occur, but worth be explicit, I think.)

::: python/mozbuild/mozbuild/test/configure/data/imply_option/infer_ko.configure:17
(Diff revision 2)
> +
> +
> +option('--enable-foo', help='enable foo')
> +
> +@depends('--enable-foo', hoge, '--help')
> +def foo(value, help):

I must be missing something.  I trying to understand the test.  Doesn't `foo` need to take a third parameter -- `value, hoge, help`?

::: python/mozbuild/mozbuild/test/configure/test_configure.py:9
(Diff revision 2)
>  
>  from __future__ import absolute_import, print_function, unicode_literals
>  
>  from StringIO import StringIO
>  import sys
> +import traceback

Appears this is not used.

::: python/mozbuild/mozbuild/test/configure/test_configure.py:469
(Diff revision 2)
> +            e.exception.message,
> +            "'--enable-bar' implied by '--enable-foo' conflicts with "
> +            "'--disable-bar' from the command-line")
> +
> +
> +        with self.assertRaises(ConfigureError) as e:

This is a vague failure, and I can't really tell what you expect `infer_ko` to mean.  Knock-out?  Something here is not allowed, but I don't know what.

Comment 42

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8733810 [details]
MozReview Request: Bug 1257823 - Move imply_option() from @depends to global scope

https://reviewboard.mozilla.org/r/41973/#review38567

::: moz.configure:40
(Diff revision 2)
>          return True
>  
>  set_config('MOZ_ARTIFACT_BUILDS', artifact_builds)
>  
> +@depends('--enable-artifact-builds')
> +def imply_disable_compile_environment(value):

Hmm.  The `False` and `None` difference again.

Comment 43

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8733811 [details]
MozReview Request: Bug 1257823 - Remove imply_option implementation for @depends functions

https://reviewboard.mozilla.org/r/41975/#review38569

Comment 44

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8733812 [details]
MozReview Request: Bug 1257823 - Move add_old_configure_assignment() to the global scope

https://reviewboard.mozilla.org/r/41977/#review38571

::: build/moz.configure/init.configure:134
(Diff revision 2)
>  @depends('--help')
>  def extra_old_configure_args(help):
>      return []
>  
>  @template
> -def add_old_configure_assignment(var, value):
> +def add_old_configure_assignment(var, value_func):

This now cannot handle `add_old_configure_assignment('NAME', '1')`, right?  Am I missing something in @depends that handles values?

I think this deserves a test assertion.

Comment 45

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/41957/#review38533

> nit: of *either* function ...
> 
> When would the `name` be `None`?

You probably saw this in the subsequent patches, but for instance, while not with set_config, but with set_define, we have MOZ_WIDGET_%s not set for some toolkits. That's the kind of case where the name would be None.

Comment 46

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/41959/#review38535

> Good comment.  Is there a reason it's not a docstring?

No reason other than templates are not using docstrings currently. Followup?

Comment 47

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/41963/#review38541

> Again, consider lifting a local `set_define`, possibly also encapsulating the `config['DEFINES']`.

encapsulating config['DEFINES'] is harder, since every call to get_config gives us a different config.

Comment 48

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/41971/#review38561

> These magic constants make me sad.  Consider destructuring to name them.

inspect.stack() is an array. [1] points to the caller.

> I don't claim to really understand this, but any such inference process should recurse, right?  That is, we want to ensure there is a single "root" reason.  Or do you want folks using `imply_option` to explicitly break ties?

So, the reason it's not doing a recursive search at the moment is that the initial imply_option code didn't do it. For most purposes with imply_option (and in fact all the cases in the tree right now) it doesn't matter that it doesn't recurse. However, I'm planning to write an introspection tool that will have a use for such a recursive search, and the implementation can be shared. But I'll just keep it to when I'll get to writing that introspection tool (probably some time next week).

> Consider distinguishing no reasons from many reasons.  (Not clear that many reasons can occur, but worth be explicit, I think.)

I don't think it's worth doing at this point. There are currently three cases where inference would fail:
- the function has only a dependency on --help
- the function has a dependency on one other function (because we don't recurse yet)
- the function has more than one dependency

The first is a weird case, and maybe we should error out differently for it, not even allowing a manual reason in that case.
The second is only due to the lack of recursion, which I'll be adding soon.
So the last is really the only case that matters. So it doesn't seem to me there is a need for a distinction.

> I must be missing something.  I trying to understand the test.  Doesn't `foo` need to take a third parameter -- `value, hoge, help`?

oops, yes.

Comment 49

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/41977/#review38571

> This now cannot handle `add_old_configure_assignment('NAME', '1')`, right?  Am I missing something in @depends that handles values?
> 
> I think this deserves a test assertion.

I'll add an assertion. Note that add_old_configure_assignment('NAME', '1'), as in, unconditionally setting a value, doesn't really make sense in the global scope. You can just set it in old-configure.in, then.

Comment 50

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #46)
> https://reviewboard.mozilla.org/r/41959/#review38535
> 
> > Good comment.  Is there a reason it's not a docstring?
> 
> No reason other than templates are not using docstrings currently. Followup?

If you feel so inclined.  I'm happy to have you juggle the exact order and timing of such things, if they happen at all.  Forward!

Comment 51

[Mike Hommey [:glandium]]

(In reply to Nick Alexander :nalexander from comment #50)
> If you feel so inclined.  I'm happy to have you juggle the exact order and
> timing of such things, if they happen at all.  Forward!

Added to the scope of bug 1259272

Comment 52

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cca90a25cbb
https://hg.mozilla.org/integration/mozilla-inbound/rev/8debac7402e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d2b6f4c3ee8
https://hg.mozilla.org/integration/mozilla-inbound/rev/62ae3968b2d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/407e18a1a024
https://hg.mozilla.org/integration/mozilla-inbound/rev/06dc23858ed7
https://hg.mozilla.org/integration/mozilla-inbound/rev/76d58b17343e

Comment 53

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1cca90a25cbb
https://hg.mozilla.org/mozilla-central/rev/8debac7402e8
https://hg.mozilla.org/mozilla-central/rev/9d2b6f4c3ee8
https://hg.mozilla.org/mozilla-central/rev/62ae3968b2d8
https://hg.mozilla.org/mozilla-central/rev/407e18a1a024
https://hg.mozilla.org/mozilla-central/rev/06dc23858ed7
https://hg.mozilla.org/mozilla-central/rev/76d58b17343e