Improving modularization, telemetry checks

- Create a Telemetry struct and impl
- Add a Telemetry to the Toolchain struct
- Clean up telemetry checks to look for a telemetry_on file
- Remove unused directives and imports
- Modifying telemetry_rustc to stread stederr to the user. This strips formatting for some reason, not sure why.

src/multirust-cli/main.rs

@@ -1,5 +1,3 @@
-#![feature(custom_derive, plugin)]
-
 extern crate multirust_dist;
 #[macro_use]
 extern crate multirust_utils;

src/multirust-cli/multirust_mode.rs

@@ -326,7 +326,7 @@ fn add_target(cfg: &Cfg, m: &ArgMatches) -> Result<()> {
         pkg: "rust-std".to_string(),
         target: TargetTriple::from_str(target),
     };
-    try!(toolchain.add_component(new_component, &cfg));
+    try!(toolchain.add_component(new_component));
 
     Ok(())
 }

src/multirust-cli/rustup_mode.rs

@@ -286,7 +286,7 @@ fn target_add(cfg: &Cfg, m: &ArgMatches) -> Result<()> {
         target: TargetTriple::from_str(target),
     };
 
-    toolchain.add_component(new_component, &cfg)
+    toolchain.add_component(new_component)
 }
 
 fn target_remove(cfg: &Cfg, m: &ArgMatches) -> Result<()> {
@@ -386,6 +386,9 @@ fn self_uninstall(m: &ArgMatches) -> Result<()> {
 
 fn telemetry(cfg: &Cfg, m: &ArgMatches) -> Result<()> {
     let telemetry_string = m.value_of("enabled").unwrap();
-
-    cfg.set_telemetry(telemetry_string)
+    match telemetry_string {
+        "on" => cfg.set_telemetry(true),
+        "off" => cfg.set_telemetry(false),
+        _ => Err(Error::Custom { id: "Telemetry".to_string(), desc: "Incorrect telemetry setting".to_string() }),
+    }
 }

src/multirust/command.rs

@@ -1,16 +1,16 @@
-use std;
+use std::collections::HashSet;
 use std::env;
 use std::ffi::OsStr;
-use std::io::{self, Write};
+use std::io::{self, Write, BufRead, BufReader};
 use std::path::PathBuf;
-use std::process::{self, Command, Output};
+use std::process::{self, Command, Stdio};
 use std::time::Instant;
+use regex::Regex;
 
 use Cfg;
 use errors::*;
 use multirust_utils;
-use telemetry;
-use telemetry::TelemetryEvent;
+use telemetry::{Telemetry, TelemetryEvent};
 
 
 pub fn run_command_for_dir<S: AsRef<OsStr>>(cmd: Command,
@@ -28,41 +28,75 @@ pub fn run_command_for_dir<S: AsRef<OsStr>>(cmd: Command,
     run_command_for_dir_without_telemetry(cmd, &args)
 }
 
-fn telemetry_rustc<S: AsRef<OsStr>>(cmd: Command, args: &[S], cfg: &Cfg) -> Result<()> {
+fn telemetry_rustc<S: AsRef<OsStr>>(mut cmd: Command, args: &[S], cfg: &Cfg) -> Result<()> {
     let now = Instant::now();
 
-    let output = bare_run_command_for_dir(cmd, &args);
+    cmd.args(&args[1..]);
+
+    // FIXME rust-lang/rust#32254. It's not clear to me
+    // when and why this is needed.
+    let mut cmd = cmd.stdin(Stdio::inherit())
+                    .stdout(Stdio::inherit())
+                    .stderr(Stdio::piped())
+                    .spawn()
+                    .unwrap();
+
+    let mut buffered_stderr = BufReader::new(cmd.stderr.take().unwrap());
+    let status = cmd.wait();
 
     let duration = now.elapsed();
 
     let ms = (duration.as_secs() as u64 * 1000) + (duration.subsec_nanos() as u64 / 1000 / 1000);
 
-    match output {
-        Ok(out) => {
-            let exit_code = out.status.code().unwrap_or(1);
-
-            let errors = match out.status.success() {
-                true => None,
-                _ => Some(String::from_utf8_lossy(&out.stderr).to_string()),
+    let t = Telemetry::new(cfg.multirust_dir.join("telemetry"));
+
+    match status {
+        Ok(status) => {
+            let exit_code = status.code().unwrap_or(1);
+
+            let re = Regex::new(r"\[(?P<error>E.{4})\]").unwrap();
+
+            let mut buffer = String::new();
+            // Chose a HashSet instead of a Vec to avoid calls to sort() and dedup().
+            // The HashSet should be faster if there are a lot of errors, too.
+            let mut errors: HashSet<String> = HashSet::new();
+
+            let stderr = io::stderr();
+            let mut handle = stderr.lock();
+
+            while buffered_stderr.read_line(&mut buffer).unwrap() > 0 {
+                let b = buffer.to_owned();
+                buffer.clear();                
+                let _ = handle.write(b.as_bytes());
+
+                let c = re.captures(&b);
+                match c {
+                    None => continue,
+                    Some(caps) => {
+                        if caps.len() > 0 {
+                            let _ = errors.insert(caps.name("error").unwrap_or("").to_owned());
+                        }
+                    }
+                };
+            }
+
+            let e = match errors.len() { 
+                0 => None,
+                _ => Some(errors),
             };
 
-            let _ = io::stdout().write(&out.stdout);
-            let _ = io::stdout().flush();
-            let _ = io::stderr().write(&out.stderr);
-            let _ = io::stderr().flush();
-
             let te = TelemetryEvent::RustcRun { duration_ms: ms, 
                                                 exit_code: exit_code,
-                                                errors: errors };
-            telemetry::log_telemetry(te, cfg);
+                                                errors: e };
+            t.log_telemetry(te);
             process::exit(exit_code);
         },
         Err(e) => {
             let exit_code = e.raw_os_error().unwrap_or(1);
             let te = TelemetryEvent::RustcRun { duration_ms: ms,
                                                 exit_code: exit_code,
-                                                errors: Some(format!("{}", e)) };
-            telemetry::log_telemetry(te, cfg);
+                                                errors: None };
+            t.log_telemetry(te);
             Err(multirust_utils::Error::RunningCommand {    
                 name: args[0].as_ref().to_owned(),
                 error: multirust_utils::raw::CommandError::Io(e),
@@ -71,17 +105,15 @@ fn telemetry_rustc<S: AsRef<OsStr>>(cmd: Command, args: &[S], cfg: &Cfg) -> Resu
     }
 }
 
-fn run_command_for_dir_without_telemetry<S: AsRef<OsStr>>(cmd: Command, args: &[S]) -> Result<()>  {
-    let output = bare_run_command_for_dir(cmd, &args);
+fn run_command_for_dir_without_telemetry<S: AsRef<OsStr>>(mut cmd: Command, args: &[S]) -> Result<()>  {
+    cmd.args(&args[1..]);
 
-    match output {
-        Ok(out) => {
-            let _ = io::stdout().write(&out.stdout);
-            let _ = io::stdout().flush();
-            let _ = io::stderr().write(&out.stderr);
-            let _ = io::stderr().flush();
+    // FIXME rust-lang/rust#32254. It's not clear to me
+    // when and why this is needed.
+    cmd.stdin(process::Stdio::inherit());
 
-            let status = out.status;
+    match cmd.status() {
+        Ok(status) => {
             // Ensure correct exit code is returned
             let code = status.code().unwrap_or(1);
             process::exit(code);
@@ -94,16 +126,3 @@ fn run_command_for_dir_without_telemetry<S: AsRef<OsStr>>(cmd: Command, args: &[
         }
     }    
 }
-
-fn bare_run_command_for_dir<S: AsRef<OsStr>>(mut cmd: Command, args: &[S]) -> std::result::Result<Output, std::io::Error> {
-    cmd.args(&args[1..]);
-
-    // FIXME rust-lang/rust#32254. It's not clear to me
-    // when and why this is needed.
-    cmd.stdin(process::Stdio::inherit());
-
-    cmd.output()
-}
-
-
-

src/multirust/config.rs

@@ -91,7 +91,7 @@ impl Cfg {
                                 .and_then(utils::if_not_empty)
                                 .map_or(Cow::Borrowed(dist::DEFAULT_DIST_ROOT), Cow::Owned);
 
-        let telemetry_mode = Cfg::find_telemetry(&multirust_dir.join("telemetry"));
+        let telemetry_mode = Cfg::find_telemetry(&multirust_dir);
 
         Ok(Cfg {
             multirust_dir: multirust_dir,
@@ -411,14 +411,31 @@ impl Cfg {
         }
     }
 
-    pub fn set_telemetry(&self, telemetry: &str) -> Result<()> {
+    pub fn set_telemetry(&self, telemetry_enabled: bool) -> Result<()> {
+        match telemetry_enabled {
+            true => self.enable_telemetry(),
+            false => self.disable_telemetry(),
+        }
+    }
+
+    fn enable_telemetry(&self) -> Result<()> {
         let work_file = try!(self.temp_cfg.new_file());
+
+        let _ = utils::ensure_dir_exists("telemetry", &self.multirust_dir.join("telemetry"), ntfy!(&NotifyHandler::none()));
 
-        try!(utils::write_file("temp", &work_file, telemetry));
+        try!(utils::write_file("temp", &work_file, ""));
 
-        try!(utils::rename_file("telemetry", &*work_file, &self.multirust_dir.join("telemetry")));
+        try!(utils::rename_file("telemetry", &*work_file, &self.multirust_dir.join("telemetry_on")));
 
-        self.notify_handler.call(Notification::SetTelemetry(telemetry));
+        self.notify_handler.call(Notification::SetTelemetry("on"));
+
+        Ok(())
+    }
+
+    fn disable_telemetry(&self) -> Result<()> {
+        let _ = utils::remove_file("telemetry_on", &self.multirust_dir.join("telemetry_on"));
+
+        self.notify_handler.call(Notification::SetTelemetry("off"));
 
         Ok(())
     }
@@ -430,28 +447,14 @@ impl Cfg {
         }
     }
 
-    fn find_telemetry(telemetry_file: &PathBuf) -> TelemetryMode {
+    fn find_telemetry(multirust_dir: &PathBuf) -> TelemetryMode {
         // default telemetry should be off - if no telemetry file is found, it's off
-        if !utils::is_file(telemetry_file) {
-            return TelemetryMode::Off;
-        }
+        let telemetry_file = multirust_dir.join("telemetry_on");
 
-        let content = utils::read_file("telemetry", telemetry_file);
-        let telemetry_string = content.unwrap_or("off".to_string());
-
-        // If the telemetry file is empty, for some reason, assume that the user
-        // would prefer no telemetry and set telemetry to off.
-        if telemetry_string.is_empty() {
-            return TelemetryMode::Off;
+        if utils::is_file(telemetry_file) {
+            return TelemetryMode::On;
         }
 
-        // Finally, match any remaining string - if this is anything but 'on',
-        // assume that the user would prefer no telemetry be collected and set
-        // telemetry to off.
-        if !telemetry_string.starts_with("on") {
-            return TelemetryMode::Off;
-        }
-
-        TelemetryMode::On
+        TelemetryMode::Off
     }
 }

src/multirust/telemetry.rs

@@ -1,8 +1,10 @@
 use time;
-use config::Cfg;
 use multirust_utils::utils;
 use rustc_serialize::json;
 
+use std::collections::HashSet;
+use std::path::PathBuf;
+
 #[derive(Debug, PartialEq)]
 pub enum TelemetryMode {
     On,
@@ -11,7 +13,7 @@ pub enum TelemetryMode {
 
 #[derive(RustcDecodable, RustcEncodable, Debug)]
 pub enum TelemetryEvent {
-    RustcRun { duration_ms: u64, exit_code: i32, errors: Option<String> },
+    RustcRun { duration_ms: u64, exit_code: i32, errors: Option<HashSet<String>> },
     ToolchainUpdate { toolchain: String, success: bool } ,
     TargetAdd { toolchain: String, target: String, success: bool },
 }
@@ -22,13 +24,24 @@ struct LogMessage {
     event: TelemetryEvent,
 }
 
-pub fn log_telemetry(event: TelemetryEvent, cfg: &Cfg) {
-    let ln = LogMessage { log_time_s: time::get_time().sec, event: event };
+#[derive(Debug)]
+pub struct Telemetry {
+    telemetry_dir: PathBuf
+}
+
+impl Telemetry {
+    pub fn new(telemetry_dir: PathBuf) -> Telemetry {
+        Telemetry { telemetry_dir: telemetry_dir }
+    }   
+
+    pub fn log_telemetry(&self, event: TelemetryEvent) {
+        let current_time = time::now_utc();
+        let ln = LogMessage { log_time_s: current_time.to_timespec().sec, event: event };
 
-    let json = json::encode(&ln).unwrap();
+        let json = json::encode(&ln).unwrap();
 
-    let now = time::now_utc();
-    let filename = format!("telemetry-{}-{:02}-{:02}", now.tm_year + 1900, now.tm_mon, now.tm_mday);
+        let filename = format!("log-{}-{:02}-{:02}.json", current_time.tm_year + 1900, current_time.tm_mon + 1, current_time.tm_mday);
 
-    let _ = utils::append_file("telemetry", &cfg.multirust_dir.join(&filename), &json);
+        let _ = utils::append_file("telemetry", &self.telemetry_dir.join(&filename), &json);
+    }
 }