901972 - Mach should have a filter system for commands at runtime

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Andrew Halberstadt [:ahal]]

Certain commands may not make sense depending on the context that they're being run in. For example, the mochitest-chrome command doesn't make sense when running mach for B2G. There should be a way for mach commands to announce which contexts they are meant for, and the mach driver should filter out commands that aren't suitable for the current context.

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.0 - parse time filters

This patch adds a filters option to the @Command decorator. The parameter is a dict that gets passed to a whatever filter functions have been registered by the driver. Currently the command gets registered if either a) no filters have been registered, or b) at least one filter function returns True.

Example usage would be:

def is_b2g_command(info):
    return info.get('B2G', False)
mach.define_filter(is_b2g_command)

Then in mach_commands.py:

@Command('a_b2g_command', category='...', filters={'B2G': True})
def run_this_command():
    ....

If this approach looks good I'll update the documentation as well.

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 786329 [details] [diff] [review]
Patch 1.0 - parse time filters

Review of attachment 786329 [details] [diff] [review]:
-----------------------------------------------------------------

I like the concept. However, I think filters should be applied at dispatch time rather than parse time (during decorator processing). This way, filter functions will have access to the run-time context and will be able to make richer decisions. e.g. it can test if the tree is built. This is much more useful and I'm going to assert it will be eventually required. Another issue with decorator-time filters is that these commands effectively disappear into a black box and are never known about. It's hard to debug something that leaves no trace!

There might be room for decorator-time filters alongside dispatch-timer filters. If so, convince me we need both and/or change the API to differentiate between the different filter "classes."

Comment 3

[Andrew Halberstadt [:ahal]]

That makes sense. Though I still think we either need parse-time filters or else we shouldn't re-use mach_commands between different bootstrap contexts.

The problem with dispatch-time filters is that there isn't a way to update ./mach help (at least easily.. maybe I'm over thinking this). E.g:
$ ./mach help
Testing:
  Run tests.

  mochitest-a11y        Run an a11y mochitest.
  mochitest-browser     Run a mochitest with browser chrome.
  mochitest-chrome      Run a chrome mochitest.
  mochitest-metro       Run a mochitest with metro browser chrome.
  mochitest-plain       Run a plain mochitest.
  mochitest-remote      Run a remote mochitest.

Only the last command would actually work, but there is no obvious way of knowing this. To me this is a deal-breaker as mach should be as self documenting as possible.

Alternatively the parse-time filters could live on the @CommandProvider decorator. Then you could have e.g MochitestCommandProvider, MochitestB2GCommandProvider, etc.. Shared commands could live in a common parent class.

Comment 4

[Andrew Halberstadt [:ahal]]

Attachment: Patch 2.0 - dispatch time filters

I think I found a way for us to have our cake and eat it too. It involves instantiating every single handler upon running ./mach help which doesn't seem great, but I believe this is much better than the alternative of having two different types of filters.

Example usage:

def is_b2g(cls):
  return cls.substs.get('MOZ_WIDGET_TOOLKIT') == 'gonk'

@CommandProvider(...)
class Provider:
  @Command(name, description, conditions=[is_b2g])
  def run_b2g_command(...):
    ...

Notes:
1) I think the term condition is more representative of what these functions are. We use the *condition* to *filter* the command. Though I don't care if you want to change it back to filters.

2) Condition functions are no longer defined by the driver, i.e there are no 'global filters'. Each mach_commands.py can define its own condition functions. Maybe a series of commonly used conditions could be kept in mach.base or exposed by the bootstrap script. The downside is that we'll need to add a condition to every command that is explicitly *not* b2g. But this should arguably be done anyway.

3) Condition functions have access to an instance of the handler class under which the command was defined. This means they can access the context (if pass_context is True) and the MozbuildObject properties (if the handler subclasses MachCommandBase).

Comment 5

[Andrew Halberstadt [:ahal]]

Since uploading I'm beginning to think that 'AND' would be the better operator between condition functions.

e.g conditions=['is_built', 'is_remote'], makes sense that both of these would need to be true. I'll leave the patch as is for now.

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 790416 [details] [diff] [review]
Patch 2.0 - dispatch time filters

Review of attachment 790416 [details] [diff] [review]:
-----------------------------------------------------------------

I like where this is going!

This will need docs in the README and tests before it can land. Although, I may budge a little on the tests bit depending how hard they are to implement - I don't want to slow down a handy feature!

