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

RESOLVED FIXED in mozilla27

Status

enhancement
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: ahal, Assigned: sebastiaan)

Tracking

unspecified
mozilla27
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=ahal])

Attachments

(1 attachment, 5 obsolete attachments)

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?
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.
Whiteboard: [good first bug][mentor=ahal]
Please feel free to ping me here or on irc (ahal) if you have any questions.
Sebastiaan is a contributor who has been working on this with me through e-mail.
Assignee: nobody → me
Status: NEW → ASSIGNED
Posted patch name.patch (obsolete) — Splinter Review
Attachment #812135 - Flags: review?(ahalberstadt)
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.
Attachment #812135 - Flags: review?(ahalberstadt)
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 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.
Attachment #812191 - Flags: feedback+
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.
Attachment #812135 - Attachment is obsolete: true
Attachment #812191 - Attachment is obsolete: true
Attachment #812242 - Flags: feedback+
Posted patch 908868.patch (obsolete) — Splinter Review
This is my suggestion to add the recommendations that Andrew asked in comment #9!
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.
Attachment #812586 - Flags: feedback+
I don't mind it at all. It's best we fix it till its perfect. Expect the patch soon!
Posted patch 908868-2.patch (obsolete) — Splinter Review
Attachment #812586 - Attachment is obsolete: true
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
Attachment #812669 - Flags: review?(gps)
Attachment #812669 - Flags: feedback+
Attachment #812242 - Attachment is obsolete: true
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!
Attachment #812669 - Flags: review?(gps) → review+
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
Posted patch 908868.patchSplinter Review
Carrying r+ forward.
Attachment #812669 - Attachment is obsolete: true
Attachment #815914 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6088d4495e46
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.