fix: Bubble up install/uninstall failures. (#210)

CHANGELOG.md

@@ -10,6 +10,16 @@
 - [Rust](https://github.com/moonrepo/rust-plugin/blob/master/CHANGELOG.md)
 - [Schema](https://github.com/moonrepo/schema-plugin/blob/master/CHANGELOG.md)
 
+## Unreleased
+
+#### 🐞 Fixes
+
+- Fixed an issue where failed installs/uninstalls would exit with a zero exit code.
+
+#### ⚙️ Internal
+
+- Fixed an issue where install/uninstall events weren't always firing.
+
 ## 0.18.1
 
 #### 🐞 Fixes

Cargo.toml

@@ -5,7 +5,7 @@ default-members = ["crates/cli"]
 
 [workspace.dependencies]
 cached = "0.45.1"
-clap = "4.4.3"
+clap = "4.4.4"
 clap_complete = "4.4.1"
 convert_case = "0.6.0"
 extism = "0.5.0"

crates/cli/src/commands/install.rs

@@ -101,15 +101,18 @@ pub async fn internal_install(args: InstallArgs) -> SystemResult {
         tool.get_resolved_version()
     ));
 
-    tool.setup(&version).await?;
-    tool.cleanup().await?;
+    let installed = tool.setup(&version).await?;
+
+    pb.finish_and_clear();
+
+    if !installed {
+        return Ok(());
+    }
 
     if args.pin {
         pin_global(&mut tool)?;
     }
 
-    pb.finish_and_clear();
-
     info!(
         "{} has been installed to {}!",
         tool.get_name(),

crates/cli/src/commands/uninstall.rs

@@ -43,10 +43,14 @@ pub async fn uninstall(args: ArgsRef<UninstallArgs>) {
         tool.get_resolved_version()
     ));
 
-    tool.teardown().await?;
+    let uninstalled = tool.teardown().await?;
 
     pb.finish_and_clear();
 
+    if !uninstalled {
+        return Ok(());
+    }
+
     info!(
         "{} {} has been uninstalled!",
         tool.get_name(),

crates/core/src/error.rs

@@ -13,6 +13,10 @@ pub enum ProtoError {
     #[error("Tool inventory directory has been overridden but is not an absolute path. Only absolute paths are supported.")]
     AbsoluteInventoryDir,
 
+    #[diagnostic(code(proto::tool::install_failed))]
+    #[error("Failed to install {tool}. {error}")]
+    InstallFailed { tool: String, error: String },
+
     #[diagnostic(code(proto::misc::offline))]
     #[error("Internet connection required, unable to download and install tools.")]
     InternetConnectionRequired,
@@ -46,6 +50,10 @@ pub enum ProtoError {
         command: String,
     },
 
+    #[diagnostic(code(proto::tool::uninstall_failed))]
+    #[error("Failed to uninstall {tool}. {error}")]
+    UninstallFailed { tool: String, error: String },
+
     #[diagnostic(code(proto::tool::unknown))]
     #[error(
         "{} is not a built-in tool or has not been configured as a plugin, unable to proceed.", .id.style(Style::Id)

crates/core/src/tool.rs

@@ -775,6 +775,7 @@ impl Tool {
 
         let install_dir = self.get_tool_dir();
         let _install_lock = fs::lock_directory(&install_dir).await?;
+        let mut installed = false;
 
         self.on_installing
             .emit(InstallingEvent {
@@ -794,13 +795,23 @@ impl Tool {
                 },
             )?;
 
-            if !result.skip_install {
-                return Ok(result.installed);
+            if !result.installed && !result.skip_install {
+                return Err(ProtoError::InstallFailed {
+                    tool: self.get_name().to_owned(),
+                    error: result.error.unwrap_or_default(),
+                }
+                .into());
+
+            // If native install fails, attempt other installers
+            } else {
+                installed = result.installed;
             }
         }
 
         // Install from a prebuilt archive
-        self.install_from_prebuilt(&install_dir).await?;
+        if !installed {
+            self.install_from_prebuilt(&install_dir).await?;
+        }
 
         self.on_installed
             .emit(InstalledEvent {
@@ -835,11 +846,11 @@ impl Tool {
         )?;
 
         if !result.installed {
-            return Err(ProtoError::Message(
-                result
-                    .error
-                    .unwrap_or_else(|| "Unknown install failure!".to_string()),
-            ))?;
+            return Err(ProtoError::InstallFailed {
+                tool: dependency.to_owned(),
+                error: result.error.unwrap_or_default(),
+            }
+            .into());
         }
 
         self.on_installed_global
@@ -880,8 +891,12 @@ impl Tool {
                 },
             )?;
 
-            if !result.skip_uninstall && !result.uninstalled {
-                return Ok(false);
+            if !result.uninstalled && !result.skip_uninstall {
+                return Err(ProtoError::UninstallFailed {
+                    tool: self.get_name().to_owned(),
+                    error: result.error.unwrap_or_default(),
+                }
+                .into());
             }
         }
 
@@ -922,11 +937,11 @@ impl Tool {
         )?;
 
         if !result.uninstalled {
-            return Err(ProtoError::Message(
-                result
-                    .error
-                    .unwrap_or_else(|| "Unknown uninstall failure!".to_string()),
-            ))?;
+            return Err(ProtoError::UninstallFailed {
+                tool: dependency.to_owned(),
+                error: result.error.unwrap_or_default(),
+            }
+            .into());
         }
 
         self.on_uninstalled_global
@@ -1148,6 +1163,7 @@ impl Tool {
 
         self.version
             .as_ref()
+            // Canary can be overwritten so treat as not-installed
             .is_some_and(|v| !v.is_latest() && !v.is_canary())
             && dir.exists()
             && !fs::is_dir_locked(dir)
@@ -1194,6 +1210,7 @@ impl Tool {
         self.resolve_version(initial_version).await?;
 
         if self.install().await? {
+            self.cleanup().await?;
             self.locate_bins().await?;
             self.setup_shims(true).await?;
 

crates/pdk-api/src/api.rs

@@ -149,6 +149,10 @@ json_struct!(
 json_struct!(
     /// Output returned by the `native_install` function.
     pub struct NativeInstallOutput {
+        /// Error message if the install failed.
+        #[serde(skip_serializing_if = "Option::is_none")]
+        pub error: Option<String>,
+
         /// Whether the install was successful.
         pub installed: bool,
 
@@ -168,6 +172,10 @@ json_struct!(
 json_struct!(
     /// Output returned by the `native_uninstall` function.
     pub struct NativeUninstallOutput {
+        /// Error message if the uninstall failed.
+        #[serde(skip_serializing_if = "Option::is_none")]
+        pub error: Option<String>,
+
         /// Whether the install was successful.
         pub uninstalled: bool,
 
@@ -299,12 +307,12 @@ json_struct!(
 json_struct!(
     /// Output returned by the `install_global` function.
     pub struct InstallGlobalOutput {
-        /// Whether the install was successful.
-        pub installed: bool,
-
         /// Error message if the install failed.
         #[serde(skip_serializing_if = "Option::is_none")]
         pub error: Option<String>,
+
+        /// Whether the install was successful.
+        pub installed: bool,
     }
 );