fix: don't panic on missing shorebird.yaml (#18)

* fix: don't panic on missing shorebird.yaml

* fix next_boot_patch C api

* don't log error in updater

* Cleanup

* Remove anyhow::bail import

library/src/c_api.rs

@@ -7,13 +7,11 @@
 use std::ffi::{CStr, CString};
 use std::os::raw::c_char;
 
-use anyhow::Context;
-
 use crate::updater;
 
 // https://stackoverflow.com/questions/67087597/is-it-possible-to-use-rusts-log-info-for-tests
 #[cfg(test)]
-use std::println as error; // Workaround to use println! for logs.
+use std::{println as info, println as error}; // Workaround to use println! for logs.
 
 /// Struct containing configuration parameters for the updater.
 /// Passed to all updater functions.
@@ -49,6 +47,14 @@ fn allocate_c_string(rust_string: &str) -> anyhow::Result<*mut c_char> {
     Ok(c_str.into_raw())
 }
 
+/// Converts a Rust string to a C string, caller must free the C string.
+fn to_c_string(maybe_string: Option<String>) -> anyhow::Result<*mut c_char> {
+    Ok(match maybe_string {
+        Some(v) => allocate_c_string(&v)?,
+        None => std::ptr::null_mut(),
+    })
+}
+
 fn to_rust_vector(
     c_array: *const *const libc::c_char,
     size: libc::c_int,
@@ -83,14 +89,10 @@ fn log_on_error<F, R>(f: F, context: &str, error_result: R) -> R
 where
     F: FnOnce() -> Result<R, anyhow::Error>,
 {
-    let result = f();
-    match result {
-        Ok(r) => r,
-        Err(e) => {
-            error!("Error {}: {:?}", context, e);
-            error_result
-        }
-    }
+    f().unwrap_or_else(|e| {
+        error!("Error {}: {:?}", context, e);
+        error_result
+    })
 }
 
 /// Configures updater.  First parameter is a struct containing configuration
@@ -119,8 +121,8 @@ pub extern "C" fn shorebird_init(
 pub extern "C" fn shorebird_next_boot_patch_number() -> *mut c_char {
     log_on_error(
         || {
-            let patch = updater::next_boot_patch().context("failed to fetch patch info")?;
-            allocate_c_string(&patch.number.to_string())
+            let maybe_patch_number = updater::next_boot_patch()?.map(|p| p.number.to_string());
+            to_c_string(maybe_patch_number)
         },
         "fetching next_boot_patch_number",
         std::ptr::null_mut(),
@@ -133,8 +135,8 @@ pub extern "C" fn shorebird_next_boot_patch_number() -> *mut c_char {
 pub extern "C" fn shorebird_next_boot_patch_path() -> *mut c_char {
     log_on_error(
         || {
-            let patch = updater::next_boot_patch().context("failed to fetch patch info")?;
-            allocate_c_string(&patch.path)
+            let maybe_path = updater::next_boot_patch()?.map(|p| p.path);
+            to_c_string(maybe_path)
         },
         "fetching next_boot_patch_path",
         std::ptr::null_mut(),
@@ -155,13 +157,17 @@ pub extern "C" fn shorebird_free_string(c_string: *mut c_char) {
 /// Check for an update.  Returns true if an update is available.
 #[no_mangle]
 pub extern "C" fn shorebird_check_for_update() -> bool {
-    return updater::check_for_update();
+    log_on_error(updater::check_for_update, "checking for update", false)
 }
 
 /// Synchronously download an update if one is available.
 #[no_mangle]
 pub extern "C" fn shorebird_update() {
-    updater::update();
+    log_on_error(
+        || updater::update().and_then(|result| Ok(info!("Update result: {}", result))),
+        "downloading update",
+        (),
+    );
 }
 
 /// Start a thread to download an update if one is available.
@@ -177,11 +183,7 @@ pub extern "C" fn shorebird_start_update_thread() {
 /// shorebird_report_launch_success or shorebird_report_launch_failure.
 #[no_mangle]
 pub extern "C" fn shorebird_report_launch_start() {
-    log_on_error(
-        || updater::report_launch_start(),
-        "reporting launch start",
-        (),
-    );
+    log_on_error(updater::report_launch_start, "reporting launch start", ());
 }
 
 /// Report that the app failed to launch.  This will cause the updater to
@@ -190,10 +192,7 @@ pub extern "C" fn shorebird_report_launch_start() {
 #[no_mangle]
 pub extern "C" fn shorebird_report_launch_failure() {
     log_on_error(
-        || {
-            updater::report_launch_failure()?;
-            Ok(())
-        },
+        updater::report_launch_failure,
         "reporting launch failure",
         (),
     );
@@ -209,10 +208,7 @@ pub extern "C" fn shorebird_report_launch_failure() {
 #[no_mangle]
 pub extern "C" fn shorebird_report_launch_success() {
     log_on_error(
-        || {
-            updater::report_launch_success()?;
-            Ok(())
-        },
+        updater::report_launch_success,
         "reporting launch success",
         (),
     );

library/src/config.rs

@@ -1,5 +1,4 @@
 // This file handles the global config for the updater library.
-
 #[cfg(test)]
 use crate::network::{DownloadFileFn, PatchCheckRequestFn};
 #[cfg(test)]
@@ -7,6 +6,7 @@ use std::cell::RefCell;
 
 use crate::updater::AppConfig;
 use crate::yaml::YamlConfig;
+use crate::UpdateError;
 
 #[cfg(not(test))]
 use once_cell::sync::OnceCell;
@@ -60,20 +60,20 @@ pub fn testing_reset_config() {
     });
 }
 
-pub fn check_initialized_and_call<F, R>(f: F, config: &ResolvedConfig) -> R
+pub fn check_initialized_and_call<F, R>(f: F, config: &ResolvedConfig) -> anyhow::Result<R>
 where
-    F: FnOnce(&ResolvedConfig) -> R,
+    F: FnOnce(&ResolvedConfig) -> anyhow::Result<R>,
 {
     if !config.is_initialized {
-        panic!("Must call shorebird_init() before using the updater.");
+        anyhow::bail!(UpdateError::ConfigNotInitialized);
     }
     return f(&config);
 }
 
 #[cfg(test)]
-pub fn with_thread_config<F, R>(f: F) -> R
+pub fn with_thread_config<F, R>(f: F) -> anyhow::Result<R>
 where
-    F: FnOnce(&ThreadConfig) -> R,
+    F: FnOnce(&ThreadConfig) -> anyhow::Result<R>,
 {
     THREAD_CONFIG.with(|thread_config| {
         let thread_config = thread_config.borrow();
@@ -93,9 +93,9 @@ where
 }
 
 #[cfg(not(test))]
-pub fn with_config<F, R>(f: F) -> R
+pub fn with_config<F, R>(f: F) -> anyhow::Result<R>
 where
-    F: FnOnce(&ResolvedConfig) -> R,
+    F: FnOnce(&ResolvedConfig) -> anyhow::Result<R>,
 {
     // expect() here should be OK, it's job is to propagate a panic across
     // threads if the lock is poisoned.
@@ -106,9 +106,9 @@ where
 }
 
 #[cfg(test)]
-pub fn with_config<F, R>(f: F) -> R
+pub fn with_config<F, R>(f: F) -> anyhow::Result<R>
 where
-    F: FnOnce(&ResolvedConfig) -> R,
+    F: FnOnce(&ResolvedConfig) -> anyhow::Result<R>,
 {
     // Rust unit tests run on multiple threads in parallel, so we use
     // a per-thread config when unit testing instead of a global config.

library/src/lib.rs

@@ -5,7 +5,7 @@
 // C doesn't care about the namespaces, but Rust does.
 pub mod c_api;
 
-// Declare other .rs file/module exists, but make them public.
+// Declare other .rs file/module exists, but make them private.
 mod cache;
 mod config;
 mod logging;

library/src/updater.rs

@@ -1,15 +1,17 @@
 // This file's job is to be the Rust API for the updater.
 
 use std::fmt::{Display, Formatter};
+use std::fs;
+use std::io::{Cursor, Read, Seek};
+use std::path::{Path, PathBuf};
+
+use anyhow::Context;
 
 use crate::cache::{PatchInfo, UpdaterState};
 use crate::config::{set_config, with_config, ResolvedConfig};
 use crate::logging::init_logging;
 use crate::network::{download_to_path, send_patch_check_request};
 use crate::yaml::YamlConfig;
-use std::fs;
-use std::io::{Cursor, Read, Seek};
-use std::path::{Path, PathBuf};
 
 // https://stackoverflow.com/questions/67087597/is-it-possible-to-use-rusts-log-info-for-tests
 #[cfg(test)]
@@ -23,8 +25,6 @@ pub use crate::network::{
     testing_set_network_hooks, DownloadFileFn, Patch, PatchCheckRequest, PatchCheckRequestFn,
 };
 
-use anyhow::Context;
-
 pub enum UpdateStatus {
     NoUpdate,
     UpdateAvailable,
@@ -51,6 +51,7 @@ pub enum UpdateError {
     InvalidState(String),
     BadServerResponse,
     FailedToSaveState,
+    ConfigNotInitialized,
 }
 
 impl std::error::Error for UpdateError {}
@@ -64,6 +65,7 @@ impl Display for UpdateError {
             UpdateError::InvalidState(msg) => write!(f, "Invalid State: {}", msg),
             UpdateError::FailedToSaveState => write!(f, "Failed to save state"),
             UpdateError::BadServerResponse => write!(f, "Bad server response"),
+            UpdateError::ConfigNotInitialized => write!(f, "Config not initialized"),
         }
     }
 }
@@ -89,26 +91,14 @@ pub fn init(app_config: AppConfig, yaml: &str) -> Result<(), UpdateError> {
     set_config(app_config, config).map_err(|err| UpdateError::InvalidState(err.to_string()))
 }
 
-fn check_for_update_internal(config: &ResolvedConfig) -> bool {
-    // Load UpdaterState from disk
-    // If there is no state, make an empty state.
-    let state = UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
-    // Send info from app + current slot to server.
-    let response_result = send_patch_check_request(&config, &state);
-    match response_result {
-        Err(err) => {
-            error!("Failed update check: {err}");
-            return false;
-        }
-        Ok(response) => {
-            return response.patch_available;
-        }
-    }
-}
-
 /// Synchronously checks for an update and returns true if an update is available.
-pub fn check_for_update() -> bool {
-    return with_config(check_for_update_internal);
+pub fn check_for_update() -> anyhow::Result<bool> {
+    with_config(|config| {
+        // Load UpdaterState from disk
+        // If there is no state, make an empty state.
+        let state = UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+        send_patch_check_request(&config, &state).map(|res| res.patch_available)
+    })
 }
 
 fn check_hash(path: &Path, expected_string: &str) -> anyhow::Result<bool> {
@@ -396,21 +386,21 @@ where
 /// The patch which will be run on next boot (which may still be the same
 /// as the current boot).
 /// This may be changed any time update() or start_update_thread() are called.
-pub fn next_boot_patch() -> Option<PatchInfo> {
-    return with_config(|config| {
+pub fn next_boot_patch() -> anyhow::Result<Option<PatchInfo>> {
+    with_config(|config| {
         let state = UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
-        return state.next_boot_patch();
-    });
+        return Ok(state.next_boot_patch());
+    })
 }
 
 /// The patch which is currently booted.  This is None until
 /// report_launch_start() is called at which point it is copied from
 /// next_boot_patch.
-pub fn current_boot_patch() -> Option<PatchInfo> {
-    return with_config(|config| {
+pub fn current_boot_patch() -> anyhow::Result<Option<PatchInfo>> {
+    with_config(|config| {
         let state = UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
-        return state.current_boot_patch();
-    });
+        return Ok(state.current_boot_patch());
+    })
 }
 
 pub fn report_launch_start() -> anyhow::Result<()> {
@@ -426,46 +416,46 @@ pub fn report_launch_start() -> anyhow::Result<()> {
 
 /// Report that the current active path failed to launch.
 /// This will mark the patch as bad and activate the next best patch.
-pub fn report_launch_failure() -> Result<(), UpdateError> {
+pub fn report_launch_failure() -> anyhow::Result<()> {
     info!("Reporting failed launch.");
     with_config(|config| {
         let mut state =
             UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
 
-        let patch = state
-            .current_boot_patch()
-            .ok_or(UpdateError::InvalidState("No current patch".to_string()))?;
+        let patch =
+            state
+                .current_boot_patch()
+                .ok_or(anyhow::Error::from(UpdateError::InvalidState(
+                    "No current patch".to_string(),
+                )))?;
         state.mark_patch_as_bad(&patch);
-        state.activate_latest_bootable_patch()
+        state
+            .activate_latest_bootable_patch()
+            .map_err(|err| anyhow::Error::from(err))
     })
 }
 
-pub fn report_launch_success() -> Result<(), UpdateError> {
+pub fn report_launch_success() -> anyhow::Result<()> {
     with_config(|config| {
         let mut state =
             UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
 
-        let patch = state
-            .current_boot_patch()
-            .ok_or(UpdateError::InvalidState("No current patch".to_string()))?;
+        let patch =
+            state
+                .current_boot_patch()
+                .ok_or(anyhow::Error::from(UpdateError::InvalidState(
+                    "No current patch".to_string(),
+                )))?;
         state.mark_patch_as_good(&patch);
-        state.save().map_err(|_| UpdateError::FailedToSaveState)
+        state
+            .save()
+            .map_err(|_| anyhow::Error::from(UpdateError::FailedToSaveState))
     })
 }
 
 /// Synchronously checks for an update and downloads and installs it if available.
-pub fn update() -> UpdateStatus {
-    return with_config(|config| {
-        let result = update_internal(&config);
-        match result {
-            Err(err) => {
-                error!("Problem updating: {err}");
-                error!("{}", err.backtrace());
-                return UpdateStatus::UpdateHadError;
-            }
-            Ok(status) => status,
-        }
-    });
+pub fn update() -> anyhow::Result<UpdateStatus> {
+    with_config(|config| update_internal(&config))
 }
 
 /// This does not return status.  The only output is the change to the saved
@@ -476,7 +466,7 @@ pub fn start_update_thread() {
     // call which is wrong. We should be able to release the lock during the
     // network requests.
     std::thread::spawn(move || {
-        let status = update();
+        let status = update().unwrap_or(UpdateStatus::UpdateHadError);
         info!("Update thread finished with status: {}", status);
     });
 }
@@ -526,17 +516,19 @@ mod tests {
                 })
                 .expect("move failed");
             state.save().expect("save failed");
-        });
-        assert!(crate::next_boot_patch().is_some());
+            Ok(())
+        })
+        .unwrap();
+        assert!(crate::next_boot_patch().unwrap().is_some());
         // pretend we booted from it
         crate::report_launch_start().unwrap();
         crate::report_launch_success().unwrap();
-        assert!(crate::next_boot_patch().is_some());
+        assert!(crate::next_boot_patch().unwrap().is_some());
         // mark it bad.
         crate::report_launch_failure().unwrap();
         // Technically might need to "reload"
         // ask for current patch (should get none).
-        assert!(crate::next_boot_patch().is_none());
+        assert!(crate::next_boot_patch().unwrap().is_none());
     }
 
     #[test]
@@ -597,16 +589,18 @@ mod tests {
         let tmp_dir = TempDir::new("example").unwrap();
         init_for_testing(&tmp_dir);
         assert_eq!(
-            crate::report_launch_failure(),
-            Err(crate::UpdateError::InvalidState(
-                "No current patch".to_string()
-            ))
+            crate::report_launch_failure()
+                .unwrap_err()
+                .downcast::<crate::UpdateError>()
+                .unwrap(),
+            crate::UpdateError::InvalidState("No current patch".to_string())
         );
         assert_eq!(
-            crate::report_launch_success(),
-            Err(crate::UpdateError::InvalidState(
-                "No current patch".to_string()
-            ))
+            crate::report_launch_success()
+                .unwrap_err()
+                .downcast::<crate::UpdateError>()
+                .unwrap(),
+            crate::UpdateError::InvalidState("No current patch".to_string())
         );
     }
 }