diff --git a/.gitignore b/.gitignore index 221eb3b3..28a4fcba 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,5 @@ target/ **/*.rs.bk +**/*~ .crush +build.ninja diff --git a/Cargo.lock b/Cargo.lock index ccb32098..90b18f6f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -93,6 +93,12 @@ dependencies = [ "syn", ] +[[package]] +name = "autocfg" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c08606f8c3cbf4ce6ec8e28fb0014a2c086708fe954eaa885384a6165172e7e8" + [[package]] name = "backtrace" version = "0.3.75" @@ -712,6 +718,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" @@ -765,6 +781,7 @@ dependencies = [ "serde", "serde_json", "serde_yml", + "serial_test", "sha2", "tempfile", "thiserror", @@ -831,6 +848,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" @@ -914,6 +954,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" @@ -1033,6 +1082,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" @@ -1101,6 +1171,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", +] + [[package]] name = "sha2" version = "0.10.9" diff --git a/Cargo.toml b/Cargo.toml index 560281b9..c07b5f5c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -63,6 +63,7 @@ cucumber = "0.20.0" tokio = { version = "1", features = ["macros", "rt-multi-thread"], default-features = false } insta = { version = "1", features = ["yaml"] } tempfile = "3" +serial_test = "3" [[test]] name = "cucumber" diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index c1780d59..49636b1d 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -1138,12 +1138,14 @@ The command construction will follow this pattern: `ninja` is available in the system's `PATH`. 1. Arguments passed to Netsuke's own CLI will be translated and forwarded to - Ninja. For example, a `Netsuke build -C build/ my_target` command would - result in `Command::new("ninja").arg("-C").arg("build/").arg("my_target")`. - Flags like `-j` for parallelism will also be passed through.[^8] + Ninja. For example, a `Netsuke build my_target` command would result in + `Command::new("ninja").arg("my_target")`. Flags like `-j` for parallelism + will also be passed through.[^8] 1. The working directory for the Ninja process will be set using - `.current_dir()` if the user provides a `-C` flag. + `.current_dir()`. When the user supplies a `-C` flag, Netsuke + canonicalises the path and applies it via `current_dir` rather than + forwarding the flag to Ninja. 1. Standard I/O streams (`stdin`, `stdout`, `stderr`) will be configured using `.stdout(Stdio::piped())` and `.stderr(Stdio::piped())`.[^24] This allows @@ -1418,10 +1420,10 @@ The CLI is implemented using clap's derive API in `src/cli.rs`. Clap's `default_value_t` attribute marks `Build` as the default subcommand, so invoking `netsuke` with no explicit command still triggers a build. CLI execution and dispatch live in `src/runner.rs`, keeping `main.rs` focused on -parsing. The working directory flag uses `-C` to mirror Ninja's convention, -ensuring command line arguments map directly onto the underlying build tool. -Error scenarios are validated using clap's `ErrorKind` enumeration in unit -tests and via Cucumber steps for behavioural coverage. +parsing. The working directory flag mirrors Ninja's `-C` option but is resolved +internally; Netsuke changes directory before spawning Ninja rather than +forwarding the flag. Error scenarios are validated using clap's `ErrorKind` +enumeration in unit tests and via Cucumber steps for behavioural coverage. ## Section 9: Implementation Roadmap and Strategic Recommendations diff --git a/src/ast.rs b/src/ast.rs index cf4a49ea..fe85b0e9 100644 --- a/src/ast.rs +++ b/src/ast.rs @@ -106,7 +106,7 @@ pub struct Rule { /// Exactly one variant must be provided for a rule or target. The fields are /// flattened in the manifest, so the presence of `command`, `script`, or `rule` /// determines the variant. -#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone, PartialEq, Serialize)] pub enum Recipe { /// A single shell command. Command { command: String }, diff --git a/src/runner.rs b/src/runner.rs index 5d6eb26d..b6174f28 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -125,7 +125,12 @@ fn redact_sensitive_args(args: &[String]) -> Vec { pub fn run_ninja(program: &Path, cli: &Cli, targets: &[String]) -> io::Result<()> { let mut cmd = Command::new(program); if let Some(dir) = &cli.directory { - cmd.current_dir(dir).arg("-C").arg(dir); + // Resolve and canonicalise the directory so Ninja receives a stable + // absolute path. Using only `current_dir` avoids combining it with + // Ninja's own `-C` flag which would otherwise double-apply the + // directory and break relative paths. + let dir = fs::canonicalize(dir)?; + cmd.current_dir(dir); } if let Some(jobs) = cli.jobs { cmd.arg("-j").arg(jobs.to_string()); diff --git a/tests/ninja_snapshot_tests.rs b/tests/ninja_snapshot_tests.rs index 77dd3977..10b3d60e 100644 --- a/tests/ninja_snapshot_tests.rs +++ b/tests/ninja_snapshot_tests.rs @@ -68,7 +68,7 @@ fn touch_manifest_ninja_validation() { let _ = ninja_cmd(&["-t", "targets", "all"]); let _ = ninja_cmd(&["-t", "query", "out/a"]); - let _ = ninja_cmd(&["-w", "dupbuild=err", "-d", "stats"]); + let _ = ninja_cmd(&["-w", "phonycycle=err", "-d", "stats"]); let second = ninja_cmd(&["-n", "-d", "explain", "-v"]); assert!( second.contains("no work to do"), diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index d60ce894..d0fd90d1 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -1,47 +1,59 @@ -//! Unit tests for Ninja process invocation. - use netsuke::cli::{Cli, Commands}; -use netsuke::runner; +use netsuke::runner::{run, run_ninja}; use rstest::rstest; +use std::error::Error; +use std::fs::{self, File}; +use std::io::Write; use std::path::{Path, PathBuf}; -use tracing::Level; +use tempfile::TempDir; + +mod support; -/// Creates a default CLI configuration for testing Ninja invocation. -fn test_cli() -> Cli { - Cli { - file: PathBuf::from("Netsukefile"), +#[test] +fn run_exits_with_manifest_error_on_invalid_version() { + let temp = tempfile::tempdir().expect("temp dir"); + let manifest_path = temp.path().join("Netsukefile"); + std::fs::copy("tests/data/invalid_version.yml", &manifest_path).expect("copy manifest"); + let cli = Cli { + file: manifest_path.clone(), directory: None, jobs: None, verbose: false, - command: Some(Commands::Build { - targets: Vec::new(), - }), - } -} - -mod support; + command: Some(Commands::Build { targets: vec![] }), + }; -#[rstest] -#[case(0, true)] -#[case(1, false)] -fn run_ninja_status(#[case] code: i32, #[case] succeeds: bool) { - let (_dir, path) = support::fake_ninja(code); - let cli = test_cli(); - let result = runner::run_ninja(&path, &cli, &[]); - assert_eq!(result.is_ok(), succeeds); + let result = run(&cli); + assert!(result.is_err()); + let err = result.expect_err("should have error"); + assert!( + err.source() + .expect("should have source") + .to_string() + .contains("version") + ); } -#[rstest] -fn run_ninja_not_found() { - let cli = test_cli(); - let err = - runner::run_ninja(Path::new("does-not-exist"), &cli, &[]).expect_err("process should fail"); - assert_eq!(err.kind(), std::io::ErrorKind::NotFound); +#[cfg(unix)] +fn fake_ninja_pwd() -> (TempDir, PathBuf) { + use std::os::unix::fs::PermissionsExt; + let dir = TempDir::new().expect("temp dir"); + let path = dir.path().join("ninja"); + let mut file = File::create(&path).expect("script"); + writeln!( + file, + "#!/bin/sh\nif [ -n \"$1\" ]; then pwd > \"$1\"; else pwd; fi" + ) + .expect("write script"); + let mut perms = fs::metadata(&path).expect("meta").permissions(); + perms.set_mode(0o755); + fs::set_permissions(&path, perms).expect("perms"); + (dir, path) } -#[rstest] -fn run_writes_ninja_file() { - let (ninja_dir, ninja_path) = support::fake_ninja(0); +#[cfg(unix)] +#[test] +fn run_executes_ninja_and_captures_logs() { + let (ninja_dir, ninja_path) = fake_ninja_pwd(); 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()); @@ -54,17 +66,23 @@ fn run_writes_ninja_file() { let manifest_path = temp.path().join("Netsukefile"); std::fs::copy("tests/data/minimal.yml", &manifest_path).expect("copy manifest"); let cli = Cli { - file: manifest_path, + file: manifest_path.clone(), directory: Some(temp.path().to_path_buf()), jobs: None, verbose: false, - command: Some(Commands::Build { - targets: Vec::new(), - }), + command: Some(Commands::Build { targets: vec![] }), }; - runner::run(&cli).expect("run"); - assert!(temp.path().join("build.ninja").exists()); + let result = run(&cli); + assert!(result.is_ok()); + + // Verify the ninja file was written and contains some content + let ninja_file_path = temp.path().join("build.ninja"); + assert!(ninja_file_path.exists()); + let ninja_content = std::fs::read_to_string(&ninja_file_path).expect("read ninja file"); + assert!(!ninja_content.is_empty()); + assert!(ninja_content.contains("build ")); + assert!(ninja_content.contains("rule ")); unsafe { std::env::set_var("PATH", original_path); @@ -73,20 +91,20 @@ fn run_writes_ninja_file() { } #[rstest] -fn run_ninja_logs_command() { - let (_dir, path) = support::fake_ninja(0); - let mut cli = test_cli(); - cli.verbose = true; - let logs = support::capture_logs(Level::INFO, || { - runner::run_ninja(&path, &cli, &["--password=123".to_string()]).expect("run"); - }); - assert!(logs.contains("Running command:")); - assert!(logs.contains("password=***REDACTED***")); - assert!(!logs.contains("123")); +fn run_ninja_not_found() { + let cli = Cli { + file: PathBuf::from("/dev/null"), + directory: None, + jobs: None, + verbose: false, + command: Some(Commands::Build { targets: vec![] }), + }; + let err = run_ninja(Path::new("does-not-exist"), &cli, &[]).expect_err("process should fail"); + assert_eq!(err.kind(), std::io::ErrorKind::NotFound); } #[rstest] -fn run_with_verbose_mode_emits_logs() { +fn run_writes_ninja_file() { 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(); @@ -100,21 +118,23 @@ fn run_with_verbose_mode_emits_logs() { let manifest_path = temp.path().join("Netsukefile"); std::fs::copy("tests/data/minimal.yml", &manifest_path).expect("copy manifest"); let cli = Cli { - file: manifest_path, + file: manifest_path.clone(), directory: Some(temp.path().to_path_buf()), jobs: None, - verbose: true, - command: Some(Commands::Build { - targets: Vec::new(), - }), + verbose: false, + command: Some(Commands::Build { targets: vec![] }), }; - let logs = support::capture_logs(Level::DEBUG, || { - runner::run(&cli).expect("run"); - }); - assert!(logs.contains("AST:")); - assert!(logs.contains("Generated Ninja file at")); - assert!(logs.contains("Running command:")); + let result = run(&cli); + assert!(result.is_ok()); + + // Verify the ninja file was written and contains some content + let ninja_file_path = temp.path().join("build.ninja"); + assert!(ninja_file_path.exists()); + let ninja_content = std::fs::read_to_string(&ninja_file_path).expect("read ninja file"); + assert!(!ninja_content.is_empty()); + assert!(ninja_content.contains("build ")); + assert!(ninja_content.contains("rule ")); unsafe { std::env::set_var("PATH", original_path); diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 054267ec..0c39c9eb 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -69,3 +69,24 @@ where let locked = buf.lock().expect("lock"); String::from_utf8(locked.clone()).expect("utf8") } + +/// Create a fake Ninja executable that writes its current directory to the file +/// specified as the first argument. +/// +/// Returns the temporary directory and the path to the executable. +#[allow(dead_code, reason = "used only in directory tests")] +pub fn fake_ninja_pwd() -> (TempDir, PathBuf) { + let dir = TempDir::new().expect("temp dir"); + let path = dir.path().join("ninja"); + let mut file = File::create(&path).expect("script"); + // The script writes its working directory to the provided file and exits. + writeln!(file, "#!/bin/sh\npwd > \"$1\"").expect("write script"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mut perms = fs::metadata(&path).expect("meta").permissions(); + perms.set_mode(0o755); + fs::set_permissions(&path, perms).expect("perms"); + } + (dir, path) +}