1526938 - Error descriptions are discarded and no error printed on flag parsing

Status: RESOLVED FIXED

(Testing :: geckodriver, enhancement, P1)

Description

[Andreas Tolfsen ❲:ato❳]

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.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1526938: geckodriver: display same version on --help as on --version;

Comment 2

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1526938: geckodriver: make use of clap default values;

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

Depends on D19423

Comment 3

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1526938: geckodriver: convert ExitCode to consts;

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

Comment 4

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1526938: geckodriver: import logging::Level into the global scope;

Depends on D19425

Comment 5

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1526938: geckodriver: generalise cli errors;

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

Comment 6

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1526938: geckodriver: remove --webdriver-port alias;

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

Comment 7

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1526938: geckodriver: organise args a bit more consistently;

Depends on D19428

Comment 8

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1526938: geckodriver: fix missing flag parsing errors;

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

Comment 9

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1526938: geckodriver: display help message on non-clap errors;

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

Comment 10

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1526938: geckodriver: separate flag parsing and application logic;

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

Comment 11

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1526938: geckodriver: ensure --help is listed in help message;

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

Depends on D19432

Comment 12

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1526938: geckodriver: provide better error messages for flags;

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

Comment 13

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1526938: geckodriver: lint main.rs;

Minor post-patch linting to silence some clippy warnings.

Depends on D19434

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/a9357f295d5f
https://hg.mozilla.org/mozilla-central/rev/e2a10e5049be
https://hg.mozilla.org/mozilla-central/rev/f11ef5763129
https://hg.mozilla.org/mozilla-central/rev/342fcc83b613
https://hg.mozilla.org/mozilla-central/rev/0fdfd128248c
https://hg.mozilla.org/mozilla-central/rev/dfb9154473ca
https://hg.mozilla.org/mozilla-central/rev/6ad3a665abdf
https://hg.mozilla.org/mozilla-central/rev/e37346a02373
https://hg.mozilla.org/mozilla-central/rev/16aac9625fde
https://hg.mozilla.org/mozilla-central/rev/b2768473a280
https://hg.mozilla.org/mozilla-central/rev/561a8cb2db9b
https://hg.mozilla.org/mozilla-central/rev/a02142b7aeff
https://hg.mozilla.org/mozilla-central/rev/9469362459f9