Merge pull request #300 from posborne/no-log-clutter-by-default

Avoid cluttering tree with stdout/stderr log files

Author: fitzgen

crates/cli/src/benchmark.rs

@@ -133,6 +133,11 @@ pub struct BenchmarkCommand {
     /// `cpu_affinity` in the `sightglass-recorder` crate for more information.
     #[structopt(long)]
     pin: bool,
+
+    /// Keep log files after successful benchmark runs. By default, logs are
+    /// only kept on failures.
+    #[structopt(short = "k", long = "keep-logs")]
+    keep_logs: bool,
 }
 
 impl BenchmarkCommand {
@@ -203,10 +208,15 @@ impl BenchmarkCommand {
                     wasm_file.hash(&mut hasher);
                     hasher.finish()
                 };
-                let stdout = format!("stdout-{:x}-{}.log", wasm_hash, std::process::id());
-                let stdout = Path::new(&stdout);
-                let stderr = format!("stderr-{:x}-{}.log", wasm_hash, std::process::id());
-                let stderr = Path::new(&stderr);
+
+                // Create log files in temp directory
+                let log_dir = std::env::temp_dir().join("sightglass-logs");
+                std::fs::create_dir_all(&log_dir).context("Failed to create log directory")?;
+
+                let stdout =
+                    log_dir.join(format!("stdout-{wasm_hash:x}-{}.log", std::process::id()));
+                let stderr =
+                    log_dir.join(format!("stderr-{wasm_hash:x}-{}.log", std::process::id()));
                 let stdin = None;
 
                 let engine = sightglass_data::Engine {
@@ -226,8 +236,8 @@ impl BenchmarkCommand {
                 let engine = Engine::new(
                     &mut bench_api,
                     &working_dir,
-                    stdout,
-                    stderr,
+                    &stdout,
+                    &stderr,
                     stdin,
                     &mut measurements,
                     &mut measure,
@@ -265,7 +275,31 @@ impl BenchmarkCommand {
                         }
                     }
 
-                    self.check_output(Path::new(wasm_file), stdout, stderr)?;
+                    let check_result = self.check_output(Path::new(wasm_file), &stdout, &stderr);
+
+                    // Handle log cleanup based on success/failure and --keep-logs flag
+                    match check_result {
+                        Ok(()) => {
+                            if !self.keep_logs {
+                                let _ = fs::remove_file(&stdout);
+                                let _ = fs::remove_file(&stderr);
+                            } else {
+                                log::info!(
+                                    "Kept log files: {} and {}",
+                                    stdout.display(),
+                                    stderr.display()
+                                );
+                            }
+                        }
+                        Err(e) => {
+                            // Failure: keep logs and inform user
+                            eprintln!("Benchmark output check failed. Log files preserved:");
+                            eprintln!("  stdout: {}", stdout.display());
+                            eprintln!("  stderr: {}", stderr.display());
+                            return Err(e);
+                        }
+                    }
+
                     engine
                         .as_mut()
                         .map(|e| e.measurements())
@@ -394,6 +428,10 @@ impl BenchmarkCommand {
                 command.arg("--pin");
             }
 
+            if self.keep_logs {
+                command.arg("--keep-logs");
+            }
+
             if self.small_workloads {
                 command.env("WASM_BENCH_USE_SMALL_WORKLOAD", "1");
             }

crates/cli/src/clean.rs

@@ -9,35 +9,55 @@ pub struct CleanCommand {}
 
 impl CleanCommand {
     pub fn execute(&self) -> Result<()> {
-        // Remove log files.
-        let log_file_regex = Regex::new(r"^(stdout|stderr)\-\w+\-\d+-\d+.log$").unwrap();
-        for entry in std::env::current_dir()?.read_dir()? {
-            let entry = entry?;
+        let log_file_regex = Regex::new(r"^(stdout|stderr)\-\w+\-\d+.log$").unwrap();
+        let mut removed_count = 0;
 
-            // Only consider files, not dirs or symlinks.
-            if !entry.file_type()?.is_file() {
+        // Clean log files from both current directory (legacy) and temp directory (new)
+        let dirs_to_clean = vec![
+            std::env::current_dir()?,
+            std::env::temp_dir().join("sightglass-logs"),
+        ];
+
+        for dir in dirs_to_clean {
+            if !dir.exists() {
                 continue;
             }
 
-            let path = entry.path();
-
-            // If it doesn't have a file name, it definitely isn't a log file.
-            let name = match path.file_name().and_then(|n| n.to_str()) {
-                None => continue,
-                Some(n) => n,
+            let read_dir = match dir.read_dir() {
+                Ok(rd) => rd,
+                Err(_) => continue,
             };
 
-            // If it doesn't match our log file regex, it isn't a log file.
-            if !log_file_regex.is_match(name) {
-                continue;
-            }
+            for entry in read_dir {
+                let entry = entry?;
+
+                // Only consider files, not dirs or symlinks.
+                if !entry.file_type()?.is_file() {
+                    continue;
+                }
 
-            // Okay! It's one of our log files!
-            log::info!("Removing log file: {}", path.display());
-            std::fs::remove_file(&path)
-                .with_context(|| format!("failed to remove {}", path.display()))?;
+                let path = entry.path();
+
+                // If it doesn't have a file name, it definitely isn't a log file.
+                let name = match path.file_name().and_then(|n| n.to_str()) {
+                    None => continue,
+                    Some(n) => n,
+                };
+
+                // If it doesn't match our log file regex, it isn't a log file.
+                if !log_file_regex.is_match(name) {
+                    continue;
+                }
+
+                // Okay! It's one of our log files!
+                log::info!("Removing log file: {}", path.display());
+                std::fs::remove_file(&path)
+                    .with_context(|| format!("failed to remove {}", path.display()))?;
+                removed_count += 1;
+            }
         }
 
+        println!("Removed {} log file(s)", removed_count);
         Ok(())
     }
 }