From 08e5d24f0ac61f23e2e12b6954f64b3f1d5c1143 Mon Sep 17 00:00:00 2001 From: Chris Allen Date: Tue, 7 Jan 2025 21:48:45 -0600 Subject: [PATCH] Use std::path and canonicalization for the paths in Choo Purge inappropriate uses of std::process::exit, make task execution cooperative --- apps/choo/main.rs | 109 +++++++++++++++++++++++++---------- crown/src/nockapp/error.rs | 5 ++ crown/src/nockapp/nockapp.rs | 31 ++++++---- crown/src/noun/extensions.rs | 6 ++ 4 files changed, 111 insertions(+), 40 deletions(-) diff --git a/apps/choo/main.rs b/apps/choo/main.rs index c37bb7e3..f75404f1 100644 --- a/apps/choo/main.rs +++ b/apps/choo/main.rs @@ -1,5 +1,6 @@ use crown::kernel::boot; use crown::nockapp::driver::Operation; +use crown::nockapp::NockAppError; use crown::noun::slab::NounSlab; use crown::AtomExt; use futures::FutureExt; @@ -8,7 +9,7 @@ use sword::noun::{Atom, D, T}; use sword_macros::tas; use tokio::fs::File; use tokio::io::AsyncReadExt; -use tracing::error; +use tracing::{error, info}; use walkdir::{DirEntry, WalkDir}; use clap::{arg, command, ColorChoice, Parser}; @@ -29,10 +30,10 @@ struct ChooCli { boot: BootCli, #[arg(help = "Path to file to compile")] - entry: String, + entry: std::path::PathBuf, #[arg(help = "Path to root of dependency directory", default_value = "hoon")] - directory: String, + directory: std::path::PathBuf, #[arg( long, @@ -67,7 +68,7 @@ async fn main() -> Result<(), Error> { let cli = ChooCli::parse(); let result = std::panic::AssertUnwindSafe(async { let nockapp = initialize_nockapp(cli).await?; - work_loop(nockapp).await; + work_loop(nockapp).await?; Ok::<(), Error>(()) }) .catch_unwind() @@ -91,10 +92,18 @@ async fn main() -> Result<(), Error> { Ok(()) } -async fn initialize_nockapp(cli: ChooCli) -> Result { - // Remove trailing slash from directory if present - let directory = cli.directory.trim_end_matches('/').to_string(); +fn canonicalize_and_string(path: &std::path::Path) -> Result { + let path = path.canonicalize()?; + let path = path.to_str().ok_or_else(|| { + std::io::Error::new( + std::io::ErrorKind::NotFound, + "Path is not valid or file cannot be found", + ) + })?; + Ok(path.to_string()) +} +async fn initialize_nockapp(cli: ChooCli) -> Result { let mut nockapp = boot::setup(KERNEL_JAM, Some(cli.boot.clone()), &[], "choo")?; boot::init_default_tracing(&cli.boot.clone()); let mut slab = NounSlab::new(); @@ -114,18 +123,11 @@ async fn initialize_nockapp(cli: ChooCli) -> Result Result Result<(), NockAppError> { loop { let work_res = nockapp.work().await; - if let Err(e) = work_res { - error!("work error: {:?}", e); - break; - } + match work_res { + Ok(nockapp_run) => { + match nockapp_run { + crown::nockapp::NockAppRun::Pending => continue, + crown::nockapp::NockAppRun::Done => return Ok(()), + } + } + Err(NockAppError::Exit(code)) => { + if code == 0 { + // zero is success, we're simply done. + info!("nockapp exited successfully with code: {}", code); + return Ok(()); + } else { + error!("nockapp exited with error code: {}", code); + return Err(NockAppError::Exit(code)); + } + } + Err(e) => { + error!("Got error running nockapp: {:?}", e); + return Err(e); + } + }; } } @@ -186,20 +206,38 @@ mod tests { use tokio::fs; use tracing::info; + #[test] + fn test_result() -> Result<(), Box> { + let result: Result<(), Box> = Err(Box::new(std::io::Error::new( + std::io::ErrorKind::NotFound, + "Path is not valid or file cannot be found", + ))); + result + } + #[ignore] #[tokio::test] async fn test_compile_test_app() -> Result<(), Box> { - let mut test_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + // let mut test_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + // test_dir.pop(); + // test_dir.push("test-app"); + + // use std::path to get pwd() and then canonicalize + let pwd = std::env::current_dir().unwrap(); + let mut test_dir = pwd.clone(); test_dir.pop(); test_dir.push("test-app"); + let entry = test_dir.join("bootstrap/kernel.hoon"); + + // TODO: Add -o flag to specify output file and then use the tmp-dir + // TODO: instead of mutating the non-tmp filesystem in this test // Clean up any existing output file let _ = fs::remove_file("out.jam").await; - let mut deps_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + let mut deps_dir = pwd.clone(); deps_dir.pop(); deps_dir.push("hoon-deps"); - let entry = format!("{}/bootstrap/kernel.hoon", test_dir.display()); let result = async { let cli = ChooCli { @@ -212,14 +250,14 @@ mod tests { state_jam: None, }, entry: entry.clone(), - directory: deps_dir.display().to_string(), + directory: deps_dir.clone(), arbitrary: false, }; let nockapp = initialize_nockapp(cli).await?; - info!("Test directory: {}", test_dir.display()); - info!("Dependencies directory: {}", deps_dir.display()); - info!("Entry file: {}", entry.clone()); + info!("Test directory: {:?}", test_dir); + info!("Dependencies directory: {:?}", deps_dir); + info!("Entry file: {:?}", entry); work_loop(nockapp).await; // TODO this doesn't work because choo exits when compilation is done. @@ -235,4 +273,17 @@ mod tests { result } + + #[test] + #[cfg_attr(miri, ignore)] + fn test_canonicalize_and_string() { + let path = std::path::Path::new("Cargo.toml"); + let result = super::canonicalize_and_string(path); + assert!(result.is_ok()); + // left: "/Users/callen/work/zorp/nockapp/apps/choo/Cargo.toml" + // right: "Cargo.toml" + let pwd = std::env::current_dir().unwrap(); + let cargo_toml = pwd.join("Cargo.toml").canonicalize().unwrap(); + assert_eq!(result.unwrap(), cargo_toml.to_str().unwrap()); + } } diff --git a/crown/src/nockapp/error.rs b/crown/src/nockapp/error.rs index 6b9f46ed..4e57c532 100644 --- a/crown/src/nockapp/error.rs +++ b/crown/src/nockapp/error.rs @@ -8,8 +8,13 @@ use super::driver::IOAction; /// Error type for NockApps #[derive(Debug, Error)] pub enum NockAppError { + /// NockApp exited with a code, shouldn't ever be 0, that's a Done/Success. + #[error("NockApp exited with error code: {0}")] + Exit(usize), #[error("Timeout")] Timeout, + #[error("Shut down for cancel token")] + ShutdownForCancelToken, #[error("IO error: {0}")] IoError(#[source] std::io::Error), #[error("Save error: {0}")] diff --git a/crown/src/nockapp/nockapp.rs b/crown/src/nockapp/nockapp.rs index b58eeff3..17d1751e 100644 --- a/crown/src/nockapp/nockapp.rs +++ b/crown/src/nockapp/nockapp.rs @@ -44,6 +44,11 @@ pub struct NockApp { pub npc_socket_path: Option, } +pub enum NockAppRun { + Pending, + Done, +} + impl NockApp { pub fn new(kernel: Kernel, save_interval: Duration) -> Self { let (action_channel_sender, action_channel) = mpsc::channel(100); @@ -149,7 +154,7 @@ impl NockApp { Ok(()) } - pub async fn work(&mut self) -> Result<(), NockAppError> { + pub async fn work(&mut self) -> Result { let tasks_fut = async { let mut joinset = self.tasks.clone().lock_owned().await; joinset.join_next().await @@ -165,7 +170,7 @@ impl NockApp { } } } - std::process::exit(1); + return Err(NockAppError::ShutdownForCancelToken); } select!( @@ -181,7 +186,7 @@ impl NockApp { Some(Err(e)) => { Err(e)? }, - _ => {Ok(())}, + _ => {Ok(NockAppRun::Pending)}, } }, permit = self.save_sem.clone().acquire_owned() => { @@ -189,7 +194,7 @@ impl NockApp { let curr_event_num = self.kernel.serf.event_num; let saved_event_num = self.watch_recv.borrow(); if curr_event_num <= *saved_event_num { - return Ok(()) + return Ok(NockAppRun::Pending) } drop(saved_event_num); @@ -202,11 +207,12 @@ impl NockApp { if elapsed < self.save_interval && !self.exit_status.load(Ordering::SeqCst) { tokio::time::sleep(self.save_interval - elapsed).await; } - return res + return res.map(|_| NockAppRun::Pending); }, exit = self.exit_recv.recv() => { if let Some(code) = exit { self.action_channel.close(); + // TODO: See if exit_status is duplicative of what the cancel token is for. self.exit_status.store(true, Ordering::SeqCst); let exit_event_num = self.kernel.serf.event_num; info!("Exit request received, waiting for save checkpoint with event_num {}", exit_event_num); @@ -226,14 +232,17 @@ impl NockApp { } } } - info!("Save event_num reached, exiting with code {}", code); - std::process::exit(code as i32); + info!("Save event_num reached, finishing with code {}", code); + if code == 0 { + return Ok(NockAppRun::Done); + } else { + return Err(NockAppError::Exit(code)); + } } } }); - Ok(()) - } - else { + Ok(NockAppRun::Pending) + } else { error!("Exit signal channel closed prematurely"); Err(NockAppError::ChannelClosedError) } @@ -275,7 +284,7 @@ impl NockApp { }, } } - Ok(()) + Ok(NockAppRun::Pending) } ) } diff --git a/crown/src/noun/extensions.rs b/crown/src/noun/extensions.rs index 9196b389..24eb8cca 100644 --- a/crown/src/noun/extensions.rs +++ b/crown/src/noun/extensions.rs @@ -50,6 +50,10 @@ impl NounExt for Noun { } } +// TODO: This exists largely because crown doesn't own the [`Atom`] type from [`sword`]. +// TODO: The next step for this should be to lower the methods on this trait to a concrete `impl` stanza for [`Atom`] in [`sword`]. +// TODO: In the course of doing so, we should split out a serialization trait that has only the [`AtomExt::from_value`] method as a public API in [`sword`]. +// The goal would be to canonicalize the Atom representations of various Rust types. When it needs to be specialized, users can make a newtype. pub trait AtomExt { fn from_bytes(allocator: &mut A, bytes: &Bytes) -> Atom; fn from_value(allocator: &mut A, value: T) -> Result; @@ -59,12 +63,14 @@ pub trait AtomExt { } impl AtomExt for Atom { + // TODO: This is iffy. What byte representation is it expecting and why? fn from_bytes(allocator: &mut A, bytes: &Bytes) -> Atom { unsafe { IndirectAtom::new_raw_bytes(allocator, bytes.len(), bytes.as_ptr()).normalize_as_atom() } } + // TODO: This is worth making into a public/supported part of [`sword`]'s API. fn from_value(allocator: &mut A, value: T) -> Result { unsafe { let data: Bytes = value.as_bytes()?;