::: python/mach/mach/base.py
@@ +86,5 @@
>          'arguments',
>      )
>  
>      def __init__(self, cls, method, name, category=None, description=None,
> +        allow_all_arguments=False, conditions= None, arguments=None,

Nit: No space after =.

::: python/mach/mach/decorators.py
@@ +61,5 @@
>          if command_name is None:
>              continue
>  
> +        conditions = conditions or []
> +        if not isinstance(conditions, (list, tuple, set)):

Meh. Can we just assume the input is iterable?

@@ +68,5 @@
> +        for c in conditions:
> +            if not hasattr(c, '__call__'):
> +                msg = 'Mach command \'%s\' implemented incorrectly. ' + \
> +                      'Conditions argument must take a function or list ' + \
> +                      'of functions. Found %s instead.'

Since conditions will be shared across many mach commands and we wish to avoid an import hell dependency issue, I think it might be worth supporting registered functions like in the early version of the patch.

::: python/mach/mach/dispatcher.py
@@ +166,5 @@
> +                    for c in handler.conditions:
> +                        if c(instance):
> +                            break
> +                    else:
> +                        continue

TIL for..else is a thing. This is the first time I've seen that loop construct used. I don't think it's very readable. Can you rewrite it?

And yes, I concede your l33t Python skillz surpass my own.

::: python/mach/mach/main.py
@@ +94,5 @@
> +
> +The command %s must meet one of the following conditions: %s
> +
> +Run |mach help| to show a list of all commands available to the current context.
> +'''.lstrip()

This string assumes mach is being used for Mozilla development. Mach is a generic tool. While some references to Mozilla foo still linger in the core, please factor this out into something more generic.

Perhaps the filter functions can return the reason why they can't be called? Since we may want to overload the return value of filters in the future, my vote is to use a NamedTuple or class to represents filter execution results.

Comment 7

[Andrew Halberstadt [:ahal]]

(In reply to Gregory Szorc [:gps] from comment #6)
> @@ +68,5 @@
> > +        for c in conditions:
> > +            if not hasattr(c, '__call__'):
> > +                msg = 'Mach command \'%s\' implemented incorrectly. ' + \
> > +                      'Conditions argument must take a function or list ' + \
> > +                      'of functions. Found %s instead.'
> 
> Since conditions will be shared across many mach commands and we wish to
> avoid an import hell dependency issue, I think it might be worth supporting
> registered functions like in the early version of the patch.

I thought about that.. but I don't really like how that would work. If the condition function is registered by the driver, then it would take both an instance of the class and the dictionary defined by the decorator. There would be a lot of magic knowledge needed between the bootstrap script and the mach_commands. Having them defined (or imported) in the mach_commands.py makes it easy to see what is going on.

Another problem we might run into is if we make a condition function that depends on an attribute of the instance (e.g cls.substs) and assume that every handler we apply the filter to actually has it (e.g subclasses MachBaseCommand).

> 
> ::: python/mach/mach/dispatcher.py
> @@ +166,5 @@
> > +                    for c in handler.conditions:
> > +                        if c(instance):
> > +                            break
> > +                    else:
> > +                        continue
> 
> TIL for..else is a thing. This is the first time I've seen that loop
> construct used. I don't think it's very readable. Can you rewrite it?
> 
> And yes, I concede your l33t Python skillz surpass my own.

It's actually a pattern I find myself needing quite often! Though that's moot, this is making sure at least one condition needs to be True. I think I want to change it so that all conditions need to be True.


> ::: python/mach/mach/main.py
> @@ +94,5 @@
> > +
> > +The command %s must meet one of the following conditions: %s
> > +
> > +Run |mach help| to show a list of all commands available to the current context.
> > +'''.lstrip()
> 
> This string assumes mach is being used for Mozilla development. Mach is a
> generic tool. While some references to Mozilla foo still linger in the core,
> please factor this out into something more generic.
> 
> Perhaps the filter functions can return the reason why they can't be called?
> Since we may want to overload the return value of filters in the future, my
> vote is to use a NamedTuple or class to represents filter execution results.

So like "Condition = namedtuple('Condition', [fn, name, description])"? Alternatively we could just use fn.__name__ as name and fn.__doc__ as description. I'd be fine either way.

Comment 8

[Andrew Halberstadt [:ahal]]

I realize I may have misinterpreted your registered functions comment. I guess you meant having the ability to register global conditions that apply to every command *in addition* to command specific conditions? If so, then yes, let's do that.

Comment 9

[Andrew Halberstadt [:ahal]]

Attachment: Patch 3.0 - dispatch time filters

Beh sorry to keep flip flopping, but I confused myself. My original assessment about global conditions in comment 7 was correct. Supporting global conditions requires magic knowledge between bootstrap script and command. Also the decorator param would need to be a dict instead of a function, which makes it difficult to see what the condition is doing when trying to figure out why a command is skipped.

En lieu of this, I added a 'require_conditions' property to the mach object which defaults to False. When True, any command that doesn't have a condition applied will be skipped. This is useful for the contexts (such as B2G) which may want to whitelist commands instead of blacklisting them. It'll let us avoid having to apply conditions to every single mach command which I think will mitigate your "import hell" concern.

Last note, I decided to use the condition function's docstring as a description. This keeps conditions simpler and easier to create than a named tuple (though we can certainly use a named tuple as well).

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 792868 [details] [diff] [review]
Patch 3.0 - dispatch time filters

Review of attachment 792868 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great! Still some open questions I'd like your thoughts on.

Also, please add a test where the @CommandProvider takes a context object so we have explicit test coverage there.

::: python/mach/mach/decorators.py
@@ +68,5 @@
> +              'Conditions argument must take a list ' + \
> +              'of functions. Found %s instead.'
> +
> +        conditions = conditions or []
> +        if not isinstance(conditions, (list, tuple, set)):

if not isinstance(collections.Iterable) ?

::: python/mach/mach/dispatcher.py
@@ +164,5 @@
> +                        instance = handler.cls()
> +
> +                    is_filtered = False
> +                    for c in handler.conditions:
> +                        if not c(instance):

What do you think about trapping exceptions when calling the filter and interpreting an exception as a failed condition? I can go both ways on it - just throwing an idea out there.

@@ +168,5 @@
> +                        if not c(instance):
> +                            is_filtered = True
> +                            break
> +                    if is_filtered:
> +                        continue

It would be nice to have a --all flag or something to |mach help| that displayed all available commands. Followup fodder?

::: python/mach/mach/main.py
@@ +354,5 @@
> +                msg = ''
> +                for c in fail_conditions:
> +                    msg += '\n  %s' % c.__name__
> +                    if c.__doc__ is not None:
> +                        msg += ' - %s' % c.__doc__

Normally I'd say something about += for string appends being evil and you should create a list and ''.join(), but I don't think it matters here.

::: python/mach/mach/test/test_conditions.py
@@ +17,5 @@
> +
> +class TestConditions(unittest.TestCase):
> +    provider_dir = os.path.join(os.path.dirname(__file__), 'providers')
> +
> +    def _run_mach(self, args):

Did you copy paste this mostly from somewhere else? Can we factor this into a base class for the unit tests?

@@ +39,5 @@
> +        msg = ''
> +        for c in fail_conditions:
> +            msg += '\n  %s' % c['name']
> +            if c['doc'] is not None:
> +                msg += ' - %s' % c['doc']

This looks familiar. Perhaps it should be factored into a function in main.py?

Comment 11

[Andrew Halberstadt [:ahal]]

(In reply to Gregory Szorc [:gps] from comment #10)
> ::: python/mach/mach/dispatcher.py
> @@ +164,5 @@
> > +                        instance = handler.cls()
> > +
> > +                    is_filtered = False
> > +                    for c in handler.conditions:
> > +                        if not c(instance):
> 
> What do you think about trapping exceptions when calling the filter and
> interpreting an exception as a failed condition? I can go both ways on it -
> just throwing an idea out there.

I think returning True/False is simpler and more intuitive. In most cases condition functions will be a one-liner. If they raised exceptions we'd need if statements.


> @@ +168,5 @@
> > +                        if not c(instance):
> > +                            is_filtered = True
> > +                            break
> > +                    if is_filtered:
> > +                        continue
> 
> It would be nice to have a --all flag or something to |mach help| that
> displayed all available commands. Followup fodder?

Yes, that's a good idea. I filed bug 908868

Comment 12

[Andrew Halberstadt [:ahal]]

Attachment: Patch 3.1 - address review comments, refactor tests (inc)

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 794849 [details] [diff] [review]
Patch 3.1 - address review comments, refactor tests (inc)

Please merge the two patches and submit as one.

Comment 14

[Andrew Halberstadt [:ahal]]

Attachment: Patch 4.0 - dispatch time filters

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 795431 [details] [diff] [review]
Patch 4.0 - dispatch time filters

Review of attachment 795431 [details] [diff] [review]:
-----------------------------------------------------------------

Beautiful. Thank you so much for implementing this feature!

::: python/mach/mach/main.py
@@ +354,5 @@
> +                if not c(instance):
> +                    fail_conditions.append(c)
> +
> +            if fail_conditions:
> +                print self._condition_failed_message(handler.name, fail_conditions)

Use Python 3 compatible print().

::: python/mach/mach/registrar.py
@@ +9,5 @@
>  
>  class MachRegistrar(object):
>      """Container for mach command and config providers."""
>  
> +    require_conditions = False

Can you move this to __init__?

Comment 16

[Andrew Halberstadt [:ahal]]

Fixed nits and pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b71067682f1

Comment 17

[Gregory Szorc [:gps]]

I rebased this to Git. There were conflicts. So, I'd appreciate a second set of eyes to ensure I got this right.

https://github.com/indygreg/mach/commit/a12e5af7bce64b4e9f65b2bccbee228704c42d04

(I really need to get Git's version merged into m-c.)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2b71067682f1

Comment 19

[Andrew Halberstadt [:ahal]]

(In reply to Gregory Szorc [:gps] from comment #17)
> I rebased this to Git. There were conflicts. So, I'd appreciate a second set
> of eyes to ensure I got this right.
> 
> https://github.com/indygreg/mach/commit/
> a12e5af7bce64b4e9f65b2bccbee228704c42d04
> 
> (I really need to get Git's version merged into m-c.)

That diff looks sane to me.