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

RESOLVED FIXED in mozilla18

Status

Firefox Build System
Mach Core
RESOLVED FIXED
6 years ago
5 months ago

People

(Reporter: dholbert, Assigned: gps)

Tracking

Trunk
mozilla18
x86_64
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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)
(Reporter)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Comment 3

6 years ago
Created attachment 666128 [details] [diff] [review]
Improve help text, v1

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)
(Assignee)

Comment 4

6 years ago
Created attachment 666131 [details] [diff] [review]
Improve help text, v2

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 5

6 years ago
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+
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/mozilla-central/rev/573907d351e3
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Updated

5 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.