856392 - mach's commands should be categorized in its help

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

'mach help' lists quite an eclectic list of commands these days. Commands for building, running tests, searching the interwebs, hacking on mach, etc - all intermingled in one list. It's not the easiest to read to find something relevant for what you want to do. 

It'd be nice if the commands were categorized, so the command list were presented something like:


Development setup commands:
    bootstrap           Install required system packages for building.
    clang-complete      Generate a .clang_complete file.
    install             Install the package on the machine, or on a device.

Build commands:
    build               Build the tree.
    buildsymbols        Produce a package of Breakpad-format symbols.
    clobber             Clobber the tree (delete the object directory).
    warnings-list       Show a list of compiler warnings.
    warnings-summary    Show a summary of compiler warnings.

Tree commands:
    empty-makefiles     Find empty Makefile.in in the tree.

Testing commands:
    crashtest           Run a crashtest.
    jetpack-test        Runs the jetpack test suite.
    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.
    reftest             Run a reftest.
    xpcshell-test       Run an xpcshell test.

Post-build commands:
    package             Package the built product for distribution as an APK,
                        DMG, etc.
    run                 Run the compiled program.

Searching commands:
    dxr                 Search for something in DXR.
    google              Search for something on Google.
    mdn                 Search for something on MDN.
    mxr                 Search for something in MXR.
    search              Search for something on the Internets. This will open
                        3 new browser tabs and search for the term on Google,
                        MDN, and MXR.

Mach meta-commands
    help                Show mach usage info or help for a command.
    mach-debug-commands
                        Show info about available mach commands.
    mozbuild-reference  View reference documentation on mozbuild files.

Comment 1

[Gregory Szorc [:gps]]

I agree. We /may/ already have this one on file...

Comment 2

[Gregory Szorc [:gps]]

I'm attempting to implement this. I'm currently knee deep in light argparse voodoo.

Comment 3

[Gregory Szorc [:gps]]

Worth noting that grouping commands (a subparser in Python speak) is not built-in to argparse. http://bugs.python.org/issue14037 is on file. That will get fixed in Python 3.3 at the earliest. Boo.

Comment 4

[Gregory Szorc [:gps]]

I am waist deep.

Comment 6

[Gregory Szorc [:gps]]

Attachment: Categorize mach commands, v1

I /think/ this will do it!

There is some moderate magic employed in this patch. At some point we should probably just cut our losses with argparse and use one of the many 3rd party libraries instead (as I'm sure you'll remind me).

Given the magic employed by this patch, it should probably be documented more. I'm tired and want something to show for my toils, so I'm putting it up. Please remind me to post a new version with better docs!

Comment 7

[Jeff Hammel]

Which library would you choose?  Sadly, while I prefer argparse to optparse, I'm more familiar with the latter due to python 2.old still clinging to me like a limpet. (I also view choosing non-stdlib software for e.g. m-c as a serious choice, as I would like, in my dream world, to consolidate around a good set of choices, though in practice it seems like everyone just uses whatever and I don't know if we can change that behaviour.)

Comment 8

[Gregory Szorc [:gps]]

Attachment: Categorize mach commands, v2

Now with more docs and comments. And a helpful new avatar!

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 737750 [details] [diff] [review]
Categorize mach commands, v2

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

