From 527d807d9c32cf0409fd990876e95261a0fafed1 Mon Sep 17 00:00:00 2001 From: Schneems Date: Wed, 16 Oct 2024 13:10:36 -0500 Subject: [PATCH] Fix mtime not persisting in the cache I previously introduced a commit that had a test intended to prove that we were persisting the mtime of files copied into the cache to close out this comment https://github.com/heroku/buildpacks-ruby/issues/302#issuecomment-2415128173. The test passed locally, but failed on CI. It appears the behavior of fs-extra with regards to mtime for copying/moving directories is system dependent. To compensate, I switched to `cp_r` which explicitly is designed to preserve mtime https://crates.io/crates/cp_r. It has ~27,000 lifetime downloads and has activity as recently as a 9 days ago. https://docs.rs/cp_r/latest/cp_r/#release-history Copying and preserving mtime is the primary reason this library exists. From the docs.rs: > Copy a directory tree, including mtimes and permissions. It does not have the ability to skip-overwriting existing files which I need, however it has a generic `filter` closure which allows me to assemble this behavior myself. --- Cargo.lock | 12 +++- buildpacks/ruby/Cargo.toml | 1 - buildpacks/ruby/src/main.rs | 3 - commons/Cargo.toml | 2 +- commons/src/cache/app_cache.rs | 127 ++++++++++++++++++++------------- commons/src/cache/error.rs | 6 +- 6 files changed, 93 insertions(+), 58 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 77ae6b5..283d5a8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -281,10 +281,10 @@ dependencies = [ "ascii_table", "byte-unit", "const_format", + "cp_r", "fancy-regex", "filetime", "fs-err", - "fs_extra", "fun_run", "glob", "indoc", @@ -322,6 +322,15 @@ dependencies = [ "unicode-xid", ] +[[package]] +name = "cp_r" +version = "0.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "837ca07dfd27a2663ac7c4701bb35856b534c2a61dd47af06ccf65d3bec79ebc" +dependencies = [ + "filetime", +] + [[package]] name = "cpufeatures" version = "0.2.12" @@ -574,7 +583,6 @@ dependencies = [ "bullet_stream", "clap", "commons", - "filetime", "flate2", "fs-err", "fun_run", diff --git a/buildpacks/ruby/Cargo.toml b/buildpacks/ruby/Cargo.toml index 8d079af..1b66806 100644 --- a/buildpacks/ruby/Cargo.toml +++ b/buildpacks/ruby/Cargo.toml @@ -33,4 +33,3 @@ toml = "0.8" [dev-dependencies] libcnb-test = "=0.23.0" -filetime = "0.2.25" diff --git a/buildpacks/ruby/src/main.rs b/buildpacks/ruby/src/main.rs index 7d0f81f..62e42c2 100644 --- a/buildpacks/ruby/src/main.rs +++ b/buildpacks/ruby/src/main.rs @@ -29,9 +29,6 @@ mod user_errors; #[cfg(test)] use libcnb_test as _; -#[cfg(test)] -use filetime as _; - use clap as _; struct RubyBuildpack; diff --git a/commons/Cargo.toml b/commons/Cargo.toml index c1b1fd1..073138c 100644 --- a/commons/Cargo.toml +++ b/commons/Cargo.toml @@ -16,7 +16,6 @@ byte-unit = "5" const_format = "0.2" # TODO: Consolidate on either the regex crate or the fancy-regex crate, since this repo currently uses both. fancy-regex = "0.13" -fs_extra = "1" fs-err = "2" fun_run = "0.2" glob = "0.3" @@ -32,6 +31,7 @@ sha2 = "0.10" tempfile = "3" thiserror = "1" walkdir = "2" +cp_r = "0.5.2" [dev-dependencies] filetime = "0.2" diff --git a/commons/src/cache/app_cache.rs b/commons/src/cache/app_cache.rs index db42d2e..5ee397d 100644 --- a/commons/src/cache/app_cache.rs +++ b/commons/src/cache/app_cache.rs @@ -2,7 +2,6 @@ use crate::cache::clean::{lru_clean, FilesWithSize}; use crate::cache::in_app_dir_cache_layer::InAppDirCacheLayer; use crate::cache::{CacheConfig, CacheError, KeepPath}; use byte_unit::{AdjustedByte, Byte, UnitType}; -use fs_extra::dir::CopyOptions; use libcnb::build::BuildContext; use libcnb::data::layer::LayerName; use std::path::Path; @@ -148,22 +147,20 @@ impl AppCache { fs_err::create_dir_all(&self.path).map_err(CacheError::IoError)?; fs_err::create_dir_all(&self.cache).map_err(CacheError::IoError)?; - fs_extra::dir::move_dir( - &self.cache, - &self.path, - &CopyOptions { - overwrite: false, - skip_exist: true, - copy_inside: true, - content_only: true, - ..CopyOptions::default() - }, - ) - .map_err(|error| CacheError::CopyCacheToAppError { - path: self.path.clone(), - cache: self.cache.clone(), - error, - })?; + cp_r::CopyOptions::new() + .create_destination(true) + // Do not overwrite + .filter(|to_file, _| { + let destination = self.path.join(to_file); + let exists = destination.try_exists().unwrap_or(false); + Ok(!exists) + }) + .copy_tree(&self.cache, &self.path) + .map_err(|error| CacheError::CopyCacheToAppError { + path: self.path.clone(), + cache: self.cache.clone(), + error, + })?; Ok(self) } @@ -276,21 +273,14 @@ pub fn build( /// /// - If the copy command fails an `IoExtraError` will be raised. fn preserve_path_save(store: &AppCache) -> Result<&AppCache, CacheError> { - fs_extra::dir::copy( - &store.path, - &store.cache, - &CopyOptions { - overwrite: true, - copy_inside: true, // Recursive - content_only: true, // Don't copy top level directory name - ..CopyOptions::default() - }, - ) - .map_err(|error| CacheError::CopyAppToCacheError { - path: store.path.clone(), - cache: store.cache.clone(), - error, - })?; + cp_r::CopyOptions::new() + .create_destination(true) + .copy_tree(&store.path, &store.cache) + .map_err(|error| CacheError::CopyAppToCacheError { + path: store.path.clone(), + cache: store.cache.clone(), + error, + })?; Ok(store) } @@ -306,21 +296,16 @@ fn preserve_path_save(store: &AppCache) -> Result<&AppCache, CacheError> { /// /// - If the move command fails an `IoExtraError` will be raised. fn remove_path_save(store: &AppCache) -> Result<&AppCache, CacheError> { - fs_extra::dir::move_dir( - &store.path, - &store.cache, - &CopyOptions { - overwrite: true, - copy_inside: true, // Recursive - content_only: true, // Don't copy top level directory name - ..CopyOptions::default() - }, - ) - .map_err(|error| CacheError::DestructiveMoveAppToCacheError { - path: store.path.clone(), - cache: store.cache.clone(), - error, - })?; + cp_r::CopyOptions::new() + .create_destination(true) + .copy_tree(&store.path, &store.cache) + .map_err(|error| CacheError::DestructiveMoveAppToCacheError { + path: store.path.clone(), + cache: store.cache.clone(), + error, + })?; + + fs_err::remove_dir_all(&store.path).map_err(CacheError::IoError)?; Ok(store) } @@ -475,7 +460,7 @@ mod tests { } #[test] - fn mtime_preserved() { + fn mtime_preserved_keep_path_build_only() { let tmpdir = tempfile::tempdir().unwrap(); let cache_path = tmpdir.path().join("cache"); let app_path = tmpdir.path().join("app"); @@ -501,7 +486,53 @@ mod tests { let metadata = fs_err::metadata(store.cache.join("lol.txt")).unwrap(); let actual = filetime::FileTime::from_last_modification_time(&metadata); + assert_eq!(mtime, actual); + + assert!(!store.path.join("lol.txt").exists()); + + store.load().unwrap(); + + let metadata = fs_err::metadata(store.cache.join("lol.txt")).unwrap(); + let actual = filetime::FileTime::from_last_modification_time(&metadata); + assert_eq!(mtime, actual); + } + #[test] + fn mtime_preserved_keep_path_runtime() { + let tmpdir = tempfile::tempdir().unwrap(); + let cache_path = tmpdir.path().join("cache"); + let app_path = tmpdir.path().join("app"); + + fs_err::create_dir_all(&cache_path).unwrap(); + fs_err::create_dir_all(&app_path).unwrap(); + + let store = AppCache { + path: app_path.clone(), + cache: cache_path, + limit: Byte::from_u64(512), + keep_path: KeepPath::Runtime, + cache_state: CacheState::NewEmpty, + }; + + let mtime = filetime::FileTime::from_unix_time(1000, 0); + + let path = app_path.join("lol.txt"); + fs_err::write(&path, "hahaha").unwrap(); + filetime::set_file_mtime(&path, mtime).unwrap(); + + store.save().unwrap(); + + let metadata = fs_err::metadata(store.cache.join("lol.txt")).unwrap(); + let actual = filetime::FileTime::from_last_modification_time(&metadata); + assert_eq!(mtime, actual); + + fs_err::remove_dir_all(&app_path).unwrap(); + assert!(!store.path.join("lol.txt").exists()); + + store.load().unwrap(); + + let metadata = fs_err::metadata(store.cache.join("lol.txt")).unwrap(); + let actual = filetime::FileTime::from_last_modification_time(&metadata); assert_eq!(mtime, actual); } } diff --git a/commons/src/cache/error.rs b/commons/src/cache/error.rs index 4a5f4e7..1a7bec6 100644 --- a/commons/src/cache/error.rs +++ b/commons/src/cache/error.rs @@ -14,21 +14,21 @@ pub enum CacheError { CopyCacheToAppError { path: PathBuf, cache: PathBuf, - error: fs_extra::error::Error, + error: cp_r::Error, }, #[error("Could not copy files from the application to cache.\nFrom: {path} To: {cache}\nError: {error}")] CopyAppToCacheError { path: PathBuf, cache: PathBuf, - error: fs_extra::error::Error, + error: cp_r::Error, }, #[error("Could not move files out of the application to the cache.\nFrom: {path} To: {cache}\nError: {error}")] DestructiveMoveAppToCacheError { path: PathBuf, cache: PathBuf, - error: fs_extra::error::Error, + error: cp_r::Error, }, #[error("IO error: {0}")]