From b4c3e95d14ef7f0117f87d6f2a95b82b11056360 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 14 Sep 2025 12:05:47 +0100 Subject: [PATCH 1/4] Avoid clobbering manifest in tests --- test_support/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test_support/src/lib.rs b/test_support/src/lib.rs index c685ef02..86f891b4 100644 --- a/test_support/src/lib.rs +++ b/test_support/src/lib.rs @@ -152,7 +152,8 @@ pub fn ensure_manifest_exists(temp_dir: &Path, cli_file: &Path) -> PathBuf { "Failed to write manifest content to {}", manifest_path.display() )); - file.persist(&manifest_path).expect(&format!( + // Avoid clobbering an existing manifest if concurrently created. + file.persist_noclobber(&manifest_path).expect(&format!( "Failed to persist manifest file to {}", manifest_path.display() )); From 291abda00f65789dfc666eb5b4a3013acc7a9157 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 14 Sep 2025 21:24:21 +0100 Subject: [PATCH 2/4] Handle existing manifest in test helper --- test_support/src/lib.rs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/test_support/src/lib.rs b/test_support/src/lib.rs index 86f891b4..8d315473 100644 --- a/test_support/src/lib.rs +++ b/test_support/src/lib.rs @@ -27,7 +27,7 @@ mod error; pub use error::display_error_chain; use std::fs::{self, File}; -use std::io::Write; +use std::io::{self, Write}; use std::path::{Path, PathBuf}; use tempfile::{NamedTempFile, TempDir}; @@ -153,10 +153,16 @@ pub fn ensure_manifest_exists(temp_dir: &Path, cli_file: &Path) -> PathBuf { manifest_path.display() )); // Avoid clobbering an existing manifest if concurrently created. - file.persist_noclobber(&manifest_path).expect(&format!( - "Failed to persist manifest file to {}", - manifest_path.display() - )); + // Treat AlreadyExists as success when another process creates it. + if let Err(err) = file.persist_noclobber(&manifest_path) { + if err.error.kind() != io::ErrorKind::AlreadyExists { + panic!( + "Failed to persist manifest file to {}: {}", + manifest_path.display(), + err.error + ); + } + } } manifest_path From 40c926325024ad5a15a14d1349921986e768ca97 Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 15 Sep 2025 18:21:50 +0100 Subject: [PATCH 3/4] Refactor manifest creation helper Break ensure_manifest_exists into smaller helpers to reduce nested control flow while preserving behaviour. --- test_support/src/lib.rs | 89 +++++++++++++++++++++++++++-------------- 1 file changed, 58 insertions(+), 31 deletions(-) diff --git a/test_support/src/lib.rs b/test_support/src/lib.rs index 8d315473..6b81a429 100644 --- a/test_support/src/lib.rs +++ b/test_support/src/lib.rs @@ -130,42 +130,69 @@ pub fn fake_ninja(exit_code: u8) -> (TempDir, PathBuf) { /// assert!(manifest.exists()); /// ``` pub fn ensure_manifest_exists(temp_dir: &Path, cli_file: &Path) -> PathBuf { - let manifest_path = if cli_file.is_absolute() { + let manifest_path = resolve_manifest_path(temp_dir, cli_file); + + if manifest_path.exists() { + return manifest_path; + } + + let dest_dir = ensure_directory_exists(&manifest_path, temp_dir); + let file = create_manifest_file(&dest_dir, &manifest_path); + persist_manifest_file(file, &manifest_path); + + manifest_path +} + +fn resolve_manifest_path(temp_dir: &Path, cli_file: &Path) -> PathBuf { + if cli_file.is_absolute() { cli_file.to_path_buf() } else { temp_dir.join(cli_file) - }; - - if !manifest_path.exists() { - let dest_dir = manifest_path.parent().unwrap_or(temp_dir); - if !dest_dir.exists() { - fs::create_dir_all(dest_dir).expect(&format!( - "Failed to create manifest parent directory for {}", - manifest_path.display() - )); - } - let mut file = NamedTempFile::new_in(dest_dir).expect(&format!( - "Failed to create temporary manifest file for {}", - manifest_path.display() - )); - crate::env::write_manifest(&mut file).expect(&format!( - "Failed to write manifest content to {}", - manifest_path.display() - )); - // Avoid clobbering an existing manifest if concurrently created. - // Treat AlreadyExists as success when another process creates it. - if let Err(err) = file.persist_noclobber(&manifest_path) { - if err.error.kind() != io::ErrorKind::AlreadyExists { - panic!( - "Failed to persist manifest file to {}: {}", - manifest_path.display(), - err.error - ); - } - } } +} - manifest_path +fn ensure_directory_exists(manifest_path: &Path, temp_dir: &Path) -> PathBuf { + let dest_dir = manifest_path + .parent() + .map(Path::to_path_buf) + .unwrap_or_else(|| temp_dir.to_path_buf()); + + if dest_dir.exists() { + return dest_dir; + } + + fs::create_dir_all(&dest_dir).expect(&format!( + "Failed to create manifest parent directory for {}", + manifest_path.display() + )); + + dest_dir +} + +fn create_manifest_file(dest_dir: &Path, manifest_path: &Path) -> NamedTempFile { + let mut file = NamedTempFile::new_in(dest_dir).expect(&format!( + "Failed to create temporary manifest file for {}", + manifest_path.display() + )); + crate::env::write_manifest(&mut file).expect(&format!( + "Failed to write manifest content to {}", + manifest_path.display() + )); + file +} + +fn persist_manifest_file(file: NamedTempFile, manifest_path: &Path) { + // Avoid clobbering an existing manifest if concurrently created. + // Treat AlreadyExists as success when another process creates it. + if let Err(err) = file.persist_noclobber(manifest_path) { + if err.error.kind() != io::ErrorKind::AlreadyExists { + panic!( + "Failed to persist manifest file to {}: {}", + manifest_path.display(), + err.error + ); + } + } } // Additional helpers can be added here as the test suite evolves. From e1ac20b5f37c2ce2639dff4150717dfce272012a Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 15 Sep 2025 19:17:24 +0100 Subject: [PATCH 4/4] Handle AlreadyExists when persisting manifests --- test_support/src/lib.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test_support/src/lib.rs b/test_support/src/lib.rs index 6b81a429..8a7c96a7 100644 --- a/test_support/src/lib.rs +++ b/test_support/src/lib.rs @@ -184,8 +184,10 @@ fn create_manifest_file(dest_dir: &Path, manifest_path: &Path) -> NamedTempFile fn persist_manifest_file(file: NamedTempFile, manifest_path: &Path) { // Avoid clobbering an existing manifest if concurrently created. // Treat AlreadyExists as success when another process creates it. - if let Err(err) = file.persist_noclobber(manifest_path) { - if err.error.kind() != io::ErrorKind::AlreadyExists { + match file.persist_noclobber(manifest_path) { + Ok(_) => {} + Err(err) if err.error.kind() == io::ErrorKind::AlreadyExists => {} + Err(err) => { panic!( "Failed to persist manifest file to {}: {}", manifest_path.display(),