From e5e1568196293d7fc88d61d78a8c049c9595a151 Mon Sep 17 00:00:00 2001 From: andrews05 Date: Tue, 13 Jan 2026 07:11:45 +1300 Subject: [PATCH] Rework preserving attributes (#759) --- Cargo.lock | 33 ----------------- Cargo.toml | 7 +--- src/lib.rs | 96 ++++++++++++++++++-------------------------------- tests/flags.rs | 32 ++++++++--------- 4 files changed, 51 insertions(+), 117 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d8bf3455..6deb8076 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -245,18 +245,6 @@ dependencies = [ "simd-adler32", ] -[[package]] -name = "filetime" -version = "0.2.26" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bc0505cd1b6fa6580283f6bdf70a73fcf4aba1184038c90902b92b3dd0df63ed" -dependencies = [ - "cfg-if", - "libc", - "libredox", - "windows-sys 0.60.2", -] - [[package]] name = "find-msvc-tools" version = "0.1.2" @@ -346,17 +334,6 @@ dependencies = [ "libdeflate-sys", ] -[[package]] -name = "libredox" -version = "0.1.10" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "416f7e718bdb06000964960ffa43b4335ad4012ae8b99060261aa4a8088d5ccb" -dependencies = [ - "bitflags", - "libc", - "redox_syscall", -] - [[package]] name = "linux-raw-sys" version = "0.11.0" @@ -412,7 +389,6 @@ dependencies = [ "clap", "crossbeam-channel", "env_logger", - "filetime", "glob", "image", "indexmap", @@ -485,15 +461,6 @@ dependencies = [ "crossbeam-utils", ] -[[package]] -name = "redox_syscall" -version = "0.5.17" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5407465600fb0548f1442edf71dd20683c6ed326200ace4b1ef0763521bb3b77" -dependencies = [ - "bitflags", -] - [[package]] name = "rgb" version = "0.8.52" diff --git a/Cargo.toml b/Cargo.toml index f6636aba..8c8e216a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -56,10 +56,6 @@ version = "0.11.8" optional = true version = "0.5.15" -[dependencies.filetime] -optional = true -version = "0.2.26" - [dependencies.rayon] optional = true version = "1.11.0" @@ -85,12 +81,11 @@ version = "0.25.9" [features] binary = ["dep:clap", "dep:glob", "dep:env_logger", "dep:parse-size"] -default = ["binary", "parallel", "zopfli", "filetime"] +default = ["binary", "parallel", "zopfli"] parallel = ["dep:rayon", "indexmap/rayon", "dep:crossbeam-channel"] freestanding = ["libdeflater/freestanding"] sanity-checks = ["dep:image"] zopfli = ["dep:zopfli"] -filetime = ["dep:filetime"] system-libdeflate = ["libdeflater/dynamic"] [lib] diff --git a/src/lib.rs b/src/lib.rs index a447352d..b02b8ffd 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -7,9 +7,9 @@ extern crate rayon; mod rayon; use std::{ - fs::{File, Metadata}, + fs::File, io::{BufWriter, Read, Write, stdin, stdout}, - path::{Path, PathBuf}, + path::PathBuf, sync::{ Arc, atomic::{AtomicBool, Ordering}, @@ -171,34 +171,9 @@ 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) => { - if matches!( - output, - OutFile::Path { - preserve_attrs: true, - .. - } - ) { - 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)?; - trace!("preserving metadata: {opt_metadata_preserved:?}"); - } else { - opt_metadata_preserved = None; - } - PngData::read_file(input_path)? - } + InFile::Path(ref input_path) => PngData::read_file(input_path)?, InFile::StdIn => { - opt_metadata_preserved = None; let mut data = Vec::new(); stdin() .read_to_end(&mut data) @@ -253,17 +228,32 @@ pub fn optimize(input: &InFile, output: &OutFile, opts: &Options) -> PngResult<( .write_all(&optimized_output) .map_err(|e| PngError::WriteFailed("stdout".into(), e))?; } - (OutFile::Path { path, .. }, _) => { + ( + OutFile::Path { + path, + preserve_attrs, + }, + _, + ) => { + let input_metadata = if *preserve_attrs { + input.path().and_then(|in_path| { + let meta = in_path.metadata(); + if let Err(e) = &meta { + warn!("Unable to read metadata from {in_path:?}: {e}"); + } + meta.ok() + }) + } else { + None + }; + let output_path = path .as_ref() .map_or_else(|| input.path().unwrap(), PathBuf::as_path); let out_file = File::create(output_path) .map_err(|err| PngError::WriteFailed(output_path.display().to_string(), err))?; - if let Some(metadata_input) = &opt_metadata_preserved { - copy_permissions(metadata_input, &out_file)?; - } - let mut buffer = BufWriter::new(out_file); + let mut buffer = BufWriter::new(&out_file); buffer .write_all(&optimized_output) // flush BufWriter so IO errors don't get swallowed silently on close() by drop! @@ -271,8 +261,18 @@ pub fn optimize(input: &InFile, output: &OutFile, opts: &Options) -> PngResult<( .map_err(|e| PngError::WriteFailed(output_path.display().to_string(), 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)?; + + if let Some(metadata_input) = &input_metadata { + let set_time = metadata_input + .modified() + .and_then(|m| out_file.set_modified(m)); + if let Err(e) = set_time { + warn!("Unable to set modification time on {output_path:?}: {e}"); + } + let set_perm = out_file.set_permissions(metadata_input.permissions()); + if let Err(e) = set_perm { + warn!("Unable to set permissions on {output_path:?}: {e}"); + } } info!("{}: {}", savings, output_path.display()); } @@ -642,29 +642,3 @@ fn recompress_frames( const fn is_fully_optimized(original_size: usize, optimized_size: usize, opts: &Options) -> bool { original_size <= optimized_size && !opts.force } - -fn copy_permissions(metadata_input: &Metadata, out_file: &File) -> PngResult<()> { - out_file - .set_permissions(metadata_input.permissions()) - .map_err(|err_io| { - PngError::new(&format!( - "Unable to set permissions for output file: {err_io}" - )) - }) -} - -#[cfg(not(feature = "filetime"))] -const fn copy_times(_: &Metadata, _: &Path) -> PngResult<()> { - Ok(()) -} - -#[cfg(feature = "filetime")] -fn copy_times(input_path_meta: &Metadata, out_path: &Path) -> PngResult<()> { - let mtime = filetime::FileTime::from_last_modification_time(input_path_meta); - trace!("attempting to set file modification time: {mtime:?}"); - filetime::set_file_mtime(out_path, mtime).map_err(|err_io| { - PngError::new(&format!( - "Unable to set file times on {out_path:?}: {err_io}" - )) - }) -} diff --git a/tests/flags.rs b/tests/flags.rs index ebb9c7e7..63646c9d 100644 --- a/tests/flags.rs +++ b/tests/flags.rs @@ -430,12 +430,9 @@ fn interlaced_0_to_1_other_filter_mode() { fn preserve_attrs() { let input = PathBuf::from("tests/files/preserve_attrs.png"); - #[cfg(feature = "filetime")] let meta_input = input .metadata() - .expect("unable to get file metadata for output file"); - #[cfg(feature = "filetime")] - let mtime_canon = filetime::FileTime::from_last_modification_time(&meta_input); + .expect("unable to get metadata for input file"); let (mut output, opts) = get_opts(&input); if let OutFile::Path { preserve_attrs, .. } = &mut output { @@ -449,17 +446,6 @@ fn preserve_attrs() { let output = output.path().unwrap(); assert!(output.exists()); - #[cfg(feature = "filetime")] - let meta_output = output - .metadata() - .expect("unable to get file metadata for output file"); - #[cfg(feature = "filetime")] - assert_eq!( - &mtime_canon, - &filetime::FileTime::from_last_modification_time(&meta_output), - "expected modification time to be identical to that of input", - ); - match PngData::new(output, &opts) { Ok(x) => x, Err(x) => { @@ -468,9 +454,21 @@ fn preserve_attrs() { } }; - remove_file(output).ok(); + let meta_output = output + .metadata() + .expect("unable to get metadata for output file"); + assert_eq!( + meta_input.modified().ok(), + meta_output.modified().ok(), + "expected modification time to be identical to that of input", + ); + assert_eq!( + meta_input.permissions(), + meta_output.permissions(), + "expected permissions to be identical to that of input", + ); - // TODO: Actually check permissions + remove_file(output).ok(); } #[test]