908868 - Add some kind of --all option to mach so it displays skipped commands

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Andrew Halberstadt [:ahal]]

https://bugzilla.mozilla.org/show_bug.cgi?id=901972#c10

Or maybe it would be enough to have them show up in the 'mach-commands' command?

Comment 1

[Andrew Halberstadt [:ahal]]

For context:

Mach is the command dispatching tool used by the mozilla source tree. In bug 901972 I added a mechanism that allows mach to filter which commands are available based on a person's environment (do they have a build, is it a debug build, etc..). This means the commands won't show up when running 'mach help' and they will error out if someone tries to run them from the wrong context.

There should be some kind of a ./mach help --all option that includes these disabled commands so that they are more discoverable. Alternatively we could create a 'Disabled' command category and lump them all in there together (I like this one better).

The code for handling mach help is here: http://mxr.mozilla.org/mozilla-central/source/python/mach/mach/dispatcher.py#142

The fix involves either skipping that handler.conditions block if something like --all was passed in or else moving the command to the 'disabled' category instead of calling continue.

Comment 2

[Andrew Halberstadt [:ahal]]

Please feel free to ping me here or on irc (ahal) if you have any questions.

Comment 3

[Andrew Halberstadt [:ahal]]

Sebastiaan is a contributor who has been working on this with me through e-mail.

Comment 4

[Sebastiaan de Haan [:kyr0]]

Attachment: name.patch

Comment 5

[Andrew Halberstadt [:ahal]]

Comment on attachment 812135 [details] [diff] [review]
name.patch

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

This is great! I have a few minor changes for you to make. Please update your patch and upload the new version to bugzilla and I'll get :gps to review it.

::: build/mach_bootstrap.py
@@ +105,5 @@
>      },
> +    'disabled': {
> +        'short': 'Disabled commands',
> +        'long': 'These commands are disabled for your configuration',
> +        'priority': 10,

Make this a priority 0 and place it below the 'misc' category to signify these are the least important.

::: python/mach/mach/dispatcher.py
@@ +145,5 @@
>          # arguments corresponding to command names. This has the side-effect
>          # that argparse renders it nicely.
>          r = self._mach_registrar
>  
> +        # creating the disabled group for commands disabled to the system. bug:908868

No need for this comment, it's clear what's going on from the code

@@ +147,5 @@
>          r = self._mach_registrar
>  
> +        # creating the disabled group for commands disabled to the system. bug:908868
> +        title, description, _priority = r.categories['disabled']
> +        group_disabled = parser.add_argument_group(title, description)

This works, but if you create the group up here, then there will always be a disabled group that shows up, even if no commands are actually disabled.

Here you should just have group_disabled = None, and then move the part that actually creates it down below the 'if is_filtered' statement (don't forget to add an if group_disabled is None around it :)). Hint: take a look at how the normal groups are created.

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 812135 [details] [diff] [review]
name.patch

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

::: build/mach_bootstrap.py
@@ +104,5 @@
>          'priority': 20,
>      },
> +    'disabled': {
> +        'short': 'Disabled commands',
> +        'long': 'These commands are disabled for your configuration',

Please add the string ", run 'mach <command>' to see why"

Comment 7

[Sebastiaan de Haan [:kyr0]]

Attachment: Patch with updates recommended through the comments.

Comment 8

[Andrew Halberstadt [:ahal]]

Comment on attachment 812191 [details] [diff] [review]
Patch with updates recommended through the comments.

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

Looks good! Though for the final review, you'll want to have a single patch with all the changes in it (so it's easier to commit). I'll get that uploaded here for you.

Comment 9

[Andrew Halberstadt [:ahal]]

Attachment: Last two patches folded into one

This is just Sebastiaan's last two patches folded into one.

I tested this locally and it appears the disabled group doesn't appear at the bottom of the category list. I thought it would be based on priority, but I guess it's based on order it gets added. We'll need to work around this.

Sebastiaan, so now there's a new requirement :). The call to add_argument_group for the 'disabled' category has to happen after all of the calls for the normal categories. This means instead of adding disabled commands to the disabled group right away, you'll need to save them in a separate list, and then add them all after the fact.

Comment 10

[Sebastiaan de Haan [:kyr0]]

Attachment: 908868.patch

This is my suggestion to add the recommendations that Andrew asked in comment #9!

Comment 11

[Andrew Halberstadt [:ahal]]

Comment on attachment 812586 [details] [diff] [review]
908868.patch

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

Thanks! This is almost there. I know I'm being super picky (and I'm really sorry to ask you to make another patch), but I really appreciate the work you're doing.

::: build/mach_bootstrap.py
@@ +109,5 @@
>          'priority': 10,
> +    },
> +    'disabled': {
> +        'short': 'Disabled commands',
> +        'long': 'Run "mach <command>" to see why.',

Sorry, I wasn't clear in my last feedback (my fault). I meant change the long text to 'These commands are unavailable for your current context, run "mach <command>" to see why.'

::: python/mach/mach/dispatcher.py
@@ +144,5 @@
>          # we create groups in the ArgumentParser and populate each group with
>          # arguments corresponding to command names. This has the side-effect
>          # that argparse renders it nicely.
>          r = self._mach_registrar
> +        group_disabled = None

You can get rid of this variable now (see comment below)

@@ +172,5 @@
>                              break
>                      if is_filtered:
> +                        description = handler.description
> +                        entry = {'command': command, 'description': description}
> +                        

Please remove this trailing whitespace

@@ +186,5 @@
>                      action='store_true')
>  
> +        if group_disabled is None:
> +            title, description, _priority = r.categories['disabled']
> +            group_disabled = parser.add_argument_group(title, description)

You can get rid of this whole if statement and put the group creation code below the 'if len(commands_disabled)' statement below. You might as well just call the variable 'group' instead of 'group_disabled' too.

Comment 12

[Sebastiaan de Haan [:kyr0]]

I don't mind it at all. It's best we fix it till its perfect. Expect the patch soon!

Comment 13

[Sebastiaan de Haan [:kyr0]]

Attachment: 908868-2.patch

Comment 14

[Andrew Halberstadt [:ahal]]

Comment on attachment 812669 [details] [diff] [review]
908868-2.patch

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

Awesome thanks! Let's get a final review from :gps who is the mach module owner.

::: python/mach/mach/dispatcher.py
@@ +181,5 @@
>  
>                  description = handler.description
>                  group.add_argument(command, help=description,
>                      action='store_true')
> +        

nit: some trailing whitespace here

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 812669 [details] [diff] [review]
908868-2.patch

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

Sorry for taking so long on review - this got caught up in my travels to Brussels for the summit!

Please upload a new patch with nits addressed, carry over the r+, and put checkin-needed in the keywords and this will magically land.

::: python/mach/mach/dispatcher.py
@@ +182,5 @@
>                  description = handler.description
>                  group.add_argument(command, help=description,
>                      action='store_true')
> +        
> +        if len(disabled_commands) > 0:

Nit: if disabled_commands:

does the same thing since bool(<list>) is False iff the list is empty!

Comment 16

[Andrew Halberstadt [:ahal]]

Comment on attachment 812669 [details] [diff] [review]
908868-2.patch

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

Oh, I have one last minute nit to include as well

::: build/mach_bootstrap.py
@@ +108,5 @@
>          'long': 'Potent potables and assorted snacks.',
>          'priority': 10,
> +    },
> +    'disabled': {
> +        'short': 'disabled',

nit: please capitalize the short name for consistency with the others

Comment 17

[Sebastiaan de Haan [:kyr0]]

Attachment: 908868.patch

Carrying r+ forward.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6088d4495e46

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6088d4495e46