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

@@ -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<crown::nockapp::NockApp, Error> {
-    // 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<String, Error> {
+    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<crown::nockapp::NockApp, Error> {
     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<crown::nockapp::NockApp, Err
         Atom::from_value(&mut slab, contents_vec).unwrap().as_noun()
     };
 
-    let mut entry = cli.entry.clone();
-
-    //  Insert a leading slash if it is not present
-    //  Needed to make the entry path an actual hoon $path type
-    if !entry.starts_with('/') {
-        entry.insert(0, '/');
-    }
-
-    // hoon does not support uppercase paths
-    let entry_path = Atom::from_value(&mut slab, entry.to_lowercase()).unwrap().as_noun();
+    let entry_string = canonicalize_and_string(&cli.entry)?;
+    let entry_path = Atom::from_value(&mut slab, entry_string).unwrap().as_noun();
 
     let mut directory_noun = D(0);
+    let directory = canonicalize_and_string(&cli.directory)?;
 
     let walker = WalkDir::new(&directory)
         .follow_links(true)
@@ -169,13 +171,31 @@ async fn initialize_nockapp(cli: ChooCli) -> Result<crown::nockapp::NockApp, Err
     Ok(nockapp)
 }
 
-async fn work_loop(mut nockapp: crown::nockapp::NockApp) {
+async fn work_loop(mut nockapp: crown::nockapp::NockApp) -> 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<dyn std::error::Error>> {
+        let result: Result<(), Box<dyn std::error::Error>> = 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<dyn std::error::Error>> {
-        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());
+    }
 }

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}")]

crown/src/nockapp/nockapp.rs

@@ -44,6 +44,11 @@ pub struct NockApp {
     pub npc_socket_path: Option<PathBuf>,
 }
 
+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<NockAppRun, NockAppError> {
         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,15 +186,15 @@ impl NockApp {
                     Some(Err(e)) => {
                         Err(e)?
                     },
-                    _ => {Ok(())},
+                    _ => {Ok(NockAppRun::Pending)},
                 }
             },
             permit = self.save_sem.clone().acquire_owned() => {
                 //  Check if we should write in the first place
                 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)
             }
         )
     }

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<A: NounAllocator>(allocator: &mut A, bytes: &Bytes) -> Atom;
     fn from_value<A: NounAllocator, T: ToBytes>(allocator: &mut A, value: T) -> Result<Atom>;
@@ -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<A: NounAllocator>(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<A: NounAllocator, T: ToBytes>(allocator: &mut A, value: T) -> Result<Atom> {
         unsafe {
             let data: Bytes = value.as_bytes()?;