Merge branch 'kevinglasson/hotfix-anim-hotreloading-compile-error' into 'master'

Refactor and document hotreloading feature

See merge request veloren/veloren!1102

Cargo.lock

@@ -4590,6 +4590,7 @@ dependencies = [
 name = "veloren-voxygen-anim"
 version = "0.6.0"
 dependencies = [
+ "find_folder",
  "lazy_static",
  "libloading 0.6.2",
  "notify",

voxygen/src/anim/Cargo.toml

@@ -8,10 +8,10 @@ edition = "2018"
 name = "voxygen_anim"
 # Uncomment to use animation hot reloading
 # Note: this breaks `cargo test`
-#crate-type = ["lib", "cdylib"]
+# crate-type = ["lib", "cdylib"]
 
 [features]
-use-dyn-lib = ["libloading", "notify", "lazy_static", "tracing"]
+use-dyn-lib = ["libloading", "notify", "lazy_static", "tracing", "find_folder"]
 be-dyn-lib = []
 
 default = ["be-dyn-lib"]
@@ -23,3 +23,4 @@ libloading = { version = "0.6.2", optional = true }
 notify = { version = "5.0.0-pre.2", optional = true }
 lazy_static = { version = "1.4.0", optional = true }
 tracing = { version = "0.1", optional = true }
+find_folder = { version = "0.3.0", optional = true}

voxygen/src/anim/src/dyn_lib.rs

@@ -7,46 +7,133 @@ use std::{
     thread,
     time::Duration,
 };
-use tracing::{error, warn};
 
+use find_folder::Search;
+use std::{env, path::PathBuf};
+use tracing::{debug, error, info};
+
+#[cfg(target_os = "windows")]
+const COMPILED_FILE: &str = "voxygen_anim.dll";
+#[cfg(target_os = "windows")]
+const ACTIVE_FILE: &str = "voxygen_anim_active.dll";
+
+#[cfg(not(target_os = "windows"))]
+const COMPILED_FILE: &str = "libvoxygen_anim.so";
+#[cfg(not(target_os = "windows"))]
+const ACTIVE_FILE: &str = "libvoxygen_anim_active.so";
+
+// This option is required as `hotreload()` moves the `LoadedLib`.
 lazy_static! {
     pub static ref LIB: Mutex<Option<LoadedLib>> = Mutex::new(Some(LoadedLib::compile_load()));
 }
 
+/// LoadedLib holds a loaded dynamic library and the location of library file
+/// with the appropriate OS specific name and extension i.e.
+/// `libvoxygen_anim_active.dylib`, `voxygen_anim_active.dll`.
+///
+/// # NOTE
+/// DOES NOT WORK ON MACOS, due to some limitations with hot-reloading the
+/// `.dylib`.
 pub struct LoadedLib {
+    /// Loaded library.
     pub lib: Library,
+    /// Path to the library.
+    pub lib_path: PathBuf,
 }
 
 impl LoadedLib {
+    /// Compile and load the dynamic library
+    ///
+    /// This is necessary because the very first time you use hot reloading you
+    /// wont have the library, so you can't load it until you have compiled it!
     fn compile_load() -> Self {
-        // Compile
+        #[cfg(target_os = "macos")]
-        compile();
+        error!("The hot reloading feature does not work on macos.");
 
-        #[cfg(target_os = "windows")]
+        // Compile
-        copy();
+        if !compile() {
+            panic!("Animation compile failed.");
+        } else {
+            info!("Animation compile succeeded.");
+        }
+
+        copy(&LoadedLib::determine_path());
 
         Self::load()
     }
 
+    /// Load a library from disk.
+    ///
+    /// Currently this is pretty fragile, it gets the path of where it thinks
+    /// the dynamic library should be and tries to load it. It will panic if it
+    /// is missing.
     fn load() -> Self {
-        #[cfg(target_os = "windows")]
+        let lib_path = LoadedLib::determine_path();
-        let lib = Library::new("../target/debug/voxygen_anim_active.dll").expect(
-            "To use hot animation reloading you need to uncomment a line in \
-             `voxygen/src/anim/Cargo.toml`.",
-        );
-        #[cfg(not(target_os = "windows"))]
-        let lib = Library::new("../target/debug/libvoxygen_anim.so").expect(
-            "To use hot animation reloading you need to uncomment a line in \
-             `voxygen/src/anim/Cargo.toml`.",
-        );
 
-        Self { lib }
+        // Try to load the library.
+        let lib = match Library::new(lib_path.clone()) {
+            Ok(lib) => lib,
+            Err(e) => panic!(
+                "Tried to load dynamic library from {:?}, but it could not be found. The first \
+                 reason might be that you need to uncomment a line in \
+                 `voxygen/src/anim/Cargo.toml` to build the library required for hot reloading. \
+                 The second is we may require a special case for your OS so we can find it. {:?}",
+                lib_path, e
+            ),
+        };
+
+        Self { lib, lib_path }
+    }
+
+    /// Determine the path to the dynamic library based on the path of the
+    /// current executable.
+    fn determine_path() -> PathBuf {
+        let current_exe = env::current_exe();
+
+        // If we got the current_exe, we need to go up a level and then down
+        // in to debug (in case we were in release or another build dir).
+        let mut lib_path = match current_exe {
+            Ok(mut path) => {
+                // Remove the filename to get the directory.
+                path.pop();
+
+                // Search for the debug directory.
+                let dir = Search::ParentsThenKids(1, 1)
+                    .of(path)
+                    .for_folder("debug")
+                    .expect(
+                        "Could not find the debug build directory relative to the current \
+                         executable.",
+                    );
+
+                debug!(?dir, "Found the debug build directory.");
+                dir
+            },
+            Err(e) => {
+                panic!(
+                    "Could not determine the path of the current executable, this is needed to \
+                     hotreload the dynamic library. {:?}",
+                    e
+                );
+            },
+        };
+
+        // Determine the platform specific path and push it onto our already
+        // established target/debug dir.
+        lib_path.push(ACTIVE_FILE);
+
+        lib_path
     }
 }
 
-// Starts up watcher
+/// Initialise a watcher.
+///
+/// The assumption is that this is run from the voxygen crate's root directory
+/// as it will watch the relative path `src/anim` for any changes to `.rs`
+/// files. Upon noticing changes it will wait a moment and then recompile.
 pub fn init() {
-    // Make sure first compile is done
+    // Make sure first compile is done by accessing the lazy_static and then
+    // immediately dropping (because we don't actually need it).
     drop(LIB.lock());
 
     // TODO: use crossbeam
@@ -69,13 +156,12 @@ pub fn init() {
                 modified_paths.insert(path);
             }
 
-            warn!(
+            info!(
                 ?modified_paths,
-                "Hot reloading animations because these files were modified"
+                "Hot reloading animations because files in `src/anim` modified."
             );
 
-            // Reload
+            hotreload();
-            reload();
         }
     });
 
@@ -83,8 +169,10 @@ pub fn init() {
     std::mem::forget(watcher);
 }
 
-// Recompiles and hotreloads the lib if the source has been changed
+/// Event function to hotreload the dynamic library
-// Note: designed with voxygen dir as working dir, could be made more flexible
+///
+/// This is called by the watcher to filter for modify events on `.rs` files
+/// before sending them back.
 fn event_fn(res: notify::Result<notify::Event>, sender: &mpsc::Sender<String>) {
     match res {
         Ok(event) => match event.kind {
@@ -99,33 +187,34 @@ fn event_fn(res: notify::Result<notify::Event>, sender: &mpsc::Sender<String>) {
             },
             _ => {},
         },
-        Err(e) => error!("Animation hotreload watcher error: {:?}", e),
+        Err(e) => error!(?e, "Animation hotreload watcher error."),
     }
 }
 
-fn reload() {
+/// Hotreload the dynamic library
-    // Stop if recompile failed
+///
-    if !compile() {
+/// This will reload the dynamic library by first internally calling compile
-        return;
+/// and then reloading the library.
+fn hotreload() {
+    // Do nothing if recompile failed.
+    if compile() {
+        let mut lock = LIB.lock().unwrap();
+
+        // Close lib.
+        let loaded_lib = lock.take().unwrap();
+        loaded_lib.lib.close().unwrap();
+        copy(&loaded_lib.lib_path);
+
+        // Open new lib.
+        *lock = Some(LoadedLib::load());
+
+        info!("Updated animations.");
     }
-
-    let mut lock = LIB.lock().unwrap();
-
-    // Close lib
-    lock.take().unwrap().lib.close().unwrap();
-
-    // Rename lib file on windows
-    // Called after closing lib so file will be unlocked
-    #[cfg(target_os = "windows")]
-    copy();
-
-    // Open new lib
-    *lock = Some(LoadedLib::load());
-
-    warn!("Updated animations");
 }
 
-// Returns false if compile failed
+/// Recompile the anim package
+///
+/// Returns `false` if the compile failed.
 fn compile() -> bool {
     let output = Command::new("cargo")
         .stderr(Stdio::inherit())
@@ -136,23 +225,22 @@ fn compile() -> bool {
         .output()
         .unwrap();
 
-    // If compile failed
+    output.status.success()
-    if !output.status.success() {
-        error!("Failed to compile anim crate");
-        false
-    } else {
-        warn!("Animation recompile success!!");
-        true
-    }
 }
 
-// Copy lib file if on windows since loading the lib locks the file blocking
+/// Copy the lib file, so we have an `_active` copy.
-// future compilation
+///
-#[cfg(target_os = "windows")]
+/// We do this for all OS's although it is only strictly necessary for windows.
-fn copy() {
+/// The reason we do this is to make the code easier to understand and debug.
-    std::fs::copy(
+fn copy(lib_path: &PathBuf) {
-        "../target/debug/voxygen_anim.dll",
+    // Use the platform specific names.
-        "../target/debug/voxygen_anim_active.dll",
+    let lib_compiled_path = lib_path.with_file_name(COMPILED_FILE);
-    )
+    let lib_output_path = lib_path.with_file_name(ACTIVE_FILE);
-    .expect("Failed to rename animations dll");
+
+    // Get the path to where the lib was compiled to.
+    debug!(?lib_compiled_path, ?lib_output_path, "Moving.");
+
+    // Copy the library file from where it is output, to where we are going to
+    // load it from i.e. lib_path.
+    std::fs::copy(lib_compiled_path, lib_output_path).expect("Failed to rename dynamic library.");
 }