::: build/mach_bootstrap.py
@@ +43,5 @@
>      'tools/mach_commands.py',
>  ]
>  
> +CATEGORIES = [
> +    ('build', 'Build commands', 'Interact with the build system.', 80),

The numbers here are completely non-obvious.

Comment 10

[Gregory Szorc [:gps]]

Attachment: Part 2: Keep global and command arguments separate, v1

As was discovered in bug 860898, the merging of global arguments with the command's arguments was causing all kinds of nastiness. So, since we now have the ability to keep the argument namespace separate (since we are manually controlling the argument parser), I did just that. This removes the ugly argument filtering from the main dispatch routine and should make the patch in bug 860898 trivial (just a change to the mach command method rather than a new feature in the mach core).

Comment 11

[Jeff Hammel]

(In reply to :Ms2ger from comment #9)
> Comment on attachment 737750 [details] [diff] [review]
> Categorize mach commands, v2
> 
> Review of attachment 737750 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/mach_bootstrap.py
> @@ +43,5 @@
> >      'tools/mach_commands.py',
> >  ]
> >  
> > +CATEGORIES = [
> > +    ('build', 'Build commands', 'Interact with the build system.', 80),
> 
> The numbers here are completely non-obvious.

Not done with my review (nor have looked at the new patch) but this was one of my complaints as well.  Will say more about that later.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 738105 [details] [diff] [review]
Part 2: Keep global and command arguments separate, v1

This does indeed separate the global and subcommand arguments out nicely.

Comment 13

[Jeff Hammel]

Comment on attachment 737750 [details] [diff] [review]
Categorize mach commands, v2

Giving an r- right now until points are addressed.  But very close

+CATEGORIES = [
+    ('build', 'Build commands', 'Interact with the build system.',
80),
+    ('post-build', 'Post-build commands',
...

I'd rather this data not look like this.

The priority numbers are not only "arbitrary", they're also
confusing.  What do they mean?  If we just want a sorting order, I'd
prefer taking (in this case) order in a list.

I'm also personally less fond of tuples (-> trouble) for this sort of
thing.

+    ('devenv', 'Development Environment',

nit: maybe devenv -> environment?

-        self.parser_args = parser_args
+        self.name = name
+        self.category = category
+        self.description = description
+        self.allow_all_arguments = allow_all_arguments
         self.arguments = arguments or []
         self.pass_context = pass_context
+

Assuming that this is trailing whitespace (that is, not just a fix to
"no newline at end of file"), kill it.

+    def __init__(self, cls, method, name, category=None,
description=None,
+        allow_all_arguments=False, arguments=None,
pass_context=False):
 
         self.cls = cls
         self.method = method
-        self.parser_args = parser_args
+        self.name = name
+        self.category = category

What happens if category is None, ultimately?  ABICT, this is an error.
(I tried tracing through the code a few times but decided to throw the
question back on you.) If category cannot be None, here or elsewhere,
in practice, we should not have default arguments for it.

-        parser_args = getattr(value, '_mach_command', None)
-        if parser_args is None:
+        command_name = getattr(value, '_mach_command_name', None)
+        if command_name is None:
             continue
 
+        category = getattr(value, '_mach_command_category', None)
+        description = getattr(value, '_mach_command_description',
None)
         arguments = getattr(value, '_mach_command_args', None)
+        allow_all = getattr(value, '_mach_command_allow_all_args',
False)

I don't really like this "pattern".  Everything, except name, is
exactly the same.  Why repeat code vs institutionalizing a
pattern?

     def __call__(self, func):
-        func._mach_command = self._command_args
+        func._mach_command_name = self._name
+        func._mach_command_category = self._category
+        func._mach_command_description = self._description
+        func._mach_command_allow_all_args = self._allow_all_args

Like here, _mach_command could be a dict or what not and the data live
inside it.  I very much hesitate to have several places where one has
to know the various attributes, name, category, etc

+def unknown_command(action, command):
+    print(AVATAR)
+    print('It looks like you are trying to %s an unknown mach
command: %s' % (
+        action, command))
+    print('')
+    print('Run |mach help| to show a list of commands.')
+    sys.exit(1)
+

Well, of course I have to +1 the avatar. ;)  In the future, this could
conceivably suggest commands based on some sort of something, but fine
for now.

+    This class is essentially a reimplementation of argparse's
sub-parsers
+    feature. We first tried to use sub-parsers. However, they were
missing
+    features like grouping of commands.

Upstream bug#? Other link(s)?

+    def __init__(self, option_strings, dest, required=True,
default=None,
+        registrar=None):
+        argparse.Action.__init__(self, option_strings, dest,
required=required,
+            help=argparse.SUPPRESS, nargs=argparse.REMAINDER)
+

Missing `default`?

+        if command == 'help':
+            if len(args):
+                self._handle_subcommand_help(parser, args[0])
+            else:
+                self._handle_main_help(parser)
+
+            sys.exit(0)

So....it is a bit strange to do just args[0] for command help.  At the
very least ambiguous.

Is it possible to hook up `help` to be just another command?  That
would be ideal (and we're rolling our own here anyway...).

+        if not handler:
+            unknown_command('run', command)
+            return

Why return?  You're sys.exiting anyway.

+        # If we wanted to conditionally enable commands based on
whether
+        # it's possible to run them given the current state of
system, here
+        # would be a good place to hook that up.

Probably should note TODO/FUTURE here.

+        if extra:
+            print(AVATAR)
+            print('It looks like you passed an unrecognized argument
into mach.')
+            print('')
...

IMHO, this sorta thing should be grouped with unknown command or what
not.  At the very least, it should be in a class function.

+            self._handle_subcommand_help(parser, command)
+            sys.exit(0)

Why 0? Shouldn't this be 1?

-        Registrar.populate_argument_parser(subparser)
+        # We need to be last because its argument parser is greedy.
+        parser.add_argument('command', action=CommandAction,
+            registrar=Registrar)

Again, a link would be nice.

+            raise Exception('Cannot register a command to an
undefined '
+                'category: %s -> %s' % (name, handler.category))

Well, you know how I feel about literal string concatenation.

But more so, if we go this way of literally enforcing categories, we
should probably print what categories are available.  It might also be
nice if this were a more specific exception type.

To be honest, I'd probably make categories optional.  Descriptions of
categories can live where they are, but I would make 'misc' -> None
for categories and registering to non-existant categories work.  But I
see the reason for your approach...just priority confuses me.

The way that argparse is circumvented is fairly complicated, but I've
never tried to do this exact thing in argparse, so I will assume that
this is the best way of doing this sort of thing.  (Ironically, I have
used the same sort of pattern with less work/code involved for
optparse...yay for progress o_O)

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 738105 [details] [diff] [review]
Part 2: Keep global and command arguments separate, v1

I'll just roll this into the new patch.

Comment 15

[Jeff Hammel]

Thanks.  Sorry for the delay, but have been sick. First glance looked fine :)

Comment 16

[Gregory Szorc [:gps]]

(In reply to Jeff Hammel [:jhammel] from comment #13)
> +CATEGORIES = [
> +    ('build', 'Build commands', 'Interact with the build system.',
> 80),
> +    ('post-build', 'Post-build commands',
> ...
> 
> I'd rather this data not look like this.
> 
> The priority numbers are not only "arbitrary", they're also
> confusing.  What do they mean?  If we just want a sorting order, I'd
> prefer taking (in this case) order in a list.
> 
> I'm also personally less fond of tuples (-> trouble) for this sort of
> thing.

I'm going to push back on this a little. The problem with a unified list is that categories may not always be centrally defined. If comm-central or b2g were to ever adopt mach, they would likely supplement their category set with mozilla-central's. This necessitates merging lists. And that necessitates explicit priority in the category definition.

As a compromise, I'm going to change this to a dict of dicts instead of a list of tuples. That buys us better readability. I'll also document what the priority means. Is that fair?


> +    def __init__(self, cls, method, name, category=None,
> description=None,
> +        allow_all_arguments=False, arguments=None,
> pass_context=False):
>  
>          self.cls = cls
>          self.method = method
> -        self.parser_args = parser_args
> +        self.name = name
> +        self.category = category
> 
> What happens if category is None, ultimately?  ABICT, this is an error.
> (I tried tracing through the code a few times but decided to throw the
> question back on you.) If category cannot be None, here or elsewhere,
> in practice, we should not have default arguments for it.

I will address this.

> 
> -        parser_args = getattr(value, '_mach_command', None)
> -        if parser_args is None:
> +        command_name = getattr(value, '_mach_command_name', None)
> +        if command_name is None:
>              continue
>  
> +        category = getattr(value, '_mach_command_category', None)
> +        description = getattr(value, '_mach_command_description',
> None)
>          arguments = getattr(value, '_mach_command_args', None)
> +        allow_all = getattr(value, '_mach_command_allow_all_args',
> False)
> 
> I don't really like this "pattern".  Everything, except name, is
> exactly the same.  Why repeat code vs institutionalizing a
> pattern?
> 
>      def __call__(self, func):
> -        func._mach_command = self._command_args
> +        func._mach_command_name = self._name
> +        func._mach_command_category = self._category
> +        func._mach_command_description = self._description
> +        func._mach_command_allow_all_args = self._allow_all_args
> 
> Like here, _mach_command could be a dict or what not and the data live
> inside it.  I very much hesitate to have several places where one has
> to know the various attributes, name, category, etc

Agreed. I will happily bloat scope if you will review it :)

> +    def __init__(self, option_strings, dest, required=True,
> default=None,
> +        registrar=None):
> +        argparse.Action.__init__(self, option_strings, dest,
> required=required,
> +            help=argparse.SUPPRESS, nargs=argparse.REMAINDER)
> +
> 
> Missing `default`?

Intentionally. Will document.

> 
> +        if command == 'help':
> +            if len(args):
> +                self._handle_subcommand_help(parser, args[0])
> +            else:
> +                self._handle_main_help(parser)
> +
> +            sys.exit(0)
> 
> So....it is a bit strange to do just args[0] for command help.  At the
> very least ambiguous.
> 
> Is it possible to hook up `help` to be just another command?  That
> would be ideal (and we're rolling our own here anyway...).

I would love to make `help` just another command. However, it needs one-off attention no matter how you slice it since we perform argparse manipulation inside the command handler. I think you'll find argparse itself also handles help specially.

> +        if not handler:
> +            unknown_command('run', command)
> +            return
> 
> Why return?  You're sys.exiting anyway.

Habit. I'll change it to an |assert False| or something.

> +            self._handle_subcommand_help(parser, command)
> +            sys.exit(0)
> 
> Why 0? Shouldn't this be 1?

Good catch.

> To be honest, I'd probably make categories optional.  Descriptions of
> categories can live where they are, but I would make 'misc' -> None
> for categories and registering to non-existant categories work.  But I
> see the reason for your approach...just priority confuses me. 

I like the idea of optional categories. But, I also like strict software with fewer optionals and the corner cases that go with them.

Let's leave optional categories out for now. We can always grow that feature later.

Comment 17

[Gregory Szorc [:gps]]

Attachment: Categorize mach commands, v3

I think I addressed all points of feedback (minus the push back, of course).

Comment 18

[Jeff Hammel]

Comment on attachment 741913 [details] [diff] [review]
Categorize mach commands, v3

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

Looks good!  Sorry for the lag

::: build/mach_bootstrap.py
@@ +75,5 @@
> +        'priority': 10,
> +    }
> +}
> +
> +

I'm still not a huge fan of magic numbers for priorities but....okay.

Comment 19

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dac8cb02fd21

Fixed minor bit as part of push. Was all in mach_commands.py files and was minor, so no new review was needed, IMO.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dac8cb02fd21