Closed Bug 795491 Opened 7 years ago Closed 7 years ago

when I invoke mach with a misspelled command, its error output is redundant and includes odd "u" prefixes

Categories

(Firefox Build System :: Mach Core, enhancement)

x86_64
Linux
enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla18

People

(Reporter: dholbert, Assigned: gps)

Details

Attachments

(1 file, 1 obsolete file)

STR:
 Run ./mach warning-summary

(note that I've intentionally misspelled the command -- the correct command is uses plural "warnings" in "warnings-summary")

ACTUAL RESULTS: This is printed to my terminal:
=================
usage: mach [-h] [--settings FILENAME] [-v] [-l FILENAME] [--log-interval]
            
            {build,settings-list,settings-create,test,mochitest-plain,mochitest-chrome,mochitest-browser,xpcshell-test,warnings-summary,warnings-list}
            ...
mach: error: argument action: invalid choice: 'warning-summary' (choose from u'build', u'settings-list', u'settings-create', u'test', u'mochitest-plain', u'mochitest-chrome', u'mochitest-browser', u'xpcshell-test', u'warnings-summary', u'warnings-list')
=================

EXPECTED RESULTS:
 * No duplicate list of commands (there are 2 lists of the build,settings-list, etc. commands above)
 * No weird u'...' prefixes on the options. (in the second list above)
Maybe the first list of options is supposed to be part of the "usage: ..." line?  If so, that's not immediately clear, to me at least, due to the blank line between "usage" and that list.
Removing the duplicate list of commands is definitely on my todo list. I think it requires reimplementing parts of argparse, sadly. I could be wrong. Maybe a Python guru knows better than me.

Again, blame Python on the u'' prefixes. I'm really not sure why it is doing that. Typically when you print() a unicode it drops the u'' prefix. Oh, Python.
Attached patch Improve help text, v1 (obsolete) — Splinter Review
The monkeypatching is the least hacky and simplest way to fix this. The alternative would be implementing alternative classes for things in the argparse hierarchy. It would be a lot of work and I'm not sure if the payoff would be that good.

Anyway, the output now:

$ ./mach foo
Invalid command specified. The list of commands is printed below.

usage: mach [global arguments] command [command arguments]

Commands:
  {build,settings-list,settings-create,test,mochitest-plain,mochitest-chrome,mochitest-browser,xpcshell-test,warnings-summary,warnings-list}
    build               Build the tree.
    settings-list       Show available config settings.
    settings-create     Print a new settings file with usage info.
    test                Perform tests.
    mochitest-plain     Run a plain mochitest.
    mochitest-chrome    Run a chrome mochitest.
    mochitest-browser   Run a mochitest with browser chrome.
    xpcshell-test       Run an individual xpcshell test.
    warnings-summary    Show a summary of compiler warnings.
    warnings-list       Show a list of compiler warnings

Global Arguments:
  -h, --help            Show this help message and exit.
  --settings FILENAME   Path to settings file.
  -v, --verbose         Print verbose output.
  -l FILENAME, --log-file FILENAME
                        Filename to write log data to.
  --log-interval        Prefix log line with interval from last message rather
                        than relative time. Note that this is NOT execution
                        time if there are parallel operations.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #666128 - Flags: review?(jhammel)
Now with that silly {foo,bar} list stripped from the help text:

$ ./mach foo
Invalid command specified. The list of commands is below.

usage: mach [global arguments] command [command arguments]

Commands:
    build               Build the tree.
    settings-list       Show available config settings.
    settings-create     Print a new settings file with usage info.
    test                Perform tests.
    mochitest-plain     Run a plain mochitest.
    mochitest-chrome    Run a chrome mochitest.
    mochitest-browser   Run a mochitest with browser chrome.
    xpcshell-test       Run an individual xpcshell test.
    warnings-summary    Show a summary of compiler warnings.
    warnings-list       Show a list of compiler warnings

Global Arguments:
  -h, --help            Show this help message and exit.
  --settings FILENAME   Path to settings file.
  -v, --verbose         Print verbose output.
  -l FILENAME, --log-file FILENAME
                        Filename to write log data to.
  --log-interval        Prefix log line with interval from last message rather
                        than relative time. Note that this is NOT execution
                        time if there are parallel operations.
Attachment #666128 - Attachment is obsolete: true
Attachment #666128 - Flags: review?(jhammel)
Attachment #666131 - Flags: review?(jhammel)
Comment on attachment 666131 [details] [diff] [review]
Improve help text, v2

+    def format_help(self):
+        text = argparse.ArgumentParser.format_help(self)
+
+        # Strip out the silly command list that would preceed the pretty list.
+        #
+        # Commands:
+        #   {foo,bar}
+        #     foo  Do foo.
+        #     bar  Do bar.
+        search = 'Commands:\n  {'

This is pretty hacky :( However, I don't know a better way of doing this OTTOMH. Might be worth noting that this is pretty fragile vs e.g. using an actual API
Attachment #666131 - Flags: review?(jhammel) → review+
I realize it is fragile. I looked at the source of argparse and unfortunately there is no formal API for formatters aside from the high-level "format_help" and "format_usage" functions. While I would like to change specific low-level functions (e.g. the one that does the formatting for a "choices" list), since this isn't part of the formal API, this would be "extremely hacky" rather than "pretty hacky" so I decided against it.

Long term I envision this culminating with us writing our own full-fledged formatter (I'm pretty sure that's what Mercurial did). But, I'd rather not cross that bridge just yet.
https://hg.mozilla.org/mozilla-central/rev/573907d351e3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.