Bug 1076649 - Remove the '+' prefixing from mach commands with allow_…

…all_arguments=True. r=gps

The reason to use '+' prefixing was to distinguish between options to the
mach command itself, and options that are passed down to whatever the
command does (like mach run passing down args to the built application).
That makes things unnecessarily awkward, and quite non-standard.

Instead, use standard '-' prefixing, and pass all the unknown arguments
down. If there is overlap between the known arguments and arguments supported
by the underlying tool (like -remote when using mach run), it is possible to
use '--' to mark all following arguments as being targetted at the underlying
tool.

For instance:
    mach run -- -remote something
would run
    firefox -remote something
while
    mach run -remote something
would run
    firefox something

As allow_all_arguments is redundant with the presence of a argparse.REMAINDER
CommandArgument, allow_all_arguments is removed. The only mach command with a
argparse.REMAINDER CommandArgument without allow_all_arguments was "mach dmd",
and it did so because it didn't want to use '+' prefixes.

python/mach/mach/base.py

@@ -77,9 +77,6 @@ class MethodHandler(object):
         # Description of the purpose of this command.
         'description',
 
-        # Whether to allow all arguments from the parser.
-        'allow_all_arguments',
-
         # Functions used to 'skip' commands if they don't meet the conditions
         # in a given context.
         'conditions',
@@ -94,15 +91,13 @@ class MethodHandler(object):
     )
 
     def __init__(self, cls, method, name, category=None, description=None,
-        allow_all_arguments=False, conditions=None, parser=None, arguments=None,
-        pass_context=False):
+        conditions=None, parser=None, arguments=None, pass_context=False):
 
         self.cls = cls
         self.method = method
         self.name = name
         self.category = category
         self.description = description
-        self.allow_all_arguments = allow_all_arguments
         self.conditions = conditions or []
         self.parser = parser
         self.arguments = arguments or []

python/mach/mach/decorators.py

@@ -4,6 +4,7 @@
 
 from __future__ import unicode_literals
 
+import argparse
 import collections
 import inspect
 import types
@@ -56,8 +57,8 @@ def CommandProvider(cls):
         if not isinstance(value, types.FunctionType):
             continue
 
-        command_name, category, description, allow_all, conditions, parser = getattr(
-            value, '_mach_command', (None, None, None, None, None, None))
+        command_name, category, description, conditions, parser = getattr(
+            value, '_mach_command', (None, None, None, None, None))
 
         if command_name is None:
             continue
@@ -82,9 +83,8 @@ def CommandProvider(cls):
         arguments = getattr(value, '_mach_command_args', None)
 
         handler = MethodHandler(cls, attr, command_name, category=category,
-            description=description, allow_all_arguments=allow_all,
-            conditions=conditions, parser=parser, arguments=arguments,
-            pass_context=pass_context)
+            description=description, conditions=conditions, parser=parser,
+            arguments=arguments, pass_context=pass_context)
 
         Registrar.register_command_handler(handler)
 
@@ -102,9 +102,6 @@ class Command(object):
 
          description -- A brief description of what the command does.
 
-         allow_all_args -- Bool indicating whether to allow unknown arguments
-             through to the command.
-
          parser -- an optional argparse.ArgumentParser instance to use as
              the basis for the command arguments.
 
@@ -114,18 +111,17 @@ class Command(object):
         def foo(self):
             pass
     """
-    def __init__(self, name, category=None, description=None,
-                 allow_all_args=False, conditions=None, parser=None):
+    def __init__(self, name, category=None, description=None, conditions=None,
+                 parser=None):
         self._name = name
         self._category = category
         self._description = description
-        self._allow_all_args = allow_all_args
         self._conditions = conditions
         self._parser = parser
 
     def __call__(self, func):
         func._mach_command = (self._name, self._category, self._description,
-                              self._allow_all_args, self._conditions, self._parser)
+                              self._conditions, self._parser)
 
         return func
 
@@ -145,6 +141,11 @@ def foo(self):
             pass
     """
     def __init__(self, *args, **kwargs):
