1179488 - Accept any arguments passed to deprecated commands

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[(no longer active)]

Without this, we throw UnrecognizedArgumentError when running commands
such as `./mach mochitest-plain test`, which causes an error message
such as the below to be emitted:

It looks like you passed an unrecognized argument into mach.

The mochitest-plain command does not accept the arguments: test

This patch will fix the above command to instead print the corresponding
deprecation message.

Comment 1

[(no longer active)]

Attachment: Accept any arguments passed to deprecated commands

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8628491 [details] [diff] [review]
Accept any arguments passed to deprecated commands

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

See comment for preferred resolution.

::: python/mach/mach/dispatcher.py
@@ +229,5 @@
>              if extra:
>                  setattr(command_namespace, name, extra)
>              else:
>                  setattr(command_namespace, name, options.get('default', []))
> +        elif extra and not handler.cls.__name__ == 'DeprecatedCommands':

The DeprecatedCommands class comes from testing/mochitest/mach_commands.py. This is touching code in mach core. It is therefore a layering violation to check for the presence of a class defined outside the mach core.

The proper way to do this is to implement a "deprecated commands" mach feature or change the arguments of the deprecated commands to accept all arguments. e.g.:

@CommandArgument('args', nargs=argparse.REMAINDER)

Comment 3

[(no longer active)]

(In reply to Gregory Szorc [:gps] from comment #2)
> ::: python/mach/mach/dispatcher.py
> @@ +229,5 @@
> >              if extra:
> >                  setattr(command_namespace, name, extra)
> >              else:
> >                  setattr(command_namespace, name, options.get('default', []))
> > +        elif extra and not handler.cls.__name__ == 'DeprecatedCommands':
> 
> The DeprecatedCommands class comes from testing/mochitest/mach_commands.py.
> This is touching code in mach core. It is therefore a layering violation to
> check for the presence of a class defined outside the mach core.

dispatcher.py already depends on this *exact* same pattern <http://mxr.mozilla.org/mozilla-central/source/python/mach/mach/dispatcher.py#114>.

> The proper way to do this is to implement a "deprecated commands" mach
> feature or change the arguments of the deprecated commands to accept all
> arguments. e.g.:
> 
> @CommandArgument('args', nargs=argparse.REMAINDER)

Do you mean doing that for all DeprecatedCommands forever is better than handling this in a central place?

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8628491 [details] [diff] [review]
Accept any arguments passed to deprecated commands

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

OK. Since we already have the cls.__name__ == 'DeprecatedCommands' hack in dispatcher.py, this is acceptable.

::: python/mach/mach/dispatcher.py
@@ +229,5 @@
>              if extra:
>                  setattr(command_namespace, name, extra)
>              else:
>                  setattr(command_namespace, name, options.get('default', []))
> +        elif extra and not handler.cls.__name__ == 'DeprecatedCommands':

and handler.cls.__name__ != 'DeprecatedCommands':

Comment 5

[(no longer active)]

Thanks!

BTW, if there is a less hacky way to detect the type of this object than looking at its class name, please let me know how and I'll fix it.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b2a95a34829

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/9b2a95a34829