Bug 1076649 - Remove the '+' prefixing from mach commands with allow_all_arguments=True. r=gps
authorMike Hommey <mh+mozilla@glandium.org>
Tue, 07 Oct 2014 07:36:27 +0900
changeset 232299 4e8f29e386bda1cb17ffb0e02e62cf6ece3cc000
parent 232298 85663bd538c30f0456f3fd041ab9cfefb6f8154f
child 232300 7c160422459608e12142f82b50822b27d2ffdedb
push id4187
push userbhearsum@mozilla.com
push dateFri, 28 Nov 2014 15:29:12 +0000
treeherdermozilla-beta@f23cc6a30c11 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgps
bugs1076649
milestone35.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
python/mach/mach/decorators.py
python/mach/mach/dispatcher.py
python/mach_commands.py
python/mozbuild/mozbuild/mach_commands.py
tools/mach_commands.py
--- a/python/mach/mach/base.py
+++ b/python/mach/mach/base.py
@@ -72,39 +72,34 @@ class MethodHandler(object):
         'name',
 
         # String category this command belongs to.
         'category',
 
         # 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',
 
         # argparse.ArgumentParser instance to use as the basis for command
         # arguments.
         'parser',
 
         # Arguments added to this command's parser. This is a 2-tuple of
         # positional and named arguments, respectively.
         'arguments',
     )
 
     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 []
         self.pass_context = pass_context
 
--- a/python/mach/mach/decorators.py
+++ b/python/mach/mach/decorators.py
@@ -1,14 +1,15 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 from __future__ import unicode_literals
 
+import argparse
 import collections
 import inspect
 import types
 
 from .base import (
     MachError,
     MethodHandler
 )
@@ -51,18 +52,18 @@ def CommandProvider(cls):
     # define commands multiple times. We also sort keys so commands defined in
     # the same class are grouped in a sane order.
     for attr in sorted(cls.__dict__.keys()):
         value = cls.__dict__[attr]
 
         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
 
         if conditions is None and Registrar.require_conditions:
             continue
 
         msg = 'Mach command \'%s\' implemented incorrectly. ' + \
@@ -77,19 +78,18 @@ def CommandProvider(cls):
         for c in conditions:
             if not hasattr(c, '__call__'):
                 msg = msg % (command_name, type(c))
                 raise MachError(msg)
 
         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)
 
     return cls
 
 
 class Command(object):
     """Decorator for functions or methods that provide a mach subcommand.
@@ -97,40 +97,36 @@ class Command(object):
     The decorator accepts arguments that define basic attributes of the
     command. The following arguments are recognized:
 
          category -- The string category to which this command belongs. Mach's
              help will group commands by category.
 
          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.
 
     For example:
 
         @Command('foo', category='misc', description='Run the foo action')
         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
 
 
 class CommandArgument(object):
     """Decorator for additional arguments to mach subcommands.
 
     This decorator should be used to add arguments to mach commands. Arguments
@@ -140,16 +136,21 @@ class CommandArgument(object):
 
         @Command('foo', help='Run the foo action')
         @CommandArgument('-b', '--bar', action='store_true', default=False,
             help='Enable bar mode.')
         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):
         command_args = getattr(func, '_mach_command_args', [])
 
         command_args.insert(0, self._command_args)
 
         func._mach_command_args = command_args
--- a/python/mach/mach/dispatcher.py
+++ b/python/mach/mach/dispatcher.py
@@ -130,38 +130,71 @@ class CommandAction(argparse.Action):
         # We create a new parser, populate it with the command's arguments,
         # then feed all remaining arguments to it, merging the results
         # with ourselves. This is essentially what argparse subparsers
         # do.
 
         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.
         setattr(namespace, 'mach_handler', handler)
         setattr(namespace, 'command', command)
 
         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):
         # Since we don't need full sub-parser support for the main help output,
         # 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
@@ -229,19 +262,16 @@ class CommandAction(argparse.Action):
         # just the command data then supplement the main help's output with
         # this 2nd parser's. We use a custom formatter class to ignore some of
         # the help output.
         parser_args = {
             'formatter_class': CommandFormatter,
             'add_help': False,
         }
 
-        if handler.allow_all_arguments:
-            parser_args['prefix_chars'] = '+'
-
         if handler.parser:
             c_parser = handler.parser
             c_parser.formatter_class = NoUsageFormatter
             # Accessing _action_groups is a bit shady. We are highly dependent
             # on the argparse implementation not changing. We fail fast to
             # detect upstream changes so we can intelligently react to them.
             group = c_parser._action_groups[1]
 
--- a/python/mach_commands.py
+++ b/python/mach_commands.py
@@ -20,17 +20,16 @@ from mach.decorators import (
     CommandProvider,
     Command,
 )
 
 
 @CommandProvider
 class MachCommands(MachCommandBase):
     @Command('python', category='devenv',
-        allow_all_args=True,
         description='Run Python.')
     @CommandArgument('args', nargs=argparse.REMAINDER)
     def python(self, args):
         # Avoid logging the command
         self.log_manager.terminal_handler.setLevel(logging.CRITICAL)
 
         self._activate_virtualenv()
 
--- a/python/mozbuild/mozbuild/mach_commands.py
+++ b/python/mozbuild/mozbuild/mach_commands.py
@@ -805,75 +805,72 @@ def get_run_args(mach_command, params, r
         if not os.path.isdir(path):
             os.makedirs(path)
         args.append('-profile')
         args.append(path)
 
     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)
         if not args:
             return 1
 
         return self.run_process(args=args, ensure_exit_code=False,
             pass_thru=True)
 
 @CommandProvider
 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;
 
         import mozdebug
 
         if not debugger:
             # No debugger name was provided. Look for the default ones on current OS.
@@ -920,17 +917,17 @@ class DebugProgram(MachCommandBase):
             ensure_exit_code=False, pass_thru=True)
 
 @CommandProvider
 class RunDmd(MachCommandBase):
     """Launch the compiled binary with DMD enabled"""
 
     @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 '
               'indicate the start of params to pass to firefox.'))
     @CommandArgument('--remote', '-r', action='store_true',
         help='Do not pass the -no-remote argument by default.')
     @CommandArgument('--background', '-b', action='store_true',
         help='Do not pass the -foreground argument by default on Mac')
--- a/tools/mach_commands.py
+++ b/tools/mach_commands.py
@@ -289,17 +289,17 @@ class PastebinProvider(object):
         except urllib2.URLError:
             print('ERROR. Could not connect to pastebin.mozilla.org.')
             return 1
         return 0
 
 
 @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):
         if not args:
             args = ['help']
 
         self._activate_virtualenv()
         self.virtualenv_manager.install_pip_package('RBTools==0.6')