From 3bacb01a82a01b04cabdd4da37fbb1729dada467 Mon Sep 17 00:00:00 2001 From: Filip Tibell Date: Fri, 14 Jun 2024 13:16:39 +0200 Subject: [PATCH] Skip recreating links when not necessary (#27) --- CHANGELOG.md | 6 ++- Cargo.lock | 81 +++++++++++++++++++++++++++++++ Cargo.toml | 1 + lib/result.rs | 9 ++++ lib/storage/metadata.rs | 97 +++++++++++++++++++++++++++++++++++++ lib/storage/mod.rs | 1 + lib/storage/tool_storage.rs | 61 +++++++++++++---------- lib/util/fs.rs | 54 --------------------- 8 files changed, 230 insertions(+), 80 deletions(-) create mode 100644 lib/storage/metadata.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e8426b..9dedb82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Fixed `tarmac` not installing correctly on non-arm systems due to its name containing `arm` +- Fixed `tarmac` not installing correctly on non-arm systems due to its name containing `arm` ([#26]) +- Fixed OS permission errors during `rokit install` for tools that are currently running ([#27]) + +[#26]: https://github.com/filiptibell/rokit/pull/26 +[#27]: https://github.com/filiptibell/rokit/pull/27 ## `0.1.1` - June 9th, 2024 diff --git a/Cargo.lock b/Cargo.lock index 1090a78..fa1541d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -210,6 +210,15 @@ dependencies = [ "syn", ] +[[package]] +name = "atomic-polyfill" +version = "1.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8cf2bce30dfe09ef0bfaef228b9d414faaf7e563035494d7fe092dba54b300f4" +dependencies = [ + "critical-section", +] + [[package]] name = "atomic-waker" version = "1.1.2" @@ -410,6 +419,12 @@ version = "0.7.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4b82cf0babdbd58558212896d1a4272303a57bdb245c2bf1147185fb45640e70" +[[package]] +name = "cobs" +version = "0.2.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "67ba02a97a2bd10f4b59b25c7973101c79642302776489e030cd13cdab09ed15" + [[package]] name = "colorchoice" version = "1.0.1" @@ -483,6 +498,12 @@ dependencies = [ "cfg-if", ] +[[package]] +name = "critical-section" +version = "1.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7059fff8937831a9ae6f0fe4d658ffabf58f2ca96aa9dec1c889f936f705f216" + [[package]] name = "crossbeam-utils" version = "0.8.20" @@ -643,6 +664,12 @@ version = "1.12.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3dca9240753cf90908d7e4aac30f630662b02aebaa1b58a3cadabdb23385b58b" +[[package]] +name = "embedded-io" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ef1a6892d9eef45c8fa6b9e0086428a2cca8491aca8f787c534a3d6d0bcb3ced" + [[package]] name = "encode_unicode" version = "0.3.6" @@ -897,6 +924,15 @@ dependencies = [ "tracing", ] +[[package]] +name = "hash32" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b0c35f58762feb77d74ebe43bdbc3210f09be9fe6742234d573bacc26ed92b67" +dependencies = [ + "byteorder", +] + [[package]] name = "hashbrown" version = "0.12.3" @@ -909,6 +945,20 @@ version = "0.14.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e5274423e17b7c9fc20b6e7e208532f9b19825d82dfd615708b70edd83df41f1" +[[package]] +name = "heapless" +version = "0.7.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cdc6457c0eb62c71aac4bc17216026d8410337c4126773b9c5daba343f17964f" +dependencies = [ + "atomic-polyfill", + "hash32", + "rustc_version", + "serde", + "spin", + "stable_deref_trait", +] + [[package]] name = "heck" version = "0.5.0" @@ -1495,6 +1545,18 @@ version = "1.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7170ef9988bc169ba16dd36a7fa041e5c4cbeb6a35b76d4c03daded371eae7c0" +[[package]] +name = "postcard" +version = "1.0.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a55c51ee6c0db07e68448e336cf8ea4131a620edefebf9893e759b2d793420f8" +dependencies = [ + "cobs", + "embedded-io", + "heapless", + "serde", +] + [[package]] name = "powerfmt" version = "0.2.0" @@ -1793,6 +1855,7 @@ dependencies = [ "goblin", "indicatif", "once_cell", + "postcard", "process-wrap", "reqwest", "reqwest-middleware", @@ -1822,6 +1885,15 @@ version = "0.1.24" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "719b953e2095829ee67db738b3bfa9fa368c94900df327b3f07fe6e794d2fe1f" +[[package]] +name = "rustc_version" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bfa0f585226d2e68097d4f95d113b15b83a82e819ab25717ec0590d9584ef366" +dependencies = [ + "semver", +] + [[package]] name = "rustix" version = "0.38.34" @@ -2070,6 +2142,15 @@ name = "spin" version = "0.9.8" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6980e8d7511241f8acf4aebddbb1ff938df5eebe98691418c4468d0b72a96a67" +dependencies = [ + "lock_api", +] + +[[package]] +name = "stable_deref_trait" +version = "1.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a8f112729512f8e442d81f95a8a7ddf2b7c6b8a1a6f509a95864142b30cab2d3" [[package]] name = "strsim" diff --git a/Cargo.toml b/Cargo.toml index 9ee5eac..4e7892d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -41,6 +41,7 @@ filepath = "0.1" flate2 = "1.0" goblin = "0.8" once_cell = "1.8" +postcard = { version = "1.0", features = ["alloc"] } semver = { version = "1.0", features = ["serde"] } tar = "0.4" tempfile = "3.3" diff --git a/lib/result.rs b/lib/result.rs index 2da2dd4..a92b5cf 100644 --- a/lib/result.rs +++ b/lib/result.rs @@ -1,6 +1,7 @@ use std::io::Error as IoError; use std::path::PathBuf; +use postcard::Error as PostcardError; use serde_json::Error as JsonError; use thiserror::Error; use tokio::task::JoinError; @@ -27,6 +28,8 @@ pub enum RokitError { Io(Box), #[error("JSON error: {0}")] Json(Box), + #[error("Postcard error: {0}")] + Postcard(Box), #[error("Zip file error: {0}")] Zip(Box), #[error("GitHub error: {0}")] @@ -67,6 +70,12 @@ impl From for RokitError { } } +impl From for RokitError { + fn from(err: PostcardError) -> Self { + RokitError::Postcard(err.into()) + } +} + impl From for RokitError { fn from(err: ZipError) -> Self { RokitError::Zip(err.into()) diff --git a/lib/storage/metadata.rs b/lib/storage/metadata.rs new file mode 100644 index 0000000..56f4291 --- /dev/null +++ b/lib/storage/metadata.rs @@ -0,0 +1,97 @@ +use serde::{Deserialize, Serialize}; + +use crate::result::RokitResult; + +const ROKIT_META_TRAILER: [u8; 10] = *b"ROKIT_LINK"; +const ROKIT_META_VERSION: u16 = 1; + +const CARGO_VERSION: &str = env!("CARGO_PKG_VERSION"); + +// FUTURE: We could probably accept impl Read + Seek / impl Write instead +// of [u8] for the metadata functions here to make things faster. For now +// it is fine because the Rokit links are pretty small (just a few MB). + +/** + Metadata for a Rokit link - typically used for storing version + information and skipping unnecessary work (recreating the link). + + Metadata, as bytes, is stored in the following + format to make it easy to read and write: + + - Contents + - Contents length (4 bytes) + - Metadata version (2 bytes) + - Metadata trailer (10 bytes) + + This makes for a full 16 bytes for our little + metadata block + whatever contents it stores. +*/ +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub(crate) struct RokitLinkMetadata { + pub(crate) version: String, +} + +impl RokitLinkMetadata { + /** + Creates new metadata with the current version of Rokit. + */ + pub(crate) fn current() -> Self { + Self { + version: CARGO_VERSION.to_string(), + } + } + + /** + Checks if the metadata is for the current version of Rokit. + */ + pub(crate) fn is_current(&self) -> bool { + self.version == CARGO_VERSION + } + + /** + Parses metadata from the end of a file. + */ + pub(crate) fn parse_from(contents: impl AsRef<[u8]>) -> Option { + let contents = contents.as_ref(); + let len = contents.len(); + + if contents.ends_with(&ROKIT_META_TRAILER) && len >= 16 { + let meta_version = u16::from_le_bytes([contents[len - 12], contents[len - 11]]); + let meta_len = u32::from_le_bytes([ + contents[len - 16], + contents[len - 15], + contents[len - 14], + contents[len - 13], + ]) as usize; + if len < (16 + meta_len) { + return None; + } + // FUTURE: Handle multiple metadata versions? + if meta_version == ROKIT_META_VERSION { + return postcard::from_bytes(&contents[(len - 16 - meta_len)..(len - 16)]).ok(); + } + } + + None + } + + /** + Appends metadata to the end of a file. + */ + pub(crate) fn append_to(&self, contents: impl Into>) -> RokitResult> { + let mut contents = contents.into(); + + let metadata_bytes = postcard::to_allocvec(self)?; + let metadata_len = + u32::try_from(metadata_bytes.len()).expect("rokit does not support metadata > 4GB"); + let metadata_version = ROKIT_META_VERSION.to_le_bytes(); + let metadata_len = metadata_len.to_le_bytes(); + + contents.extend_from_slice(&metadata_bytes); + contents.extend_from_slice(&metadata_len); + contents.extend_from_slice(&metadata_version); + contents.extend_from_slice(&ROKIT_META_TRAILER); + + Ok(contents) + } +} diff --git a/lib/storage/mod.rs b/lib/storage/mod.rs index 00656ee..f29746d 100644 --- a/lib/storage/mod.rs +++ b/lib/storage/mod.rs @@ -1,4 +1,5 @@ mod home; +mod metadata; mod tool_cache; mod tool_storage; diff --git a/lib/storage/tool_storage.rs b/lib/storage/tool_storage.rs index 3913067..2f4fe78 100644 --- a/lib/storage/tool_storage.rs +++ b/lib/storage/tool_storage.rs @@ -1,8 +1,5 @@ use std::{ - env::{ - consts::{EXE_EXTENSION, EXE_SUFFIX}, - var, - }, + env::consts::{EXE_EXTENSION, EXE_SUFFIX}, path::{Path, PathBuf}, sync::Arc, }; @@ -18,9 +15,10 @@ use tracing::{debug, trace}; use crate::{ manifests::{AuthManifest, RokitManifest}, result::RokitResult, + storage::metadata::RokitLinkMetadata, system::current_exe_contents, tool::{ToolAlias, ToolSpec}, - util::fs::{path_exists, write_executable_file, write_executable_link}, + util::fs::{path_exists, write_executable_file}, }; /** @@ -34,7 +32,6 @@ pub struct ToolStorage { pub(super) tools_dir: Arc, pub(super) aliases_dir: Arc, current_rokit_contents: Arc>>>, - no_symlinks: bool, } impl ToolStorage { @@ -133,13 +130,9 @@ impl ToolStorage { } // Create the new link - if cfg!(unix) && !self.no_symlinks { - let rokit_path = self.rokit_path(); - write_executable_link(path, &rokit_path).await?; - } else { - let contents = self.rokit_contents().await?; - write_executable_file(path, &contents).await?; - } + let rokit_contents = self.rokit_contents().await?; + let rokit_metadata = RokitLinkMetadata::current(); + skip_or_write_link_with_meta(path, &rokit_contents, &rokit_metadata).await?; Ok(()) } @@ -226,17 +219,13 @@ impl ToolStorage { true }; - // Then we can write the rest of the links - on unix we can use - // symlinks pointing to the Rokit binary to save on disk space. + // If any link already has the correct Rokit contents, we + // can skip creating it, to avoid OS permission errors if the + // link is currently being used to run some Rokit-managed program. + let rokit_metadata = RokitLinkMetadata::current(); link_paths .into_iter() - .map(|link_path| async { - if cfg!(unix) && !self.no_symlinks { - write_executable_link(link_path, &rokit_path).await - } else { - write_executable_file(link_path, &rokit_contents).await - } - }) + .map(|path| skip_or_write_link_with_meta(path, &rokit_contents, &rokit_metadata)) .collect::>() .try_collect::>() .await?; @@ -258,14 +247,11 @@ impl ToolStorage { )?; let current_rokit_contents = Arc::new(AsyncMutex::new(None)); - let no_symlinks = var("ROKIT_NO_SYMLINKS") - .is_ok_and(|val| matches!(val.to_ascii_lowercase().as_str(), "1" | "true")); Ok(Self { tools_dir, aliases_dir, current_rokit_contents, - no_symlinks, }) } @@ -306,3 +292,28 @@ fn append_exe_extension(path: impl Into) -> PathBuf { } path } + +// Utility functions for checking and writing metadata at the _end_ of link executables + +async fn skip_or_write_link_with_meta( + path: impl AsRef, + rokit_contents: &[u8], + rokit_metadata: &RokitLinkMetadata, +) -> RokitResult<()> { + let link_path = path.as_ref(); + + let existing_contents = read(&link_path).await.unwrap_or_default(); + let existing_metadata = RokitLinkMetadata::parse_from(&existing_contents); + if let Some(meta) = existing_metadata { + if meta.is_current() { + trace!(?link_path, ?meta, "link is up-to-date"); + return Ok(()); + } + trace!(?link_path, ?meta, "link is outdated"); + } + + let link_contents = rokit_metadata.append_to(rokit_contents)?; + write_executable_file(link_path, link_contents).await?; + + Ok(()) +} diff --git a/lib/util/fs.rs b/lib/util/fs.rs index b26530a..2e62fd8 100644 --- a/lib/util/fs.rs +++ b/lib/util/fs.rs @@ -83,60 +83,6 @@ pub async fn write_executable_file( Ok(()) } -/** - Writes a symlink at the given link path to the given - target path, and sets the symlink to be executable. - - # Panics - - This function will panic if called on a non-unix system. -*/ -#[cfg(unix)] -pub async fn write_executable_link( - link_path: impl AsRef, - target_path: impl AsRef, -) -> RokitResult<()> { - use tokio::fs::{remove_file, symlink}; - - let link_path = link_path.as_ref(); - let target_path = target_path.as_ref(); - - // NOTE: If a symlink already exists, we may need to remove it - // for the new symlink to be created successfully - the only error we - // should be able to get here is if the file doesn't exist, which is fine. - remove_file(link_path).await.ok(); - - if let Err(e) = symlink(target_path, link_path).await { - error!("Failed to create symlink at {link_path:?}:\n{e}"); - return Err(e.into()); - } - - // NOTE: We set the permissions of the symlink itself only on macOS - // since that is the only supported OS where symlink permissions matter - #[cfg(target_os = "macos")] - { - add_executable_permissions(link_path).await?; - } - - Ok(()) -} - -/** - Writes a symlink at the given link path to the given - target path, and sets the symlink to be executable. - - # Panics - - This function will panic if called on a non-unix system. -*/ -#[cfg(not(unix))] -pub async fn write_executable_link( - _link_path: impl AsRef, - _target_path: impl AsRef, -) -> RokitResult<()> { - panic!("write_executable_link should only be called on unix systems"); -} - #[cfg(unix)] async fn add_executable_permissions(path: impl AsRef) -> RokitResult<()> { use std::fs::Permissions;