Redesign spirv-builder's watch API

Author: tuguzT

crates/spirv-builder/src/lib.rs

@@ -92,7 +92,7 @@ pub use rustc_codegen_spirv_types::Capability;
 pub use rustc_codegen_spirv_types::{CompileResult, ModuleResult};
 
 #[cfg(feature = "watch")]
-pub use self::watch::Watch;
+pub use self::watch::{SpirvBuilderWatchError, Watch};
 
 #[cfg(feature = "include-target-specs")]
 pub use rustc_codegen_spirv_target_specs::TARGET_SPEC_DIR_PATH;
@@ -125,14 +125,15 @@ pub enum SpirvBuilderError {
     BuildFailed,
     #[error("multi-module build cannot be used with print_metadata = MetadataPrintout::Full")]
     MultiModuleWithPrintMetadata,
-    #[error("watching within build scripts will prevent build completion")]
-    WatchWithPrintMetadata,
     #[error("multi-module metadata file missing")]
     MetadataFileMissing(#[from] std::io::Error),
     #[error("unable to parse multi-module metadata file")]
     MetadataFileMalformed(#[from] serde_json::Error),
     #[error("cargo metadata error")]
     CargoMetadata(#[from] cargo_metadata::Error),
+    #[cfg(feature = "watch")]
+    #[error(transparent)]
+    WatchFailed(#[from] SpirvBuilderWatchError),
 }
 
 const SPIRV_TARGET_PREFIX: &str = "spirv-unknown-";

crates/spirv-builder/src/watch.rs

@@ -10,7 +10,7 @@ use rustc_codegen_spirv_types::CompileResult;
 use crate::{SpirvBuilder, SpirvBuilderError, leaf_deps};
 
 impl SpirvBuilder {
-    /// Watches the module for changes using [`notify`], rebuilding it upon changes.
+    /// Watches the module for changes, rebuilding it upon them.
     ///
     /// Calls `on_compilation_finishes` after each successful compilation.
     /// The second `Option<AcceptFirstCompile<T>>` param allows you to return some `T`
@@ -27,36 +27,18 @@ impl SpirvBuilder {
             .as_ref()
             .ok_or(SpirvBuilderError::MissingCratePath)?;
         if !matches!(self.print_metadata, crate::MetadataPrintout::None) {
-            return Err(SpirvBuilderError::WatchWithPrintMetadata);
+            return Err(SpirvBuilderWatchError::WatchWithPrintMetadata.into());
         }
 
-        let metadata_result = crate::invoke_rustc(self);
-        // Load the dependencies of the thing
-        let metadata_file = if let Ok(path) = metadata_result {
-            path
-        } else {
-            // Fall back to watching from the crate root if the initial compilation fails
-            // This is likely to notice changes in the `target` dir, however, given that `cargo watch` doesn't seem to handle that,
-            let mut watcher = Watcher::new();
-            watcher
-                .watcher
-                .watch(path_to_crate, RecursiveMode::Recursive)
-                .expect("Could watch crate root");
-            loop {
-                watcher.recv();
-                let metadata_file = crate::invoke_rustc(self);
-                if let Ok(f) = metadata_file {
-                    break f;
-                }
-            }
-        };
+        let metadata_file = metadata_file(path_to_crate, self)?;
         let metadata = self.parse_metadata_file(&metadata_file)?;
+
         let mut first_compile = None;
         on_compilation_finishes(metadata, Some(AcceptFirstCompile(&mut first_compile)));
 
         let builder = self.clone();
+        let mut watcher = Watcher::new()?;
         let watch_thread = std::thread::spawn(move || {
-            let mut watcher = Watcher::new();
             watcher.watch_leaf_deps(&metadata_file);
 
             loop {
@@ -65,7 +47,7 @@ impl SpirvBuilder {
                 if let Ok(file) = metadata_result {
                     let metadata = builder
                         .parse_metadata_file(&file)
-                        .expect("Metadata file is correct");
+                        .expect("metadata file should be valid");
                     watcher.watch_leaf_deps(&metadata_file);
                     on_compilation_finishes(metadata, None);
                 }
@@ -79,6 +61,52 @@ impl SpirvBuilder {
     }
 }
 
+fn notify_channel() -> Result<(RecommendedWatcher, Receiver<()>), SpirvBuilderWatchError> {
+    let (tx, rx) = sync_channel(0);
+    let event_handler = move |result: notify::Result<Event>| match result {
+        Ok(event) => match event.kind {
+            notify::EventKind::Any
+            | notify::EventKind::Create(_)
+            | notify::EventKind::Modify(_)
+            | notify::EventKind::Remove(_)
+            | notify::EventKind::Other => {
+                if let Err(err) = tx.try_send(()) {
+                    log::error!("send error: {err:?}");
+                }
+            }
+            notify::EventKind::Access(_) => {}
+        },
+        Err(err) => log::error!("notify error: {err:?}"),
+    };
+    let watcher = notify::recommended_watcher(event_handler)?;
+    Ok((watcher, rx))
+}
+
+fn metadata_file<P>(
+    crate_path: P,
+    builder: &SpirvBuilder,
+) -> Result<PathBuf, SpirvBuilderWatchError>
+where
+    P: AsRef<Path>,
+{
+    match crate::invoke_rustc(builder) {
+        Ok(path) => return Ok(path),
+        Err(err) => log::error!("{err}"),
+    }
+
+    // Fall back to watching from the crate root if the initial compilation fails
+    // This is likely to notice changes in the `target` dir, however, given that `cargo watch` doesn't seem to handle that,
+    let (mut watcher, rx) = notify_channel()?;
+    watcher.watch(crate_path.as_ref(), RecursiveMode::Recursive)?;
+    loop {
+        rx.recv().expect("watcher should be alive");
+        match crate::invoke_rustc(builder) {
+            Ok(path) => break Ok(path),
+            Err(err) => log::error!("{err}"),
+        }
+    }
+}
+
 pub struct AcceptFirstCompile<'a, T>(&'a mut Option<T>);
 
 impl<'a, T> AcceptFirstCompile<'a, T> {
@@ -111,28 +139,13 @@ struct Watcher {
 }
 
 impl Watcher {
-    fn new() -> Self {
-        let (tx, rx) = sync_channel(0);
-        let watcher =
-            notify::recommended_watcher(move |event: notify::Result<Event>| match event {
-                Ok(e) => match e.kind {
-                    notify::EventKind::Access(_) => (),
-                    notify::EventKind::Any
-                    | notify::EventKind::Create(_)
-                    | notify::EventKind::Modify(_)
-                    | notify::EventKind::Remove(_)
-                    | notify::EventKind::Other => {
-                        let _ = tx.try_send(());
-                    }
-                },
-                Err(e) => println!("notify error: {e:?}"),
-            })
-            .expect("Could create watcher");
-        Self {
+    fn new() -> Result<Self, SpirvBuilderWatchError> {
+        let (watcher, rx) = notify_channel()?;
+        Ok(Self {
             watcher,
             rx,
             watched_paths: HashSet::new(),
-        }
+        })
     }
 
     fn watch_leaf_deps(&mut self, metadata_file: &Path) {
@@ -141,13 +154,21 @@ impl Watcher {
             if self.watched_paths.insert(path.to_owned()) {
                 self.watcher
                     .watch(it.to_path().unwrap(), RecursiveMode::NonRecursive)
-                    .expect("Cargo dependencies are valid files");
+                    .expect("files of cargo dependencies should be valid");
             }
         })
-        .expect("Could read dependencies file");
+        .expect("should be able to read dependencies file");
     }
 
     fn recv(&self) {
-        self.rx.recv().expect("Watcher still alive");
+        self.rx.recv().expect("watcher should be alive");
     }
 }
+
+#[derive(Debug, thiserror::Error)]
+pub enum SpirvBuilderWatchError {
+    #[error("watching within build scripts will prevent build completion")]
+    WatchWithPrintMetadata,
+    #[error("could not notify for changes: {0}")]
+    NotifyFailed(#[from] notify::Error),
+}