Closed Bug 1255470 Opened 4 years ago Closed 4 years ago

Don't assume we're trying to run a subcommand just because there are args

Categories

(Firefox Build System :: Mach Core, enhancement)

enhancement
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: ahal, Assigned: ahal)

Details

Attachments

(1 file)

It's not currently possible to have a command with subcommands as well as args. This we raise if args[0] is not in subcommand_handlers:
https://dxr.mozilla.org/mozilla-central/source/python/mach/mach/dispatcher.py#164

Instead we should continue running the parent command with all args.
This is currently preventing a command from having both args and subcommands at the same
time.

Review commit: https://reviewboard.mozilla.org/r/39227/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39227/
Attachment #8729058 - Flags: review?(gps)
Comment on attachment 8729058 [details]
MozReview Request: Bug 1255470 - [mach] don't raise if a subcommand exists and but wasn't found in args, r?gps

https://reviewboard.mozilla.org/r/39227/#review36061

::: python/mach/mach/dispatcher.py
(Diff revision 1)
> -                if subcommand[0] == '-':

I wonder why I added this when I wrote this code. Sadly, there is no inline comment, nothing in the commit message, and nothing in the code review.

I have a bad feeling this is here for a reason. But without any evidence of its importance, I have no reason to cling to it.

::: python/mach/mach/dispatcher.py
(Diff revision 1)
> -                if subcommand not in handler.subcommand_handlers:
> -                    raise UnknownCommandError(subcommand, 'run',
> -                        handler.subcommand_handlers.keys())
> -
>                  handler = handler.subcommand_handlers[subcommand]

The last line will raise a KeyError if subcommand isn't present. So why are you removing the check that raises a better exception type?
Attachment #8729058 - Flags: review?(gps)
https://reviewboard.mozilla.org/r/39227/#review36061

> I wonder why I added this when I wrote this code. Sadly, there is no inline comment, nothing in the commit message, and nothing in the code review.
> 
> I have a bad feeling this is here for a reason. But without any evidence of its importance, I have no reason to cling to it.

I presume it's here to enforce:
./mach command subcommand --argument

and prevent:
./mach command --argument subcommand

But this check assumed that commands with a subcommand cannot have arguments themselves. What this patch is trying to accomplish is to support both:
./mach command arg1 arg2

and:
./mach command subcommand arg1 arg2

So the side effect you're worried about is that:
./mach command --argument subcommand

will no longer be an error, and the subcommand handler will not be used. This patch was the simplest way forward, but if you're worried I could refactor this a bit more heavily and tackle all edge cases. Fwiw, |mach file-info| is the only command currently using SubCommands.

> The last line will raise a KeyError if subcommand isn't present. So why are you removing the check that raises a better exception type?

If subcommand isn't present, then we wouldn't be in this if block in the first place so that `raise UnknownCommandError` would be dead code.
Comment on attachment 8729058 [details]
MozReview Request: Bug 1255470 - [mach] don't raise if a subcommand exists and but wasn't found in args, r?gps

https://reviewboard.mozilla.org/r/39227/#review36235

Thanks for the explanation. r+
Attachment #8729058 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/5c9c719705b1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.