From 79644c7912afe6dd241165914d22a27ac84f53a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonathan=20Wid=C3=A9n?= Date: Fri, 31 Mar 2023 15:17:03 +0200 Subject: [PATCH] GH-195: Cleanup --- cli/src/error.rs | 5 +--- cli/src/main.rs | 62 ++++++++++++++++++++++----------------- cli/src/package.rs | 8 ++--- core/src/context.rs | 11 ++++--- core/src/package.rs | 2 +- core/src/package_store.rs | 24 ++++++++------- core/src/std_packages.rs | 4 +-- 7 files changed, 63 insertions(+), 53 deletions(-) diff --git a/cli/src/error.rs b/cli/src/error.rs index 616cee43..174056cf 100644 --- a/cli/src/error.rs +++ b/cli/src/error.rs @@ -19,7 +19,7 @@ pub enum CliError { #[error("Core error: {0}")] Core(#[from] CoreError), - #[error("Cannot infer output format, please specifiy --format")] + #[error("Cannot infer output format, please specify --format")] UnknownOutputFormat, #[error("Reqwest error '{0}'")] @@ -37,9 +37,6 @@ pub enum CliError { #[error("Could not get registry source")] Registry, - #[error("Specifier '{0}' not supported")] - Specifier(String), - #[error("Could not find local path to '{0}'")] Local(String), diff --git a/cli/src/main.rs b/cli/src/main.rs index 78e4fa89..65d682e9 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -1,4 +1,3 @@ -use std::cell::RefCell; use std::{ collections::HashMap, env, @@ -35,11 +34,11 @@ use warp::{ Filter, Rejection, Reply, }; -use crate::file_access::CliAccessManager; use error::CliError; use modmark_core::{context::CompilationState, eval, Context, CoreError, OutputFormat}; use parser::{parse, Ast}; +use crate::file_access::CliAccessManager; use crate::package::PackageManager; mod error; @@ -150,6 +149,7 @@ static RECEIVER: OnceCell>> = OnceCell::new(); static PREVIEW_PORT: OnceCell> = OnceCell::new(); static ABSOLUTE_OUTPUT_PATH: OnceCell = OnceCell::new(); static CONNECTION_ID_COUNTER: AtomicUsize = AtomicUsize::new(1); +static MAX_COMPILATION_TRIES: usize = 3; type PreviewConnections = Arc>>>; type PreviewDoc = Arc>; @@ -160,31 +160,27 @@ type CompilationResult = Result<(String, CompilationState, Ast), Vec> async fn compile_file(input_file: &Path, output_format: &OutputFormat) -> CompilationResult { let source = fs::read_to_string(input_file).map_err(|e| vec![e.into()])?; let ast = parse(&source).map_err(|e| vec![e.into()])?; - if let Some((output, state)) = eval( - &source, - &mut CTX.get().unwrap().lock().unwrap(), - output_format, - )? { - return Ok((output, state, ast)); - } - spawn_blocking(|| { - RECEIVER.get().unwrap().lock().unwrap().blocking_recv(); - }) - .await - .unwrap(); + for i in 1..=MAX_COMPILATION_TRIES { + if let Some((output, state)) = eval( + &source, + &mut CTX.get().unwrap().lock().unwrap(), + output_format, + )? { + return Ok((output, state, ast)); + } - if let Some((output, state)) = eval( - &source, - &mut CTX.get().unwrap().lock().unwrap(), - output_format, - )? { - return Ok((output, state, ast)); + if i != MAX_COMPILATION_TRIES { + spawn_blocking(|| { + RECEIVER.get().unwrap().lock().unwrap().blocking_recv(); + }) + .await + .unwrap(); + } } - panic!("Something went wrong!!!") + Err(vec![]) } - fn print_compiling_message() -> Result<(), CliError> { let mut stdout = stdout(); stdout.execute(terminal::Clear(terminal::ClearType::All))?; @@ -202,11 +198,23 @@ fn print_result(result: &CompilationResult, args: &Args) -> Result<(), CliError> Err(errors) => { stdout.execute(terminal::Clear(terminal::ClearType::All))?; stdout.execute(cursor::MoveTo(0, 0))?; - stdout.execute(style::PrintStyledContent( - format!("{} compilation error(s):\n", errors.len()).red(), - ))?; - for error in errors { - stdout.execute(style::PrintStyledContent(format!("{error:?}\n").red()))?; + let num_errors = errors.len(); + if num_errors == 0 { + stdout.execute(style::PrintStyledContent( + "No result retrieved from compiler\n".red(), + ))?; + } else if num_errors == 1 { + let error = errors.first().unwrap(); + stdout.execute(style::PrintStyledContent( + format!("1 compilation error:\n{error}\n").red(), + ))?; + } else { + stdout.execute(style::PrintStyledContent( + format!("{} compilation errors:\n", num_errors).red(), + ))?; + for error in errors { + stdout.execute(style::PrintStyledContent(format!("{error:?}\n").red()))?; + } } return Ok(()); } diff --git a/cli/src/package.rs b/cli/src/package.rs index 1198d14c..9cf173e0 100644 --- a/cli/src/package.rs +++ b/cli/src/package.rs @@ -21,20 +21,20 @@ pub struct PackageManager { impl Resolve for PackageManager { fn resolve_all(&self, paths: Vec) { + // I don't know how to get rid of all these clones... let self_clone = self.clone(); tokio::spawn(async move { - let self_clone2 = self_clone.clone(); join_all( paths .into_iter() .map(|task| { - let self_clone = self_clone.clone(); - tokio::spawn(async move { self_clone.clone().resolve(task).await }) + let another_self = self_clone.clone(); + tokio::spawn(async move { another_self.resolve(task).await }) }) .collect::>(), ) .await; - self_clone2.sender.send(()).await.unwrap(); + self_clone.sender.send(()).await.unwrap(); }); } } diff --git a/core/src/context.rs b/core/src/context.rs index 4a033b07..4e6c0a6f 100644 --- a/core/src/context.rs +++ b/core/src/context.rs @@ -80,10 +80,9 @@ impl CompilationState { impl Debug for Context { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { f.debug_struct("Context") - // .field("native packages", &self.native_packages) - // .field("standard packages", &self.standard_packages) - // .field("external packages", &self.external_packages) - // .field("transforms", &self.transforms) + .field("package manager", &self.package_manager) + .field("compilation state", &self.state) + .field("filesystem", &self.filesystem) .finish() } } @@ -125,6 +124,7 @@ impl TransformVariant { } impl Context { + /// Creates a new Context with the given resolver and policy pub fn new(resolver: T, policy: U) -> Result where T: Resolve, @@ -178,6 +178,9 @@ where lock.expose_transforms(config.try_into()?)?; Ok(true) } else { + // IMPORTANT: It is important that we drop the lock here. If resolve_all were to resolve + // the packages in this thread, they would need to acquire the lock, which is impossible + // if it isn't dropped here and would result in a dead-lock drop(lock); self.resolver.resolve_all(missings); Ok(false) diff --git a/core/src/package.rs b/core/src/package.rs index 31dbf43f..0f7a9f71 100644 --- a/core/src/package.rs +++ b/core/src/package.rs @@ -134,7 +134,7 @@ impl ArgType { ArgType::Enum(values) => value .as_str() .map(ToString::to_string) - .filter(|s| values.contains(&s)) + .filter(|s| values.contains(s)) .map(ArgValue::EnumVariant) .ok_or(CoreError::EnumVariant(values.clone(), value.to_string())), ArgType::Primitive(t) => t.try_from_value(value), diff --git a/core/src/package_store.rs b/core/src/package_store.rs index 5d31694c..5c1aee39 100644 --- a/core/src/package_store.rs +++ b/core/src/package_store.rs @@ -179,7 +179,6 @@ impl PackageStore { .chain(self.external_packages.values()) .map(|pkg| pkg.info.clone()) .collect::>() - .clone() } /// This gets the transform info for a given element and output format. If a native package @@ -202,7 +201,7 @@ impl PackageStore { output_format: &OutputFormat, ) -> Result, CoreError> { self.get_transform_info(element_name, output_format) - .map(|info| info.arguments.clone()) + .map(|info| info.arguments) .ok_or(CoreError::MissingTransform( element_name.to_string(), output_format.0.to_string(), @@ -254,7 +253,7 @@ impl PackageStore { }) .map(|id| ResolveTask { manager: Arc::clone(&arc_mutex), - package: id.clone(), + package: id, resolved: false, }) .collect(); @@ -345,8 +344,8 @@ impl PackageStore { } mem::take(&mut config.0) - .into_iter() - .map(|(id, _)| CoreError::UnusedConfig(id.name)) + .into_keys() + .map(|id| CoreError::UnusedConfig(id.name)) .for_each(|e| errors.push(e)); if errors.is_empty() { @@ -401,7 +400,7 @@ impl PackageStore { Ok(()) } - pub(crate) fn is_missing_packages(&self) -> bool { + pub fn is_missing_packages(&self) -> bool { !self.awaited_packages.is_empty() } @@ -491,10 +490,6 @@ pub trait Resolve { fn resolve_all(&self, paths: Vec); } -pub struct ResolveWrapper { - task: ResolveTask, -} - #[derive(Debug)] pub struct ResolveTask { manager: Arc>, @@ -536,11 +531,18 @@ impl Drop for ResolveTask { if !self.resolved { let mut manager = self.manager.lock().unwrap(); let package = mem::take(&mut self.package); - manager.reject_request(package.clone(), CoreError::DroppedRequest); + manager.reject_request(package, CoreError::DroppedRequest); } } } +pub struct DenyAllResolver; +impl Resolve for DenyAllResolver { + fn resolve_all(&self, _paths: Vec) { + // Dropping the tasks should reject them + } +} + #[derive(Debug, Clone, Hash, PartialEq, Eq, Default)] pub struct PackageID { pub name: String, diff --git a/core/src/std_packages.rs b/core/src/std_packages.rs index 17a39bd9..57fd1e59 100644 --- a/core/src/std_packages.rs +++ b/core/src/std_packages.rs @@ -188,9 +188,9 @@ pub fn native_err( // Push the issue to errors ctx.state.errors.push(Issue { source: source.to_string(), - target: target.to_string(), + target, description: body.to_string(), - input: input.map(|s| s.to_string()), + input, }); // Check if we have an __error transform