Bug 1431459 - Update rust_mozrunner to use a Builder API, r=ato draft
authorJames Graham <james@hoppipolla.co.uk>
Tue, 19 Dec 2017 18:39:10 +0000
changeset 750089 bc47fa146e0abb222bfeec9e92b6e1173264df1e
parent 750078 5201997e7e01500176ce7d6e790593468f3b4259
push id97541
push userbmo:james@hoppipolla.co.uk
push dateThu, 01 Feb 2018 13:49:50 +0000
reviewersato
bugs1431459
milestone60.0a1
Bug 1431459 - Update rust_mozrunner to use a Builder API, r=ato This is a major API revision to replace the Python-like API with something more idiomatically Rust. In particular you now create a FirefoxRunner object and then call start() and end up with a FirefoxProcess. This is pretty similar to the Command builder in std. MozReview-Commit-ID: DmEfIfKSukA
testing/geckodriver/src/marionette.rs
testing/mozbase/rust/mozrunner/src/runner.rs
--- a/testing/geckodriver/src/marionette.rs
+++ b/testing/geckodriver/src/marionette.rs
@@ -1,14 +1,14 @@
 use hyper::method::Method;
 use logging;
 use logging::LogLevel;
 use mozprofile::preferences::Pref;
 use mozprofile::profile::Profile;
-use mozrunner::runner::{Runner, FirefoxRunner};
+use mozrunner::runner::{FirefoxRunner, FirefoxProcess, Runner, RunnerProcess};
 use regex::Captures;
 use rustc_serialize::base64::FromBase64;
 use rustc_serialize::json;
 use rustc_serialize::json::{Json, ToJson};
 use std::collections::BTreeMap;
 use std::env;
 use std::error::Error;
 use std::fs::File;
