Bug 1682239 - [geckodriver] Use anyhow for errors in geckodriver. r=whimboo,webdriver-reviewers,jgraham

Differential Revision: https://phabricator.services.mozilla.com/D195255

Cargo.lock

@@ -2055,6 +2055,7 @@ dependencies = [
 name = "geckodriver"
 version = "0.33.0"
 dependencies = [
+ "anyhow",
  "base64 0.21.3",
  "chrono",
  "clap",
@@ -2073,6 +2074,7 @@ dependencies = [
  "serde_json",
  "serde_yaml",
  "tempfile",
+ "thiserror",
  "unicode-segmentation",
  "url",
  "uuid",

testing/geckodriver/Cargo.toml

@@ -21,6 +21,7 @@ license = "MPL-2.0"
 repository = "https://hg.mozilla.org/mozilla-central/file/tip/testing/geckodriver"
 
 [dependencies]
+anyhow = "1"
 base64 = "0.21"
 chrono = "0.4.6"
 clap = { version = "4", default-features = false, features = ["cargo", "std", "suggestions", "wrap_help", "string"] }
@@ -38,6 +39,7 @@ serde_derive = "1.0"
 serde_json = "1.0"
 serde_yaml = "0.8"
 tempfile = "3"
+thiserror = "1"
 unicode-segmentation = "1.9"
 url = "2.4"
 uuid = { version = "1.0", features = ["v4"] }

testing/geckodriver/src/android.rs

@@ -3,9 +3,9 @@ use mozdevice::{AndroidStorage, Device, Host, UnixPathBuf};
 use mozprofile::profile::Profile;
 use serde::Serialize;
 use serde_yaml::{Mapping, Value};
-use std::fmt;
 use std::io;
 use std::time;
+use thiserror::Error;
 use webdriver::error::{ErrorStatus, WebDriverError};
 
 // TODO: avoid port clashes across GeckoView-vehicles.
@@ -20,47 +20,22 @@ const CONFIG_FILE_HEADING: &str = r#"## GeckoView configuration YAML
 
 pub type Result<T> = std::result::Result<T, AndroidError>;
 
-#[derive(Debug)]
+#[derive(Debug, Error)]
 pub enum AndroidError {
+    #[error("Activity for package '{0}' not found")]
     ActivityNotFound(String),
-    Device(mozdevice::DeviceError),
-    IO(io::Error),
+
+    #[error(transparent)]
+    Device(#[from] mozdevice::DeviceError),
+
+    #[error(transparent)]
+    IO(#[from] io::Error),
+
+    #[error("Package '{0}' not found")]
     PackageNotFound(String),
-    Serde(serde_yaml::Error),
-}
 
-impl fmt::Display for AndroidError {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match *self {
-            AndroidError::ActivityNotFound(ref package) => {
-                write!(f, "Activity for package '{}' not found", package)
-            }
-            AndroidError::Device(ref message) => message.fmt(f),
-            AndroidError::IO(ref message) => message.fmt(f),
-            AndroidError::PackageNotFound(ref package) => {
-                write!(f, "Package '{}' not found", package)
-            }
-            AndroidError::Serde(ref message) => message.fmt(f),
-        }
-    }
-}
-
-impl From<io::Error> for AndroidError {
-    fn from(value: io::Error) -> AndroidError {
-        AndroidError::IO(value)
-    }
-}
-
-impl From<mozdevice::DeviceError> for AndroidError {
-    fn from(value: mozdevice::DeviceError) -> AndroidError {
-        AndroidError::Device(value)
-    }
-}
-
-impl From<serde_yaml::Error> for AndroidError {
-    fn from(value: serde_yaml::Error) -> AndroidError {
-        AndroidError::Serde(value)
-    }
+    #[error(transparent)]
+    Serde(#[from] serde_yaml::Error),
 }
 
 impl From<AndroidError> for WebDriverError {

testing/geckodriver/src/capabilities.rs

@@ -18,37 +18,24 @@ use serde_json::{Map, Value};
 use std::collections::BTreeMap;
 use std::default::Default;
 use std::ffi::OsString;
-use std::fmt::{self, Display};
 use std::fs;
 use std::io;
 use std::io::BufWriter;
 use std::io::Cursor;
 use std::path::{Path, PathBuf};
 use std::str::{self, FromStr};
+use thiserror::Error;
 use webdriver::capabilities::{BrowserCapabilities, Capabilities};
 use webdriver::error::{ErrorStatus, WebDriverError, WebDriverResult};
 
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Error)]
 enum VersionError {
-    VersionError(mozversion::Error),
+    #[error(transparent)]
+    VersionError(#[from] mozversion::Error),
+    #[error("No binary provided")]
     MissingBinary,
 }
 
-impl From<mozversion::Error> for VersionError {
-    fn from(err: mozversion::Error) -> VersionError {
-        VersionError::VersionError(err)
-    }
-}
-
-impl Display for VersionError {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match *self {
-            VersionError::VersionError(ref x) => x.fmt(f),
-            VersionError::MissingBinary => "No binary provided".fmt(f),
-        }
-    }
-}
-
 impl From<VersionError> for WebDriverError {
     fn from(err: VersionError) -> WebDriverError {
         WebDriverError::new(ErrorStatus::SessionNotCreated, err.to_string())
@@ -398,8 +385,6 @@ pub enum ProfileType {
     Temporary,
 }
 
-
-
 /// Rust representation of `moz:firefoxOptions`.
 ///
 /// Calling `FirefoxOptions::from_capabilities(binary, capabilities)` causes

testing/geckodriver/src/main.rs

@@ -27,11 +27,10 @@ extern crate zip;
 extern crate log;
 
 use std::env;
-use std::fmt;
-use std::io;
 use std::net::{IpAddr, SocketAddr, ToSocketAddrs};
 use std::path::PathBuf;
-use std::result;
+use std::process::ExitCode;
+
 use std::str::FromStr;
 
 use clap::{Arg, ArgAction, Command};
@@ -60,69 +59,13 @@ pub mod test;
 use crate::command::extension_routes;
 use crate::logging::Level;
 use crate::marionette::{MarionetteHandler, MarionetteSettings};
+use anyhow::{bail, Result as ProgramResult};
+use clap::ArgMatches;
 use mozdevice::AndroidStorageInput;
 use url::{Host, Url};
 
-const EXIT_SUCCESS: i32 = 0;
-const EXIT_USAGE: i32 = 64;
-const EXIT_UNAVAILABLE: i32 = 69;
-
-enum FatalError {
-    Parsing(clap::Error),
-    Usage(String),
-    Server(io::Error),
-}
-
-impl FatalError {
-    fn exit_code(&self) -> i32 {
-        use FatalError::*;
-        match *self {
-            Parsing(_) | Usage(_) => EXIT_USAGE,
-            Server(_) => EXIT_UNAVAILABLE,
-        }
-    }
-
-    fn help_included(&self) -> bool {
-        matches!(*self, FatalError::Parsing(_))
-    }
-}
-
-impl From<clap::Error> for FatalError {
-    fn from(err: clap::Error) -> FatalError {
-        FatalError::Parsing(err)
-    }
-}
-
-impl From<io::Error> for FatalError {
-    fn from(err: io::Error) -> FatalError {
-        FatalError::Server(err)
-    }
-}
-
-// harmonise error message from clap to avoid duplicate "error:" prefix
-impl fmt::Display for FatalError {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        use FatalError::*;
-        let s = match *self {
-            Parsing(ref err) => err.to_string(),
-            Usage(ref s) => format!("error: {}", s),
-            Server(ref err) => format!("error: {}", err),
-        };
-        write!(f, "{}", s)
-    }
-}
-
-macro_rules! usage {
-    ($msg:expr) => {
-        return Err(FatalError::Usage($msg.to_string()))
-    };
-
-    ($fmt:expr, $($arg:tt)+) => {
-        return Err(FatalError::Usage(format!($fmt, $($arg)+)))
-    };
-}
-
-type ProgramResult<T> = result::Result<T, FatalError>;
+const EXIT_USAGE: u8 = 64;
+const EXIT_UNAVAILABLE: u8 = 69;
 
 #[allow(clippy::large_enum_variant)]
 enum Operation {
@@ -151,10 +94,10 @@ fn server_address(webdriver_host: &str, webdriver_port: u16) -> ProgramResult<So
     let mut socket_addrs = match format!("{}:{}", webdriver_host, webdriver_port).to_socket_addrs()
     {
         Ok(addrs) => addrs.collect::<Vec<_>>(),
-        Err(e) => usage!("{}: {}:{}", e, webdriver_host, webdriver_port),
+        Err(e) => bail!("{}: {}:{}", e, webdriver_host, webdriver_port),
     };
     if socket_addrs.is_empty() {
-        usage!(
+        bail!(
             "Unable to resolve host: {}:{}",
             webdriver_host,
             webdriver_port
@@ -224,9 +167,7 @@ fn get_allowed_origins(allow_origins: Option<clap::parser::ValuesRef<Url>>) -> V
     allow_origins.into_iter().flatten().cloned().collect()
 }
 
-fn parse_args(cmd: &mut Command) -> ProgramResult<Operation> {
-    let args = cmd.try_get_matches_from_mut(env::args())?;
-
+fn parse_args(args: &ArgMatches) -> ProgramResult<Operation> {
     if args.get_flag("help") {
         return Ok(Operation::Help);
     } else if args.get_flag("version") {
@@ -248,7 +189,7 @@ fn parse_args(cmd: &mut Command) -> ProgramResult<Operation> {
         let s = args.get_one::<String>("webdriver_port").unwrap();
         match u16::from_str(s) {
             Ok(n) => n,
-            Err(e) => usage!("invalid --port: {}: {}", e, s),
+            Err(e) => bail!("invalid --port: {}: {}", e, s),
         }
     };
 
@@ -269,7 +210,7 @@ fn parse_args(cmd: &mut Command) -> ProgramResult<Operation> {
             tempfile::tempdir()
         };
         if tmp_dir.is_err() {
-            usage!("Unable to write to temporary directory; consider --profile-root with a writeable directory")
+            bail!("Unable to write to temporary directory; consider --profile-root with a writeable directory")
         }
     }
 
@@ -277,7 +218,7 @@ fn parse_args(cmd: &mut Command) -> ProgramResult<Operation> {
     let marionette_port = match args.get_one::<String>("marionette_port") {
         Some(s) => match u16::from_str(s) {
             Ok(n) => Some(n),
-            Err(e) => usage!("invalid --marionette-port: {}", e),
+            Err(e) => bail!("invalid --marionette-port: {}", e),
         },
         None => None,
     };
@@ -287,14 +228,14 @@ fn parse_args(cmd: &mut Command) -> ProgramResult<Operation> {
     let websocket_port = match args.get_one::<String>("websocket_port") {
         Some(s) => match u16::from_str(s) {
             Ok(n) => n,
-            Err(e) => usage!("invalid --websocket-port: {}", e),
+            Err(e) => bail!("invalid --websocket-port: {}", e),
         },
         None => 9222,
     };
 
     let host = match parse_hostname(webdriver_host) {
         Ok(name) => name,
-        Err(e) => usage!("invalid --host {}: {}", webdriver_host, e),
+        Err(e) => bail!("invalid --host {}: {}", webdriver_host, e),
     };
 
     let allow_hosts = get_allowed_hosts(host, args.get_many("allow_hosts"));
@@ -326,8 +267,8 @@ fn parse_args(cmd: &mut Command) -> ProgramResult<Operation> {
     })
 }
 
-fn inner_main(cmd: &mut Command) -> ProgramResult<()> {
-    match parse_args(cmd)? {
+fn inner_main(operation: Operation, cmd: &mut Command) -> ProgramResult<()> {
+    match operation {
         Operation::Help => print_help(cmd),
         Operation::Version => print_version(),
 
@@ -365,24 +306,34 @@ fn inner_main(cmd: &mut Command) -> ProgramResult<()> {
     Ok(())
 }
 
-fn main() {
-    use std::process::exit;
-
+fn main() -> ExitCode {
     let mut cmd = make_command();
 
-    // use std::process:Termination when it graduates
-    exit(match inner_main(&mut cmd) {
-        Ok(_) => EXIT_SUCCESS,
-
+    let args = match cmd.try_get_matches_from_mut(env::args()) {
+        Ok(args) => args,
         Err(e) => {
+            // Clap already says "error:" and don't repeat help.
             eprintln!("{}: {}", get_program_name(), e);
-            if !e.help_included() {
-                print_help(&mut cmd);
-            }
-
-            e.exit_code()
+            return ExitCode::from(EXIT_USAGE);
         }
-    });
+    };
+
+    let operation = match parse_args(&args) {
+        Ok(op) => op,
+        Err(e) => {
+            eprintln!("{}: error: {}", get_program_name(), e);
+            print_help(&mut cmd);
+            return ExitCode::from(EXIT_USAGE);
+        }
+    };
+
+    if let Err(e) = inner_main(operation, &mut cmd) {
+        eprintln!("{}: error: {}", get_program_name(), e);
+        print_help(&mut cmd);
+        return ExitCode::from(EXIT_UNAVAILABLE);
+    }
+
+    ExitCode::SUCCESS
 }
 
 fn make_command() -> Command {