From 4ca3cff07e3a85a66d67f9d736f900a7265bcce1 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 9 Aug 2025 00:01:16 +0100 Subject: [PATCH 01/13] Create parent dirs for Ninja file --- src/runner.rs | 7 +++++++ tests/runner_tests.rs | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/src/runner.rs b/src/runner.rs index ff170246..6c6fb9ee 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -163,6 +163,13 @@ fn create_temp_ninja_file(content: &NinjaContent) -> Result { /// write_ninja_file(Path::new("out.ninja"), &content).unwrap(); /// ``` fn write_ninja_file(path: &Path, content: &NinjaContent) -> Result<()> { + // Ensure the parent directory exists; this prevents failures when the + // user specifies a nested output path with --emit or when temporary + // files are created under a new directory structure. + if let Some(parent) = path.parent() { + fs::create_dir_all(parent) + .with_context(|| format!("failed to create parent directory {}", parent.display()))?; + } fs::write(path, content.as_str()) .with_context(|| format!("failed to write Ninja file to {}", path.display()))?; info!("Generated Ninja file at {}", path.display()); diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index 5d4ace19..4143b7b7 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -132,6 +132,47 @@ fn run_build_with_emit_keeps_file() { drop(ninja_path); } +#[cfg(unix)] +#[test] +#[serial] +fn run_build_with_emit_creates_parent_dirs() { + let (ninja_dir, ninja_path) = support::fake_ninja(0); + let original_path = std::env::var_os("PATH").unwrap_or_default(); + let mut paths: Vec<_> = std::env::split_paths(&original_path).collect(); + paths.insert(0, ninja_dir.path().to_path_buf()); + let new_path = std::env::join_paths(paths).expect("join paths"); + unsafe { + std::env::set_var("PATH", &new_path); + } + + let temp = tempfile::tempdir().expect("temp dir"); + let manifest_path = temp.path().join("Netsukefile"); + std::fs::copy("tests/data/minimal.yml", &manifest_path).expect("copy manifest"); + let nested_dir = temp.path().join("nested").join("dir"); + let emit_path = nested_dir.join("emitted.ninja"); + assert!(!nested_dir.exists()); + let cli = Cli { + file: manifest_path.clone(), + directory: Some(temp.path().to_path_buf()), + jobs: None, + verbose: false, + command: Some(Commands::Build(BuildArgs { + emit: Some(emit_path.clone()), + targets: vec![], + })), + }; + + let result = run(&cli); + assert!(result.is_ok()); + assert!(emit_path.exists()); + assert!(nested_dir.exists()); + + unsafe { + std::env::set_var("PATH", original_path); + } + drop(ninja_path); +} + #[test] #[serial] fn run_manifest_subcommand_writes_file() { From 50126c8c579f47e1493a121b572f3c0bb19fd6b9 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 10 Aug 2025 12:57:44 +0100 Subject: [PATCH 02/13] Scope env vars in tests --- tests/runner_tests.rs | 36 +++++++++--------------------------- tests/support/mod.rs | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 27 deletions(-) diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index 4143b7b7..a79e8e2b 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -2,9 +2,13 @@ use netsuke::cli::{BuildArgs, Cli, Commands}; use netsuke::runner::{BuildTargets, NINJA_ENV, run, run_ninja}; use rstest::rstest; use serial_test::serial; -use std::path::{Path, PathBuf}; +use std::{ + ffi::OsStr, + path::{Path, PathBuf}, +}; mod support; +use support::ScopedEnv; #[test] fn run_exits_with_manifest_error_on_invalid_version() { @@ -59,9 +63,7 @@ fn run_executes_ninja_without_persisting_file() { let mut paths: Vec<_> = std::env::split_paths(&original_path).collect(); paths.insert(0, ninja_dir.path().to_path_buf()); let new_path = std::env::join_paths(paths).expect("join paths"); - unsafe { - std::env::set_var("PATH", &new_path); - } // Nightly marks set_var unsafe. + let _guard = ScopedEnv::set("PATH", &new_path); let temp = tempfile::tempdir().expect("temp dir"); let manifest_path = temp.path().join("Netsukefile"); @@ -83,9 +85,6 @@ fn run_executes_ninja_without_persisting_file() { // Ensure no ninja file remains in project directory assert!(!temp.path().join("build.ninja").exists()); - unsafe { - std::env::set_var("PATH", original_path); - } // Nightly marks set_var unsafe. drop(ninja_path); } @@ -98,9 +97,7 @@ fn run_build_with_emit_keeps_file() { let mut paths: Vec<_> = std::env::split_paths(&original_path).collect(); paths.insert(0, ninja_dir.path().to_path_buf()); let new_path = std::env::join_paths(paths).expect("join paths"); - unsafe { - std::env::set_var("PATH", &new_path); - } + let _guard = ScopedEnv::set("PATH", &new_path); let temp = tempfile::tempdir().expect("temp dir"); let manifest_path = temp.path().join("Netsukefile"); @@ -126,9 +123,6 @@ fn run_build_with_emit_keeps_file() { assert!(emitted.contains("build ")); assert!(!temp.path().join("build.ninja").exists()); - unsafe { - std::env::set_var("PATH", original_path); - } drop(ninja_path); } @@ -141,9 +135,7 @@ fn run_build_with_emit_creates_parent_dirs() { let mut paths: Vec<_> = std::env::split_paths(&original_path).collect(); paths.insert(0, ninja_dir.path().to_path_buf()); let new_path = std::env::join_paths(paths).expect("join paths"); - unsafe { - std::env::set_var("PATH", &new_path); - } + let _guard = ScopedEnv::set("PATH", &new_path); let temp = tempfile::tempdir().expect("temp dir"); let manifest_path = temp.path().join("Netsukefile"); @@ -167,19 +159,13 @@ fn run_build_with_emit_creates_parent_dirs() { assert!(emit_path.exists()); assert!(nested_dir.exists()); - unsafe { - std::env::set_var("PATH", original_path); - } drop(ninja_path); } #[test] #[serial] fn run_manifest_subcommand_writes_file() { - let original_path = std::env::var_os("PATH").unwrap_or_default(); - unsafe { - std::env::set_var("PATH", ""); - } + let _guard = ScopedEnv::set("PATH", OsStr::new("")); let temp = tempfile::tempdir().expect("temp dir"); let manifest_path = temp.path().join("Netsukefile"); @@ -199,10 +185,6 @@ fn run_manifest_subcommand_writes_file() { assert!(result.is_ok()); assert!(output_path.exists()); assert!(!temp.path().join("build.ninja").exists()); - - unsafe { - std::env::set_var("PATH", original_path); - } } #[test] diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 443d7221..4081058a 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -3,6 +3,7 @@ //! This module provides helpers for creating fake executables and //! generating minimal manifests used in behavioural tests. +use std::ffi::{OsStr, OsString}; use std::fs::{self, File}; use std::io::{self, Write}; use std::path::PathBuf; @@ -166,3 +167,40 @@ pub fn write_manifest(file: &mut impl Write) -> io::Result<()> { ), ) } + +/// Scoped environment variable setter that restores the original value on drop. +/// +/// # Examples +/// +/// ```ignore +/// let _guard = ScopedEnv::set("PATH", ""); +/// // PATH is now empty within this scope +/// ``` +#[allow(dead_code, reason = "used only in some test crates")] +pub struct ScopedEnv { + key: &'static str, + original: Option, +} + +impl ScopedEnv { + /// Set `key` to `value`, returning a guard that resets it when dropped. + #[must_use] + #[allow(dead_code, reason = "used only in some test crates")] + pub fn set(key: &'static str, value: &OsStr) -> Self { + let original = std::env::var_os(key); + unsafe { std::env::set_var(key, value) }; + Self { key, original } + } +} + +impl Drop for ScopedEnv { + fn drop(&mut self) { + unsafe { + if let Some(val) = &self.original { + std::env::set_var(self.key, val); + } else { + std::env::remove_var(self.key); + } + } + } +} From ad35a178f2bfa584c3e24f2ce538fb62ab869223 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 10 Aug 2025 13:31:23 +0100 Subject: [PATCH 03/13] Inject env for ninja path --- Cargo.lock | 243 +++++++++++++++++++++++------------------- Cargo.toml | 3 +- src/runner.rs | 83 ++++++++++++++- tests/runner_tests.rs | 26 +++-- tests/support/mod.rs | 38 ------- 5 files changed, 230 insertions(+), 163 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 26605112..0c78f7ad 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -92,7 +92,7 @@ dependencies = [ "bstr", "doc-comment", "libc", - "predicates", + "predicates 3.1.3", "predicates-core", "predicates-tree", "wait-timeout", @@ -106,7 +106,7 @@ checksum = "e539d3fca749fcee5236ab05e93a52867dd549cc157c8cb7f99595f3cedffdb5" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.104", ] [[package]] @@ -206,7 +206,7 @@ dependencies = [ "heck 0.5.0", "proc-macro2", "quote", - "syn", + "syn 2.0.104", ] [[package]] @@ -298,7 +298,7 @@ dependencies = [ "globwalk", "humantime", "inventory", - "itertools", + "itertools 0.12.1", "lazy-regex", "linked-hash-map", "once_cell", @@ -316,11 +316,11 @@ checksum = "01091e28d1f566c8b31b67948399d2efd6c0a8f6228a9785519ed7b73f7f0aef" dependencies = [ "cucumber-expressions", "inflections", - "itertools", + "itertools 0.12.1", "proc-macro2", "quote", "regex", - "syn", + "syn 2.0.104", "synthez", ] @@ -346,7 +346,7 @@ checksum = "6edb4b64a43d977b8e99788fe3a04d483834fba1215a7e02caa415b626497f7f" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.104", ] [[package]] @@ -371,6 +371,12 @@ version = "0.3.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "fea41bba32d969b513997752735605054bc0dfa92b4c56bf1189f2e174be7a10" +[[package]] +name = "downcast" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1435fa1053d8b2fbbe9be7e97eca7f33d37b28409959813daefc1446a14247f1" + [[package]] name = "drain_filter_polyfill" version = "0.1.3" @@ -411,6 +417,21 @@ version = "2.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "37909eebbb50d72f9059c3b6d82c0463f2ff062c9e95845c43a6c9c0355411be" +[[package]] +name = "float-cmp" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "98de4bbd547a563b716d8dfa9aad1cb19bfab00f4fa09a6a4ed21dbcf44ce9c4" +dependencies = [ + "num-traits", +] + +[[package]] +name = "fragile" +version = "2.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "28dd6caf6059519a65843af8fe2a3ae298b14b80179855aeb4adc2c1934ee619" + [[package]] name = "futures" version = "0.3.31" @@ -467,7 +488,7 @@ checksum = "162ee34ebcb7c64a8abebc059ce0fee27c2262618d7b60ed8faf72fef13c3650" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.104", ] [[package]] @@ -539,7 +560,7 @@ dependencies = [ "quote", "serde", "serde_json", - "syn", + "syn 2.0.104", "textwrap", "thiserror", "typed-builder", @@ -675,6 +696,15 @@ version = "1.70.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7943c866cc5cd64cbc25b2e01621d07fa8eb2a1a23160ee81ce38704e97b8ecf" +[[package]] +name = "itertools" +version = "0.10.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b0fd2260e829bddf4cb6ea802289de2f86d6a7a690192fbe91b3f46e0f2c8473" +dependencies = [ + "either", +] + [[package]] name = "itertools" version = "0.12.1" @@ -710,7 +740,7 @@ dependencies = [ "proc-macro2", "quote", "regex", - "syn", + "syn 2.0.104", ] [[package]] @@ -747,16 +777,6 @@ version = "0.9.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cd945864f07fe9f5371a27ad7b52a172b4b499999f1d97574c9fa68373937e12" -[[package]] -name = "lock_api" -version = "0.4.13" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "96936507f153605bddfcda068dd804796c84324ed2510809e5b2a624c81da765" -dependencies = [ - "autocfg", - "scopeguard", -] - [[package]] name = "log" version = "0.4.27" @@ -804,6 +824,43 @@ dependencies = [ "windows-sys 0.59.0", ] +[[package]] +name = "mockable" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ce05993e13a317aef6ba5be77bb84b7e5b6bf67a7ba9a5168754c6a7aa3921da" +dependencies = [ + "mockall", + "tracing", +] + +[[package]] +name = "mockall" +version = "0.11.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4c84490118f2ee2d74570d114f3d0493cbf02790df303d2707606c3e14e07c96" +dependencies = [ + "cfg-if", + "downcast", + "fragile", + "lazy_static", + "mockall_derive", + "predicates 2.1.5", + "predicates-tree", +] + +[[package]] +name = "mockall_derive" +version = "0.11.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "22ce75669015c4f47b289fd4d4f56e894e4c96003ffdf3ac51313126f94c6cbb" +dependencies = [ + "cfg-if", + "proc-macro2", + "quote", + "syn 1.0.109", +] + [[package]] name = "netsuke" version = "0.1.0" @@ -813,16 +870,16 @@ dependencies = [ "clap", "cucumber", "insta", - "itertools", + "itertools 0.12.1", "itoa", "minijinja", + "mockable", "rstest", "semver", "serde", "serde_json", "serde_json_canonicalizer", "serde_yml", - "serial_test", "sha2", "tempfile", "thiserror", @@ -852,6 +909,12 @@ dependencies = [ "nom", ] +[[package]] +name = "normalize-line-endings" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "61807f77802ff30975e01f4f071c8ba10c022052f98b3294119f3e615d13e5be" + [[package]] name = "nu-ansi-term" version = "0.46.0" @@ -862,6 +925,15 @@ dependencies = [ "winapi", ] +[[package]] +name = "num-traits" +version = "0.2.19" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "071dfc062690e90b734c0b2273ce72ad0ffa95f0c74596bc250dcfd960262841" +dependencies = [ + "autocfg", +] + [[package]] name = "object" version = "0.36.7" @@ -889,29 +961,6 @@ version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b15813163c1d831bf4a13c3610c05c0d03b39feb07f7e09fa234dac9b15aaf39" -[[package]] -name = "parking_lot" -version = "0.12.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "70d58bf43669b5795d1576d0641cfb6fbb2057bf629506267a92807158584a13" -dependencies = [ - "lock_api", - "parking_lot_core", -] - -[[package]] -name = "parking_lot_core" -version = "0.9.11" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bc838d2a56b5b1a6c25f55575dfc605fabb63bb2365f6c2353ef9159aa69e4a5" -dependencies = [ - "cfg-if", - "libc", - "redox_syscall", - "smallvec", - "windows-targets 0.52.6", -] - [[package]] name = "peg" version = "0.6.3" @@ -956,7 +1005,7 @@ checksum = "6e918e4ff8c4549eb882f14b3a4bc8c8bc93de829416eacf579f1207a8fbf861" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.104", ] [[package]] @@ -971,6 +1020,20 @@ version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8b870d8c151b6f2fb93e84a13146138f05d02ed11c7e7c54f8826aaaf7c9f184" +[[package]] +name = "predicates" +version = "2.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "59230a63c37f3e18569bdb90e4a89cbf5bf8b06fea0b84e65ea10cc4df47addd" +dependencies = [ + "difflib", + "float-cmp", + "itertools 0.10.5", + "normalize-line-endings", + "predicates-core", + "regex", +] + [[package]] name = "predicates" version = "3.1.3" @@ -1022,15 +1085,6 @@ version = "5.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "69cdb34c158ceb288df11e18b4bd39de994f6657d83847bdffdbd7f346754b0f" -[[package]] -name = "redox_syscall" -version = "0.5.17" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5407465600fb0548f1442edf71dd20683c6ed326200ace4b1ef0763521bb3b77" -dependencies = [ - "bitflags 2.9.1", -] - [[package]] name = "regex" version = "1.11.1" @@ -1097,7 +1151,7 @@ dependencies = [ "regex", "relative-path", "rustc_version", - "syn", + "syn 2.0.104", "unicode-ident", ] @@ -1156,27 +1210,6 @@ dependencies = [ "winapi-util", ] -[[package]] -name = "scc" -version = "2.3.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "22b2d775fb28f245817589471dd49c5edf64237f4a19d10ce9a92ff4651a27f4" -dependencies = [ - "sdd", -] - -[[package]] -name = "scopeguard" -version = "1.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "94143f37725109f92c262ed2cf5e59bce7498c01bcc1502d7b9afe439a4e9f49" - -[[package]] -name = "sdd" -version = "3.0.10" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "490dcfcbfef26be6800d11870ff2df8774fa6e86d047e3e8c8a76b25655e41ca" - [[package]] name = "sealed" version = "0.5.0" @@ -1186,7 +1219,7 @@ dependencies = [ "heck 0.4.1", "proc-macro2", "quote", - "syn", + "syn 2.0.104", ] [[package]] @@ -1215,7 +1248,7 @@ checksum = "5b0276cf7f2c73365f7157c8123c21cd9a50fbbd844757af28ca1f5925fc2a00" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.104", ] [[package]] @@ -1256,31 +1289,6 @@ dependencies = [ "version_check", ] -[[package]] -name = "serial_test" -version = "3.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1b258109f244e1d6891bf1053a55d63a5cd4f8f4c30cf9a1280989f80e7a1fa9" -dependencies = [ - "futures", - "log", - "once_cell", - "parking_lot", - "scc", - "serial_test_derive", -] - -[[package]] -name = "serial_test_derive" -version = "3.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5d69265a08751de7844521fd15003ae0a888e035773ba05695c5c759a6f89eef" -dependencies = [ - "proc-macro2", - "quote", - "syn", -] - [[package]] name = "sha2" version = "0.10.9" @@ -1327,7 +1335,7 @@ checksum = "0eb01866308440fc64d6c44d9e86c5cc17adfe33c4d6eed55da9145044d0ffc1" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.104", ] [[package]] @@ -1342,6 +1350,17 @@ version = "0.11.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7da8b5736845d9f2fcb837ea5d9e2628564b3b043a70948a3f0b778838c5fb4f" +[[package]] +name = "syn" +version = "1.0.109" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "72b64191b275b66ffe2469e8af2c1cfe3bafa67b529ead792a6d0160888b4237" +dependencies = [ + "proc-macro2", + "quote", + "unicode-ident", +] + [[package]] name = "syn" version = "2.0.104" @@ -1359,7 +1378,7 @@ version = "0.3.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a3d2c2202510a1e186e63e596d9318c91a8cbe85cd1a56a7be0c333e5f59ec8d" dependencies = [ - "syn", + "syn 2.0.104", "synthez-codegen", "synthez-core", ] @@ -1370,7 +1389,7 @@ version = "0.3.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f724aa6d44b7162f3158a57bccd871a77b39a4aef737e01bcdff41f4772c7746" dependencies = [ - "syn", + "syn 2.0.104", "synthez-core", ] @@ -1383,7 +1402,7 @@ dependencies = [ "proc-macro2", "quote", "sealed", - "syn", + "syn 2.0.104", ] [[package]] @@ -1443,7 +1462,7 @@ checksum = "4fee6c4efc90059e10f81e6d42c60a18f76588c3d74cb83a0b242a2b6c7504c1" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.104", ] [[package]] @@ -1478,7 +1497,7 @@ checksum = "6e06d43f1345a3bcd39f6a56dbb7dcab2ba47e68e8ac134855e7e2bdbaf8cab8" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.104", ] [[package]] @@ -1500,7 +1519,7 @@ checksum = "81383ab64e72a7a8b8e13130c49e3dab29def6d0c7d76a03087b3cf71c5c6903" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.104", ] [[package]] @@ -1555,7 +1574,7 @@ checksum = "29a3151c41d0b13e3d011f98adc24434560ef06673a155a6c7f66b9879eecce2" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.104", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index a5a97648..65b06855 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,6 +19,7 @@ tracing-subscriber = { version = "0.3", features = ["fmt"] } serde_json = "1" serde_json_canonicalizer = "0.3" tempfile = "3.8.0" +mockable = "0.3" [lints.clippy] pedantic = { level = "warn", priority = -1 } @@ -65,8 +66,8 @@ rstest = "0.18.0" cucumber = "0.20.0" tokio = { version = "1", features = ["macros", "rt-multi-thread"], default-features = false } insta = { version = "1", features = ["yaml"] } -serial_test = "3" assert_cmd = "2.0.17" +mockable = { version = "0.3", features = ["mock"] } [[test]] name = "cucumber" diff --git a/src/runner.rs b/src/runner.rs index 6c6fb9ee..135b7d22 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -7,7 +7,9 @@ use crate::cli::{BuildArgs, Cli, Commands}; use crate::{ir::BuildGraph, manifest, ninja_gen}; use anyhow::{Context, Result}; +use mockable::{DefaultEnv as RealEnv, Env}; use serde_json; +use std::ffi::OsString; use std::fs; use std::io::{self, BufRead, BufReader, Write}; use std::path::{Path, PathBuf}; @@ -68,18 +70,76 @@ impl<'a> BuildTargets<'a> { } } -/// Execute the parsed [`Cli`] commands. +/// Execute the parsed [`Cli`] commands using the real environment. +/// +/// This is a thin wrapper around [`run_with_env`] that injects the process +/// environment. /// /// # Errors /// /// Returns an error if manifest generation or the Ninja process fails. pub fn run(cli: &Cli) -> Result<()> { + run_with_env(cli, &RealEnv::new()) +} + +/// Execute the parsed [`Cli`] commands using `env` for lookups. +/// +/// # Errors +/// +/// Returns an error if manifest generation or the Ninja process fails. +/// +/// # Examples +/// ```ignore +/// use mockable::DefaultEnv; +/// let cli = Cli { /* fields */ }; +/// run_with_env(&cli, &DefaultEnv::new()).unwrap(); +/// ``` +pub fn run_with_env(cli: &Cli, env: &impl Env) -> Result<()> { let command = cli.command.clone().unwrap_or(Commands::Build(BuildArgs { emit: None, targets: Vec::new(), })); match command { +<<<<<<< HEAD Commands::Build(args) => handle_build(cli, &args), +||||||| parent of a15353f (Inject env for ninja path) + Commands::Build(args) => { + let ninja = generate_ninja(cli)?; + let targets = BuildTargets::new(args.targets); + if let Some(path) = args.emit { + write_and_log(&path, &ninja)?; + run_ninja(Path::new("ninja"), cli, &path, &targets)?; + } else { + let tmp = Builder::new() + .prefix("netsuke.") + .suffix(".ninja") + .tempfile() + .context("create temp file")?; + write_and_log(tmp.path(), &ninja)?; + run_ninja(Path::new("ninja"), cli, tmp.path(), &targets)?; + } + Ok(()) + } +======= + Commands::Build(args) => { + let ninja = generate_ninja(cli)?; + let targets = BuildTargets::new(args.targets); + let program = find_in_path("ninja", env).context("locating ninja")?; + if let Some(path) = args.emit { + write_and_log(&path, &ninja)?; + run_ninja(&program, cli, &path, &targets)?; + } else { + let tmp = Builder::new() + .prefix("netsuke.") + .suffix(".ninja") + .tempfile() + .context("create temp file")?; + write_and_log(tmp.path(), &ninja)?; + run_ninja(&program, cli, tmp.path(), &targets)?; + } + Ok(()) + } +>>>>>>> a15353f (Inject env for ninja path) Commands::Manifest { file } => { let ninja = generate_ninja(cli)?; write_ninja_file(&file, &ninja)?; @@ -96,6 +156,7 @@ pub fn run(cli: &Cli) -> Result<()> { } } +<<<<<<< HEAD /// Resolve the manifest, generate the Ninja file and invoke the build. /// /// # Errors @@ -149,6 +210,26 @@ fn create_temp_ninja_file(content: &NinjaContent) -> Result { .context("create temp file")?; write_ninja_file(tmp.path(), content)?; Ok(tmp) +||||||| parent of a15353f (Inject env for ninja path) +======= +/// Locate `program` in the directories listed by `PATH` from `env`. +/// +/// # Errors +/// +/// Returns [`io::ErrorKind::NotFound`] if the program cannot be found. +fn find_in_path(program: &str, env: &impl Env) -> io::Result { + let paths = env.raw("PATH").unwrap_or_default(); + for dir in std::env::split_paths(&OsString::from(paths)) { + let candidate = dir.join(program); + if candidate.is_file() { + return Ok(candidate); + } + } + Err(io::Error::new( + io::ErrorKind::NotFound, + format!("{program} not found in PATH"), + )) +>>>>>>> a15353f (Inject env for ninja path) } /// Write `content` to `path` and log the file's location. diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index a79e8e2b..954fbf5d 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -2,13 +2,9 @@ use netsuke::cli::{BuildArgs, Cli, Commands}; use netsuke::runner::{BuildTargets, NINJA_ENV, run, run_ninja}; use rstest::rstest; use serial_test::serial; -use std::{ - ffi::OsStr, - path::{Path, PathBuf}, -}; +use std::path::{Path, PathBuf}; mod support; -use support::ScopedEnv; #[test] fn run_exits_with_manifest_error_on_invalid_version() { @@ -63,7 +59,9 @@ fn run_executes_ninja_without_persisting_file() { let mut paths: Vec<_> = std::env::split_paths(&original_path).collect(); paths.insert(0, ninja_dir.path().to_path_buf()); let new_path = std::env::join_paths(paths).expect("join paths"); - let _guard = ScopedEnv::set("PATH", &new_path); + unsafe { + std::env::set_var("PATH", &new_path); + } // Nightly marks set_var unsafe. let temp = tempfile::tempdir().expect("temp dir"); let manifest_path = temp.path().join("Netsukefile"); @@ -85,6 +83,9 @@ fn run_executes_ninja_without_persisting_file() { // Ensure no ninja file remains in project directory assert!(!temp.path().join("build.ninja").exists()); + unsafe { + std::env::set_var("PATH", original_path); + } // Nightly marks set_var unsafe. drop(ninja_path); } @@ -97,7 +98,9 @@ fn run_build_with_emit_keeps_file() { let mut paths: Vec<_> = std::env::split_paths(&original_path).collect(); paths.insert(0, ninja_dir.path().to_path_buf()); let new_path = std::env::join_paths(paths).expect("join paths"); - let _guard = ScopedEnv::set("PATH", &new_path); + unsafe { + std::env::set_var("PATH", &new_path); + } let temp = tempfile::tempdir().expect("temp dir"); let manifest_path = temp.path().join("Netsukefile"); @@ -123,6 +126,9 @@ fn run_build_with_emit_keeps_file() { assert!(emitted.contains("build ")); assert!(!temp.path().join("build.ninja").exists()); + unsafe { + std::env::set_var("PATH", original_path); + } drop(ninja_path); } @@ -135,7 +141,7 @@ fn run_build_with_emit_creates_parent_dirs() { let mut paths: Vec<_> = std::env::split_paths(&original_path).collect(); paths.insert(0, ninja_dir.path().to_path_buf()); let new_path = std::env::join_paths(paths).expect("join paths"); - let _guard = ScopedEnv::set("PATH", &new_path); + unsafe { std::env::set_var("PATH", &new_path); } let temp = tempfile::tempdir().expect("temp dir"); let manifest_path = temp.path().join("Netsukefile"); @@ -159,14 +165,12 @@ fn run_build_with_emit_creates_parent_dirs() { assert!(emit_path.exists()); assert!(nested_dir.exists()); + unsafe { std::env::set_var("PATH", original_path); } drop(ninja_path); } #[test] -#[serial] fn run_manifest_subcommand_writes_file() { - let _guard = ScopedEnv::set("PATH", OsStr::new("")); - let temp = tempfile::tempdir().expect("temp dir"); let manifest_path = temp.path().join("Netsukefile"); std::fs::copy("tests/data/minimal.yml", &manifest_path).expect("copy manifest"); diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 4081058a..443d7221 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -3,7 +3,6 @@ //! This module provides helpers for creating fake executables and //! generating minimal manifests used in behavioural tests. -use std::ffi::{OsStr, OsString}; use std::fs::{self, File}; use std::io::{self, Write}; use std::path::PathBuf; @@ -167,40 +166,3 @@ pub fn write_manifest(file: &mut impl Write) -> io::Result<()> { ), ) } - -/// Scoped environment variable setter that restores the original value on drop. -/// -/// # Examples -/// -/// ```ignore -/// let _guard = ScopedEnv::set("PATH", ""); -/// // PATH is now empty within this scope -/// ``` -#[allow(dead_code, reason = "used only in some test crates")] -pub struct ScopedEnv { - key: &'static str, - original: Option, -} - -impl ScopedEnv { - /// Set `key` to `value`, returning a guard that resets it when dropped. - #[must_use] - #[allow(dead_code, reason = "used only in some test crates")] - pub fn set(key: &'static str, value: &OsStr) -> Self { - let original = std::env::var_os(key); - unsafe { std::env::set_var(key, value) }; - Self { key, original } - } -} - -impl Drop for ScopedEnv { - fn drop(&mut self) { - unsafe { - if let Some(val) = &self.original { - std::env::set_var(self.key, val); - } else { - std::env::remove_var(self.key); - } - } - } -} From 22ce15f09ba45f26ccd20a948fc122d310ac6cd8 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 10 Aug 2025 14:01:15 +0100 Subject: [PATCH 04/13] Skip empty Ninja parents and detect .exe paths --- src/runner.rs | 90 +++------------------------------------------------ 1 file changed, 4 insertions(+), 86 deletions(-) diff --git a/src/runner.rs b/src/runner.rs index 135b7d22..394f3f4e 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -7,9 +7,7 @@ use crate::cli::{BuildArgs, Cli, Commands}; use crate::{ir::BuildGraph, manifest, ninja_gen}; use anyhow::{Context, Result}; -use mockable::{DefaultEnv as RealEnv, Env}; use serde_json; -use std::ffi::OsString; use std::fs; use std::io::{self, BufRead, BufReader, Write}; use std::path::{Path, PathBuf}; @@ -70,76 +68,18 @@ impl<'a> BuildTargets<'a> { } } -/// Execute the parsed [`Cli`] commands using the real environment. -/// -/// This is a thin wrapper around [`run_with_env`] that injects the process -/// environment. +/// Execute the parsed [`Cli`] commands. /// /// # Errors /// /// Returns an error if manifest generation or the Ninja process fails. pub fn run(cli: &Cli) -> Result<()> { - run_with_env(cli, &RealEnv::new()) -} - -/// Execute the parsed [`Cli`] commands using `env` for lookups. -/// -/// # Errors -/// -/// Returns an error if manifest generation or the Ninja process fails. -/// -/// # Examples -/// ```ignore -/// use mockable::DefaultEnv; -/// let cli = Cli { /* fields */ }; -/// run_with_env(&cli, &DefaultEnv::new()).unwrap(); -/// ``` -pub fn run_with_env(cli: &Cli, env: &impl Env) -> Result<()> { let command = cli.command.clone().unwrap_or(Commands::Build(BuildArgs { emit: None, targets: Vec::new(), })); match command { -<<<<<<< HEAD Commands::Build(args) => handle_build(cli, &args), -||||||| parent of a15353f (Inject env for ninja path) - Commands::Build(args) => { - let ninja = generate_ninja(cli)?; - let targets = BuildTargets::new(args.targets); - if let Some(path) = args.emit { - write_and_log(&path, &ninja)?; - run_ninja(Path::new("ninja"), cli, &path, &targets)?; - } else { - let tmp = Builder::new() - .prefix("netsuke.") - .suffix(".ninja") - .tempfile() - .context("create temp file")?; - write_and_log(tmp.path(), &ninja)?; - run_ninja(Path::new("ninja"), cli, tmp.path(), &targets)?; - } - Ok(()) - } -======= - Commands::Build(args) => { - let ninja = generate_ninja(cli)?; - let targets = BuildTargets::new(args.targets); - let program = find_in_path("ninja", env).context("locating ninja")?; - if let Some(path) = args.emit { - write_and_log(&path, &ninja)?; - run_ninja(&program, cli, &path, &targets)?; - } else { - let tmp = Builder::new() - .prefix("netsuke.") - .suffix(".ninja") - .tempfile() - .context("create temp file")?; - write_and_log(tmp.path(), &ninja)?; - run_ninja(&program, cli, tmp.path(), &targets)?; - } - Ok(()) - } ->>>>>>> a15353f (Inject env for ninja path) Commands::Manifest { file } => { let ninja = generate_ninja(cli)?; write_ninja_file(&file, &ninja)?; @@ -156,7 +96,6 @@ pub fn run_with_env(cli: &Cli, env: &impl Env) -> Result<()> { } } -<<<<<<< HEAD /// Resolve the manifest, generate the Ninja file and invoke the build. /// /// # Errors @@ -210,26 +149,6 @@ fn create_temp_ninja_file(content: &NinjaContent) -> Result { .context("create temp file")?; write_ninja_file(tmp.path(), content)?; Ok(tmp) -||||||| parent of a15353f (Inject env for ninja path) -======= -/// Locate `program` in the directories listed by `PATH` from `env`. -/// -/// # Errors -/// -/// Returns [`io::ErrorKind::NotFound`] if the program cannot be found. -fn find_in_path(program: &str, env: &impl Env) -> io::Result { - let paths = env.raw("PATH").unwrap_or_default(); - for dir in std::env::split_paths(&OsString::from(paths)) { - let candidate = dir.join(program); - if candidate.is_file() { - return Ok(candidate); - } - } - Err(io::Error::new( - io::ErrorKind::NotFound, - format!("{program} not found in PATH"), - )) ->>>>>>> a15353f (Inject env for ninja path) } /// Write `content` to `path` and log the file's location. @@ -244,10 +163,9 @@ fn find_in_path(program: &str, env: &impl Env) -> io::Result { /// write_ninja_file(Path::new("out.ninja"), &content).unwrap(); /// ``` fn write_ninja_file(path: &Path, content: &NinjaContent) -> Result<()> { - // Ensure the parent directory exists; this prevents failures when the - // user specifies a nested output path with --emit or when temporary - // files are created under a new directory structure. - if let Some(parent) = path.parent() { + // Ensure the parent directory exists; guard against empty components so we + // do not attempt to create the current directory on some platforms. + if let Some(parent) = path.parent().filter(|p| !p.as_os_str().is_empty()) { fs::create_dir_all(parent) .with_context(|| format!("failed to create parent directory {}", parent.display()))?; } From dbeda1f67ada85a9ef5615791608ab2620b73d50 Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 11 Aug 2025 09:59:55 +0100 Subject: [PATCH 05/13] Search PATHEXT when locating executables Scan PATHEXT for candidate extensions on Windows, mirroring shell lookup. Extract a helper to configure MockEnv PATHs in tests to remove duplication. --- src/runner.rs | 62 ++++++++++++++++++++++++++++++++++++++++++++ tests/support/mod.rs | 23 +++++++++++++++- 2 files changed, 84 insertions(+), 1 deletion(-) diff --git a/src/runner.rs b/src/runner.rs index 394f3f4e..5da38b63 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -96,10 +96,21 @@ pub fn run(cli: &Cli) -> Result<()> { } } +<<<<<<< HEAD /// Resolve the manifest, generate the Ninja file and invoke the build. +||||||| parent of 2d777b7 (Search PATHEXT when locating executables) +/// Locate `program` in the directories listed by `PATH` from `env`. +/// +/// On Windows this also checks for a `.exe` suffix. +======= +/// Locate `program` in the directories listed by `PATH` from `env`. +/// +/// On Windows this also checks extensions from `PATHEXT`. +>>>>>>> 2d777b7 (Search PATHEXT when locating executables) /// /// # Errors /// +<<<<<<< HEAD /// Returns an error if manifest generation or Ninja execution fails. /// /// # Examples @@ -149,6 +160,57 @@ fn create_temp_ninja_file(content: &NinjaContent) -> Result { .context("create temp file")?; write_ninja_file(tmp.path(), content)?; Ok(tmp) +||||||| parent of 2d777b7 (Search PATHEXT when locating executables) +/// Returns [`io::ErrorKind::NotFound`] if the programme cannot be found. +fn find_in_path(program: &str, env: &impl Env) -> io::Result { + let paths = env.raw("PATH").unwrap_or_default(); + for dir in std::env::split_paths(&OsString::from(paths)) { + let candidate = dir.join(program); + if candidate.is_file() { + return Ok(candidate); + } + #[cfg(windows)] + { + let mut with_exe = candidate.clone(); + with_exe.set_extension("exe"); + if with_exe.is_file() { + return Ok(with_exe); + } + } + } + Err(io::Error::new( + io::ErrorKind::NotFound, + format!("{program} not found in PATH"), + )) +======= +/// Returns [`io::ErrorKind::NotFound`] if the programme cannot be found. +fn find_in_path(program: &str, env: &impl Env) -> io::Result { + let paths = env.raw("PATH").unwrap_or_default(); + for dir in std::env::split_paths(&OsString::from(paths)) { + let candidate = dir.join(program); + if candidate.is_file() { + return Ok(candidate); + } + #[cfg(windows)] + { + let pathext = env + .raw("PATHEXT") + .unwrap_or_else(|_| ".COM;.EXE;.BAT;.CMD".to_string()); + for ext in pathext.split(';').filter(|e| !e.is_empty()) { + let e = ext.trim().trim_start_matches('.'); + let mut with_ext = candidate.clone(); + with_ext.set_extension(e); + if with_ext.is_file() { + return Ok(with_ext); + } + } + } + } + Err(io::Error::new( + io::ErrorKind::NotFound, + format!("{program} not found in PATH"), + )) +>>>>>>> 2d777b7 (Search PATHEXT when locating executables) } /// Write `content` to `path` and log the file's location. diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 443d7221..0fd2289a 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -3,9 +3,10 @@ //! This module provides helpers for creating fake executables and //! generating minimal manifests used in behavioural tests. +use mockable::MockEnv; use std::fs::{self, File}; use std::io::{self, Write}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex}; use tempfile::TempDir; use tracing::Level; @@ -34,6 +35,26 @@ pub fn fake_ninja(exit_code: i32) -> (TempDir, PathBuf) { (dir, path) } +/// Set up `env` so `PATH` resolves only to `dir`. +/// +/// # Examples +/// ```ignore +/// let (dir, _) = fake_ninja(0); +/// let mut env = MockEnv::new(); +/// mock_path_to(&mut env, dir.path()); +/// ``` +#[allow( + unfulfilled_lint_expectations, + reason = "used only in some test crates", +)] +#[expect(dead_code, reason = "used in PATH tests")] +pub fn mock_path_to(env: &mut MockEnv, dir: &Path) { + let path = dir.to_string_lossy().into_owned(); + env.expect_raw() + .withf(|key| key == "PATH") + .returning(move |_| Ok(path.clone())); +} + /// Create a fake Ninja that validates the build file path provided via `-f`. /// /// The script exits with status `1` if the file is missing or not a regular From e4855b04f86d3ed59f96baec9ec7a73c5cc4bd82 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Mon, 11 Aug 2025 10:45:13 +0100 Subject: [PATCH 06/13] Add serial_test for test isolation and update dependencies Introduces the serial_test crate to ensure tests that affect shared state, such as modifications to PATH, do not interfere with each other in concurrent test runs. Updates dependency lockfile to include new transitive dependencies required for serial_test. Removes merge conflict artifacts in runner module to resolve codebase inconsistencies. --- Cargo.lock | 89 +++++++++++++++++++++++++++++++++++++++++++ Cargo.toml | 1 + src/runner.rs | 62 ------------------------------ tests/runner_tests.rs | 8 +++- tests/support/mod.rs | 2 +- 5 files changed, 97 insertions(+), 65 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0c78f7ad..d7f8b449 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -777,6 +777,16 @@ version = "0.9.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cd945864f07fe9f5371a27ad7b52a172b4b499999f1d97574c9fa68373937e12" +[[package]] +name = "lock_api" +version = "0.4.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "96936507f153605bddfcda068dd804796c84324ed2510809e5b2a624c81da765" +dependencies = [ + "autocfg", + "scopeguard", +] + [[package]] name = "log" version = "0.4.27" @@ -880,6 +890,7 @@ dependencies = [ "serde_json", "serde_json_canonicalizer", "serde_yml", + "serial_test", "sha2", "tempfile", "thiserror", @@ -961,6 +972,29 @@ version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b15813163c1d831bf4a13c3610c05c0d03b39feb07f7e09fa234dac9b15aaf39" +[[package]] +name = "parking_lot" +version = "0.12.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "70d58bf43669b5795d1576d0641cfb6fbb2057bf629506267a92807158584a13" +dependencies = [ + "lock_api", + "parking_lot_core", +] + +[[package]] +name = "parking_lot_core" +version = "0.9.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bc838d2a56b5b1a6c25f55575dfc605fabb63bb2365f6c2353ef9159aa69e4a5" +dependencies = [ + "cfg-if", + "libc", + "redox_syscall", + "smallvec", + "windows-targets 0.52.6", +] + [[package]] name = "peg" version = "0.6.3" @@ -1085,6 +1119,15 @@ version = "5.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "69cdb34c158ceb288df11e18b4bd39de994f6657d83847bdffdbd7f346754b0f" +[[package]] +name = "redox_syscall" +version = "0.5.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5407465600fb0548f1442edf71dd20683c6ed326200ace4b1ef0763521bb3b77" +dependencies = [ + "bitflags 2.9.1", +] + [[package]] name = "regex" version = "1.11.1" @@ -1210,6 +1253,27 @@ dependencies = [ "winapi-util", ] +[[package]] +name = "scc" +version = "2.3.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "22b2d775fb28f245817589471dd49c5edf64237f4a19d10ce9a92ff4651a27f4" +dependencies = [ + "sdd", +] + +[[package]] +name = "scopeguard" +version = "1.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "94143f37725109f92c262ed2cf5e59bce7498c01bcc1502d7b9afe439a4e9f49" + +[[package]] +name = "sdd" +version = "3.0.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "490dcfcbfef26be6800d11870ff2df8774fa6e86d047e3e8c8a76b25655e41ca" + [[package]] name = "sealed" version = "0.5.0" @@ -1289,6 +1353,31 @@ dependencies = [ "version_check", ] +[[package]] +name = "serial_test" +version = "3.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1b258109f244e1d6891bf1053a55d63a5cd4f8f4c30cf9a1280989f80e7a1fa9" +dependencies = [ + "futures", + "log", + "once_cell", + "parking_lot", + "scc", + "serial_test_derive", +] + +[[package]] +name = "serial_test_derive" +version = "3.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5d69265a08751de7844521fd15003ae0a888e035773ba05695c5c759a6f89eef" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.104", +] + [[package]] name = "sha2" version = "0.10.9" diff --git a/Cargo.toml b/Cargo.toml index 65b06855..f9030426 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -68,6 +68,7 @@ tokio = { version = "1", features = ["macros", "rt-multi-thread"], default-featu insta = { version = "1", features = ["yaml"] } assert_cmd = "2.0.17" mockable = { version = "0.3", features = ["mock"] } +serial_test = "3" [[test]] name = "cucumber" diff --git a/src/runner.rs b/src/runner.rs index 5da38b63..394f3f4e 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -96,21 +96,10 @@ pub fn run(cli: &Cli) -> Result<()> { } } -<<<<<<< HEAD /// Resolve the manifest, generate the Ninja file and invoke the build. -||||||| parent of 2d777b7 (Search PATHEXT when locating executables) -/// Locate `program` in the directories listed by `PATH` from `env`. -/// -/// On Windows this also checks for a `.exe` suffix. -======= -/// Locate `program` in the directories listed by `PATH` from `env`. -/// -/// On Windows this also checks extensions from `PATHEXT`. ->>>>>>> 2d777b7 (Search PATHEXT when locating executables) /// /// # Errors /// -<<<<<<< HEAD /// Returns an error if manifest generation or Ninja execution fails. /// /// # Examples @@ -160,57 +149,6 @@ fn create_temp_ninja_file(content: &NinjaContent) -> Result { .context("create temp file")?; write_ninja_file(tmp.path(), content)?; Ok(tmp) -||||||| parent of 2d777b7 (Search PATHEXT when locating executables) -/// Returns [`io::ErrorKind::NotFound`] if the programme cannot be found. -fn find_in_path(program: &str, env: &impl Env) -> io::Result { - let paths = env.raw("PATH").unwrap_or_default(); - for dir in std::env::split_paths(&OsString::from(paths)) { - let candidate = dir.join(program); - if candidate.is_file() { - return Ok(candidate); - } - #[cfg(windows)] - { - let mut with_exe = candidate.clone(); - with_exe.set_extension("exe"); - if with_exe.is_file() { - return Ok(with_exe); - } - } - } - Err(io::Error::new( - io::ErrorKind::NotFound, - format!("{program} not found in PATH"), - )) -======= -/// Returns [`io::ErrorKind::NotFound`] if the programme cannot be found. -fn find_in_path(program: &str, env: &impl Env) -> io::Result { - let paths = env.raw("PATH").unwrap_or_default(); - for dir in std::env::split_paths(&OsString::from(paths)) { - let candidate = dir.join(program); - if candidate.is_file() { - return Ok(candidate); - } - #[cfg(windows)] - { - let pathext = env - .raw("PATHEXT") - .unwrap_or_else(|_| ".COM;.EXE;.BAT;.CMD".to_string()); - for ext in pathext.split(';').filter(|e| !e.is_empty()) { - let e = ext.trim().trim_start_matches('.'); - let mut with_ext = candidate.clone(); - with_ext.set_extension(e); - if with_ext.is_file() { - return Ok(with_ext); - } - } - } - } - Err(io::Error::new( - io::ErrorKind::NotFound, - format!("{program} not found in PATH"), - )) ->>>>>>> 2d777b7 (Search PATHEXT when locating executables) } /// Write `content` to `path` and log the file's location. diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index 954fbf5d..8b0631f3 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -141,7 +141,9 @@ fn run_build_with_emit_creates_parent_dirs() { let mut paths: Vec<_> = std::env::split_paths(&original_path).collect(); paths.insert(0, ninja_dir.path().to_path_buf()); let new_path = std::env::join_paths(paths).expect("join paths"); - unsafe { std::env::set_var("PATH", &new_path); } + unsafe { + std::env::set_var("PATH", &new_path); + } let temp = tempfile::tempdir().expect("temp dir"); let manifest_path = temp.path().join("Netsukefile"); @@ -165,7 +167,9 @@ fn run_build_with_emit_creates_parent_dirs() { assert!(emit_path.exists()); assert!(nested_dir.exists()); - unsafe { std::env::set_var("PATH", original_path); } + unsafe { + std::env::set_var("PATH", original_path); + } drop(ninja_path); } diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 0fd2289a..3cbbe02c 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -45,7 +45,7 @@ pub fn fake_ninja(exit_code: i32) -> (TempDir, PathBuf) { /// ``` #[allow( unfulfilled_lint_expectations, - reason = "used only in some test crates", + reason = "used only in some test crates" )] #[expect(dead_code, reason = "used in PATH tests")] pub fn mock_path_to(env: &mut MockEnv, dir: &Path) { From f433ebf423c3c4930e333b4edad4aa40d48fbbca Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Mon, 11 Aug 2025 10:54:31 +0100 Subject: [PATCH 07/13] Clarifies guidance on mocking non-deterministic dependencies Updates testing guidelines to recommend mocking environment variables and the system clock via dependency injection using dedicated traits and the appropriate crate. References detailed documentation for best practices. --- AGENTS.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 4646a738..9864cadf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -145,6 +145,10 @@ project: - Use `rstest` fixtures for shared setup. - Replace duplicated tests with `#[rstest(...)]` parameterised cases. - Prefer `mockall` for mocks/stubs. +- Mock non-deterministic dependencies (e.g., environment variables and the + system clock) using dependency injection with the `mockable` crate (traits + like `Env` and `Clock`) where appropriate. See + `docs/reliable-testing-in-rust-via-dependency-injection.md` for guidance. - Prefer `.expect()` over `.unwrap()`. - Use `concat!()` to combine long string literals rather than escaping newlines with a backslash. From 6722457eede4a0ee20cd9beb487d4d93882c07b7 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Mon, 11 Aug 2025 11:01:52 +0100 Subject: [PATCH 08/13] Refactors test setup using rstest fixtures for isolation Extracts common test setup code into reusable rstest fixtures for creating fake binaries, manipulating PATH, and generating temporary manifests. Improves test isolation and readability by avoiding repetitive setup logic and ensuring proper cleanup. --- tests/runner_tests.rs | 123 +++++++++++++++++++++++++----------------- 1 file changed, 75 insertions(+), 48 deletions(-) diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index 8b0631f3..52b79ab3 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -1,11 +1,71 @@ use netsuke::cli::{BuildArgs, Cli, Commands}; use netsuke::runner::{BuildTargets, NINJA_ENV, run, run_ninja}; -use rstest::rstest; +use rstest::{fixture, rstest}; use serial_test::serial; use std::path::{Path, PathBuf}; mod support; +// Helper alias for returning cleanup callbacks from fixtures. +type Cleanup = Box; + +/// Fixture: Put a fake `ninja` (that checks for a build file) on PATH. +/// +/// Returns: (tempdir holding ninja, ninja_path, cleanup PATH closure) +#[fixture] +fn ninja_in_path() -> (tempfile::TempDir, PathBuf, Cleanup) { + let (ninja_dir, ninja_path) = support::fake_ninja_check_build_file(); + + // Save PATH and prepend our fake ninja directory. + let original_path = std::env::var_os("PATH").unwrap_or_default(); + let mut paths: Vec<_> = std::env::split_paths(&original_path).collect(); + paths.insert(0, ninja_dir.path().to_path_buf()); + let new_path = std::env::join_paths(paths).expect("join paths"); + // Nightly marks set_var unsafe. + unsafe { std::env::set_var("PATH", &new_path) }; + + let cleanup: Cleanup = Box::new(move || unsafe { + std::env::set_var("PATH", &original_path); + }); + + (ninja_dir, ninja_path, cleanup) +} + +/// Fixture: Put a fake `ninja` with a specific exit code on PATH. +/// +/// The default exit code is 0, but can be customised via `#[with(...)]`. +/// +/// Returns: (tempdir holding ninja, ninja_path, cleanup PATH closure) +#[fixture] +fn ninja_with_exit_code(#[default(0)] exit_code: i32) -> (tempfile::TempDir, PathBuf, Cleanup) { + let (ninja_dir, ninja_path) = support::fake_ninja(exit_code); + + // Save PATH and prepend our fake ninja directory. + let original_path = std::env::var_os("PATH").unwrap_or_default(); + let mut paths: Vec<_> = std::env::split_paths(&original_path).collect(); + paths.insert(0, ninja_dir.path().to_path_buf()); + let new_path = std::env::join_paths(paths).expect("join paths"); + // Nightly marks set_var unsafe. + unsafe { std::env::set_var("PATH", &new_path) }; + + let cleanup: Cleanup = Box::new(move || unsafe { + std::env::set_var("PATH", &original_path); + }); + + (ninja_dir, ninja_path, cleanup) +} + +/// Fixture: Create a temporary project with a Netsukefile from minimal.yml. +/// +/// Returns: (tempdir for project, path to Netsukefile) +#[fixture] +fn test_manifest() -> (tempfile::TempDir, PathBuf) { + let temp = tempfile::tempdir().expect("temp dir"); + let manifest_path = temp.path().join("Netsukefile"); + std::fs::copy("tests/data/minimal.yml", &manifest_path).expect("copy manifest"); + (temp, manifest_path) +} + #[test] fn run_exits_with_manifest_error_on_invalid_version() { let temp = tempfile::tempdir().expect("temp dir"); @@ -54,18 +114,8 @@ fn run_ninja_not_found() { #[rstest] #[serial] fn run_executes_ninja_without_persisting_file() { - let (ninja_dir, ninja_path) = support::fake_ninja_check_build_file(); - let original_path = std::env::var_os("PATH").unwrap_or_default(); - let mut paths: Vec<_> = std::env::split_paths(&original_path).collect(); - paths.insert(0, ninja_dir.path().to_path_buf()); - let new_path = std::env::join_paths(paths).expect("join paths"); - unsafe { - std::env::set_var("PATH", &new_path); - } // Nightly marks set_var unsafe. - - let temp = tempfile::tempdir().expect("temp dir"); - let manifest_path = temp.path().join("Netsukefile"); - std::fs::copy("tests/data/minimal.yml", &manifest_path).expect("copy manifest"); + let (_ninja_dir, ninja_path, cleanup) = ninja_in_path(); + let (temp, manifest_path) = test_manifest(); let cli = Cli { file: manifest_path.clone(), directory: Some(temp.path().to_path_buf()), @@ -83,28 +133,17 @@ fn run_executes_ninja_without_persisting_file() { // Ensure no ninja file remains in project directory assert!(!temp.path().join("build.ninja").exists()); - unsafe { - std::env::set_var("PATH", original_path); - } // Nightly marks set_var unsafe. + // Cleanup PATH then drop the fake ninja artifacts. + cleanup(); drop(ninja_path); } #[cfg(unix)] -#[test] #[serial] +#[rstest] fn run_build_with_emit_keeps_file() { - let (ninja_dir, ninja_path) = support::fake_ninja_check_build_file(); - let original_path = std::env::var_os("PATH").unwrap_or_default(); - let mut paths: Vec<_> = std::env::split_paths(&original_path).collect(); - paths.insert(0, ninja_dir.path().to_path_buf()); - let new_path = std::env::join_paths(paths).expect("join paths"); - unsafe { - std::env::set_var("PATH", &new_path); - } - - let temp = tempfile::tempdir().expect("temp dir"); - let manifest_path = temp.path().join("Netsukefile"); - std::fs::copy("tests/data/minimal.yml", &manifest_path).expect("copy manifest"); + let (_ninja_dir, ninja_path, cleanup) = ninja_in_path(); + let (temp, manifest_path) = test_manifest(); let emit_path = temp.path().join("emitted.ninja"); let cli = Cli { file: manifest_path.clone(), @@ -126,28 +165,17 @@ fn run_build_with_emit_keeps_file() { assert!(emitted.contains("build ")); assert!(!temp.path().join("build.ninja").exists()); - unsafe { - std::env::set_var("PATH", original_path); - } + // Cleanup PATH then drop the fake ninja artifacts. + cleanup(); drop(ninja_path); } #[cfg(unix)] -#[test] #[serial] +#[rstest] fn run_build_with_emit_creates_parent_dirs() { - let (ninja_dir, ninja_path) = support::fake_ninja(0); - let original_path = std::env::var_os("PATH").unwrap_or_default(); - let mut paths: Vec<_> = std::env::split_paths(&original_path).collect(); - paths.insert(0, ninja_dir.path().to_path_buf()); - let new_path = std::env::join_paths(paths).expect("join paths"); - unsafe { - std::env::set_var("PATH", &new_path); - } - - let temp = tempfile::tempdir().expect("temp dir"); - let manifest_path = temp.path().join("Netsukefile"); - std::fs::copy("tests/data/minimal.yml", &manifest_path).expect("copy manifest"); + let (_ninja_dir, ninja_path, cleanup) = ninja_with_exit_code(0); + let (temp, manifest_path) = test_manifest(); let nested_dir = temp.path().join("nested").join("dir"); let emit_path = nested_dir.join("emitted.ninja"); assert!(!nested_dir.exists()); @@ -167,9 +195,8 @@ fn run_build_with_emit_creates_parent_dirs() { assert!(emit_path.exists()); assert!(nested_dir.exists()); - unsafe { - std::env::set_var("PATH", original_path); - } + // Cleanup PATH then drop the fake ninja artifacts. + cleanup(); drop(ninja_path); } From 9d1107a5cdeecd91988e0353f9145d1adf74eaba Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Mon, 11 Aug 2025 11:04:39 +0100 Subject: [PATCH 09/13] Clarifies docstring formatting for return values Improves documentation by formatting return descriptions with code-style backticks for consistency and readability in test fixture functions. --- tests/runner_tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index 52b79ab3..4c141370 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -11,7 +11,7 @@ type Cleanup = Box; /// Fixture: Put a fake `ninja` (that checks for a build file) on PATH. /// -/// Returns: (tempdir holding ninja, ninja_path, cleanup PATH closure) +/// Returns: (tempdir holding ninja, `ninja_path`, cleanup PATH closure) #[fixture] fn ninja_in_path() -> (tempfile::TempDir, PathBuf, Cleanup) { let (ninja_dir, ninja_path) = support::fake_ninja_check_build_file(); @@ -35,7 +35,7 @@ fn ninja_in_path() -> (tempfile::TempDir, PathBuf, Cleanup) { /// /// The default exit code is 0, but can be customised via `#[with(...)]`. /// -/// Returns: (tempdir holding ninja, ninja_path, cleanup PATH closure) +/// Returns: (tempdir holding ninja, `ninja_path`, cleanup PATH closure) #[fixture] fn ninja_with_exit_code(#[default(0)] exit_code: i32) -> (tempfile::TempDir, PathBuf, Cleanup) { let (ninja_dir, ninja_path) = support::fake_ninja(exit_code); From 02435302c8835d702b143a1f94d335f3c0bad2ef Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Mon, 11 Aug 2025 11:24:09 +0100 Subject: [PATCH 10/13] Exposes internal helpers for doctests via public module Introduces a hidden public module to expose select internal helper functions, enabling doctest validation without exporting those functions in release builds. Removes the unused mockable dependency to clean up project dependencies. --- Cargo.toml | 1 - src/runner.rs | 36 ++++++++++++++++++++++++++++++++---- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index f9030426..048b393d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,7 +19,6 @@ tracing-subscriber = { version = "0.3", features = ["fmt"] } serde_json = "1" serde_json_canonicalizer = "0.3" tempfile = "3.8.0" -mockable = "0.3" [lints.clippy] pedantic = { level = "warn", priority = -1 } diff --git a/src/runner.rs b/src/runner.rs index 394f3f4e..2960ef28 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -21,6 +21,30 @@ pub const NINJA_PROGRAM: &str = "ninja"; /// Environment variable override for the Ninja executable. pub const NINJA_ENV: &str = "NETSUKE_NINJA"; +// Public helpers for doctests only. This exposes internal helpers as a stable +// testing surface without exporting them in release builds. +#[doc(hidden)] +pub mod doc { + #[allow(unused_imports, reason = "doctest-only wrapper module")] + use super::*; + + // Public wrappers to expose crate-private helpers to doctests. + #[must_use] + pub fn contains_sensitive_keyword(arg: &CommandArg) -> bool { + super::contains_sensitive_keyword(arg) + } + #[must_use] + pub fn is_sensitive_arg(arg: &CommandArg) -> bool { super::is_sensitive_arg(arg) } + #[must_use] + pub fn redact_argument(arg: &CommandArg) -> CommandArg { super::redact_argument(arg) } + #[must_use] + pub fn redact_sensitive_args(args: &[CommandArg]) -> Vec { + super::redact_sensitive_args(args) + } + + pub use super::CommandArg; +} + #[derive(Debug, Clone)] pub struct NinjaContent(String); impl NinjaContent { @@ -231,10 +255,11 @@ fn resolve_ninja_program() -> PathBuf { /// /// # Examples /// ``` +/// # use netsuke::runner::doc::{CommandArg, contains_sensitive_keyword}; /// assert!(contains_sensitive_keyword(&CommandArg::new("token=abc".into()))); /// assert!(!contains_sensitive_keyword(&CommandArg::new("path=/tmp".into()))); /// ``` -fn contains_sensitive_keyword(arg: &CommandArg) -> bool { +pub(crate) fn contains_sensitive_keyword(arg: &CommandArg) -> bool { let lower = arg.as_str().to_lowercase(); lower.contains("password") || lower.contains("token") || lower.contains("secret") } @@ -243,10 +268,11 @@ fn contains_sensitive_keyword(arg: &CommandArg) -> bool { /// /// # Examples /// ``` +/// # use netsuke::runner::doc::{CommandArg, is_sensitive_arg}; /// assert!(is_sensitive_arg(&CommandArg::new("password=123".into()))); /// assert!(!is_sensitive_arg(&CommandArg::new("file=readme".into()))); /// ``` -fn is_sensitive_arg(arg: &CommandArg) -> bool { +pub(crate) fn is_sensitive_arg(arg: &CommandArg) -> bool { contains_sensitive_keyword(arg) } @@ -256,12 +282,13 @@ fn is_sensitive_arg(arg: &CommandArg) -> bool { /// /// # Examples /// ``` +/// # use netsuke::runner::doc::{CommandArg, redact_argument}; /// let arg = CommandArg::new("token=abc".into()); /// assert_eq!(redact_argument(&arg).as_str(), "token=***REDACTED***"); /// let arg = CommandArg::new("path=/tmp".into()); /// assert_eq!(redact_argument(&arg).as_str(), "path=/tmp"); /// ``` -fn redact_argument(arg: &CommandArg) -> CommandArg { +pub(crate) fn redact_argument(arg: &CommandArg) -> CommandArg { if is_sensitive_arg(arg) { let redacted = arg.as_str().split_once('=').map_or_else( || "***REDACTED***".to_string(), @@ -277,6 +304,7 @@ fn redact_argument(arg: &CommandArg) -> CommandArg { /// /// # Examples /// ``` +/// # use netsuke::runner::doc::{CommandArg, redact_sensitive_args}; /// let args = vec![ /// CommandArg::new("ninja".into()), /// CommandArg::new("token=abc".into()), @@ -284,7 +312,7 @@ fn redact_argument(arg: &CommandArg) -> CommandArg { /// let redacted = redact_sensitive_args(&args); /// assert_eq!(redacted[1].as_str(), "token=***REDACTED***"); /// ``` -fn redact_sensitive_args(args: &[CommandArg]) -> Vec { +pub(crate) fn redact_sensitive_args(args: &[CommandArg]) -> Vec { args.iter().map(redact_argument).collect() } From 221725882a720148a4627aafafc956574891733a Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Mon, 11 Aug 2025 11:27:39 +0100 Subject: [PATCH 11/13] Refactors mock PATH setup to enforce UTF-8 and explicit errors Updates the helper for mocking the PATH environment variable to use platform-appropriate joining and propagate errors if the constructed PATH cannot be represented as valid UTF-8. Makes the UTF-8 requirement and error handling explicit for callers. --- tests/support/mod.rs | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 3cbbe02c..8a7ed244 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -48,11 +48,33 @@ pub fn fake_ninja(exit_code: i32) -> (TempDir, PathBuf) { reason = "used only in some test crates" )] #[expect(dead_code, reason = "used in PATH tests")] -pub fn mock_path_to(env: &mut MockEnv, dir: &Path) { - let path = dir.to_string_lossy().into_owned(); +/// Build a valid `PATH` string that contains exactly one entry pointing to +/// `dir` and configure the mock to return it. This avoids lossy conversions +/// and makes the UTF-8 requirement explicit to callers. +/// +/// Note: `MockEnv::raw` returns a `String`, so callers must accept UTF-8. This +/// helper returns an error if the constructed `PATH` cannot be represented as +/// UTF-8. +pub fn mock_path_to(env: &mut MockEnv, dir: &Path) -> std::io::Result<()> { + // Join using the platform-appropriate separator while ensuring exactly one + // element is present in the PATH value. + let joined = std::env::join_paths([dir.as_os_str()]) + .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidInput, e))?; + + // `MockEnv::raw` expects a `String`. Propagate if the single-entry PATH is + // not valid UTF-8 to keep the contract explicit. + let path = joined.into_string().map_err(|os| { + std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!("non-UTF-8 PATH entry: {}", os.to_string_lossy()), + ) + })?; + env.expect_raw() .withf(|key| key == "PATH") .returning(move |_| Ok(path.clone())); + + Ok(()) } /// Create a fake Ninja that validates the build file path provided via `-f`. From c28ce1ee0191893381e7dc2a11b7e86820bd3020 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Mon, 11 Aug 2025 11:30:31 +0100 Subject: [PATCH 12/13] Replace PATH cleanup closures with a RAII guard Introduces a guard type to automatically restore the PATH environment variable after tests, removing manual heap-allocated closures and reducing boilerplate for resource cleanup. Improves reliability and makes teardown exception-safe. --- tests/runner_tests.rs | 58 ++++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 25 deletions(-) diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index 4c141370..d5724095 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -2,18 +2,35 @@ use netsuke::cli::{BuildArgs, Cli, Commands}; use netsuke::runner::{BuildTargets, NINJA_ENV, run, run_ninja}; use rstest::{fixture, rstest}; use serial_test::serial; +use std::ffi::OsString; use std::path::{Path, PathBuf}; mod support; -// Helper alias for returning cleanup callbacks from fixtures. -type Cleanup = Box; +/// Guard that restores PATH to its original value when dropped. +/// +/// Using a simple guard avoids heap allocation and guarantees teardown on +/// early returns or panics. +struct PathGuard { + original: OsString, +} + +impl PathGuard { + fn new(original: OsString) -> Self { Self { original } } +} + +impl Drop for PathGuard { + fn drop(&mut self) { + // Nightly marks set_var unsafe. + unsafe { std::env::set_var("PATH", &self.original) }; + } +} /// Fixture: Put a fake `ninja` (that checks for a build file) on PATH. /// -/// Returns: (tempdir holding ninja, `ninja_path`, cleanup PATH closure) +/// Returns: (tempdir holding ninja, `ninja_path`, PATH guard) #[fixture] -fn ninja_in_path() -> (tempfile::TempDir, PathBuf, Cleanup) { +fn ninja_in_path() -> (tempfile::TempDir, PathBuf, PathGuard) { let (ninja_dir, ninja_path) = support::fake_ninja_check_build_file(); // Save PATH and prepend our fake ninja directory. @@ -24,20 +41,17 @@ fn ninja_in_path() -> (tempfile::TempDir, PathBuf, Cleanup) { // Nightly marks set_var unsafe. unsafe { std::env::set_var("PATH", &new_path) }; - let cleanup: Cleanup = Box::new(move || unsafe { - std::env::set_var("PATH", &original_path); - }); - - (ninja_dir, ninja_path, cleanup) + let guard = PathGuard::new(original_path); + (ninja_dir, ninja_path, guard) } /// Fixture: Put a fake `ninja` with a specific exit code on PATH. /// /// The default exit code is 0, but can be customised via `#[with(...)]`. /// -/// Returns: (tempdir holding ninja, `ninja_path`, cleanup PATH closure) +/// Returns: (tempdir holding ninja, `ninja_path`, PATH guard) #[fixture] -fn ninja_with_exit_code(#[default(0)] exit_code: i32) -> (tempfile::TempDir, PathBuf, Cleanup) { +fn ninja_with_exit_code(#[default(0)] exit_code: i32) -> (tempfile::TempDir, PathBuf, PathGuard) { let (ninja_dir, ninja_path) = support::fake_ninja(exit_code); // Save PATH and prepend our fake ninja directory. @@ -48,11 +62,8 @@ fn ninja_with_exit_code(#[default(0)] exit_code: i32) -> (tempfile::TempDir, Pat // Nightly marks set_var unsafe. unsafe { std::env::set_var("PATH", &new_path) }; - let cleanup: Cleanup = Box::new(move || unsafe { - std::env::set_var("PATH", &original_path); - }); - - (ninja_dir, ninja_path, cleanup) + let guard = PathGuard::new(original_path); + (ninja_dir, ninja_path, guard) } /// Fixture: Create a temporary project with a Netsukefile from minimal.yml. @@ -114,7 +125,7 @@ fn run_ninja_not_found() { #[rstest] #[serial] fn run_executes_ninja_without_persisting_file() { - let (_ninja_dir, ninja_path, cleanup) = ninja_in_path(); + let (_ninja_dir, ninja_path, _guard) = ninja_in_path(); let (temp, manifest_path) = test_manifest(); let cli = Cli { file: manifest_path.clone(), @@ -133,8 +144,7 @@ fn run_executes_ninja_without_persisting_file() { // Ensure no ninja file remains in project directory assert!(!temp.path().join("build.ninja").exists()); - // Cleanup PATH then drop the fake ninja artifacts. - cleanup(); + // Drop the fake ninja artifacts. PATH is restored by guard drop. drop(ninja_path); } @@ -142,7 +152,7 @@ fn run_executes_ninja_without_persisting_file() { #[serial] #[rstest] fn run_build_with_emit_keeps_file() { - let (_ninja_dir, ninja_path, cleanup) = ninja_in_path(); + let (_ninja_dir, ninja_path, _guard) = ninja_in_path(); let (temp, manifest_path) = test_manifest(); let emit_path = temp.path().join("emitted.ninja"); let cli = Cli { @@ -165,8 +175,7 @@ fn run_build_with_emit_keeps_file() { assert!(emitted.contains("build ")); assert!(!temp.path().join("build.ninja").exists()); - // Cleanup PATH then drop the fake ninja artifacts. - cleanup(); + // Drop the fake ninja artifacts. PATH is restored by guard drop. drop(ninja_path); } @@ -174,7 +183,7 @@ fn run_build_with_emit_keeps_file() { #[serial] #[rstest] fn run_build_with_emit_creates_parent_dirs() { - let (_ninja_dir, ninja_path, cleanup) = ninja_with_exit_code(0); + let (_ninja_dir, ninja_path, _guard) = ninja_with_exit_code(0); let (temp, manifest_path) = test_manifest(); let nested_dir = temp.path().join("nested").join("dir"); let emit_path = nested_dir.join("emitted.ninja"); @@ -195,8 +204,7 @@ fn run_build_with_emit_creates_parent_dirs() { assert!(emit_path.exists()); assert!(nested_dir.exists()); - // Cleanup PATH then drop the fake ninja artifacts. - cleanup(); + // Drop the fake ninja artifacts. PATH is restored by guard drop. drop(ninja_path); } From 6d51ef8e21adba55dd0e295cfd205518a3d99b63 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Mon, 11 Aug 2025 11:33:30 +0100 Subject: [PATCH 13/13] Apply formatting --- src/runner.rs | 8 ++++++-- tests/runner_tests.rs | 4 +++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/runner.rs b/src/runner.rs index 2960ef28..197c4968 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -34,9 +34,13 @@ pub mod doc { super::contains_sensitive_keyword(arg) } #[must_use] - pub fn is_sensitive_arg(arg: &CommandArg) -> bool { super::is_sensitive_arg(arg) } + pub fn is_sensitive_arg(arg: &CommandArg) -> bool { + super::is_sensitive_arg(arg) + } #[must_use] - pub fn redact_argument(arg: &CommandArg) -> CommandArg { super::redact_argument(arg) } + pub fn redact_argument(arg: &CommandArg) -> CommandArg { + super::redact_argument(arg) + } #[must_use] pub fn redact_sensitive_args(args: &[CommandArg]) -> Vec { super::redact_sensitive_args(args) diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index d5724095..b2a5b69b 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -16,7 +16,9 @@ struct PathGuard { } impl PathGuard { - fn new(original: OsString) -> Self { Self { original } } + fn new(original: OsString) -> Self { + Self { original } + } } impl Drop for PathGuard {