From 138df63be80a1fb5447ecd6e3dc4420b67b51f43 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Wed, 26 Jun 2024 17:04:33 -0400 Subject: [PATCH] [fontc] Tweak root error type - Removes AnyWorkError, and adds variants for BeError and and Panic to fontc::Error - instead of returning a vec of errors when tasks fails, just return the first error we see - Ensure that we always know the relevant path when we encounter an io::Error --- fontc/src/change_detector.rs | 6 ++++- fontc/src/config.rs | 12 ++++++++-- fontc/src/error.rs | 23 ++++++++++++------- fontc/src/lib.rs | 22 ++++++++++++------ fontc/src/main.rs | 14 ++++++++---- fontc/src/work.rs | 43 ++++-------------------------------- fontc/src/workload.rs | 32 +++++++++++++++++---------- 7 files changed, 79 insertions(+), 73 deletions(-) diff --git a/fontc/src/change_detector.rs b/fontc/src/change_detector.rs index 0fe4c833..94116b7c 100644 --- a/fontc/src/change_detector.rs +++ b/fontc/src/change_detector.rs @@ -357,7 +357,11 @@ impl ChangeDetector { if self.emit_ir { let current_sources = serde_yaml::to_string(&self.current_inputs).map_err(Error::YamlSerError)?; - fs::write(self.ir_paths.ir_input_file(), current_sources).map_err(Error::IoError) + let out_path = self.ir_paths.ir_input_file(); + fs::write(out_path, current_sources).map_err(|source| Error::FileIo { + path: out_path.to_owned(), + source, + }) } else { Ok(()) } diff --git a/fontc/src/config.rs b/fontc/src/config.rs index 060e726c..35155823 100644 --- a/fontc/src/config.rs +++ b/fontc/src/config.rs @@ -49,7 +49,12 @@ impl Config { .map_err(|_| Error::FileExpected(ir_input_file.to_owned()))?; } if self.args.incremental { - fs::write(config_file, serde_yaml::to_string(self)?)?; + fs::write(&config_file, serde_yaml::to_string(self)?).map_err(|source| { + Error::FileIo { + path: config_file, + source, + } + })? } }; @@ -57,7 +62,10 @@ impl Config { return Ok(Input::new()); } - let yml = fs::read_to_string(ir_input_file)?; + let yml = fs::read_to_string(ir_input_file).map_err(|source| Error::FileIo { + path: ir_input_file.to_owned(), + source, + })?; serde_yaml::from_str(&yml).map_err(Into::into) } diff --git a/fontc/src/error.rs b/fontc/src/error.rs index ee57d967..8fcc3472 100644 --- a/fontc/src/error.rs +++ b/fontc/src/error.rs @@ -1,27 +1,34 @@ use std::{io, path::PathBuf}; -use fontbe::orchestration::AnyWorkId; use fontir::error::TrackFileError; use thiserror::Error; #[derive(Debug, Error)] pub enum Error { + #[error("'{0}' exists but is not a directory")] + ExpectedDirectory(PathBuf), + #[error("io failed for '{path}': '{source}'")] + FileIo { + path: PathBuf, + #[source] + source: io::Error, + }, + #[error("failed to write to stdout or stderr: '{0}'")] + StdioWriteFail(#[source] io::Error), #[error("Unrecognized source {0}")] UnrecognizedSource(PathBuf), - #[error("yaml error: '{0}'")] + #[error(transparent)] YamlSerError(#[from] serde_yaml::Error), - #[error("IO error: '{0}'")] - IoError(#[from] io::Error), #[error(transparent)] TrackFile(#[from] TrackFileError), #[error("Font IR error: '{0}'")] FontIrError(#[from] fontir::error::Error), - #[error("Unable to produce IR")] - IrGenerationError, + #[error(transparent)] + Backend(#[from] fontbe::error::Error), #[error("Missing file '{0}'")] FileExpected(PathBuf), - #[error("Tasks failed: {0:?}")] - TasksFailed(Vec<(AnyWorkId, String)>), #[error("Unable to proceed; {0} jobs stuck pending")] UnableToProceed(usize), + #[error("A task panicked: '{0}'")] + Panic(String), } diff --git a/fontc/src/lib.rs b/fontc/src/lib.rs index 2103d4db..7c0f7997 100644 --- a/fontc/src/lib.rs +++ b/fontc/src/lib.rs @@ -16,7 +16,7 @@ pub use error::Error; pub use timing::{create_timer, JobTimer}; use workload::Workload; -use std::{fs, io, path::Path}; +use std::{fs, path::Path}; use fontbe::{ avar::create_avar_work, @@ -49,16 +49,18 @@ use fontir::paths::Paths as IrPaths; use log::{debug, warn}; -pub fn require_dir(dir: &Path) -> Result<(), io::Error> { - // skip empty paths +pub fn require_dir(dir: &Path) -> Result<(), Error> { if dir == Path::new("") { return Ok(()); } if dir.exists() && !dir.is_dir() { - panic!("{dir:#?} is taken by something that isn't a directory"); + return Err(Error::ExpectedDirectory(dir.to_owned())); } if !dir.exists() { - fs::create_dir(dir)? + fs::create_dir(dir).map_err(|source| Error::FileIo { + path: dir.to_owned(), + source, + })?; } debug!("require_dir {:?}", dir); Ok(()) @@ -90,7 +92,10 @@ pub fn init_paths(args: &Args) -> Result<(IrPaths, BePaths), Error> { } // It's confusing to have leftover debug files if be_paths.debug_dir().is_dir() { - fs::remove_dir_all(be_paths.debug_dir()).map_err(Error::IoError)?; + fs::remove_dir_all(be_paths.debug_dir()).map_err(|source| Error::FileIo { + path: be_paths.debug_dir().to_owned(), + source, + })?; } if args.emit_debug { require_dir(be_paths.debug_dir())?; @@ -102,7 +107,10 @@ pub fn write_font_file(args: &Args, be_context: &BeContext) -> Result<(), Error> // if IR is off the font didn't get written yet (nothing did), otherwise it's done already let font_file = be_context.font_file(); if !args.incremental { - fs::write(font_file, be_context.font.get().get()).map_err(Error::IoError)?; + fs::write(&font_file, be_context.font.get().get()).map_err(|source| Error::FileIo { + path: font_file, + source, + })?; } else if !font_file.exists() { return Err(Error::FileExpected(font_file)); } diff --git a/fontc/src/main.rs b/fontc/src/main.rs index 97d926b4..cc65e325 100644 --- a/fontc/src/main.rs +++ b/fontc/src/main.rs @@ -25,7 +25,7 @@ fn run() -> Result<(), Error> { let args = Args::parse(); // handle `--vv` verbose version argument request if args.verbose_version { - print_verbose_version()?; + print_verbose_version().map_err(Error::StdioWriteFail)?; std::process::exit(0); } let mut timer = JobTimer::new(Instant::now()); @@ -75,14 +75,20 @@ fn run() -> Result<(), Error> { let mut timing = workload.exec(&fe_root, &be_root)?; if config.args.flags().contains(Flags::EMIT_TIMING) { + let path = config.args.build_dir.join("threads.svg"); let out_file = OpenOptions::new() .write(true) .create(true) .truncate(true) - .open(config.args.build_dir.join("threads.svg")) - .map_err(Error::IoError)?; + .open(&path) + .map_err(|source| Error::FileIo { + path: path.clone(), + source, + })?; let mut buf = BufWriter::new(out_file); - timing.write_svg(&mut buf)? + timing + .write_svg(&mut buf) + .map_err(|source| Error::FileIo { path, source })?; } change_detector.finish_successfully()?; diff --git a/fontc/src/work.rs b/fontc/src/work.rs index 228c2af6..fbfc8bc5 100644 --- a/fontc/src/work.rs +++ b/fontc/src/work.rs @@ -2,46 +2,11 @@ //! //! Basically enums that can be a FeWhatever or a BeWhatever. -use std::fmt::Display; - -use fontbe::{ - error::Error as BeError, - orchestration::{AnyWorkId, BeWork, Context as BeContext}, -}; +use fontbe::orchestration::{AnyWorkId, BeWork, Context as BeContext}; use fontdrasil::orchestration::{Access, AccessType}; -use fontir::{ - error::Error as FeError, - orchestration::{Context as FeContext, IrWork, WorkId}, -}; - -#[derive(Debug)] -pub enum AnyWorkError { - Fe(FeError), - Be(BeError), - Panic(String), -} - -impl From for AnyWorkError { - fn from(e: BeError) -> Self { - AnyWorkError::Be(e) - } -} +use fontir::orchestration::{Context as FeContext, IrWork, WorkId}; -impl From for AnyWorkError { - fn from(e: FeError) -> Self { - AnyWorkError::Fe(e) - } -} - -impl Display for AnyWorkError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - AnyWorkError::Be(e) => e.fmt(f), - AnyWorkError::Fe(e) => e.fmt(f), - AnyWorkError::Panic(e) => write!(f, "Job panicked: '{e}'"), - } - } -} +use crate::Error; // Work of any type, FE, BE, ... some future pass, w/e #[derive(Debug)] @@ -95,7 +60,7 @@ impl AnyWork { } } - pub fn exec(&self, context: AnyContext) -> Result<(), AnyWorkError> { + pub fn exec(&self, context: AnyContext) -> Result<(), Error> { match self { AnyWork::Be(work) => work.exec(context.unwrap_be()).map_err(|e| e.into()), AnyWork::Fe(work) => work.exec(context.unwrap_fe()).map_err(|e| e.into()), diff --git a/fontc/src/workload.rs b/fontc/src/workload.rs index 069edd2c..501bbaf4 100644 --- a/fontc/src/workload.rs +++ b/fontc/src/workload.rs @@ -27,7 +27,7 @@ use log::{debug, trace, warn}; use crate::{ timing::{create_timer, JobTime, JobTimeQueued, JobTimer}, - work::{AnyAccess, AnyContext, AnyWork, AnyWorkError}, + work::{AnyAccess, AnyContext, AnyWork}, ChangeDetector, Error, }; @@ -37,7 +37,9 @@ pub struct Workload<'a> { pub(crate) change_detector: &'a ChangeDetector, job_count: usize, success: HashSet, - error: Vec<(AnyWorkId, String)>, + error: Option, + // we count the number of errors encountered but only store the first we see + n_failures: usize, // When K completes also mark all entries in V complete also_completes: HashMap>, @@ -98,6 +100,7 @@ impl<'a> Workload<'a> { job_count: 0, success: Default::default(), error: Default::default(), + n_failures: 0, also_completes: Default::default(), jobs_pending: Default::default(), count_pending: Default::default(), @@ -437,7 +440,7 @@ impl<'a> Workload<'a> { pub fn exec(mut self, fe_root: &FeContext, be_root: &BeContext) -> Result { // Async work will send us it's ID on completion let (send, recv) = - crossbeam_channel::unbounded::<(AnyWorkId, Result<(), AnyWorkError>, JobTime)>(); + crossbeam_channel::unbounded::<(AnyWorkId, Result<(), Error>, JobTime)>(); // a flag we set if we panic let abort_queued_jobs = Arc::new(AtomicBool::new(false)); @@ -570,7 +573,7 @@ impl<'a> Workload<'a> { Err(err) => { let msg = get_panic_message(err); abort.store(true, Ordering::Relaxed); - Err(AnyWorkError::Panic(msg)) + Err(Error::Panic(msg)) } }; // Decrement counters immediately so all-of detection checks true @@ -618,7 +621,7 @@ impl<'a> Workload<'a> { // If ^ exited due to error the scope awaited any live tasks; capture their results self.read_completions(&mut Vec::new(), &recv, RecvType::NonBlocking)?; - if self.error.is_empty() { + if self.error.is_none() { if self.success.len() != self.job_count { panic!( "No errors but only {}/{} succeeded?!", @@ -644,7 +647,7 @@ impl<'a> Workload<'a> { fn read_completions( &mut self, successes: &mut Vec<(AnyWorkId, JobTime)>, - recv: &Receiver<(AnyWorkId, Result<(), AnyWorkError>, JobTime)>, + recv: &Receiver<(AnyWorkId, Result<(), Error>, JobTime)>, initial_read: RecvType, ) -> Result<(), Error> { successes.clear(); @@ -671,8 +674,13 @@ impl<'a> Workload<'a> { inserted } Err(e) => { - log::error!("{completed_id:?} failed {e}"); - self.error.push((completed_id.clone(), format!("{e}"))); + self.n_failures += 1; + if self.error.is_none() { + self.error = Some(e); + } else { + // the first error will be reported on exit, log the rest: + log::error!("task '{completed_id:?}' failed: '{e}'"); + } true } } { @@ -684,7 +692,7 @@ impl<'a> Workload<'a> { log::debug!( "{}/{} complete, most recently {:?}", - self.error.len() + self.success.len(), + self.n_failures + self.success.len(), self.job_count, completed_id ); @@ -694,10 +702,10 @@ impl<'a> Workload<'a> { opt_complete = Some(completed_id); } } - if !self.error.is_empty() { - return Err(Error::TasksFailed(self.error.clone())); + match self.error.take() { + Some(error) => Err(error), + None => Ok(()), } - Ok(()) } #[cfg(test)]