From 40a88f57f7dacc98fa925a97140846e62d30d6a0 Mon Sep 17 00:00:00 2001 From: Moe Date: Mon, 12 Jul 2021 05:34:10 +0200 Subject: [PATCH] initial implementation of timestamp preservation #166 (#360) Closes #166 --- Cargo.lock | 19 ++++++ Cargo.toml | 6 +- src/lib.rs | 162 ++++++++++++++++++++++++++++++++++++++----------- tests/flags.rs | 90 +++++++++++++++++++++++++-- 4 files changed, 237 insertions(+), 40 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 213a7f31..c4feccdc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -211,6 +211,18 @@ version = "1.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e78d4f1cc4ae33bbfc157ed5d5a5ef3bc29227303d595861deb238fcec4e9457" +[[package]] +name = "filetime" +version = "0.2.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0c122a393ea57648015bf06fbd3d372378992e86b9ff5a7a497b076a28c79efe" +dependencies = [ + "cfg-if 1.0.0", + "libc", + "redox_syscall", + "winapi", +] + [[package]] name = "glob" version = "0.3.0" @@ -395,6 +407,7 @@ dependencies = [ "cloudflare-zlib", "crc", "crossbeam-channel", + "filetime", "image", "indexmap", "itertools", @@ -455,6 +468,12 @@ dependencies = [ "num_cpus", ] +[[package]] +name = "redox_syscall" +version = "0.1.57" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "41cc0f7e4d5d4544e8861606a285bb08d3e70712ccc7d2b84d7c0ccfaf4b05ce" + [[package]] name = "rgb" version = "0.8.25" diff --git a/Cargo.toml b/Cargo.toml index 5a5f19a4..fd4b63f3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -42,6 +42,10 @@ log = "0.4.14" stderrlog = { version = "0.5.1", optional = true } crossbeam-channel = "0.5.0" +[dependencies.filetime] +optional = true +version = "0.2.13" + [dependencies.rayon] optional = true version = "^1.5.0" @@ -72,7 +76,7 @@ binary = [ "wild", "stderrlog", ] -default = ["binary", "parallel", "libdeflater", "zopfli"] +default = ["binary", "filetime", "parallel", "libdeflater", "zopfli"] parallel = ["rayon", "indexmap/rayon"] [lib] diff --git a/src/lib.rs b/src/lib.rs index 239968de..5dbc94c9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -36,7 +36,7 @@ use image::{DynamicImage, GenericImageView, ImageFormat, Pixel}; use log::{debug, error, info, warn}; use rayon::prelude::*; use std::fmt; -use std::fs::{copy, File}; +use std::fs::{copy, File, Metadata}; use std::io::{stdin, stdout, BufWriter, Read, Write}; use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicBool, Ordering}; @@ -330,9 +330,29 @@ pub fn optimize(input: &InFile, output: &OutFile, opts: &Options) -> PngResult<( let deadline = Arc::new(Deadline::new(opts.timeout)); + // grab metadata before even opening input file to preserve atime + let opt_metadata_preserved; let in_data = match *input { - InFile::Path(ref input_path) => PngData::read_file(input_path)?, + InFile::Path(ref input_path) => { + if opts.preserve_attrs { + opt_metadata_preserved = input_path + .metadata() + .map_err(|err| { + // Fail if metadata cannot be preserved + PngError::new(&format!( + "Unable to read metadata from input file {:?}: {}", + input_path, err + )) + }) + .map(Some)?; + debug!("preserving metadata: {:?}", opt_metadata_preserved); + } else { + opt_metadata_preserved = None; + } + PngData::read_file(input_path)? + } InFile::StdIn => { + opt_metadata_preserved = None; let mut data = Vec::new(); stdin() .read_to_end(&mut data) @@ -388,20 +408,27 @@ pub fn optimize(input: &InFile, output: &OutFile, opts: &Options) -> PngResult<( err )) })?; - if opts.preserve_attrs { - if let Some(input_path) = input.path() { - copy_permissions(input_path, &out_file); - } + if let Some(metadata_input) = &opt_metadata_preserved { + copy_permissions(&metadata_input, &out_file)?; } let mut buffer = BufWriter::new(out_file); - buffer.write_all(&optimized_output).map_err(|e| { - PngError::new(&format!( - "Unable to write to {}: {}", - output_path.display(), - e - )) - })?; + buffer + .write_all(&optimized_output) + // flush BufWriter so IO errors don't get swallowed silently on close() by drop! + .and_then(|()| buffer.flush()) + .map_err(|e| { + PngError::new(&format!( + "Unable to write to {}: {}", + output_path.display(), + e + )) + })?; + // force drop and thereby closing of file handle before modifying any timestamp + std::mem::drop(buffer); + if let Some(metadata_input) = &opt_metadata_preserved { + copy_times(&metadata_input, &output_path)?; + } info!("Output: {}", output_path.display()); } } @@ -942,33 +969,98 @@ fn perform_backup(input_path: &Path) -> PngResult<()> { } #[cfg(not(unix))] -fn copy_permissions(input_path: &Path, out_file: &File) { - if let Ok(f) = File::open(input_path) { - if let Ok(metadata) = f.metadata() { - if let Ok(out_meta) = out_file.metadata() { - let readonly = metadata.permissions().readonly(); - out_meta.permissions().set_readonly(readonly); - return; - } - } - }; - warn!("Failed to set permissions on output file"); +fn copy_permissions(metadata_input: &Metadata, out_file: &File) -> PngResult<()> { + let readonly_input = metadata_input.permissions().readonly(); + + out_file + .metadata() + .map_err(|err_io| { + PngError::new(&format!( + "unable to read filesystem metadata of output file: {}", + err_io + )) + }) + .and_then(|out_meta| { + out_meta.permissions().set_readonly(readonly_input); + out_file + .metadata() + .map_err(|err_io| { + PngError::new(&format!( + "unable to re-read filesystem metadata of output file: {}", + err_io + )) + }) + .and_then(|out_meta_reread| { + if out_meta_reread.permissions().readonly() != readonly_input { + Err(PngError::new(&format!( + "failed to set readonly, expected: {}, found: {}", + readonly_input, + out_meta_reread.permissions().readonly() + ))) + } else { + Ok(()) + } + }) + }) } #[cfg(unix)] -fn copy_permissions(input_path: &Path, out_file: &File) { +fn copy_permissions(metadata_input: &Metadata, out_file: &File) -> PngResult<()> { use std::os::unix::fs::PermissionsExt; - if let Ok(f) = File::open(input_path) { - if let Ok(metadata) = f.metadata() { - if let Ok(out_meta) = out_file.metadata() { - let permissions = metadata.permissions().mode(); - out_meta.permissions().set_mode(permissions); - return; - } - } - }; - warn!("Failed to set permissions on output file"); + let permissions = metadata_input.permissions().mode(); + + out_file + .metadata() + .map_err(|err_io| { + PngError::new(&format!( + "unable to read filesystem metadata of output file: {}", + err_io + )) + }) + .and_then(|out_meta| { + out_meta.permissions().set_mode(permissions); + out_file + .metadata() + .map_err(|err_io| { + PngError::new(&format!( + "unable to re-read filesystem metadata of output file: {}", + err_io + )) + }) + .and_then(|out_meta_reread| { + if out_meta_reread.permissions().mode() != permissions { + Err(PngError::new(&format!( + "failed to set permissions, expected: {:04o}, found: {:04o}", + permissions, + out_meta_reread.permissions().mode() + ))) + } else { + Ok(()) + } + }) + }) +} + +#[cfg(not(feature = "filetime"))] +fn copy_times(_: &Metadata, _: &Path) -> PngResult<()> { + Ok(()) +} + +#[cfg(feature = "filetime")] +fn copy_times(input_path_meta: &Metadata, out_path: &Path) -> PngResult<()> { + let atime = filetime::FileTime::from_last_access_time(input_path_meta); + let mtime = filetime::FileTime::from_last_modification_time(input_path_meta); + debug!( + "attempting to set file times: atime: {:?}, mtime: {:?}", + atime, mtime + ); + filetime::set_file_times(out_path, atime, mtime).map_err(|err_io| { + PngError::new(&format!( + "unable to set file times on {:?}: {}", + out_path, err_io + )) + }) } /// Compares images pixel by pixel for equivalent content diff --git a/tests/flags.rs b/tests/flags.rs index 466e16f4..07f29d19 100644 --- a/tests/flags.rs +++ b/tests/flags.rs @@ -1,7 +1,11 @@ use indexmap::IndexSet; use oxipng::internal_tests::*; use oxipng::{InFile, OutFile}; +#[cfg(feature = "filetime")] +use std::cell::RefCell; use std::fs::remove_file; +#[cfg(feature = "filetime")] +use std::ops::Deref; use std::path::Path; use std::path::PathBuf; @@ -20,7 +24,8 @@ fn get_opts(input: &Path) -> (OutFile, oxipng::Options) { ) } -fn test_it_converts( +/// Add callback to allow checks before the output file is deleted again +fn test_it_converts_callbacks( input: PathBuf, output: &OutFile, opts: &oxipng::Options, @@ -28,19 +33,28 @@ fn test_it_converts( bit_depth_in: BitDepth, color_type_out: ColorType, bit_depth_out: BitDepth, -) { + mut callback_pre: CBPRE, + mut callback_post: CBPOST, +) where + CBPOST: FnMut(&Path) -> (), + CBPRE: FnMut(&Path) -> (), +{ let png = PngData::new(&input, opts.fix_errors).unwrap(); assert_eq!(png.raw.ihdr.color_type, color_type_in); assert_eq!(png.raw.ihdr.bit_depth, bit_depth_in); - match oxipng::optimize(&InFile::Path(input), output, opts) { + callback_pre(&input); + + match oxipng::optimize(&InFile::Path(input), &output, &opts) { Ok(_) => (), Err(x) => panic!("{}", x), }; let output = output.path().unwrap(); assert!(output.exists()); + callback_post(&output); + let png = match PngData::new(output, opts.fix_errors) { Ok(x) => x, Err(x) => { @@ -55,6 +69,29 @@ fn test_it_converts( remove_file(output).ok(); } +/// Shim for new callback functionality +fn test_it_converts( + input: PathBuf, + output: &OutFile, + opts: &oxipng::Options, + color_type_in: ColorType, + bit_depth_in: BitDepth, + color_type_out: ColorType, + bit_depth_out: BitDepth, +) { + test_it_converts_callbacks( + input, + output, + opts, + color_type_in, + bit_depth_in, + color_type_out, + bit_depth_out, + |_| {}, + |_| {}, + ) +} + #[test] fn verbose_mode() { use crossbeam_channel::{unbounded, Sender}; @@ -446,10 +483,53 @@ fn interlaced_0_to_1_other_filter_mode() { #[test] fn preserve_attrs() { let input = PathBuf::from("tests/files/preserve_attrs.png"); + + #[cfg(feature = "filetime")] + let atime_canon = RefCell::new(filetime::FileTime::from_unix_time(0, 0)); + #[cfg(feature = "filetime")] + let mtime_canon = RefCell::new(filetime::FileTime::from_unix_time(0, 0)); + let (output, mut opts) = get_opts(&input); opts.preserve_attrs = true; - test_it_converts( + #[cfg(feature = "filetime")] + let callback_pre = |path_in: &Path| { + let meta_input = path_in + .metadata() + .expect("unable to get file metadata for output file"); + + atime_canon.replace(filetime::FileTime::from_last_access_time(&meta_input)); + mtime_canon.replace(filetime::FileTime::from_last_modification_time(&meta_input)); + }; + #[cfg(not(feature = "filetime"))] + let callback_pre = |_: &Path| {}; + + #[cfg(feature = "filetime")] + let callback_post = |path_out: &Path| { + let meta_output = path_out + .metadata() + .expect("unable to get file metadata for output file"); + + let cellref_atime_canon = atime_canon.borrow(); + let cellref_mtime_canon = mtime_canon.borrow(); + let ref_atime_canon: &filetime::FileTime = cellref_atime_canon.deref(); + let ref_mtime_canon: &filetime::FileTime = cellref_mtime_canon.deref(); + + assert_eq!( + ref_atime_canon, + &filetime::FileTime::from_last_access_time(&meta_output), + "expected access time to be identical to that of input", + ); + assert_eq!( + ref_mtime_canon, + &filetime::FileTime::from_last_modification_time(&meta_output), + "expected modification time to be identical to that of input", + ); + }; + #[cfg(not(feature = "filetime"))] + let callback_post = |_: &Path| {}; + + test_it_converts_callbacks( input, &output, &opts, @@ -457,6 +537,8 @@ fn preserve_attrs() { BitDepth::Eight, ColorType::RGB, BitDepth::Eight, + callback_pre, + callback_post, ); // TODO: Actually check permissions