@@ -386,17 +386,17 @@ pub struct MarionetteSettings {
     /// level. The Gecko default is LogLevel::Info for optimised
     /// builds and LogLevel::Debug for debug builds.
     pub log_level: Option<LogLevel>,
 }
 
 pub struct MarionetteHandler {
     connection: Mutex<Option<MarionetteConnection>>,
     settings: MarionetteSettings,
-    browser: Option<FirefoxRunner>,
+    browser: Option<FirefoxProcess>,
     current_log_level: Option<LogLevel>,
 }
 
 impl MarionetteHandler {
     pub fn new(settings: MarionetteSettings) -> MarionetteHandler {
         MarionetteHandler {
             connection: Mutex::new(None),
             settings: settings,
@@ -433,55 +433,59 @@ impl MarionetteHandler {
 
         let mut connection = MarionetteConnection::new(port, session_id.clone());
         try!(connection.connect(&mut self.browser));
         self.connection = Mutex::new(Some(connection));
 
         Ok(capabilities)
     }
 
-    fn start_browser(&mut self, port: u16, mut options: FirefoxOptions) -> WebDriverResult<()> {
-        let binary = try!(options.binary
+    fn start_browser(&mut self, port: u16, options: FirefoxOptions) -> WebDriverResult<()> {
+        let binary = options.binary
             .ok_or(WebDriverError::new(ErrorStatus::SessionNotCreated,
                                        "Expected browser binary location, but unable to find \
                                         binary in default location, no \
                                         'moz:firefoxOptions.binary' capability provided, and \
-                                        no binary flag set on the command line")));
-
-        let custom_profile = options.profile.is_some();
+                                        no binary flag set on the command line"))?;
 
-        let mut runner = try!(FirefoxRunner::new(&binary, options.profile.take())
-                              .map_err(|e| WebDriverError::new(ErrorStatus::SessionNotCreated,
-                                                               e.description().to_owned())));
+        let is_custom_profile = options.profile.is_some();
 
-        // double-dashed flags are not accepted on Windows systems
-        runner.args().push("-marionette".to_owned());
-
-        // https://developer.mozilla.org/docs/Environment_variables_affecting_crash_reporting
-        runner.envs().insert("MOZ_CRASHREPORTER".to_string(), "1".to_string());
-        runner.envs().insert("MOZ_CRASHREPORTER_NO_REPORT".to_string(), "1".to_string());
-        runner.envs().insert("MOZ_CRASHREPORTER_SHUTDOWN".to_string(), "1".to_string());
-
-        if let Some(args) = options.args.take() {
-            runner.args().extend(args);
+        let mut profile = match options.profile {
+            Some(x) => x,
+            None => Profile::new(None)?
         };
 
-        try!(self.set_prefs(port, &mut runner.profile, custom_profile, options.prefs)
+        self.set_prefs(port, &mut profile, is_custom_profile, options.prefs)
             .map_err(|e| {
                 WebDriverError::new(ErrorStatus::SessionNotCreated,
                                     format!("Failed to set preferences: {}", e))
-            }));
+            })?;
+
+
+        let mut runner = FirefoxRunner::new(&binary, profile);
 
-        try!(runner.start()
+        runner
+             // double-dashed flags are not accepted on Windows systems
+            .arg("-marionette")
+             // https://developer.mozilla.org/docs/Environment_variables_affecting_crash_reporting
+            .env("MOZ_CRASHREPORTER", "1")
+            .env("MOZ_CRASHREPORTER_NO_REPORT", "1")
+            .env("MOZ_CRASHREPORTER_SHUTDOWN", "1");
+
+        if let Some(args) = options.args.as_ref() {
+            runner.args(args);
+        };
+
+        let browser_proc = runner.start()
             .map_err(|e| {
                 WebDriverError::new(ErrorStatus::SessionNotCreated,
                                     format!("Failed to start browser {}: {}",
                                             binary.display(), e))
-            }));
-        self.browser = Some(runner);
+            })?;
+        self.browser = Some(browser_proc);
 
         Ok(())
     }
 
     pub fn set_prefs(&self, port: u16, profile: &mut Profile, custom_profile: bool,
                      extra_prefs: Vec<(String, Pref)>)
                  -> WebDriverResult<()> {
         let prefs = try!(profile.user_prefs()
@@ -1331,17 +1335,17 @@ impl MarionetteConnection {
     pub fn new(port: u16, session_id: Option<String>) -> MarionetteConnection {
         MarionetteConnection {
             port: port,
             stream: None,
             session: MarionetteSession::new(session_id)
         }
     }
 
-    pub fn connect(&mut self, browser: &mut Option<FirefoxRunner>) -> WebDriverResult<()> {
+    pub fn connect(&mut self, browser: &mut Option<FirefoxProcess>) -> WebDriverResult<()> {
         let timeout = 60 * 1000;  // ms
         let poll_interval = 100;  // ms
         let poll_attempts = timeout / poll_interval;
         let mut poll_attempt = 0;
 
         loop {
             // If the process is gone, immediately abort the connection attempts
             if let &mut Some(ref mut runner) = browser {
--- a/testing/mozbase/rust/mozrunner/src/runner.rs
+++ b/testing/mozbase/rust/mozrunner/src/runner.rs
@@ -1,29 +1,60 @@
 use mozprofile::prefreader::PrefReaderError;
 use mozprofile::profile::Profile;
 use std::ascii::AsciiExt;
 use std::collections::HashMap;
 use std::convert::From;
 use std::env;
 use std::error::Error;
+use std::ffi::{OsStr, OsString};
 use std::fmt;
-use std::io::{Result as IoResult, Error as IoError, ErrorKind};
+use std::io::{Error as IoError, ErrorKind, Result as IoResult};
 use std::path::{Path, PathBuf};
+use std::process::{Child, Command, Stdio};
 use std::process;
-use std::process::{Command, Stdio};
 
 pub trait Runner {
-    fn args(&mut self) -> &mut Vec<String>;
-    fn build_command(&self, &mut Command);
-    fn envs(&mut self) -> &mut HashMap<String, String>;
+    type Process;
+
+    fn arg<'a, S>(&'a mut self, arg: S) -> &'a mut Self
+    where
+        S: AsRef<OsStr>;
+
+    fn args<'a, I, S>(&'a mut self, args: I) -> &'a mut Self
+    where
+        I: IntoIterator<Item = S>,
+        S: AsRef<OsStr>;
+
+    fn env<'a, K, V>(&'a mut self, key: K, value: V) -> &'a mut Self
+    where
+        K: AsRef<OsStr>,
+        V: AsRef<OsStr>;
+
+    fn envs<'a, I, K, V>(&'a mut self, envs: I) -> &'a mut Self
+    where
+        I: IntoIterator<Item = (K, V)>,
+        K: AsRef<OsStr>,
+        V: AsRef<OsStr>;
+
+    fn stdout<'a, T>(&'a mut self, stdout: T) -> &'a mut Self
+    where
+        T: Into<Stdio>;
+
+    fn stderr<'a, T>(&'a mut self, stderr: T) -> &'a mut Self
+    where
+        T: Into<Stdio>;
+
+    fn start(self) -> Result<Self::Process, RunnerError>;
+}
+
+pub trait RunnerProcess {
+    fn status(&mut self) -> IoResult<Option<process::ExitStatus>>;
+    fn stop(&mut self) -> IoResult<process::ExitStatus>;
     fn is_running(&mut self) -> bool;
-    fn start(&mut self) -> Result<(), RunnerError>;
-    fn status(&mut self) -> IoResult<Option<process::ExitStatus>>;
-    fn stop(&mut self) -> IoResult<Option<process::ExitStatus>>;
 }
 
 #[derive(Debug)]
 pub enum RunnerError {
     Io(IoError),
     PrefReader(PrefReaderError),
 }
 
@@ -62,103 +93,159 @@ impl From<IoError> for RunnerError {
 
 impl From<PrefReaderError> for RunnerError {
     fn from(value: PrefReaderError) -> RunnerError {
         RunnerError::PrefReader(value)
     }
 }
 
 #[derive(Debug)]
+pub struct FirefoxProcess {
+    process: Child,
+    profile: Profile
+}
+
+impl RunnerProcess for FirefoxProcess {
+    fn status(&mut self) -> IoResult<Option<process::ExitStatus>> {
+        self.process.try_wait()
+    }
+
+    fn is_running(&mut self) -> bool {
+        self.status().unwrap().is_none()
+    }
+
+    fn stop(&mut self) -> IoResult<process::ExitStatus> {
+        self.process.kill()?;
+        self.process.wait()
+    }
+}
+
+#[derive(Debug)]
 pub struct FirefoxRunner {
-    pub binary: PathBuf,
-    args: Vec<String>,
-    envs: HashMap<String, String>,
-    process: Option<process::Child>,
-    pub profile: Profile
+    binary: PathBuf,
+    profile: Profile,
+    args: Vec<OsString>,
+    envs: HashMap<OsString, OsString>,
+    stdout: Option<Stdio>,
+    stderr: Option<Stdio>,
 }
 
 impl FirefoxRunner {
-    pub fn new(binary: &Path, profile: Option<Profile>) -> IoResult<FirefoxRunner> {
-        let prof = match profile {
-            Some(p) => p,
-            None => try!(Profile::new(None))
-        };
+    pub fn new(binary: &Path, profile: Profile) -> FirefoxRunner {
+        let mut envs: HashMap<OsString, OsString> = HashMap::new();
+        envs.insert("MOZ_NO_REMOTE".into(), "1".into());
+        envs.insert("NO_EM_RESTART".into(), "1".into());
 
-        let mut envs = HashMap::new();
-        envs.insert("MOZ_NO_REMOTE".to_string(), "1".to_string());
-        envs.insert("NO_EM_RESTART".to_string(), "1".to_string());
-
-        Ok(FirefoxRunner {
+        FirefoxRunner {
             binary: binary.to_path_buf(),
-            process: None,
-            args: Vec::new(),
             envs: envs,
-            profile: prof
-        })
+            profile: profile,
+            args: vec![],
+            stdout: None,
+            stderr: None,
+        }
     }
 }
 
 impl Runner for FirefoxRunner {
-    fn args(&mut self) -> &mut Vec<String> {
-        &mut self.args
+    type Process = FirefoxProcess;
+
+    fn arg<'a, S>(&'a mut self, arg: S) -> &'a mut FirefoxRunner
+    where
+        S: AsRef<OsStr>,
+    {
+        self.args.push((&arg).into());
+        self
     }
 
-    fn build_command(&self, command: &mut Command) {
-        command
-            .args(&self.args[..])
-            .envs(&self.envs);
-
-        if !self.args.iter().any(|x| is_profile_arg(x)) {
-            command.arg("-profile").arg(&self.profile.path);
+    fn args<'a, I, S>(&'a mut self, args: I) -> &'a mut FirefoxRunner
+    where
+        I: IntoIterator<Item = S>,
+        S: AsRef<OsStr>,
+    {
+        for arg in args {
+            self.args.push((&arg).into());
         }
-        command.stdout(Stdio::inherit())
-            .stderr(Stdio::inherit());
+        self
     }
 
-    fn envs(&mut self) -> &mut HashMap<String, String> {
-        &mut self.envs
-    }
-
-    fn is_running(&mut self) -> bool {
-        self.process.is_some() && self.status().unwrap().is_none()
+    fn env<'a, K, V>(&'a mut self, key: K, value: V) -> &'a mut FirefoxRunner
+    where
+        K: AsRef<OsStr>,
+        V: AsRef<OsStr>,
+    {
+        self.envs.insert((&key).into(), (&value).into());
+        self
     }
 
-    fn start(&mut self) -> Result<(), RunnerError> {
-        let mut cmd = Command::new(&self.binary);
-        self.build_command(&mut cmd);
+    fn envs<'a, I, K, V>(&'a mut self, envs: I) -> &'a mut FirefoxRunner
+    where
+        I: IntoIterator<Item = (K, V)>,
+        K: AsRef<OsStr>,
+        V: AsRef<OsStr>,
+    {
+        for (key, value) in envs {
+            self.envs.insert((&key).into(), (&value).into());
+        }
+        self
+    }
 
-        let prefs = try!(self.profile.user_prefs());
-        try!(prefs.write());
-
-        info!("Running command: {:?}", cmd);
-        let process = try!(cmd.spawn());
-        self.process = Some(process);
-        Ok(())
+    fn stdout<'a, T>(&'a mut self, stdout: T) -> &'a mut Self
+    where
+        T: Into<Stdio>,
+    {
+        self.stdout = Some(stdout.into());
+        self
     }
 
-    fn status(&mut self) -> IoResult<Option<process::ExitStatus>> {
-        self.process.as_mut().map(|p| p.try_wait()).unwrap_or(Ok(None))
+    fn stderr<'a, T>(&'a mut self, stderr: T) -> &'a mut Self
+    where
+        T: Into<Stdio>,
+    {
+        self.stderr = Some(stderr.into());
+        self
     }
 
-    fn stop(&mut self) -> IoResult<Option<process::ExitStatus>> {
-        let mut retval = None;
+    fn start(mut self) -> Result<FirefoxProcess, RunnerError> {
+        let stdout = self.stdout.unwrap_or_else(|| Stdio::inherit());
+        let stderr = self.stderr.unwrap_or_else(|| Stdio::inherit());
+
+        let mut cmd = Command::new(&self.binary);
+        cmd.args(&self.args[..])
+            .envs(&self.envs)
+            .stdout(stdout)
+            .stderr(stderr);
 
-        if let Some(ref mut p) = self.process {
-            try!(p.kill());
-            retval = Some(try!(p.wait()));
-        };
-        Ok(retval)
+        if !self.args.iter().any(|x| is_profile_arg(x)) {
+            cmd.arg("-profile").arg(&self.profile.path);
+        }
+        cmd.stdout(Stdio::inherit()).stderr(Stdio::inherit());
+
+        self.profile.user_prefs()?.write()?;
+
+        info!("Running command: {:?}", cmd);
+        let process = cmd.spawn()?;
+        Ok(FirefoxProcess {
+            process: process,
+            profile: self.profile
+        })
     }
 }
 
-fn parse_arg_name(arg: &str) -> Option<&str> {
+fn parse_arg_name<T>(arg: T) -> Option<String>
+where
+    T: AsRef<OsStr>,
+{
+    let arg_os_str: &OsStr = arg.as_ref();
+    let arg_str = arg_os_str.to_string_lossy();
+
     let mut start = 0;
     let mut end = 0;
 
-    for (i, c) in arg.chars().enumerate() {
+    for (i, c) in arg_str.chars().enumerate() {
         if i == 0 {
             if !platform::arg_prefix_char(c) {
                 break;
             }
         } else if i == 1 {
             if name_end_char(c) {
                 break;
             } else if c != '-' {
@@ -173,53 +260,53 @@ fn parse_arg_name(arg: &str) -> Option<&
             if name_end_char(c) {
                 end -= 1;
                 break;
             }
         }
     }
 
     if start > 0 && end > start {
-        Some(&arg[start..end])
+        Some(arg_str[start..end].into())
     } else {
         None
     }
 }
 
 fn name_end_char(c: char) -> bool {
     c == ' ' || c == '='
 }
 
 /// Check if an argument string affects the Firefox profile
 ///
 /// Returns a boolean indicating whether a given string
 /// contains one of the `-P`, `-Profile` or `-ProfileManager`
 /// arguments, respecting the various platform-specific conventions.
-pub fn is_profile_arg(arg: &str) -> bool {
+pub fn is_profile_arg<T>(arg: T) -> bool
+where
+    T: AsRef<OsStr>,
+{
     if let Some(name) = parse_arg_name(arg) {
-        name.eq_ignore_ascii_case("profile") ||
-            name.eq_ignore_ascii_case("p") ||
-            name.eq_ignore_ascii_case("profilemanager")
+        name.eq_ignore_ascii_case("profile") || name.eq_ignore_ascii_case("p")
+            || name.eq_ignore_ascii_case("profilemanager")
     } else {
         false
     }
 }
 
 fn find_binary(name: &str) -> Option<PathBuf> {
-    env::var("PATH")
-        .ok()
-        .and_then(|path_env| {
-            for mut path in env::split_paths(&*path_env) {
-                path.push(name);
-                if path.exists() {
-                    return Some(path)
-                }
+    env::var("PATH").ok().and_then(|path_env| {
+        for mut path in env::split_paths(&*path_env) {
+            path.push(name);
+            if path.exists() {
+                return Some(path);
             }
-            None
-        })
+        }
+        None
+    })
 }
 
 #[cfg(target_os = "linux")]
 pub mod platform {
     use super::find_binary;
     use std::path::PathBuf;
 
     pub fn firefox_default_path() -> Option<PathBuf> {
@@ -234,29 +321,34 @@ pub mod platform {
 #[cfg(target_os = "macos")]
 pub mod platform {
     use super::find_binary;
     use std::env;
     use std::path::PathBuf;
 
     pub fn firefox_default_path() -> Option<PathBuf> {
         if let Some(path) = find_binary("firefox-bin") {
-            return Some(path)
+            return Some(path);
         }
         let home = env::home_dir();
         for &(prefix_home, trial_path) in [
-            (false, "/Applications/Firefox.app/Contents/MacOS/firefox-bin"),
-            (true, "Applications/Firefox.app/Contents/MacOS/firefox-bin")].iter() {
+            (
+                false,
+                "/Applications/Firefox.app/Contents/MacOS/firefox-bin",
+            ),
+            (true, "Applications/Firefox.app/Contents/MacOS/firefox-bin"),
+        ].iter()
+        {
             let path = match (home.as_ref(), prefix_home) {
                 (Some(ref home_dir), true) => home_dir.join(trial_path),
                 (None, true) => continue,
-                (_, false) => PathBuf::from(trial_path)
+                (_, false) => PathBuf::from(trial_path),
             };
             if path.exists() {
-                return Some(path)
+                return Some(path);
             }
         }
         None
     }
 
     pub fn arg_prefix_char(c: char) -> bool {
         c == '-'
     }
@@ -269,17 +361,17 @@ pub mod platform {
     use std::path::PathBuf;
     use winreg::RegKey;
     use winreg::enums::*;
 
     pub fn firefox_default_path() -> Option<PathBuf> {
         let opt_path = firefox_registry_path().unwrap_or(None);
         if let Some(path) = opt_path {
             if path.exists() {
-                return Some(path)
+                return Some(path);
             }
         };
         find_binary("firefox.exe")
     }
 
     fn firefox_registry_path() -> Result<Option<PathBuf>, Error> {
         let hklm = RegKey::predef(HKEY_LOCAL_MACHINE);
         for subtree_key in ["SOFTWARE", "SOFTWARE\\WOW6432Node"].iter() {
@@ -296,17 +388,17 @@ pub mod platform {
                 let version: Result<String, _> = section_data.get_value("GeckoVer");
                 if let Ok(ver) = version {
                     if ver == current_version {
                         let mut bin_key = key.to_owned();
                         bin_key.push_str("\\bin");
                         if let Ok(bin_subtree) = mozilla.open_subkey_with_flags(bin_key, KEY_READ) {
                             let path: Result<String, _> = bin_subtree.get_value("PathToExe");
                             if let Ok(path) = path {
-                                return Ok(Some(PathBuf::from(path)))
+                                return Ok(Some(PathBuf::from(path)));
                             }
                         }
                     }
                 }
             }
         }
         Ok(None)
     }
@@ -326,21 +418,21 @@ pub mod platform {
 
     pub fn arg_prefix_char(c: char) -> bool {
         c == '-'
     }
 }
 
 #[cfg(test)]
 mod tests {
-    use super::{parse_arg_name, is_profile_arg};
+    use super::{is_profile_arg, parse_arg_name};
 
     fn parse(arg: &str, name: Option<&str>) {
         let result = parse_arg_name(arg);
-        assert_eq!(result, name);
+        assert_eq!(result, name.map(|x| x.to_string()));
     }
 
     #[test]
     fn test_parse_arg_name() {
         parse("-p", Some("p"));
         parse("--p", Some("p"));
         parse("--profile foo", Some("profile"));
         parse("--profile", Some("profile"));