+        if kwargs.get('nargs') == argparse.REMAINDER:
+            # These are the assertions we make in dispatcher.py about
+            # those types of CommandArguments.
+            assert len(args) == 1
+            assert all(k in ('default', 'nargs', 'help') for k in kwargs)
         self._command_args = (args, kwargs)
 
     def __call__(self, func):

python/mach/mach/dispatcher.py

@@ -135,19 +135,27 @@ def __call__(self, parser, namespace, values, option_string=None):
         parser_args = {
             'add_help': False,
             'usage': '%(prog)s [global arguments] ' + command +
-                ' command arguments]',
+                ' [command arguments]',
         }
 
-        if handler.allow_all_arguments:
-            parser_args['prefix_chars'] = '+'
-
         if handler.parser:
             subparser = handler.parser
         else:
             subparser = argparse.ArgumentParser(**parser_args)
 
+        remainder = None
+
         for arg in handler.arguments:
-            subparser.add_argument(*arg[0], **arg[1])
+            if arg[1].get('nargs') == argparse.REMAINDER:
+                # parse_known_args expects all argparse.REMAINDER ('...')
+                # arguments to be all stuck together. Instead, we want them to
+                # pick any extra argument, wherever they are.
+                # Assume a limited CommandArgument for those arguments.
+                assert len(arg[0]) == 1
+                assert all(k in ('default', 'nargs', 'help') for k in arg[1])
+                remainder = arg
+            else:
+                subparser.add_argument(*arg[0], **arg[1])
 
         # We define the command information on the main parser result so as to
         # not interfere with arguments passed to the command.
@@ -156,7 +164,32 @@ def __call__(self, parser, namespace, values, option_string=None):
 
         command_namespace, extra = subparser.parse_known_args(args)
         setattr(namespace, 'command_args', command_namespace)
-        if extra:
+        if remainder:
+            (name,), options = remainder
+            # parse_known_args usefully puts all arguments after '--' in
+            # extra, but also puts '--' there. We don't want to pass it down
+            # to the command handler. Note that if multiple '--' are on the
+            # command line, only the first one is removed, so that subsequent
+            # ones are passed down.
+            if '--' in extra:
+                extra.remove('--')
+
+            # Commands with argparse.REMAINDER arguments used to force the
+            # other arguments to be '+' prefixed. If a user now passes such
+            # an argument, if will silently end up in extra. So, check if any
+            # of the allowed arguments appear in a '+' prefixed form, and error
+            # out if that's the case.
+            for args, _ in handler.arguments:
+                for arg in args:
+                    arg = arg.replace('-', '+', 1)
+                    if arg in extra:
+                        raise UnrecognizedArgumentError(command, [arg])
+
+            if extra:
+                setattr(command_namespace, name, extra)
+            else:
+                setattr(command_namespace, name, options.get('default', []))
+        elif extra:
             raise UnrecognizedArgumentError(command, extra)
 
     def _handle_main_help(self, parser, verbose):
@@ -234,9 +267,6 @@ def _handle_subcommand_help(self, parser, command):
             'add_help': False,
         }
 
-        if handler.allow_all_arguments:
-            parser_args['prefix_chars'] = '+'
-
         if handler.parser:
             c_parser = handler.parser
             c_parser.formatter_class = NoUsageFormatter

python/mach_commands.py

@@ -25,7 +25,6 @@
 @CommandProvider
 class MachCommands(MachCommandBase):
     @Command('python', category='devenv',
-        allow_all_args=True,
         description='Run Python.')
     @CommandArgument('args', nargs=argparse.REMAINDER)
     def python(self, args):

python/mozbuild/mozbuild/mach_commands.py

@@ -810,24 +810,21 @@ def get_run_args(mach_command, params, remote, background, noprofile):
     if params:
         args.extend(params)
 
-    if '--' in args:
-        args.remove('--')
-
     return args
 
 @CommandProvider
 class RunProgram(MachCommandBase):
     """Launch the compiled binary"""
 
