From bb80fef49fabad5558e77786e157b4ea822d0f23 Mon Sep 17 00:00:00 2001 From: Michael Suo Date: Mon, 9 May 2022 23:35:09 -0700 Subject: [PATCH] feat: add rage command for bug reporting --- Cargo.toml | 1 + src/lib.rs | 1 + src/log_utils.rs | 22 +- src/main.rs | 44 +++- src/persistent_data.rs | 230 +++++++++++++++++- src/rage.rs | 61 +++-- tests/integration_test.rs | 39 +++ ...integration_test__rage_command_output.snap | 12 + 8 files changed, 378 insertions(+), 32 deletions(-) create mode 100644 tests/snapshots/integration_test__rage_command_output.snap diff --git a/Cargo.toml b/Cargo.toml index 5a032bd..7552a4c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,6 +25,7 @@ directories = "4.0.1" blake3 = "1.3.1" fern = { version = "0.6.1", features = ["colored"] } chrono = "0.4.19" +dialoguer = "0.10.1" [dev-dependencies] assert_cmd = "2.0.4" diff --git a/src/lib.rs b/src/lib.rs index 0b80250..fe6cfcc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -21,6 +21,7 @@ pub mod linter; pub mod log_utils; pub mod path; pub mod persistent_data; +pub mod rage; pub mod render; use git::get_changed_files; diff --git a/src/log_utils.rs b/src/log_utils.rs index 63ed968..93b838a 100644 --- a/src/log_utils.rs +++ b/src/log_utils.rs @@ -1,6 +1,7 @@ use anyhow::{bail, Result}; use console::{style, Term}; use fern::colors::{Color, ColoredLevelConfig}; +use std::path::Path; use std::process::Output; use log::Level::Trace; @@ -32,7 +33,7 @@ pub fn ensure_output(program_name: &str, output: &Output) -> Result<()> { Ok(()) } -pub fn setup_logger(log_level: LevelFilter, force_color: bool) -> Result<()> { +pub fn setup_logger(log_level: LevelFilter, log_file: &Path, force_color: bool) -> Result<()> { let builder = fern::Dispatch::new(); let isatty = Term::stderr().features().is_attended(); @@ -61,6 +62,20 @@ pub fn setup_logger(log_level: LevelFilter, force_color: bool) -> Result<()> { .level(log_level) .chain(std::io::stderr()), ) + .chain( + fern::Dispatch::new() + .format(move |out, message, record| { + out.finish(format_args!( + "[{} {} {}] {}", + chrono::Utc::now().to_rfc3339_opts(chrono::SecondsFormat::Secs, true), + record.level(), + record.target(), + message + )) + }) + .level(LevelFilter::Trace) + .chain(fern::log_file(log_file)?), + ) .apply()?; } else { builder @@ -78,6 +93,11 @@ pub fn setup_logger(log_level: LevelFilter, force_color: bool) -> Result<()> { .level(log_level) .chain(std::io::stderr()), ) + .chain( + fern::Dispatch::new() + .level(LevelFilter::Trace) + .chain(fern::log_file(log_file)?), + ) .apply()?; } Ok(()) diff --git a/src/main.rs b/src/main.rs index 8046c9d..991a668 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,6 +1,7 @@ use std::{collections::HashSet, convert::TryFrom, io::Write}; use anyhow::{Context, Result}; +use chrono::SecondsFormat; use clap::Parser; use lintrunner::{ @@ -9,7 +10,8 @@ use lintrunner::{ lint_config::{get_linters_from_config, LintRunnerConfig}, log_utils::setup_logger, path::AbsPath, - persistent_data::{PersistentDataStore}, + persistent_data::{ExitInfo, PersistentDataStore, RunInfo}, + rage::do_rage, render::print_error, PathsOpt, RenderOpt, RevisionOpt, }; @@ -102,6 +104,13 @@ enum SubCommand { /// Run linters. This is the default if no subcommand is provided. Lint, + + /// Create a bug report for a past invocation of lintrunner. + Rage { + /// Choose a specific invocation to report on. 0 is the most recent run. + #[clap(long, short)] + invocation: Option, + }, } fn do_main() -> Result { @@ -128,9 +137,17 @@ fn do_main() -> Result { (_, _) => log::LevelFilter::Trace, }; - let persistent_data_store = PersistentDataStore::new(&config_path)?; + let run_info = RunInfo { + args: std::env::args().collect(), + timestamp: chrono::Local::now().to_rfc3339_opts(SecondsFormat::Secs, true), + }; + let persistent_data_store = PersistentDataStore::new(&config_path, run_info)?; - setup_logger(log_level, args.force_color)?; + setup_logger( + log_level, + &persistent_data_store.log_file(), + args.force_color, + )?; let cmd = args.cmd.unwrap_or(SubCommand::Lint); let lint_runner_config = LintRunnerConfig::new(&config_path)?; @@ -194,7 +211,7 @@ fn do_main() -> Result { PathsOpt::Auto }; - match cmd { + let res = match cmd { SubCommand::Init { dry_run } => { // Just run initialization commands, don't actually lint. do_init(linters, dry_run, &persistent_data_store, &config_path) @@ -222,7 +239,24 @@ fn do_main() -> Result { revision_opt, ) } - } + SubCommand::Rage { invocation } => do_rage(&persistent_data_store, invocation), + }; + + let exit_info = match &res { + Ok(code) => ExitInfo { + code: *code, + err: None, + }, + Err(err) => ExitInfo { + code: 1, + err: Some(err.to_string()), + }, + }; + + // Write data related to this run out to the persistent data store. + persistent_data_store.write_run_info(exit_info)?; + + res } fn main() { diff --git a/src/persistent_data.rs b/src/persistent_data.rs index a6ccc55..d3216a8 100644 --- a/src/persistent_data.rs +++ b/src/persistent_data.rs @@ -6,23 +6,53 @@ //! we hash the absolute path to the config and include that as part of the //! directory structure for persistent data. -use anyhow::{anyhow, Result}; +use anyhow::{anyhow, bail, Context, Result}; use directories::ProjectDirs; use log::debug; -use std::path::{Path, PathBuf}; +use serde::{Deserialize, Serialize}; +use std::{ + fmt::Write, + path::{Path, PathBuf}, +}; use crate::path::AbsPath; const CONFIG_DATA_NAME: &str = ".lintrunner.toml"; +const RUNS_DIR_NAME: &str = "runs"; +const MAX_RUNS_TO_STORE: usize = 10; /// Single way to interact with persistent data for a given run of lintrunner. /// This is scoped to a single .lintrunner.toml config. pub struct PersistentDataStore { data_dir: PathBuf, + runs_dir: PathBuf, + cur_run_info: RunInfo, +} + +/// Encapsulates information about a specific run of `lintrunner` +#[derive(Serialize, Deserialize)] +pub struct RunInfo { + pub args: Vec, + pub timestamp: String, +} + +#[derive(Serialize, Deserialize)] +pub struct ExitInfo { + pub code: i32, + pub err: Option, +} + +impl RunInfo { + // Get the directory (relative to the runs dir) that stores data specific to + // this run. + fn dir_name(&self) -> String { + let args = blake3::hash(self.args.join("_").as_bytes()).to_string(); + self.timestamp.clone() + "_" + &args + } } impl PersistentDataStore { - pub fn new(config_path: &AbsPath) -> Result { + pub fn new(config_path: &AbsPath, cur_run_info: RunInfo) -> Result { // Retrieve the lintrunner-wide data directory. let project_dirs = ProjectDirs::from("", "", "lintrunner"); let project_dirs = @@ -33,13 +63,154 @@ impl PersistentDataStore { let config_path_hash = blake3::hash(config_path.to_string_lossy().as_bytes()).to_string(); let config_data_dir = project_data_dir.join(config_path_hash); - std::fs::create_dir_all(&config_data_dir)?; + // Create the runs dir as well. + let runs_dir = config_data_dir.join(RUNS_DIR_NAME); + let cur_run_dir = runs_dir.join(cur_run_info.dir_name()); + + std::fs::create_dir_all(&cur_run_dir)?; + + PersistentDataStore::clean_old_runs(&runs_dir)?; Ok(PersistentDataStore { data_dir: config_data_dir, + runs_dir, + cur_run_info, }) } + fn clean_old_runs(runs_dir: &Path) -> Result<()> { + let mut entries = std::fs::read_dir(runs_dir)? + .map(|res| res.map(|e| e.path())) + .collect::, std::io::Error>>()?; + + if entries.len() >= MAX_RUNS_TO_STORE { + debug!("Found more than {MAX_RUNS_TO_STORE} runs, cleaning some up"); + + entries.sort_unstable(); + + let num_to_delete = entries.len() - MAX_RUNS_TO_STORE; + for dir in entries.iter().take(num_to_delete) { + debug!("Deleting old run: {}", dir.display()); + std::fs::remove_dir_all(dir)?; + } + } + Ok(()) + } + + pub fn log_file(&self) -> PathBuf { + self.runs_dir + .join(self.cur_run_info.dir_name()) + .join("log.txt") + } + + pub fn write_run_info(&self, exit_info: ExitInfo) -> Result<()> { + let run_path = self.runs_dir.join(self.cur_run_info.dir_name()); + debug!("Writing run info to {}", run_path.display()); + + if !run_path.exists() { + std::fs::create_dir(&run_path)?; + } + let run_info = serde_json::to_string_pretty(&self.cur_run_info)?; + std::fs::write(&run_path.join("run_info.json"), &run_info)?; + + let exit_info = serde_json::to_string_pretty(&exit_info)?; + std::fs::write(&run_path.join("exit_info.json"), exit_info)?; + Ok(()) + } + + pub fn get_run_report(&self, run_info: &RunInfo) -> Result { + let run_path = self.runs_dir.join(run_info.dir_name()); + debug!("Generating run report from {}", run_path.display()); + + let log = + std::fs::read_to_string(run_path.join("log.txt")).context("retrieving log file")?; + + let mut ret = String::new(); + + write!( + ret, + "lintrunner rage report:\n\ + timestamp: {}\n\ + args: {}\n", + run_info.timestamp, + run_info + .args + .iter() + .map(|x| format!("'{x}'")) + .collect::>() + .join(" "), + )?; + + let exit_info_path = run_path.join("exit_info.json"); + if exit_info_path.exists() { + let exit_info = + std::fs::read_to_string(exit_info_path).context("retrieving exit info json")?; + let exit_info: ExitInfo = + serde_json::from_str(&exit_info).context("deserializing exit info")?; + write!( + ret, + "exit code: {}\n\ + err msg: {:?}\n\n", + exit_info.code, exit_info.err, + )?; + } else { + writeln!(ret, "EXIT INFO MISSING")?; + } + writeln!(ret, "========= BEGIN LOGS =========")?; + ret.write_str(&log)?; + + Ok(ret) + } + + fn past_run_dirs(&self) -> Result> { + debug!("Reading past runs from {}", self.runs_dir.display()); + + let mut run_dirs = std::fs::read_dir(&self.runs_dir)? + .map(|res| res.map(|e| e.path())) + .collect::, std::io::Error>>()?; + + run_dirs.sort_unstable(); + run_dirs.reverse(); + Ok(run_dirs) + } + + pub fn past_run(&self, invocation: usize) -> Result { + let run_dirs = self.past_run_dirs()?; + + let dir = run_dirs.get(invocation); + match dir { + Some(dir) => { + let run_info: RunInfo = + serde_json::from_str(&std::fs::read_to_string(dir.join("run_info.json"))?)?; + Ok(run_info) + } + None => { + bail!( + "Tried to request run #{invocation}, but didn't find it. \ + (lintrunner only stores the last {MAX_RUNS_TO_STORE} runs)" + ); + } + } + } + + pub fn past_runs(&self) -> Result> { + let run_dirs = self.past_run_dirs()?; + + let mut ret = Vec::new(); + + // Skip the first one as it is the current run. + for dir in run_dirs.into_iter().skip(1) { + debug!("Reading run info from {}", dir.display()); + + let run_info: RunInfo = + serde_json::from_str(&std::fs::read_to_string(dir.join("run_info.json"))?)?; + let exit_info: ExitInfo = + serde_json::from_str(&std::fs::read_to_string(dir.join("exit_info.json"))?)?; + ret.push((run_info, exit_info)); + } + Ok(ret) + } + pub fn last_init(&self) -> Result> { debug!( "Checking data file '{}/{}' to see if config has changed", @@ -72,3 +243,54 @@ impl PersistentDataStore { self.data_dir.join(path) } } + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::NamedTempFile; + + #[test] + fn basic_data_doesnt_fail() { + let f = NamedTempFile::new().unwrap(); + let config = AbsPath::try_from(f.path()).unwrap(); + + let run_info = RunInfo { + timestamp: "0".to_string(), + args: vec!["foo".to_string(), "bar".to_string()], + }; + let store = PersistentDataStore::new(&config, run_info).unwrap(); + // Try to cleanup + std::fs::remove_dir_all(store.data_dir).unwrap(); + } + + #[test] + fn old_run_cleanup() { + let f = NamedTempFile::new().unwrap(); + let config = AbsPath::try_from(f.path()).unwrap(); + + let run_info = RunInfo { + timestamp: "0".to_string(), + args: vec!["foo".to_string(), "bar".to_string()], + }; + let store = PersistentDataStore::new(&config, run_info).unwrap(); + + // Simulate some more runs. + for i in 1..20 { + let run_info = RunInfo { + timestamp: i.to_string(), + args: vec!["foo".to_string(), "bar".to_string()], + }; + let store = PersistentDataStore::new(&config, run_info).unwrap(); + store + .write_run_info(ExitInfo { code: 0, err: None }) + .unwrap() + } + + // We should have 10 runs, since old ones should have been collected. + let num_entries = std::fs::read_dir(store.runs_dir).unwrap().count(); + assert_eq!(num_entries, MAX_RUNS_TO_STORE); + + // Try to clean up + std::fs::remove_dir_all(store.data_dir).unwrap(); + } +} diff --git a/src/rage.rs b/src/rage.rs index b74c622..f784a4a 100644 --- a/src/rage.rs +++ b/src/rage.rs @@ -1,41 +1,58 @@ -use crate::persistent_data::PersistentDataStore; -use anyhow::Result; +use crate::persistent_data::{PersistentDataStore, RunInfo}; +use anyhow::{Context, Result}; use console::style; use dialoguer::{theme::ColorfulTheme, Select}; +fn select_past_runs(persistent_data_store: &PersistentDataStore) -> Result> { + let runs = persistent_data_store.past_runs()?; + if runs.is_empty() { + return Ok(None); + } + let items: Vec = runs + .iter() + .map(|(run_info, exit_info)| { + let starting_glyph = if exit_info.code == 0 { + style("✓").green() + } else { + style("✕").red() + }; + format!( + "{} {}: {}", + starting_glyph, + run_info.timestamp.to_string(), + run_info.args.join(" "), + ) + }) + .collect(); + + let selection = Select::with_theme(&ColorfulTheme::default()) + .with_prompt("Select a past invocation to report") + .items(&items) + .default(0) + .interact_opt()?; + + Ok(selection.map(|i| runs.into_iter().nth(i).unwrap().0)) +} + pub fn do_rage( persistent_data_store: &PersistentDataStore, invocation: Option, ) -> Result { let run = match invocation { - Some(invocation) => Some(persistent_data_store.run(invocation)?), - None => { - let runs = persistent_data_store.runs()?; - let items: Vec = persistent_data_store - .runs()? - .iter() - .map(|run| run.timestamp.to_string() + ": " + &run.args.join(" ")) - .collect(); - - let selection = Select::with_theme(&ColorfulTheme::default()) - .with_prompt("Select a past invocation to report") - .items(&items) - .default(0) - .interact_opt()?; - - selection.map(|i| runs.into_iter().nth(i).unwrap()) - } + Some(invocation) => Some(persistent_data_store.past_run(invocation)?), + None => select_past_runs(persistent_data_store)?, }; match run { Some(run) => { - let report = persistent_data_store.get_run_report(&run)?; + let report = persistent_data_store + .get_run_report(&run) + .context("getting selected run report")?; print!("{}", report); - Ok(0) } None => { println!("{}", style("Nothing selected, exiting.").yellow()); - Ok(1) } } + Ok(0) } diff --git a/tests/integration_test.rs b/tests/integration_test.rs index 06adcb2..a5046c2 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -627,3 +627,42 @@ fn format_command_doesnt_use_nonformat_linter() -> Result<()> { Ok(()) } + +#[test] +fn rage_command_basic() -> Result<()> { + let data_path = tempfile::tempdir()?; + let lint_message = LintMessage { + path: Some("tests/fixtures/fake_source_file.rs".to_string()), + line: Some(9), + char: Some(1), + code: "DUMMY".to_string(), + name: "dummy failure".to_string(), + severity: LintSeverity::Advice, + original: None, + replacement: None, + description: Some("A dummy linter failure".to_string()), + }; + let config = temp_config_returning_msg(lint_message)?; + + let mut cmd = Command::cargo_bin("lintrunner")?; + cmd.arg(format!("--config={}", config.path().to_str().unwrap())); + cmd.arg(format!( + "--data-path={}", + data_path.path().to_str().unwrap() + )); + // Run on a file to ensure that the linter is run. + cmd.arg("README.md"); + cmd.assert().failure(); + + // Now run rage + let mut cmd = Command::cargo_bin("lintrunner")?; + cmd.arg(format!("--config={}", config.path().to_str().unwrap())); + cmd.arg(format!( + "--data-path={}", + data_path.path().to_str().unwrap() + )); + cmd.arg("rage"); + cmd.arg("--invocation=0"); + cmd.assert().success(); + Ok(()) +} diff --git a/tests/snapshots/integration_test__rage_command_output.snap b/tests/snapshots/integration_test__rage_command_output.snap new file mode 100644 index 0000000..28e5a05 --- /dev/null +++ b/tests/snapshots/integration_test__rage_command_output.snap @@ -0,0 +1,12 @@ +--- +source: tests/integration_test.rs +expression: output_lines +--- +- "STDOUT:" +- "lintrunner rage report:" +- "timestamp: 2022-05-09T23:43:21-07:00" +- "args: '--data-path=/var/folders/by/9xrsv08s7ql0n9mzdf1dwlm80000gn/T/.tmp3uM2kH' 'README.md'" +- "exit code: 1" +- "err msg: None" +- "" +