Clean up the interface of the watching support (#663)

* Clean up the interface of the watching support

This allows us to avoid the seperate thread just for looping through
the graphics rx to the eventloop, for example
In almost all cases, intial results are blocked on the first result, and
then needs notifications for later changes

* Fix test failures

* fmt

* Remove the comment again

crates/spirv-builder/src/watch.rs

@@ -6,13 +6,15 @@ use rustc_codegen_spirv::CompileResult;
 use crate::{leaf_deps, SpirvBuilder, SpirvBuilderError};
 
 impl SpirvBuilder {
-    /// Watches the module for changes using [`notify`](https://crates.io/crates/notify).
+    /// Watches the module for changes using [`notify`](https://crates.io/crates/notify),
+    /// and rebuild it upon changes
     ///
-    /// This is a blocking operation, wand should never return in the happy path
+    /// Returns the result of the first successful compilation, then calls
+    /// `on_compilation_finishes` for each subsequent compilation.
     pub fn watch(
         mut self,
-        on_compilation_finishes: impl Fn(CompileResult),
+        mut on_compilation_finishes: impl FnMut(CompileResult) + Send + 'static,
-    ) -> Result<(), SpirvBuilderError> {
+    ) -> Result<CompileResult, SpirvBuilderError> {
         self.validate_running_conditions()?;
         if !matches!(self.print_metadata, crate::MetadataPrintout::None) {
             return Err(SpirvBuilderError::WatchWithPrintMetadata);
@@ -53,52 +55,58 @@ impl SpirvBuilder {
             }
         };
         let metadata = self.parse_metadata_file(&metadata_file)?;
-        on_compilation_finishes(metadata);
+        let first_result = metadata;
-        let mut watched_paths = HashSet::new();
-        let (tx, rx) = sync_channel(0);
-        let mut watcher =
-            notify::immediate_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");
-        leaf_deps(&metadata_file, |it| {
-            let path = it.to_path().unwrap();
-            if watched_paths.insert(path.to_owned()) {
-                watcher
-                    .watch(it.to_path().unwrap(), RecursiveMode::NonRecursive)
-                    .expect("Cargo dependencies are valid files");
-            }
-        })
-        .expect("Could read dependencies file");
-        loop {
-            rx.recv().expect("Watcher still alive");
-            let metadata_result = crate::invoke_rustc(&self);
-            if let Ok(file) = metadata_result {
-                // We can bubble this error up because it's an internal error  (e.g. rustc_codegen_spirv's version of CompileResult is somehow out of sync)
-                let metadata = self.parse_metadata_file(&file)?;
 
-                leaf_deps(&file, |it| {
+        let thread = std::thread::spawn(move || {
-                    let path = it.to_path().unwrap();
+            let mut watched_paths = HashSet::new();
-                    if watched_paths.insert(path.to_owned()) {
+            let (tx, rx) = sync_channel(0);
-                        watcher
+            let mut watcher =
-                            .watch(it.to_path().unwrap(), RecursiveMode::NonRecursive)
+                notify::immediate_watcher(move |event: notify::Result<Event>| match event {
-                            .expect("Cargo dependencies are valid files");
+                    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 read dependencies file");
+                .expect("Could create watcher");
+            leaf_deps(&metadata_file, |it| {
+                let path = it.to_path().unwrap();
+                if watched_paths.insert(path.to_owned()) {
+                    watcher
+                        .watch(it.to_path().unwrap(), RecursiveMode::NonRecursive)
+                        .expect("Cargo dependencies are valid files");
+                }
+            })
+            .expect("Could read dependencies file");
+            loop {
+                rx.recv().expect("Watcher still alive");
+                let metadata_result = crate::invoke_rustc(&self);
+                if let Ok(file) = metadata_result {
+                    let metadata = self
+                        .parse_metadata_file(&file)
+                        .expect("Metadata file is correct");
 
-                on_compilation_finishes(metadata);
+                    leaf_deps(&file, |it| {
+                        let path = it.to_path().unwrap();
+                        if watched_paths.insert(path.to_owned()) {
+                            watcher
+                                .watch(it.to_path().unwrap(), RecursiveMode::NonRecursive)
+                                .expect("Cargo dependencies are valid files");
+                        }
+                    })
+                    .expect("Could read dependencies file");
+
+                    on_compilation_finishes(metadata);
+                }
             }
-        }
+        });
+        std::mem::forget(thread);
+        Ok(first_result)
     }
 }

examples/runners/wgpu/src/compute.rs

@@ -15,8 +15,7 @@ fn block_on<T>(future: impl Future<Output = T>) -> T {
 }
 
 pub fn start(options: &Options) {
-    let rx = crate::maybe_watch(options.shader, true);
+    let shader_binary = crate::maybe_watch(options.shader, None);
-    let shader_binary = rx.recv().expect("Should send one binary");
 
     block_on(start_internal(options, shader_binary))
 }

examples/runners/wgpu/src/graphics.rs

@@ -1,5 +1,3 @@
-use std::thread::spawn;
-
 use crate::maybe_watch;
 
 use super::Options;
@@ -310,21 +308,17 @@ fn create_pipeline(
 
 pub fn start(options: &Options) {
     // Build the shader before we pop open a window, since it might take a while.
-    let rx = maybe_watch(options.shader, false);
-    let initial_shader = rx.recv().expect("Initial shader is required");
-
     let event_loop = EventLoop::with_user_event();
     let proxy = event_loop.create_proxy();
-    let thread = spawn(move || loop {
+    let initial_shader = maybe_watch(
-        while let Ok(result) = rx.recv() {
+        options.shader,
-            match proxy.send_event(result) {
+        Some(Box::new(move |res| match proxy.send_event(res) {
-                Ok(()) => {}
+            Ok(it) => it,
-                // If something goes wrong, close this thread
+            // ShaderModuleDescriptor is not `Debug`, so can't use unwrap/expect
-                Err(_) => break,
+            Err(_) => panic!("Event loop dead"),
-            }
+        })),
-        }
+    );
-    });
+
-    std::mem::forget(thread);
     let window = winit::window::WindowBuilder::new()
         .with_title("Rust GPU - wgpu")
         .with_inner_size(winit::dpi::LogicalSize::new(1280.0, 720.0))

examples/runners/wgpu/src/lib.rs

@@ -42,8 +42,6 @@
     rust_2018_idioms
 )]
 
-use std::sync::mpsc::{self, Receiver};
-
 use clap::Clap;
 use strum::{Display, EnumString};
 
@@ -60,11 +58,8 @@ pub enum RustGPUShader {
 
 fn maybe_watch(
     shader: RustGPUShader,
-    force_no_watch: bool,
+    on_watch: Option<Box<dyn FnMut(wgpu::ShaderModuleDescriptor<'static>) + Send + 'static>>,
-) -> Receiver<wgpu::ShaderModuleDescriptor<'static>> {
+) -> wgpu::ShaderModuleDescriptor<'static> {
-    // This bound needs to be 1, because in cases where this function is used for direct building (e.g. for the compute example or on android)
-    // we send the value directly in the same thread. This avoids deadlocking in those cases.
-    let (tx, rx) = mpsc::sync_channel(1);
     #[cfg(not(any(target_os = "android", target_arch = "wasm32")))]
     {
         use spirv_builder::{Capability, CompileResult, MetadataPrintout, SpirvBuilder};
@@ -94,23 +89,16 @@ fn maybe_watch(
         for &cap in capabilities {
             builder = builder.capability(cap);
         }
-        if force_no_watch {
+        let initial_result = if let Some(mut f) = on_watch {
-            let compile_result = builder.build().unwrap();
+            builder
-            handle_builder_result(compile_result, &tx);
+                .watch(move |compile_result| f(handle_compile_result(compile_result)))
+                .expect("Configuration is correct for watching")
         } else {
-            let thread = std::thread::spawn(move || {
+            builder.build().unwrap()
-                builder
+        };
-                    .watch(|compile_result| {
+        fn handle_compile_result(
-                        handle_builder_result(compile_result, &tx);
-                    })
-                    .expect("Configuration is correct for watching")
-            });
-            std::mem::forget(thread);
-        }
-        fn handle_builder_result(
             compile_result: CompileResult,
-            tx: &mpsc::SyncSender<wgpu::ShaderModuleDescriptor<'static>>,
+        ) -> wgpu::ShaderModuleDescriptor<'static> {
-        ) {
             let module_path = compile_result.module.unwrap_single();
             let data = std::fs::read(module_path).unwrap();
             let spirv = wgpu::util::make_spirv(&data);
@@ -122,25 +110,23 @@ fn maybe_watch(
                     wgpu::ShaderSource::SpirV(Cow::Owned(cow.into_owned()))
                 }
             };
-            tx.send(wgpu::ShaderModuleDescriptor {
+            wgpu::ShaderModuleDescriptor {
                 label: None,
                 source: spirv,
                 flags: wgpu::ShaderFlags::default(),
-            })
+            }
-            .expect("Rx is still alive");
         }
+        handle_compile_result(initial_result)
     }
     #[cfg(any(target_os = "android", target_arch = "wasm32"))]
     {
-        tx.send(match shader {
+        match shader {
             RustGPUShader::Simplest => wgpu::include_spirv!(env!("simplest_shader.spv")),
             RustGPUShader::Sky => wgpu::include_spirv!(env!("sky_shader.spv")),
             RustGPUShader::Compute => wgpu::include_spirv!(env!("compute_shader.spv")),
             RustGPUShader::Mouse => wgpu::include_spirv!(env!("mouse_shader.spv")),
-        })
+        }
-        .expect("rx to be alive")
     }
-    rx
 }
 
 fn is_compute_shader(shader: RustGPUShader) -> bool {