Error descriptions are discarded and no error printed on flag parsing

RESOLVED FIXED in Firefox 67

Status

enhancement
P1
normal
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: ato, Assigned: ato)

Tracking

(Blocks 1 bug)

Version 3
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(13 attachments)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

We discard error descriptions when a problem arises during flag
parsing in geckodriver, typically reducing them to a vague description
about what flag was parsed e.g. "invalid host name". We should
pick up the description from the error and print that to the user.

Secondly, we fail to print anything to stderr when flag parsing
fails due to using the error!() macro from the log crate. When
parsing flags we have not yet enabled the logging subsystem, which
means it defaults to not printing anything. We cannot initialise
it sooner because we may risk printing log entries the user hasn’t
requested.

Assignee: nobody → ato
Priority: -- → P1
Blocks: 1525659

Making use of clap::Arg::default_value() removes duplicated magic
strings, by not having to use .unwrap_or("default value").

Depends on D19423

We don't need an enum for exit codes as we never have to match
on them. Converting them to consts also has the benefit that we
will not have to coerce them to i32.

i32 is the correct type to use here, since it is what std::process::exit()
takes and what the libc crate uses.

Depends on D19424

Instead of returning a tuple of (i32, String), causing us to lose
details and the cause of an error, this patch formalises the error
scenarios that may occur during command-line parsing and startup
of the WebDriver server.

Exit codes are encoded with each variant of FatalError, instead of
being specified in each case.

Type conversions are implemented for clap::Error (flag parsing)
and io::Error (webdriver::server).

Depends on D19426

We replaced --webdriver-port with --port to better match
EdgeDriver and chromedriver, but it was discovered in
https://github.com/mozilla/geckodriver/issues/154 that this broke
Selenium clients.

We decided on a two-spearhead approach by reintroducing --webdriver-port
as a temporary alias, whilst submitting fixes for the Selenium clients.
The fixes to Selenium landed over three years ago.

This patch removes the --webdirver-port alias from geckodriver.

Depends on D19427

Because we emit the flag parsing errors to the log, through error!(),
they are subject to whether the logging subsystem is enabled.
Because logging is disabled by default, no error information is
currently displayed to the user.

Since we cannot initialise logging implicitly due to the risk of
emitting log messages the user did not request, this patch changes
geckodriver to print the flag parsing errors to stderr

Depends on D19429

The help message is implicitly included in clap error descriptions, but
they are not for errors originating from this file. By introducing
a FatalError::help_included() function we make sure we print the
help message in all cases.

An alternative implementation, which perhaps would be more idiomatic,
would be to prepare the help message within the fmt::Display trait
implementation for FatalError, but I could find no way of passing
in a reference to clap::App without storing it in an atomic global const.

Depends on D19430

Flag parsing and application logic belong to one, very long,
spaghetti-like function. This patch introduces a distinction
between checking the sanity of the CLI input and what action to
take depending on that input.

Depends on D19431

For completeness, this makes sure the --help flag is included in
its own help message.

Depends on D19432

This will print something along the line of:

geckodriver: error: invalid --port: invalid digit found in string: asd

Or:

geckodriver: error: invalid --port: number too large to fit in target type: 123123123123

We also Include the error message from IpAddr::from_str() to the user.
This will make the error seen by the user look something like this:

geckodriver: error: invalid IP address syntax: 123123:4444

Depends on D19433

Minor post-patch linting to silence some clippy warnings.

Depends on D19434

Status: NEW → ASSIGNED
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a9357f295d5f
geckodriver: display same version on --help as on --version; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/e2a10e5049be
geckodriver: make use of clap default values; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/f11ef5763129
geckodriver: convert ExitCode to consts; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/342fcc83b613
geckodriver: import logging::Level into the global scope; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/0fdfd128248c
geckodriver: generalise cli errors; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/dfb9154473ca
geckodriver: remove --webdriver-port alias; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/6ad3a665abdf
geckodriver: organise args a bit more consistently; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/e37346a02373
geckodriver: fix missing flag parsing errors; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/16aac9625fde
geckodriver: display help message on non-clap errors; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/b2768473a280
geckodriver: separate flag parsing and application logic; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/561a8cb2db9b
geckodriver: ensure --help is listed in help message; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/a02142b7aeff
geckodriver: provide better error messages for flags; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/9469362459f9
geckodriver: lint main.rs; r=whimboo
You need to log in before you can comment on or make changes to this bug.