Change ubi so it only creates the install directory once it's downloa…

…ded an asset

Cargo.toml

@@ -17,6 +17,8 @@ binstall-tar = "0.4.42"
 bzip2 = "0.5.0"
 clap = { version = "4.5.23", features = ["wrap_help"] }
 document-features = "0.2"
+# Used in some test code which can't use test_log.
+env_logger = "0.11.6"
 fern = { version = "0.7.1", features = ["colored"] }
 flate2 = "1.0.35"
 itertools = "0.13.0"

Changes.md

@@ -2,6 +2,10 @@
 
 - The `UbiBuilder::install_dir` method now takes `AsRef<Path>` instead of `PathBuf`, which should
   make it more convenient to use.
+- Previously, `ubi` would create the install directory very early in its process, well before it had
+  something to install. This meant that if it failed to find an asset, couldn't download the asset,
+  or other errors happened, it would leave this directory behind. Now it creates this directory
+  immediately before writing the executable it found to disk.
 
 ## 0.3.0 - 2024-12-26
 

ubi/Cargo.toml

@@ -46,6 +46,7 @@ native-tls-vendored = ["reqwest/native-tls-vendored"]
 logging = ["dep:fern"]
 
 [dev-dependencies]
+env_logger.workspace = true
 fern.workspace = true
 mockito.workspace = true
 test-case.workspace = true

ubi/src/builder.rs

@@ -16,7 +16,6 @@ use reqwest::{
 };
 use std::{
     env,
-    fs::create_dir_all,
     path::{Path, PathBuf},
     str::FromStr,
 };
@@ -292,7 +291,6 @@ fn install_path(install_dir: Option<PathBuf>, exe: &str) -> Result<PathBuf> {
         i.push("bin");
         i
     };
-    create_dir_all(&path)?;
     path.push(exe);
     debug!("install path = {}", path.to_string_lossy());
     Ok(path)

ubi/src/installer.rs

@@ -5,7 +5,7 @@ use bzip2::read::BzDecoder;
 use flate2::read::GzDecoder;
 use log::{debug, info};
 use std::{
-    fs::File,
+    fs::{create_dir_all, File},
     io::{Read, Write},
     path::{Path, PathBuf},
 };
@@ -75,8 +75,10 @@ impl Installer {
             if path.ends_with(&self.exe) {
                 let mut buffer: Vec<u8> = Vec::with_capacity(usize::try_from(zf.size())?);
                 zf.read_to_end(&mut buffer)?;
-                let mut file = File::create(&self.install_path)?;
-                return file.write_all(&buffer).map_err(Into::into);
+                self.create_install_dir()?;
+                return File::create(&self.install_path)?
+                    .write_all(&buffer)
+                    .map_err(Into::into);
             }
         }
 
@@ -108,10 +110,9 @@ impl Installer {
                             "extracting tarball entry to {}",
                             self.install_path.to_string_lossy(),
                         );
-                        return match entry.unpack(&self.install_path) {
-                            Ok(_) => Ok(()),
-                            Err(e) => Err(anyhow::Error::new(e)),
-                        };
+                        self.create_install_dir()?;
+                        entry.unpack(&self.install_path).unwrap();
+                        return Ok(());
                     }
                 }
             }
@@ -143,6 +144,7 @@ impl Installer {
     }
 
     fn write_to_install_path(&self, mut reader: impl Read) -> Result<()> {
+        self.create_install_dir()?;
         let mut writer = File::create(&self.install_path)
             .with_context(|| format!("Cannot write to {}", self.install_path.to_string_lossy()))?;
         std::io::copy(&mut reader, &mut writer)?;
@@ -151,11 +153,25 @@ impl Installer {
 
     fn copy_executable(&self, exe_file: &Path) -> Result<()> {
         debug!("copying binary to final location");
+        self.create_install_dir()?;
         std::fs::copy(exe_file, &self.install_path)?;
 
         Ok(())
     }
 
+    fn create_install_dir(&self) -> Result<()> {
+        let Some(path) = self.install_path.parent() else {
+            return Err(anyhow!(
+                "install path at {} has no parent",
+                self.install_path.display()
+            ));
+        };
+
+        debug!("creating directory at {}", path.display());
+        create_dir_all(&path)
+            .with_context(|| format!("could not create a directory at {}", path.display()))
+    }
+
     fn make_binary_executable(&self) -> Result<()> {
         #[cfg(target_family = "windows")]
         return Ok(());
@@ -200,6 +216,7 @@ mod tests {
     use super::*;
     #[cfg(target_family = "unix")]
     use std::os::unix::fs::PermissionsExt;
+    use std::sync::Once;
     use tempfile::tempdir;
     use test_case::test_case;
 
@@ -216,23 +233,32 @@ mod tests {
     #[test_case("test-data/project.zip", "project")]
     #[test_case("test-data/project", "project")]
     fn install(archive_path: &str, exe: &str) -> Result<()> {
-        //crate::ubi::init_logger(log::LevelFilter::Debug)?;
+        static INIT_LOGGING: Once = Once::new();
+        INIT_LOGGING.call_once(|| {
+            use env_logger;
+            let _ = env_logger::builder().is_test(true).try_init();
+        });
 
         let td = tempdir()?;
-        let mut install_path = td.path().to_path_buf();
-        install_path.push("project");
-        let installer = Installer::new(install_path.clone(), exe.to_string());
-        installer.install(&Download {
-            // It doesn't matter what we use here. We're not actually going to
-            // put anything in this temp dir.
-            _temp_dir: tempdir()?,
-            archive_path: PathBuf::from(archive_path),
-        })?;
-
-        assert!(install_path.exists());
-        assert!(install_path.is_file());
-        #[cfg(target_family = "unix")]
-        assert!(install_path.metadata()?.permissions().mode() & 0o111 != 0);
+        let mut path_without_subdir = td.path().to_path_buf();
+        path_without_subdir.push("project");
+        let mut path_with_subdir = td.path().to_path_buf();
+        path_with_subdir.extend(&["subdir", "project"]);
+
+        for install_path in [path_without_subdir, path_with_subdir] {
+            let installer = Installer::new(install_path.clone(), exe.to_string());
+            installer.install(&Download {
+                // It doesn't matter what we use here. We're not actually going to
+                // put anything in this temp dir.
+                _temp_dir: tempdir()?,
+                archive_path: PathBuf::from(archive_path),
+            })?;
+
+            assert!(install_path.exists());
+            assert!(install_path.is_file());
+            #[cfg(target_family = "unix")]
+            assert!(install_path.metadata()?.permissions().mode() & 0o111 != 0);
+        }
 
         Ok(())
     }