From 1c03904a1d29ddb1c776a0c592f83cae164e2923 Mon Sep 17 00:00:00 2001 From: Dave Rolsky Date: Thu, 26 Dec 2024 11:59:59 -0600 Subject: [PATCH] Change ubi so it only creates the install directory once it's downloaded an asset --- Cargo.lock | 9 ++++++ Cargo.toml | 2 ++ Changes.md | 4 +++ ubi/Cargo.toml | 1 + ubi/src/builder.rs | 2 -- ubi/src/installer.rs | 70 ++++++++++++++++++++++++++++++-------------- 6 files changed, 64 insertions(+), 24 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index aad8c80..0f03c58 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -462,6 +462,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "186e05a59d4c50738528153b83b0b0194d3a29507dfec16eccd4b342903397d0" dependencies = [ "log", + "regex", ] [[package]] @@ -479,6 +480,7 @@ dependencies = [ "anstream", "anstyle", "env_filter", + "humantime", "log", ] @@ -758,6 +760,12 @@ version = "1.0.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "df3b46402a9d5adb4c86a0cf463f42e19994e3ee891101b1841f30a545cb49a9" +[[package]] +name = "humantime" +version = "2.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9a3a5bfb195931eeb336b2a7b4d761daec841b97f947d34394601737a7bba5e4" + [[package]] name = "hyper" version = "1.5.2" @@ -2235,6 +2243,7 @@ dependencies = [ "binstall-tar", "bzip2 0.5.0", "document-features", + "env_logger", "fern", "flate2", "itertools", diff --git a/Cargo.toml b/Cargo.toml index 3148192..52ecd31 100644 --- a/Cargo.toml +++ b/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" diff --git a/Changes.md b/Changes.md index 5e88d99..b386418 100644 --- a/Changes.md +++ b/Changes.md @@ -2,6 +2,10 @@ - The `UbiBuilder::install_dir` method now takes `AsRef` instead of `PathBuf`, which should make it more convenient to use, because you can now pass strings and `&Path`. +- 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 diff --git a/ubi/Cargo.toml b/ubi/Cargo.toml index 2c2763c..f2b09e9 100644 --- a/ubi/Cargo.toml +++ b/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 diff --git a/ubi/src/builder.rs b/ubi/src/builder.rs index 8b53d0d..55c6d63 100644 --- a/ubi/src/builder.rs +++ b/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, exe: &str) -> Result { i.push("bin"); i }; - create_dir_all(&path)?; path.push(exe); debug!("install path = {}", path.to_string_lossy()); Ok(path) diff --git a/ubi/src/installer.rs b/ubi/src/installer.rs index 0beb294..54993e0 100644 --- a/ubi/src/installer.rs +++ b/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 = 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(()) }