-    @Command('run', category='post-build', allow_all_args=True,
+    @Command('run', category='post-build',
         description='Run the compiled program.')
-    @CommandArgument('params', default=None, nargs='...',
+    @CommandArgument('params', nargs='...',
         help='Command-line arguments to be passed through to the program. Not specifying a -profile or -P option will result in a temporary profile being used.')
-    @CommandArgument('+remote', '+r', action='store_true',
+    @CommandArgument('-remote', '-r', action='store_true',
         help='Do not pass the -no-remote argument by default.')
-    @CommandArgument('+background', '+b', action='store_true',
+    @CommandArgument('-background', '-b', action='store_true',
         help='Do not pass the -foreground argument by default on Mac')
-    @CommandArgument('+noprofile', '+n', action='store_true',
+    @CommandArgument('-noprofile', '-n', action='store_true',
         help='Do not pass the -profile argument by default.')
     def run(self, params, remote, background, noprofile):
         args = get_run_args(self, params, remote, background, noprofile)
@@ -841,34 +838,34 @@ def run(self, params, remote, background, noprofile):
 class DebugProgram(MachCommandBase):
     """Debug the compiled binary"""
 
-    @Command('debug', category='post-build', allow_all_args=True,
+    @Command('debug', category='post-build',
         description='Debug the compiled program.')
-    @CommandArgument('params', default=None, nargs='...',
+    @CommandArgument('params', nargs='...',
         help='Command-line arguments to be passed through to the program. Not specifying a -profile or -P option will result in a temporary profile being used.')
-    @CommandArgument('+remote', '+r', action='store_true',
+    @CommandArgument('-remote', '-r', action='store_true',
         help='Do not pass the -no-remote argument by default')
-    @CommandArgument('+background', '+b', action='store_true',
+    @CommandArgument('-background', '-b', action='store_true',
         help='Do not pass the -foreground argument by default on Mac')
-    @CommandArgument('+debugger', default=None, type=str,
+    @CommandArgument('-debugger', default=None, type=str,
         help='Name of debugger to launch')
-    @CommandArgument('+debugparams', default=None, metavar='params', type=str,
+    @CommandArgument('-debugparams', default=None, metavar='params', type=str,
         help='Command-line arguments to pass to the debugger itself; split as the Bourne shell would.')
     # Bug 933807 introduced JS_DISABLE_SLOW_SCRIPT_SIGNALS to avoid clever
     # segfaults induced by the slow-script-detecting logic for Ion/Odin JITted
     # code.  If we don't pass this, the user will need to periodically type
     # "continue" to (safely) resume execution.  There are ways to implement
     # automatic resuming; see the bug.
-    @CommandArgument('+slowscript', action='store_true',
+    @CommandArgument('-slowscript', action='store_true',
         help='Do not set the JS_DISABLE_SLOW_SCRIPT_SIGNALS env variable; when not set, recoverable but misleading SIGSEGV instances may occur in Ion/Odin JIT code')
-    @CommandArgument('+noprofile', '+n', action='store_true',
+    @CommandArgument('-noprofile', '-n', action='store_true',
         help='Do not pass the -profile argument by default.')
     def debug(self, params, remote, background, debugger, debugparams, slowscript, noprofile):
         # Parameters come from the CLI. We need to convert them before their use.
         if debugparams:
             import pymake.process
             argv, badchar = pymake.process.clinetoargv(debugparams, os.getcwd())
             if badchar:
-                print("The +debugparams you passed require a real shell to parse them.")
+                print("The -debugparams you passed require a real shell to parse them.")
                 print("(We can't handle the %r character.)" % (badchar,))
                 return 1
             debugparams = argv;
@@ -925,7 +922,7 @@ class RunDmd(MachCommandBase):
 
     @Command('dmd', category='post-build',
         description='Run the compiled program with DMD enabled.')
-    @CommandArgument('params', default=None, nargs='...',
+    @CommandArgument('params', nargs='...',
         help=('Command-line arguments to be passed through to the program. '
               'Not specifying a -profile or -P option will result in a '
               'temporary profile being used. If passing -params use a "--" to '

tools/mach_commands.py

@@ -294,7 +294,7 @@ def pastebin(self, language, poster, duration, file):
 
 @CommandProvider
 class ReviewboardToolsProvider(MachCommandBase):
-    @Command('rbt', category='devenv', allow_all_args=True,
+    @Command('rbt', category='devenv',
         description='Run Reviewboard Tools')
     @CommandArgument('args', nargs='...', help='Arguments to rbt tool')
     def rbt(self, args):