Skip to content

Commit

Permalink
[fontc] Tweak root error type
Browse files Browse the repository at this point in the history
- 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
  • Loading branch information
cmyr committed Jun 26, 2024
1 parent f7c6f8e commit 138df63
Show file tree
Hide file tree
Showing 7 changed files with 79 additions and 73 deletions.
6 changes: 5 additions & 1 deletion fontc/src/change_detector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
}
Expand Down
12 changes: 10 additions & 2 deletions fontc/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,23 @@ 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,
}
})?
}
};

if !ir_input_file.exists() {
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)
}

Expand Down
23 changes: 15 additions & 8 deletions fontc/src/error.rs
Original file line number Diff line number Diff line change
@@ -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),
}
22 changes: 15 additions & 7 deletions fontc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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(())
Expand Down Expand Up @@ -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())?;
Expand All @@ -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));
}
Expand Down
14 changes: 10 additions & 4 deletions fontc/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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()?;
Expand Down
43 changes: 4 additions & 39 deletions fontc/src/work.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<BeError> for AnyWorkError {
fn from(e: BeError) -> Self {
AnyWorkError::Be(e)
}
}
use fontir::orchestration::{Context as FeContext, IrWork, WorkId};

impl From<FeError> 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)]
Expand Down Expand Up @@ -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()),
Expand Down
32 changes: 20 additions & 12 deletions fontc/src/workload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand All @@ -37,7 +37,9 @@ pub struct Workload<'a> {
pub(crate) change_detector: &'a ChangeDetector,
job_count: usize,
success: HashSet<AnyWorkId>,
error: Vec<(AnyWorkId, String)>,
error: Option<Error>,
// 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<AnyWorkId, Vec<AnyWorkId>>,
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -437,7 +440,7 @@ impl<'a> Workload<'a> {
pub fn exec(mut self, fe_root: &FeContext, be_root: &BeContext) -> Result<JobTimer, Error> {
// 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));
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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?!",
Expand All @@ -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();
Expand All @@ -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
}
} {
Expand All @@ -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
);
Expand All @@ -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)]
Expand Down

0 comments on commit 138df63

Please sign in to comment.