From 77c062ce1744f66922acf04478f8e325a18d61c1 Mon Sep 17 00:00:00 2001 From: Jeremiah Peschka Date: Mon, 11 Apr 2016 13:57:46 -0700 Subject: [PATCH] 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 | 2 - src/multirust-cli/multirust_mode.rs | 2 +- src/multirust-cli/rustup_mode.rs | 9 ++- src/multirust/command.rs | 109 ++++++++++++++++------------ src/multirust/config.rs | 51 +++++++------ src/multirust/telemetry.rs | 29 ++++++-- src/multirust/toolchain.rs | 20 ++--- 7 files changed, 130 insertions(+), 92 deletions(-) diff --git a/src/multirust-cli/main.rs b/src/multirust-cli/main.rs index 2304886ecb8..29760d43130 100644 --- a/src/multirust-cli/main.rs +++ b/src/multirust-cli/main.rs @@ -1,5 +1,3 @@ -#![feature(custom_derive, plugin)] - extern crate multirust_dist; #[macro_use] extern crate multirust_utils; diff --git a/src/multirust-cli/multirust_mode.rs b/src/multirust-cli/multirust_mode.rs index a735f1f3580..14b5b292388 100644 --- a/src/multirust-cli/multirust_mode.rs +++ b/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(()) } diff --git a/src/multirust-cli/rustup_mode.rs b/src/multirust-cli/rustup_mode.rs index ad1d64e0e94..66a5642a7d6 100644 --- a/src/multirust-cli/rustup_mode.rs +++ b/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() }), + } } \ No newline at end of file diff --git a/src/multirust/command.rs b/src/multirust/command.rs index bb8b86ce741..8d113a33b0e 100644 --- a/src/multirust/command.rs +++ b/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>(cmd: Command, @@ -28,41 +28,75 @@ pub fn run_command_for_dir>(cmd: Command, run_command_for_dir_without_telemetry(cmd, &args) } -fn telemetry_rustc>(cmd: Command, args: &[S], cfg: &Cfg) -> Result<()> { +fn telemetry_rustc>(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"\[(?PE.{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 = 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>(cmd: Command, args: &[S], cfg: &Cfg) -> Resu } } -fn run_command_for_dir_without_telemetry>(cmd: Command, args: &[S]) -> Result<()> { - let output = bare_run_command_for_dir(cmd, &args); +fn run_command_for_dir_without_telemetry>(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>(cmd: Command, args: &[ } } } - -fn bare_run_command_for_dir>(mut cmd: Command, args: &[S]) -> std::result::Result { - 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() -} - - - diff --git a/src/multirust/config.rs b/src/multirust/config.rs index a07e199a498..34ca46fd85f 100644 --- a/src/multirust/config.rs +++ b/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 } } diff --git a/src/multirust/telemetry.rs b/src/multirust/telemetry.rs index d8bbf89b711..9915a1701a2 100644 --- a/src/multirust/telemetry.rs +++ b/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 }, + RustcRun { duration_ms: u64, exit_code: i32, errors: Option> }, 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); + } } \ No newline at end of file diff --git a/src/multirust/toolchain.rs b/src/multirust/toolchain.rs index dcb8bb347e7..88383d33861 100644 --- a/src/multirust/toolchain.rs +++ b/src/multirust/toolchain.rs @@ -9,7 +9,7 @@ use config::Cfg; use env_var; use install::{self, InstallMethod}; use telemetry; -use telemetry::TelemetryEvent; +use telemetry::{Telemetry, TelemetryEvent}; use std::process::Command; use std::path::{Path, PathBuf}; @@ -23,6 +23,7 @@ pub struct Toolchain<'a> { cfg: &'a Cfg, name: String, path: PathBuf, + telemetry: telemetry::Telemetry, } /// Used by the list_component function @@ -46,6 +47,7 @@ impl<'a> Toolchain<'a> { cfg: cfg, name: resolved_name, path: path.clone(), + telemetry: Telemetry::new(cfg.multirust_dir.join("telemetry")), }) } pub fn name(&self) -> &str { @@ -164,13 +166,13 @@ impl<'a> Toolchain<'a> { Ok(us) => { let te = TelemetryEvent::ToolchainUpdate { toolchain: self.name().to_string() , success: true }; - telemetry::log_telemetry(te, &self.cfg); + self.telemetry.log_telemetry(te); Ok(us) } Err(e) => { let te = TelemetryEvent::ToolchainUpdate { toolchain: self.name().to_string() , success: true }; - telemetry::log_telemetry(te, &self.cfg); + self.telemetry.log_telemetry(te); Err(e) } } @@ -382,14 +384,14 @@ impl<'a> Toolchain<'a> { } } - pub fn add_component(&self, component: Component, cfg: &Cfg) -> Result<()> { - if cfg.telemetry_enabled() { - return self.telemetry_add_component(component, cfg); + pub fn add_component(&self, component: Component) -> Result<()> { + if self.cfg.telemetry_enabled() { + return self.telemetry_add_component(component); } self.add_component_without_telemetry(component) } - fn telemetry_add_component(&self, component: Component, cfg: &Cfg) -> Result<()> { + fn telemetry_add_component(&self, component: Component) -> Result<()> { let output = self.bare_add_component(component); match output { @@ -397,7 +399,7 @@ impl<'a> Toolchain<'a> { let te = TelemetryEvent::ToolchainUpdate { toolchain: self.name.to_owned(), success: true }; - telemetry::log_telemetry(te, &cfg); + self.telemetry.log_telemetry(te); Ok(()) }, @@ -405,7 +407,7 @@ impl<'a> Toolchain<'a> { let te = TelemetryEvent::ToolchainUpdate { toolchain: self.name.to_owned(), success: false }; - telemetry::log_telemetry(te, &cfg); + self.telemetry.log_telemetry(te); Err(e) } }