diff --git a/docs/developers-guide.md b/docs/developers-guide.md index e19959482..4349c7b2f 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -33,8 +33,10 @@ The public Ninja process helpers are re-exported from `netsuke::runner`. `CommandEnv::inherit()` leaves the parent environment in place, `with_var` overrides one variable, and `with_path` replaces the child's `PATH`. The parent process is never mutated. `NinjaBuildRequest` and -`NinjaToolRequest` borrow the program, CLI settings, generated build file, -target list or tool name, and `CommandEnv` needed for one invocation. +`NinjaToolRequest` borrow the program, `NinjaProcessOptions` (`working_dir` and +`jobs`), generated build file, target list or tool name, and `CommandEnv` +needed for one invocation. The process boundary is parser-independent; callers +without CLI state construct `NinjaProcessOptions` directly. The legacy `run_ninja` and `run_ninja_tool` helpers retain their existing signatures and inherit the parent environment. Callers that need an isolated @@ -3431,6 +3433,13 @@ selected source at debug level. Process construction uses the resolved path exported by this module and must not interpret the environment override independently. +`src/runner/ninja_process_adapter.rs` owns the one-way translation from `Cli` +to `NinjaProcessOptions` and the public CLI-facing wrappers. It converts +`Cli::directory` to the options' UTF-8 `working_dir`, returning +`io::ErrorKind::InvalidData` for a non-UTF-8 path. The process module remains +parser-independent; callers without CLI state construct `NinjaProcessOptions` +directly. + ### Module: `runner::process::command_logging` `src/runner/process/command_logging.rs` owns the structured logging contract @@ -3447,7 +3456,9 @@ high-cardinality payload to a debug companion event. All command events share these structured fields: -- `operation`: caller-provided operation label such as `"build"` or tool name. +- `operation`: `run_ninja_build_internal` supplies the fixed label `"build"` + before command configuration, while `run_ninja_tool_internal` supplies the + label from `NinjaToolRequest::tool`. - `ninja_program`: command program after UTF-8 normalization. - `suppress_stderr`: bool derived from the `StderrMode` policy via `stderr_mode.is_suppress()`, true when the policy suppresses direct @@ -3469,12 +3480,13 @@ exits, which lets downstream filtering distinguish spawn failures from exit-status failures. `run_ninja_internal` is the shared execution pattern used by build and tool -paths: +paths. It takes a `NinjaInternalRequest`, a clock, and a configuration closure; +the request groups the execution fields: 1. Create `Command` with `Command::new(request.program)`. 2. Pass it into a closure that applies operation-specific configuration. -3. Call `run_command_and_stream_with_context` with optional status observer, - the request's `stderr_mode` policy, and the chosen `operation`. +3. Call `run_command_and_stream_with_context` with the request's optional + status observer and execution context. 4. Let `run_command_and_stream_with_context` handle span creation, execution logging, failure logging, and exit-status enforcement via context helpers. @@ -3587,11 +3599,11 @@ governs is the environment Ninja's own child commands see when it shells out. The explicit request APIs compose on top of `CommandEnv`: `NinjaBuildRequest`/ `NinjaToolRequest` carry `env: &CommandEnv` and `stderr_mode: StderrMode` -fields alongside the program, CLI settings, and build file, and are consumed -by `run_ninja_with`/`run_ninja_tool_with`. The convenience wrappers -`run_ninja`/`run_ninja_tool` live at the runner boundary -(`src/runner/mod.rs`), call these with `CommandEnv::inherit()`, and derive the -`stderr_mode` policy from the CLI via +fields alongside the program, `NinjaProcessOptions`, and build file, and are +consumed by `run_ninja_with`/`run_ninja_tool_with`. The convenience wrappers +`run_ninja`/`run_ninja_tool` live in `src/runner/ninja_process_adapter.rs`, +call these with `CommandEnv::inherit()`, and derive the `stderr_mode` policy +from the CLI via `StderrMode::from_json_enabled(cli.json)`, reproducing production behaviour; tests reach for `run_ninja_with`/`run_ninja_tool_with` directly to supply a `CommandEnv` built with `with_path` instead. Section 6.1 of the diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index e677126b8..a2c37199b 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2231,14 +2231,24 @@ child process's execution environment. Every invocation is described by a borrowed request bundle rather than a long parameter list: `NinjaBuildRequest` for a build and `NinjaToolRequest` for -`ninja -t `. Each names the resolved program, the parsed CLI settings, -the generated build file, the targets or tool, a `&CommandEnv` describing the -child's environment, and the `stderr_mode: StderrMode` policy field. +`ninja -t `. Each names the resolved program, `NinjaProcessOptions` +(an optional UTF-8 working directory and job count), the generated build file, +the targets or tool, a `&CommandEnv` describing the child's environment, and the +`stderr_mode: StderrMode` policy field. `run_ninja_with` and `run_ninja_tool_with` consume these; the convenience -wrappers `run_ninja` and `run_ninja_tool` live at the runner boundary, call -them with `CommandEnv::inherit()`, and derive the `stderr_mode` policy from the -CLI via `StderrMode::from_json_enabled(cli.json)`, which is production -behaviour. +wrappers `run_ninja` and `run_ninja_tool` live in +`runner::ninja_process_adapter`, translate `Cli` state at the runner boundary, +call them with `CommandEnv::inherit()`, and derive the `stderr_mode` policy +from the CLI via `StderrMode::from_json_enabled(cli.json)`, which is production +behaviour. Process requests never import `Cli`; callers without parser state +construct `NinjaProcessOptions` directly. +The adapter converts `Cli::directory` to the UTF-8 path at this boundary and +returns `io::ErrorKind::InvalidData` if the CLI path is not valid UTF-8. + +The private `run_ninja_internal` helper takes a `NinjaInternalRequest`, a +clock, and a `configure` closure. The request groups the resolved program, +stderr policy, optional status observer, operation label, and failure-output +capture setting before the closure configures the command. The command construction follows this pattern: diff --git a/docs/users-guide.md b/docs/users-guide.md index 102bd8d26..52ac3f470 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -701,21 +701,21 @@ stay isolated from the injected `PATH`. The request itself is a named type: `netsuke::runner::NinjaBuildRequest` for a build and `netsuke::runner::NinjaToolRequest` for `ninja -t `. Both -borrow their fields, so one `CommandEnv` and one `Cli` can serve several +borrow their fields, so one `CommandEnv` and one `NinjaProcessOptions` can +serve several invocations. The [v0.1.0 migration guide](v0-1-0-migration-guide.md) summarizes these additions and confirms the wrappers are unchanged. ```rust -use netsuke::cli::Cli; use netsuke::runner::{ - BuildTargets, CommandEnv, NinjaBuildRequest, NinjaToolRequest, StderrMode, run_ninja_tool_with, - run_ninja_with, + BuildTargets, CommandEnv, NinjaBuildRequest, NinjaProcessOptions, NinjaToolRequest, + StderrMode, run_ninja_tool_with, run_ninja_with, }; use std::path::Path; -let cli = Cli::default(); +let options = NinjaProcessOptions::default(); let targets = BuildTargets::default(); // `with_path` replaces the child's `PATH` outright, so compose the whole // value first. The calling process is never modified. @@ -727,21 +727,21 @@ let env = CommandEnv::inherit() let build = NinjaBuildRequest { program: Path::new("/usr/bin/ninja"), - cli: &cli, + options: &options, build_file: Path::new("build.ninja"), targets: &targets, env: &env, // `Suppress` in JSON diagnostics mode keeps the child's output out of // the machine-readable streams; `Forward` relays it to the caller. - stderr_mode: StderrMode::from_json_enabled(cli.json), + stderr_mode: StderrMode::Forward, }; let clean = NinjaToolRequest { program: Path::new("/usr/bin/ninja"), - cli: &cli, + options: &options, build_file: Path::new("build.ninja"), tool: "clean", env: &env, - stderr_mode: StderrMode::from_json_enabled(cli.json), + stderr_mode: StderrMode::Forward, }; if std::env::var_os("NETSUKE_GUIDE_RUN").is_some() { @@ -752,8 +752,9 @@ if std::env::var_os("NETSUKE_GUIDE_RUN").is_some() { The convenience wrappers `run_ninja` and `run_ninja_tool` keep their existing signatures and behaviour, so a caller that uses them needs no change; the -request bundles gained the required `stderr_mode` field, so a caller that -constructs `NinjaBuildRequest`/`NinjaToolRequest` directly must supply it. +request bundles use `options: &options` instead of `cli: &cli` and gained the +required `stderr_mode` field, so a caller that constructs +`NinjaBuildRequest`/`NinjaToolRequest` directly must supply both. Each release records such additions in [`CHANGELOG.md`](../CHANGELOG.md), which is where Netsuke signposts Rust API changes — with no stability promise attached to them ahead of 1.0. diff --git a/docs/v0-1-0-migration-guide.md b/docs/v0-1-0-migration-guide.md index e17f9a88c..51ac8ff83 100644 --- a/docs/v0-1-0-migration-guide.md +++ b/docs/v0-1-0-migration-guide.md @@ -1,13 +1,15 @@ # Migrating to v0.1.0 This guide signposts the v0.1.0 beta additions: the injectable child -environment (`CommandEnv`), the named Ninja request types, and target/action -discovery through `description` and `netsuke help targets`. Existing manifests -remain compatible, and callers of the unchanged convenience wrappers compile -unchanged. -Rust callers that construct `Target` with a struct literal must add the new +environment (`CommandEnv`), the named Ninja request types, narrow process +options (`NinjaProcessOptions`), and target/action discovery through +`description` and `netsuke help targets`. Existing manifests remain compatible, +and callers of the unchanged convenience wrappers compile unchanged. Rust +callers that construct `Target` with a struct literal must add the new `description` field (set it to `None` or `Some(...)`); deserialized manifests -remain compatible, and every other addition is opt-in. +remain compatible. Callers constructing `NinjaBuildRequest` or +`NinjaToolRequest` must replace `cli: &cli` with `options: &options`; every +other addition is opt-in. ## Netsuke is a build tool, not a library @@ -27,7 +29,7 @@ Table: documented v0.1.0 additions, including `netsuke help targets`, and their | --- | --- | --- | | Convenience wrappers | Unchanged. `run_ninja` and `run_ninja_tool` behave exactly as before, inheriting the process environment. | [Users' guide](users-guide.md) | | Child environment | New opt-in `netsuke::runner::CommandEnv` carries additive variable overrides and an injected `PATH` for Ninja child processes. | [Users' guide](users-guide.md) | -| Request types | New `netsuke::runner::NinjaBuildRequest` and `netsuke::runner::NinjaToolRequest` name the program, build file, targets or tool, a child environment, and a required `stderr_mode: StderrMode` policy for the `*_with` run functions. | [Users' guide](users-guide.md) | +| Request types | New `netsuke::runner::NinjaBuildRequest` and `netsuke::runner::NinjaToolRequest` name the program, `NinjaProcessOptions`, build file, targets or tool, a child environment, and a required `stderr_mode: StderrMode` policy for the `*_with` run functions. | [Users' guide](users-guide.md) | | Glob expansion | Parent-relative patterns such as `glob('../shared/*.h')` now expand. Metadata checks use a capability rooted at the pattern's longest literal directory prefix; missing or non-directory prefixes return no matches, and unresolvable symlink matches are skipped. | [Users' guide](users-guide.md) and [ADR-010](adr-010-scope-glob-capability-to-literal-prefix.md) | | Command recipes | Existing scalar `command` recipes are unchanged. New YAML command lists are opt-in and run in declaration order with fail-fast semantics. | [Rules and recipes](users-guide.md#rules-and-recipes) | | Manifest discovery | Optional target/action `description` values are shown by the new `netsuke help targets` command. Manifests without them and existing build output are unchanged. | [Users' guide](users-guide.md) | @@ -38,10 +40,10 @@ Table: documented v0.1.0 additions, including `netsuke help targets`, and their The convenience wrappers keep their signatures and their behaviour: the child inherits the calling process's environment, and Ninja is resolved -exactly as before. Callers of `run_ninja` or `run_ninja_tool` need no change; -a caller that constructs `NinjaBuildRequest`/`NinjaToolRequest` directly must -now supply the required `stderr_mode: StderrMode` field, derived from the CLI -with `StderrMode::from_json_enabled(cli.json)`. +exactly as before. No caller using these wrappers needs to change to adopt this +release. A caller that constructs `NinjaBuildRequest`/`NinjaToolRequest` +directly must pass `options: &options` and supply the required +`stderr_mode: StderrMode` field. ## Opting into ordered command lists @@ -66,8 +68,11 @@ names remain valid and resolve through that child `PATH`; supply an absolute or otherwise resolved `program` only when executable selection must stay isolated from the injected `PATH`. -Both request types borrow their fields, so one `CommandEnv` and one `Cli` -can serve several invocations. Worked examples live in the users' guide's +Both request types borrow their fields, so one `CommandEnv` and one +`NinjaProcessOptions` can serve several invocations. Direct request callers +now pass `options: &options` in place of `cli: &cli`; the adapter converts a +CLI working directory to UTF-8 and returns `io::ErrorKind::InvalidData` when +it cannot. Worked examples live in the users' guide's "Drive Ninja with an explicit environment" section. ## Reusing cached configuration discovery diff --git a/src/runner/mod.rs b/src/runner/mod.rs index aec261d95..8eba54b0b 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -8,9 +8,6 @@ mod dyndep_generation_telemetry; mod dyndep_publication; mod error; mod reporter; - -pub use error::RunnerError; - use crate::cli::{BuildArgs, Cli, Commands}; use crate::localization::{self, keys}; use crate::output_mode; @@ -19,8 +16,9 @@ use crate::status::{LocalizationKey, PipelineStage, StatusReporter, report_pipel use crate::{ir::BuildGraph, manifest, ninja_gen}; use anyhow::{Context, Result}; use camino::Utf8PathBuf; +pub use error::RunnerError; use std::borrow::Cow; -use std::io::{self, IsTerminal}; +use std::io::IsTerminal; use std::path::Path; use tracing::{debug, info}; @@ -42,14 +40,16 @@ pub const NINJA_ENV: &str = "NETSUKE_NINJA"; mod graph; mod help; mod ninja_content; +mod ninja_process_adapter; mod path_helpers; mod process; pub use ninja_content::NinjaContent; +pub use ninja_process_adapter::{run_ninja, run_ninja_tool}; #[cfg(doctest)] pub use process::doc; pub use process::{ - CommandEnv, MAX_RETAINED_DYNDEP_FILES, NinjaBuildRequest, NinjaToolRequest, StderrMode, - run_ninja_tool_with, run_ninja_with, + CommandEnv, MAX_RETAINED_DYNDEP_FILES, NinjaBuildRequest, NinjaJobCount, NinjaProcessOptions, + NinjaToolRequest, StderrMode, run_ninja_tool_with, run_ninja_with, }; use dyndep_publication::{materialize_dyndep_bundle, prune_dyndep_bundle}; @@ -144,56 +144,6 @@ fn run_with_ninja_program_resolver( dispatch::execute(cli, command, &context) } -/// Invoke the Ninja executable with the provided CLI settings. -/// -/// Forwards the job count and working directory and specifies the temporary -/// build file. Child output follows the `stderr_mode` policy derived from the -/// CLI's JSON diagnostic setting: `StderrMode::Suppress` drains both child -/// streams to a sink so JSON output stays machine-readable, while -/// `StderrMode::Forward` relays them to the user. -/// -/// # Errors -/// -/// Returns an [`io::Error`] if the Ninja process fails to spawn, the standard -/// streams are unavailable, or when Ninja reports a non-zero exit status. -pub fn run_ninja( - program: &Path, - cli: &Cli, - build_file: &Path, - targets: &BuildTargets<'_>, -) -> io::Result<()> { - run_ninja_with(&NinjaBuildRequest { - program, - cli, - build_file, - targets, - env: &CommandEnv::inherit(), - stderr_mode: StderrMode::from_json_enabled(cli.json), - }) -} - -/// Invoke a Ninja tool (e.g., `ninja -t clean`) with the provided CLI settings. -/// -/// Forwards the job count and working directory and specifies the build file. -/// Child output follows the `stderr_mode` policy derived from the CLI's JSON -/// diagnostic setting: `StderrMode::Suppress` drains both child streams to a -/// sink, while `StderrMode::Forward` relays them to the user. -/// -/// # Errors -/// -/// Returns an [`io::Error`] if the Ninja process fails to spawn, the standard -/// streams are unavailable, or when Ninja reports a non-zero exit status. -pub fn run_ninja_tool(program: &Path, cli: &Cli, build_file: &Path, tool: &str) -> io::Result<()> { - run_ninja_tool_with(&NinjaToolRequest { - program, - cli, - build_file, - tool, - env: &CommandEnv::inherit(), - stderr_mode: StderrMode::from_json_enabled(cli.json), - }) -} - fn on_task_progress_callback(reporter: &dyn StatusReporter) -> impl FnMut(u32, u32, &str) + '_ { move |current: u32, total: u32, description: &str| { reporter.report_task_progress(current, total, description); @@ -227,11 +177,12 @@ fn handle_build(cli: &Cli, args: &BuildArgs, context: &ExecutionContext<'_>) -> ) }; if context.progress_enabled { + let options = ninja_process_adapter::ninja_process_options(cli)?; let mut on_task_progress = on_task_progress_callback(context.reporter); process::run_ninja_with_status( process::NinjaBuildRequest { program: context.ninja_program, - cli, + options: &options, build_file: build_path, targets: &targets, env: &CommandEnv::inherit(), @@ -290,11 +241,12 @@ fn handle_ninja_tool( ) }; if context.progress_enabled { + let options = ninja_process_adapter::ninja_process_options(cli)?; let mut on_task_progress = on_task_progress_callback(context.reporter); process::run_ninja_tool_with_status( process::NinjaToolRequest { program: context.ninja_program, - cli, + options: &options, build_file: build_path, tool: tool.name, env: &CommandEnv::inherit(), diff --git a/src/runner/ninja_process_adapter.rs b/src/runner/ninja_process_adapter.rs new file mode 100644 index 000000000..7b2e9dc5e --- /dev/null +++ b/src/runner/ninja_process_adapter.rs @@ -0,0 +1,159 @@ +//! Translation between parsed CLI state and the Ninja process adapter. +//! +//! This module owns the one-way `Cli` to `NinjaProcessOptions` translation and +//! the public compatibility wrappers. Runner command handlers and callers with +//! a `Cli` use these wrappers; process requests remain parser-independent and +//! callers without CLI state construct `NinjaProcessOptions` directly. + +use super::{BuildTargets, CommandEnv, StderrMode, process}; +use crate::cli::Cli; +use camino::Utf8PathBuf; +use std::{ + io::{self, ErrorKind}, + path::Path, +}; + +/// Translate CLI state into the narrow options consumed by the process layer. +/// +/// # Errors +/// +/// Returns [`io::ErrorKind::InvalidData`] when the CLI working directory is +/// not valid UTF-8, or [`io::ErrorKind::InvalidInput`] when the job count lies +/// outside the supported `1..=64` range. +pub(super) fn ninja_process_options(cli: &Cli) -> io::Result { + let working_dir = cli + .directory + .clone() + .map(Utf8PathBuf::from_path_buf) + .transpose() + .map_err(|path| { + io::Error::new( + ErrorKind::InvalidData, + format!( + "Ninja working directory {} is not valid UTF-8", + path.display() + ), + ) + })?; + let jobs = cli.jobs.map(process::NinjaJobCount::try_new).transpose()?; + Ok(process::NinjaProcessOptions { working_dir, jobs }) +} + +/// Invoke the Ninja executable with the provided CLI settings. +/// +/// This compatibility wrapper translates parser state at the orchestration +/// boundary before delegating to the process adapter. +/// +/// # Errors +/// +/// Returns an [`std::io::Error`] if Ninja cannot execute successfully. +pub fn run_ninja( + program: &Path, + cli: &Cli, + build_file: &Path, + targets: &BuildTargets<'_>, +) -> std::io::Result<()> { + let options = ninja_process_options(cli)?; + process::run_ninja_with(&process::NinjaBuildRequest { + program, + options: &options, + build_file, + targets, + env: &CommandEnv::inherit(), + stderr_mode: StderrMode::from_json_enabled(cli.json), + }) +} + +/// Invoke a Ninja tool with the provided CLI settings. +/// +/// This compatibility wrapper translates parser state at the orchestration +/// boundary before delegating to the process adapter. +/// +/// # Errors +/// +/// Returns an [`std::io::Error`] if Ninja cannot execute successfully. +pub fn run_ninja_tool( + program: &Path, + cli: &Cli, + build_file: &Path, + tool: &str, +) -> std::io::Result<()> { + let options = ninja_process_options(cli)?; + process::run_ninja_tool_with(&process::NinjaToolRequest { + program, + options: &options, + build_file, + tool, + env: &CommandEnv::inherit(), + stderr_mode: StderrMode::from_json_enabled(cli.json), + }) +} + +#[cfg(test)] +mod tests { + //! Unit tests for CLI-to-process option translation. + + use super::*; + use anyhow::{Result, ensure}; + + #[cfg(unix)] + use std::os::unix::ffi::OsStringExt; + + #[cfg(unix)] + #[test] + fn ninja_process_options_rejects_non_utf8_working_directory() -> Result<()> { + let cli = Cli { + directory: Some(std::path::PathBuf::from(std::ffi::OsString::from_vec( + vec![0xff], + ))), + ..Cli::default() + }; + + let Err(error) = ninja_process_options(&cli) else { + anyhow::bail!("non-UTF-8 working directory should be rejected"); + }; + ensure!( + error.kind() == ErrorKind::InvalidData, + "invalid working directory returned {:?}, not InvalidData", + error.kind() + ); + Ok(()) + } + + #[test] + fn ninja_process_options_rejects_out_of_range_job_counts() -> Result<()> { + // 0 and 65 sit just outside the supported 1..=64 bound; the CLI's own + // boundary tests assert the same values. + for jobs in [0, 65] { + let cli = Cli { + jobs: Some(jobs), + ..Cli::default() + }; + let Err(error) = ninja_process_options(&cli) else { + anyhow::bail!("job count {jobs} should be rejected"); + }; + ensure!( + error.kind() == ErrorKind::InvalidInput, + "job count {jobs} returned {:?}, not InvalidInput", + error.kind() + ); + } + Ok(()) + } + + #[test] + fn ninja_process_options_preserves_a_valid_job_count() -> Result<()> { + let cli = Cli { + jobs: Some(8), + ..Cli::default() + }; + let options = ninja_process_options(&cli)?; + let expected = process::NinjaJobCount::try_new(8)?; + ensure!( + options.jobs == Some(expected), + "a present valid job count should be preserved, got {:?}", + options.jobs + ); + Ok(()) + } +} diff --git a/src/runner/process/command_logging.rs b/src/runner/process/command_logging.rs index 0cb78a68b..7f4d48da6 100644 --- a/src/runner/process/command_logging.rs +++ b/src/runner/process/command_logging.rs @@ -177,6 +177,22 @@ mod tests { use super::*; use crate::runner::CommandEnv; use rstest::rstest; + use tracing_subscriber::filter::LevelFilter; + + fn logging_context() -> CommandLogContext { + CommandLogContext::from_command(&Command::new("ninja")) + } + + fn captured_execution_event(operation: &str) -> String { + crate::test_tracing_capture::with_test_subscriber(LevelFilter::INFO, |captured| { + log_command_execution(&logging_context(), operation, StderrMode::Forward); + let events = captured.snapshot(); + let [event] = events.as_slice() else { + panic!("expected one command execution event, got {events:?}"); + }; + event.clone() + }) + } /// The override summary counts overrides and flags `PATH` without naming /// or valuing any variable, so the fields stay safe to log. @@ -205,6 +221,67 @@ mod tests { assert_eq!(context.is_path_overridden, expected_path_overridden); } + #[rstest] + #[case::build("build")] + #[case::named_tool("clean")] + fn execution_logging_preserves_operation_label(#[case] operation: &str) { + let event = captured_execution_event(operation); + + assert!( + event.contains(&format!("operation={operation:?}")), + "execution event should retain its operation label: {event}" + ); + assert!( + event.contains("message=Executing command"), + "execution event should identify command execution: {event}" + ); + } + + #[cfg(unix)] + fn failed_exit_status() -> ExitStatus { + use std::os::unix::process::ExitStatusExt; + + ExitStatus::from_raw(1 << 8) + } + + #[cfg(windows)] + fn failed_exit_status() -> ExitStatus { + use std::os::windows::process::ExitStatusExt; + + ExitStatus::from_raw(1) + } + + #[test] + fn exit_failure_logging_records_status_diagnostics() { + let event = + crate::test_tracing_capture::with_test_subscriber(LevelFilter::WARN, |captured| { + log_command_exit_failure( + &logging_context(), + "clean", + StderrMode::Suppress, + failed_exit_status(), + ); + let events = captured.snapshot(); + let [event] = events.as_slice() else { + panic!("expected one exit failure event, got {events:?}"); + }; + event.clone() + }); + + assert!( + event.contains("operation=\"clean\""), + "exit failure event should retain its operation label: {event}" + ); + assert!( + event.contains("failure_category=\"exit_status\""), + "exit failure event should identify the failure category: {event}" + ); + assert!( + event.contains("status="), + "exit failure event should include the process status: {event}" + ); + } + /// A Unix variable named `Path` is not `PATH`, and must not raise the flag. /// /// Kept separate from the table above because the expectation is diff --git a/src/runner/process/configure.rs b/src/runner/process/configure.rs index d0f69fd52..19984620f 100644 --- a/src/runner/process/configure.rs +++ b/src/runner/process/configure.rs @@ -10,7 +10,9 @@ use std::process::{Command, Stdio}; use camino::Utf8PathBuf; -use super::{Cli, CommandEnv, NinjaBuildRequest, NinjaToolRequest, canonicalize_utf8_path}; +use super::{ + CommandEnv, NinjaBuildRequest, NinjaProcessOptions, NinjaToolRequest, canonicalize_utf8_path, +}; /// Configure the base Ninja command with working directory, job count, and build file. /// @@ -18,16 +20,16 @@ use super::{Cli, CommandEnv, NinjaBuildRequest, NinjaToolRequest, canonicalize_u /// flags after this function returns. fn configure_ninja_base( cmd: &mut Command, - cli: &Cli, + options: &NinjaProcessOptions, build_file: &Path, env: &CommandEnv, ) -> io::Result<()> { env.apply(cmd); - if let Some(dir) = &cli.directory { - let canonical = canonicalize_utf8_path(dir.as_path())?; + if let Some(dir) = &options.working_dir { + let canonical = canonicalize_utf8_path(dir.as_std_path())?; cmd.current_dir(canonical.as_std_path()); } - if let Some(jobs) = cli.jobs { + if let Some(jobs) = options.jobs { cmd.arg("-j").arg(jobs.to_string()); } let build_file_path = canonicalize_utf8_path(build_file).or_else(|_| { @@ -51,7 +53,7 @@ pub(super) fn configure_ninja_build_command( cmd: &mut Command, request: &NinjaBuildRequest<'_>, ) -> io::Result<()> { - configure_ninja_base(cmd, request.cli, request.build_file, request.env)?; + configure_ninja_base(cmd, request.options, request.build_file, request.env)?; let targets = request.targets; cmd.args(targets.as_slice()); Ok(()) @@ -61,7 +63,112 @@ pub(super) fn configure_ninja_tool_command( cmd: &mut Command, request: &NinjaToolRequest<'_>, ) -> io::Result<()> { - configure_ninja_base(cmd, request.cli, request.build_file, request.env)?; + configure_ninja_base(cmd, request.options, request.build_file, request.env)?; cmd.arg("-t").arg(request.tool); Ok(()) } + +#[cfg(test)] +mod tests { + //! Unit tests for Ninja command configuration without spawning Ninja. + + use super::*; + use crate::runner::{NinjaJobCount, StderrMode}; + use anyhow::{Result, ensure}; + use rstest::{fixture, rstest}; + use std::ffi::{OsStr, OsString}; + use tempfile::NamedTempFile; + + #[fixture] + fn temp_file() -> Result { + Ok(NamedTempFile::new()?) + } + + #[fixture] + fn options() -> io::Result { + Ok(NinjaProcessOptions { + jobs: Some(NinjaJobCount::try_new(4)?), + ..NinjaProcessOptions::default() + }) + } + + #[fixture] + fn env() -> CommandEnv { + CommandEnv::inherit() + } + + fn command_arguments(cmd: &Command) -> Vec { + cmd.get_args().map(OsStr::to_os_string).collect() + } + + fn expected_base_arguments(build_file: &Path) -> Result> { + Ok(vec![ + OsString::from("-j"), + OsString::from("4"), + OsString::from("-f"), + build_file.canonicalize()?.into_os_string(), + ]) + } + + #[rstest] + fn build_configuration_preserves_argument_order( + temp_file: Result, + options: io::Result, + env: CommandEnv, + ) -> Result<()> { + let build_file = temp_file?; + let resolved_options = options?; + let target_names = vec![String::from("default")]; + let targets = super::super::BuildTargets::new(&target_names); + let request = NinjaBuildRequest { + program: Path::new("ninja"), + options: &resolved_options, + build_file: build_file.path(), + targets: &targets, + env: &env, + stderr_mode: StderrMode::Forward, + }; + let mut cmd = Command::new("ninja"); + + configure_ninja_build_command(&mut cmd, &request)?; + + let mut expected = expected_base_arguments(build_file.path())?; + expected.push(OsString::from("default")); + let actual = command_arguments(&cmd); + ensure!( + actual == expected, + "build command argument order changed: {actual:?}" + ); + Ok(()) + } + + #[rstest] + fn tool_configuration_preserves_argument_order( + temp_file: Result, + options: io::Result, + env: CommandEnv, + ) -> Result<()> { + let build_file = temp_file?; + let resolved_options = options?; + let request = NinjaToolRequest { + program: Path::new("ninja"), + options: &resolved_options, + build_file: build_file.path(), + tool: "clean", + env: &env, + stderr_mode: StderrMode::Forward, + }; + let mut cmd = Command::new("ninja"); + + configure_ninja_tool_command(&mut cmd, &request)?; + + let mut expected = expected_base_arguments(build_file.path())?; + expected.extend([OsString::from("-t"), OsString::from("clean")]); + let actual = command_arguments(&cmd); + ensure!( + actual == expected, + "tool command argument order changed: {actual:?}" + ); + Ok(()) + } +} diff --git a/src/runner/process/exit_status_tests.rs b/src/runner/process/exit_status_tests.rs new file mode 100644 index 000000000..6dcce99d9 --- /dev/null +++ b/src/runner/process/exit_status_tests.rs @@ -0,0 +1,156 @@ +//! Focused tests for Ninja exit diagnostics and execution operation labels. + +use super::child_exit::check_exit_status_with_context; +use super::*; +use crate::test_tracing_capture::with_test_subscriber; +use monotony::test_util::FixedMonotonicClock; +use std::{ + process::ExitStatus, + time::{Duration, Instant}, +}; +use tracing_subscriber::filter::LevelFilter; + +#[cfg(unix)] +fn exit_status(code: i32) -> ExitStatus { + use std::os::unix::process::ExitStatusExt; + + ExitStatus::from_raw(code << 8) +} + +#[cfg(windows)] +fn exit_status(code: i32) -> ExitStatus { + use std::os::windows::process::ExitStatusExt; + + ExitStatus::from_raw(code as u32) +} + +fn command_log_context() -> CommandLogContext { + CommandLogContext::from_command(&Command::new("ninja")) +} + +fn captured_exit_result(status: ExitStatus) -> (io::Result<()>, Vec) { + let clock = FixedMonotonicClock::with_elapsed(Duration::ZERO); + let context = command_log_context(); + let failure_context = ExitFailureContext { + operation: "clean", + stderr_mode: StderrMode::Suppress, + command_list_failure: None, + clock: &clock, + started_at: Instant::now(), + }; + with_test_subscriber(LevelFilter::WARN, |captured| { + let result = check_exit_status_with_context(status, &context, &failure_context); + (result, captured.snapshot()) + }) +} + +#[test] +fn successful_ninja_exit_returns_ok_without_exit_failure_event() { + let (result, events) = captured_exit_result(exit_status(0)); + + assert!(result.is_ok(), "a successful Ninja exit should succeed"); + assert!( + events.is_empty(), + "a successful Ninja exit should emit no exit-failure warning: {events:?}" + ); +} + +#[test] +fn failed_ninja_exit_records_exit_status_diagnostics() { + let (result, events) = captured_exit_result(exit_status(1)); + + assert!( + result.is_err(), + "a failed Ninja exit should return an error" + ); + let [event] = events.as_slice() else { + panic!("expected one exit-failure event, got {events:?}"); + }; + assert!( + event.contains("operation=\"clean\""), + "exit failure should retain the operation label: {event}" + ); + assert!( + event.contains("failure_category=\"exit_status\""), + "exit failure should identify its category: {event}" + ); + assert!( + event.contains("status="), + "exit failure should include the child status: {event}" + ); +} + +#[cfg(unix)] +fn fake_ninja_program() -> anyhow::Result<(tempfile::TempDir, std::path::PathBuf)> { + use cap_std::{ + ambient_authority, + fs::{Dir, PermissionsExt}, + }; + + let temp_dir = tempfile::tempdir()?; + let directory = Dir::open_ambient_dir(temp_dir.path(), ambient_authority())?; + directory.write("ninja", "#!/bin/sh\nexit 0\n")?; + let mut permissions = directory.metadata("ninja")?.permissions(); + permissions.set_mode(0o700); + directory.set_permissions("ninja", permissions)?; + let program = temp_dir.path().join("ninja"); + Ok((temp_dir, program)) +} + +#[cfg(unix)] +#[test] +fn build_and_tool_execution_preserve_operation_labels() -> anyhow::Result<()> { + let (_program_dir, program) = fake_ninja_program()?; + let build_file = tempfile::NamedTempFile::new()?; + let options = NinjaProcessOptions::default(); + let env = CommandEnv::inherit(); + let target_names = vec![String::from("default")]; + let targets = BuildTargets::new(&target_names); + let clock = FixedMonotonicClock::with_elapsed(Duration::ZERO); + let (build_result, build_events) = with_test_subscriber(LevelFilter::INFO, |captured| { + let result = run_ninja_build_internal( + NinjaBuildRequest { + program: &program, + options: &options, + build_file: build_file.path(), + targets: &targets, + env: &env, + stderr_mode: StderrMode::Forward, + }, + None, + &clock, + ); + (result, captured.snapshot()) + }); + build_result?; + anyhow::ensure!( + build_events + .iter() + .any(|event| event.contains("operation=\"build\"")), + "build execution should log the build operation: {build_events:?}" + ); + + let (tool_result, tool_events) = with_test_subscriber(LevelFilter::INFO, |captured| { + let result = run_ninja_tool_internal( + NinjaToolRequest { + program: &program, + options: &options, + build_file: build_file.path(), + tool: "clean", + env: &env, + stderr_mode: StderrMode::Forward, + }, + None, + &clock, + ); + (result, captured.snapshot()) + }); + tool_result?; + anyhow::ensure!( + tool_events + .iter() + .any(|event| event.contains("operation=\"clean\"")), + "tool execution should log the request tool: {tool_events:?}" + ); + Ok(()) +} diff --git a/src/runner/process/job_count.rs b/src/runner/process/job_count.rs new file mode 100644 index 000000000..0b16bb679 --- /dev/null +++ b/src/runner/process/job_count.rs @@ -0,0 +1,99 @@ +//! Validated Ninja parallel-job counts. +//! +//! The CLI accepts an absent job count (Ninja's own default) or a value +//! between 1 and [`MAX_JOBS`]. Callers without CLI state reach the process +//! layer directly, so the field in [`super::NinjaProcessOptions`] carries this +//! sealed type: construction is fallible, and an out-of-range count cannot be +//! fed to a `-j` flag from any call site. + +use std::{fmt, io}; + +/// Maximum number of parallel Ninja jobs accepted by the process layer. +/// +/// Mirrors the CLI layer's constants (`cli::MAX_JOBS` and the build-script +/// variant); the process layer cannot import them without depending on CLI +/// internals, so they must be kept in step as a single documented bound. +const MAX_JOBS: usize = 64; + +/// A validated count of parallel Ninja jobs. +/// +/// The supported semantics match the CLI's job-count validation: the value is +/// always in `1..=MAX_JOBS`. The field is private, so every instance must pass +/// through [`NinjaJobCount::try_new`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct NinjaJobCount(usize); + +impl NinjaJobCount { + /// Validate a requested job count. + /// + /// # Errors + /// + /// Returns [`io::ErrorKind::InvalidInput`] when `value` lies outside + /// `1..=MAX_JOBS`. + pub fn try_new(value: usize) -> io::Result { + if (1..=MAX_JOBS).contains(&value) { + Ok(Self(value)) + } else { + Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!("Ninja job count must be between 1 and {MAX_JOBS}, got {value}"), + )) + } + } +} + +impl fmt::Display for NinjaJobCount { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.0.fmt(f) + } +} + +#[cfg(test)] +mod tests { + //! Boundary coverage for the job-count invariant. + + use super::*; + use proptest::prelude::*; + + #[test] + fn accepts_the_supported_range_boundaries() { + for value in [1, MAX_JOBS] { + let count = NinjaJobCount::try_new(value) + .unwrap_or_else(|_| panic!("{value} should be a supported job count")); + assert_eq!( + count.to_string(), + value.to_string(), + "the accepted count should keep its value" + ); + } + } + + #[test] + fn rejects_counts_outside_the_supported_range() { + for value in [0, MAX_JOBS + 1] { + let Err(error) = NinjaJobCount::try_new(value) else { + panic!("{value} should be rejected as a job count"); + }; + assert_eq!( + error.kind(), + io::ErrorKind::InvalidInput, + "an out-of-range count should be an invalid-input error" + ); + } + } + + proptest! { + #[test] + fn validates_every_job_count(value in any::()) { + let result = NinjaJobCount::try_new(value); + + if (1..=MAX_JOBS).contains(&value) { + let count = result.expect("supported job counts should be accepted"); + prop_assert_eq!(count.to_string(), value.to_string()); + } else { + let error = result.expect_err("out-of-range job counts should be rejected"); + prop_assert_eq!(error.kind(), io::ErrorKind::InvalidInput); + } + } + } +} diff --git a/src/runner/process/mod.rs b/src/runner/process/mod.rs index 8f04d7c07..f8d892578 100644 --- a/src/runner/process/mod.rs +++ b/src/runner/process/mod.rs @@ -2,16 +2,18 @@ //! Internal to `runner`; public API is defined in `runner.rs`. use super::BuildTargets; -use crate::cli::Cli; use monotony::{MonotonicClock, StdMonotonicClock}; use std::{io, path::Path, process::Command}; mod child_exit; mod command_list_telemetry; mod command_logging; + mod dyndep_files; mod dyndep_retention; mod dyndep_telemetry; +#[cfg(test)] +mod exit_status_tests; mod failure_attribution; mod file_io; mod ninja_program; @@ -40,12 +42,14 @@ use output_forwarding::{StatusObserver, spawn_and_stream_output}; mod command_env; mod configure; +mod job_count; mod request; mod stderr_mode; pub use command_env::CommandEnv; use configure::{configure_ninja_build_command, configure_ninja_tool_command}; +pub use job_count::NinjaJobCount; pub use paths::*; -pub use request::{NinjaBuildRequest, NinjaToolRequest}; +pub use request::{NinjaBuildRequest, NinjaProcessOptions, NinjaToolRequest}; pub use stderr_mode::StderrMode; /// Per-invocation process settings passed only from Ninja setup to execution. @@ -117,13 +121,13 @@ fn run_command_and_stream_with_context( /// # Examples /// /// ```rust,no_run -/// use netsuke::cli::Cli; /// use netsuke::runner::{ -/// BuildTargets, CommandEnv, NinjaBuildRequest, StderrMode, run_ninja_with, +/// BuildTargets, CommandEnv, NinjaBuildRequest, NinjaProcessOptions, StderrMode, +/// run_ninja_with, /// }; /// use std::path::Path; /// -/// let cli = Cli::default(); +/// let options = NinjaProcessOptions::default(); /// let targets = BuildTargets::default(); /// // `inherit()` reproduces `run_ninja`; `with_path` replaces the child's /// // `PATH` outright rather than prepending, so compose the whole value @@ -133,11 +137,11 @@ fn run_command_and_stream_with_context( /// let env = CommandEnv::inherit().with_path(&path); /// run_ninja_with(&NinjaBuildRequest { /// program: Path::new("ninja"), -/// cli: &cli, +/// options: &options, /// build_file: Path::new("build.ninja"), /// targets: &targets, /// env: &env, -/// stderr_mode: StderrMode::from_json_enabled(cli.json), +/// stderr_mode: StderrMode::Forward, /// })?; /// # Ok::<(), std::io::Error>(()) /// ``` @@ -162,18 +166,19 @@ fn run_ninja_with_clock( /// # Examples /// /// ```rust,no_run -/// use netsuke::cli::Cli; -/// use netsuke::runner::{CommandEnv, NinjaToolRequest, StderrMode, run_ninja_tool_with}; +/// use netsuke::runner::{ +/// CommandEnv, NinjaProcessOptions, NinjaToolRequest, StderrMode, run_ninja_tool_with, +/// }; /// use std::path::Path; /// -/// let cli = Cli::default(); +/// let options = NinjaProcessOptions::default(); /// run_ninja_tool_with(&NinjaToolRequest { /// program: Path::new("ninja"), -/// cli: &cli, +/// options: &options, /// build_file: Path::new("build.ninja"), /// tool: "clean", /// env: &CommandEnv::inherit(), -/// stderr_mode: StderrMode::from_json_enabled(cli.json), +/// stderr_mode: StderrMode::Forward, /// })?; /// # Ok::<(), std::io::Error>(()) /// ``` diff --git a/src/runner/process/request.rs b/src/runner/process/request.rs index 27d31b146..c9d65e49b 100644 --- a/src/runner/process/request.rs +++ b/src/runner/process/request.rs @@ -4,17 +4,26 @@ //! 400-line ceiling. These are data, not behaviour: the request types describe //! what an invocation needs; `mod` holds the functions that act on them. -use super::{BuildTargets, CommandEnv, StderrMode}; -use crate::cli::Cli; +use super::{BuildTargets, CommandEnv, NinjaJobCount, StderrMode}; +use camino::Utf8PathBuf; use std::path::Path; +/// Process settings needed to configure a Ninja invocation. +#[derive(Debug, Clone, Default)] +pub struct NinjaProcessOptions { + /// Optional UTF-8 working directory passed to the child process. + pub working_dir: Option, + /// Optional maximum number of parallel Ninja jobs. + pub jobs: Option, +} + /// Borrowed parameter bundle for `ninja` build execution helpers. #[derive(Clone, Copy)] pub struct NinjaBuildRequest<'a> { /// Ninja executable to invoke. pub program: &'a Path, - /// Parsed CLI settings supplying the working directory and job count. - pub cli: &'a Cli, + /// Process settings supplying the working directory and job count. + pub options: &'a NinjaProcessOptions, /// Generated build file passed with `-f`. pub build_file: &'a Path, /// Targets appended after the base flags. @@ -32,8 +41,8 @@ pub struct NinjaBuildRequest<'a> { pub struct NinjaToolRequest<'a> { /// Ninja executable to invoke. pub program: &'a Path, - /// Parsed CLI settings. - pub cli: &'a Cli, + /// Process settings supplying the working directory and job count. + pub options: &'a NinjaProcessOptions, /// Generated build file passed with `-f`. pub build_file: &'a Path, /// Tool name passed to `ninja -t`. diff --git a/src/runner/process/tests.rs b/src/runner/process/tests.rs index 73af61d7a..8fd881210 100644 --- a/src/runner/process/tests.rs +++ b/src/runner/process/tests.rs @@ -6,7 +6,6 @@ use super::child_exit::finalize_streaming; use super::command_list_telemetry::COMMAND_LIST_FAILURE_DURATION; use super::streaming::ForwardStats; use super::*; -use crate::cli::Cli; use crate::test_tracing_capture::with_test_subscriber; use camino::Utf8PathBuf; #[cfg(unix)] @@ -16,7 +15,9 @@ use metrics_util::{ }; use mockable::MockEnv; #[cfg(unix)] -use monotony::{StdMonotonicClock, test_util::FixedMonotonicClock}; +use monotony::StdMonotonicClock; +#[cfg(unix)] +use monotony::test_util::FixedMonotonicClock; use proptest::prelude::*; use rstest::{fixture, rstest}; use std::ffi::OsString; @@ -266,10 +267,59 @@ proptest! { } } +/// The child process runs in the requested working directory. +/// +/// The fake Ninja records its effective current directory to a file whose path +/// travels via an injected environment variable (mirroring the shared +/// run-marker pattern), so no path is interpolated into the shell script. The +/// recorded value is compared against the canonicalised working directory, +/// which is the path the process layer passes to the child. +#[cfg(unix)] +#[test] +fn run_ninja_with_runs_in_the_requested_working_directory() -> anyhow::Result<()> { + use test_support::exec::write_exec_with_content; + + let working_dir = tempfile::tempdir()?; + // `configure_ninja_base` canonicalises the build file before spawning, so + // the manifest must already exist in the working directory. + test_support::fs::write(working_dir.path().join("build.ninja"), "# empty manifest\n")?; + let record_dir = tempfile::tempdir()?; + let observed_file = record_dir.path().join("observed-cwd"); + let fake_ninja = write_exec_with_content( + record_dir.path(), + "fake-ninja", + "#!/bin/sh\npwd > \"${RECORD_CWD_TO}\"\nexit 0\n", + )?; + + let working_dir_utf8 = Utf8PathBuf::from_path_buf(working_dir.path().to_path_buf()) + .map_err(|path| anyhow::anyhow!("tempdir path is not UTF-8: {}", path.display()))?; + let options = NinjaProcessOptions { + working_dir: Some(working_dir_utf8), + ..NinjaProcessOptions::default() + }; + let targets = BuildTargets::default(); + let env = CommandEnv::inherit().with_var("RECORD_CWD_TO", &observed_file); + run_ninja_with(&NinjaBuildRequest { + program: &fake_ninja, + options: &options, + build_file: working_dir.path().join("build.ninja").as_path(), + targets: &targets, + env: &env, + stderr_mode: StderrMode::Forward, + })?; + + let expected = working_dir.path().canonicalize()?; + let recorded = test_support::fs::read_to_string(&observed_file)?; + anyhow::ensure!( + Path::new(recorded.trim()) == expected, + "Ninja should run in the requested working directory: expected {expected:?}, \ + recorded {recorded:?}" + ); + Ok(()) +} + /// Spawning a missing Ninja emits a spawn-failure warning whose -/// `suppress_stderr` field follows the request's explicit `stderr_mode`, not -/// the request's `cli.json` state. The mismatch in each case proves the process -/// layer consumes the policy field and does not re-derive it from CLI JSON. +/// `suppress_stderr` field follows the request's explicit `stderr_mode`. #[test] fn spawn_failure_logging_honours_explicit_stderr_mode() { let cases = [ @@ -277,15 +327,12 @@ fn spawn_failure_logging_honours_explicit_stderr_mode() { (false, StderrMode::Suppress, "suppress_stderr=true"), ]; for (json, mode, expected_field) in cases { - let cli = Cli { - json, - ..Cli::default() - }; + let options = NinjaProcessOptions::default(); let targets = BuildTargets::default(); let events = with_test_subscriber(LevelFilter::WARN, |captured| { let result = run_ninja_with(&NinjaBuildRequest { program: Path::new("netsuke-test-missing-ninja"), - cli: &cli, + options: &options, build_file: Path::new("build.ninja"), targets: &targets, env: &CommandEnv::inherit(), @@ -300,7 +347,7 @@ fn spawn_failure_logging_honours_explicit_stderr_mode() { assert_eq!( events.len(), 1, - "exactly one warning should be captured for {mode:?} with cli.json={json}, \ + "exactly one warning should be captured for {mode:?} with input={json}, \ got: {events:?}" ); // The single captured warning is the spawn failure; inspect it alone so a @@ -311,7 +358,7 @@ fn spawn_failure_logging_honours_explicit_stderr_mode() { assert!( event.contains("failure_category=\"spawn\"") && event.contains(expected_field), "the captured warning should be a spawn failure recording {expected_field} for \ - {mode:?} with cli.json={json}, got: {events:?}" + {mode:?} with input={json}, got: {events:?}" ); } } diff --git a/tests/bdd/steps/process.rs b/tests/bdd/steps/process.rs index 0e9cae4ad..ef2f0f2cb 100644 --- a/tests/bdd/steps/process.rs +++ b/tests/bdd/steps/process.rs @@ -2,7 +2,7 @@ use crate::bdd::fixtures::{RefCellOptionExt, TestWorld}; use anyhow::{Context, Result, anyhow, ensure}; -use camino::Utf8Path; +use camino::{Utf8Path, Utf8PathBuf}; use mockable::{DefaultEnv, Env}; use netsuke::output_prefs; use netsuke::runner::{self, BuildTargets, CommandEnv, NINJA_PROGRAM}; @@ -273,9 +273,26 @@ fn run(world: &TestWorld) -> Result<()> { let result = world .cli .with_ref(|cli| { + let working_dir = cli + .directory + .clone() + .map(Utf8PathBuf::from_path_buf) + .transpose() + .map_err(|path| { + std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!( + "CLI working directory {} is not valid UTF-8", + path.display() + ), + ) + })?; runner::run_ninja_with(&runner::NinjaBuildRequest { program, - cli, + options: &runner::NinjaProcessOptions { + working_dir, + jobs: cli.jobs.map(runner::NinjaJobCount::try_new).transpose()?, + }, build_file: Path::new("build.ninja"), targets: &targets, env: &world.command_env.borrow(), diff --git a/tests/env_path_tests.rs b/tests/env_path_tests.rs index d1e260982..6367e2d92 100644 --- a/tests/env_path_tests.rs +++ b/tests/env_path_tests.rs @@ -184,23 +184,24 @@ fn observed_value(dir: &tempfile::TempDir) -> Result { fn composed_path_reaches_the_spawned_process( probe_fixture: Result<(tempfile::TempDir, PathBuf, PathBuf)>, ) -> Result<()> { - use netsuke::cli::Cli; - use netsuke::runner::{BuildTargets, NinjaBuildRequest, StderrMode, run_ninja_with}; + use netsuke::runner::{ + BuildTargets, NinjaBuildRequest, NinjaProcessOptions, StderrMode, run_ninja_with, + }; use std::path::Path; let (dir, probe, build_file) = probe_fixture?; let parent_before = std::env::var_os("PATH"); let composed = prepend_path_value(parent_before.as_deref(), Path::new("/injected/marker")) .context("compose PATH")?; - let cli = Cli::default(); + let options = NinjaProcessOptions::default(); let targets = BuildTargets::default(); run_ninja_with(&NinjaBuildRequest { program: probe.as_path(), - cli: &cli, + options: &options, build_file: build_file.as_path(), targets: &targets, env: &CommandEnv::inherit().with_path(&composed), - stderr_mode: StderrMode::from_json_enabled(cli.json), + stderr_mode: StderrMode::Forward, }) .context("run the probe")?; @@ -230,8 +231,9 @@ fn unoverridden_parent_variables_are_inherited( #[from(probe_fixture)] baseline: Result<(tempfile::TempDir, PathBuf, PathBuf)>, probe_fixture: Result<(tempfile::TempDir, PathBuf, PathBuf)>, ) -> Result<()> { - use netsuke::cli::Cli; - use netsuke::runner::{BuildTargets, NinjaBuildRequest, StderrMode, run_ninja_with}; + use netsuke::runner::{ + BuildTargets, NinjaBuildRequest, NinjaProcessOptions, StderrMode, run_ninja_with, + }; // Baseline: the probe spawned directly, outside `CommandEnv`, records // the PATH a plainly inherited child sees. Comparing child against child @@ -246,17 +248,17 @@ fn unoverridden_parent_variables_are_inherited( let inherited = observed_value(&baseline_dir)?; let (dir, probe, build_file) = probe_fixture?; - let cli = Cli::default(); + let options = NinjaProcessOptions::default(); let targets = BuildTargets::default(); // The override touches only an unrelated marker; PATH is not configured. run_ninja_with(&NinjaBuildRequest { program: probe.as_path(), - cli: &cli, + options: &options, build_file: build_file.as_path(), targets: &targets, env: &CommandEnv::inherit().with_var("NETSUKE_PROBE_MARKER", "sentinel"), - stderr_mode: StderrMode::from_json_enabled(cli.json), + stderr_mode: StderrMode::Forward, }) .context("run the probe")?; @@ -283,19 +285,18 @@ fn general_overrides_reach_the_spawned_tool_process( PathBuf, )>, ) -> Result<()> { - use netsuke::cli::Cli; - use netsuke::runner::{NinjaToolRequest, StderrMode, run_ninja_tool_with}; + use netsuke::runner::{NinjaProcessOptions, NinjaToolRequest, StderrMode, run_ninja_tool_with}; let (dir, probe, build_file) = probe_fixture?; - let cli = Cli::default(); + let options = NinjaProcessOptions::default(); run_ninja_tool_with(&NinjaToolRequest { program: probe.as_path(), - cli: &cli, + options: &options, build_file: build_file.as_path(), tool: "clean", env: &CommandEnv::inherit().with_var("NETSUKE_PROBE_MARKER", "sentinel"), - stderr_mode: StderrMode::from_json_enabled(cli.json), + stderr_mode: StderrMode::Forward, }) .context("run the probe")?; diff --git a/tests/stderr_routing_tests.rs b/tests/stderr_routing_tests.rs index 1b513dc2d..9bb7df6da 100644 --- a/tests/stderr_routing_tests.rs +++ b/tests/stderr_routing_tests.rs @@ -1,18 +1,19 @@ -//! Request-level stream-routing tests with deliberately mismatched requests. +//! Request-level stream-routing tests with explicit policies. //! -//! The process layer must honour the request's explicit `stderr_mode` field -//! rather than whatever `cli.json` happens to say. The routing happens on real -//! child streams, so the worker runs in a dedicated subprocess whose stdout -//! and stderr the parent captures: the worker builds a request whose -//! `cli.json` contradicts its `stderr_mode`, runs a marker-emitting fake -//! Ninja, and the parent asserts where the markers landed. +//! The process layer routes the child's stdout and stderr by the request's +//! explicit `stderr_mode` field alone. Converting CLI state into a policy +//! (`StderrMode::from_json_enabled`) happens upstream of these request types +//! and never reaches this layer. The routing happens on real child streams, so +//! the worker runs in a dedicated subprocess whose stdout and stderr the parent +//! captures: the worker builds a request carrying a fixed `stderr_mode`, runs a +//! marker-emitting fake Ninja, and the parent asserts where the markers landed. +#![cfg(unix)] use anyhow::{Context, Result, bail, ensure}; use mockable::{DefaultEnv, Env}; -use netsuke::cli::Cli; use netsuke::runner::{ - BuildTargets, CommandEnv, NinjaBuildRequest, NinjaToolRequest, StderrMode, run_ninja_tool_with, - run_ninja_with, + BuildTargets, CommandEnv, NinjaBuildRequest, NinjaProcessOptions, NinjaToolRequest, StderrMode, + run_ninja_tool_with, run_ninja_with, }; use std::path::{Path, PathBuf}; use std::process::Command; @@ -36,7 +37,6 @@ const STDERR_MARKER: &str = "NETSUKE_ROUTING_STDERR_MARKER"; /// the shell script. The run-marker file is written directly by the child, /// independent of any stream routing; it proves a suppression assertion /// observed output *because the child ran*, not because it never spawned. -#[cfg(unix)] fn marker_emitting_ninja() -> Result<(TempDir, PathBuf)> { let dir = tempdir().context("create fake-ninja directory")?; let script = format!( @@ -49,7 +49,6 @@ fn marker_emitting_ninja() -> Result<(TempDir, PathBuf)> { } /// Spawn the routing worker (this test binary) and capture its output. -#[cfg(unix)] fn run_routing_worker(job: &str, tool: bool, ninja: &Path, ran_file: &Path) -> Result { let mut command = Command::new(std::env::current_exe().context("locate test binary")?); command @@ -66,10 +65,9 @@ fn run_routing_worker(job: &str, tool: bool, ninja: &Path, ran_file: &Path) -> R /// Execute one request-level routing case selected by `stderr_mode` and the /// request path (`tool` selects the `ninja -t` request). /// -/// The request's `cli.json` deliberately contradicts `stderr_mode` (see the -/// `routing_worker` description), so the marker assertions prove the process -/// layer routes child streams by the explicit policy, not by the JSON setting. -#[cfg(unix)] +/// The request carries the explicit policy, so the marker assertions prove the +/// process layer routes child streams by the request field and never consults +/// CLI state to derive it. fn assert_routing_case(stderr_mode: StderrMode, tool: bool) -> Result<()> { let job = match stderr_mode { StderrMode::Forward => "forward", @@ -80,7 +78,6 @@ fn assert_routing_case(stderr_mode: StderrMode, tool: bool) -> Result<()> { } else { "build request" }; - let json = matches!(stderr_mode, StderrMode::Forward); let ran_dir = tempdir().context("create run-marker directory")?; let ran_file = ran_dir.path().join("ran"); @@ -100,35 +97,31 @@ fn assert_routing_case(stderr_mode: StderrMode, tool: bool) -> Result<()> { StderrMode::Forward => { ensure!( stdout.contains(STDOUT_MARKER), - "{path_label} with StderrMode::Forward should forward child stdout despite \ - cli.json={json}, got: {stdout}" + "{path_label} with StderrMode::Forward should forward child stdout, got: {stdout}" ); ensure!( stderr.contains(STDERR_MARKER), - "{path_label} with StderrMode::Forward should forward child stderr despite \ - cli.json={json}, got: {stderr}" + "{path_label} with StderrMode::Forward should forward child stderr, got: {stderr}" ); } StderrMode::Suppress => { ensure!(ran_file.exists(), "fake Ninja should have run"); ensure!( !stdout.contains(STDOUT_MARKER) && !stdout.contains(STDERR_MARKER), - "{path_label} with StderrMode::Suppress should drain child stdout despite \ - cli.json={json}, got: {stdout}" + "{path_label} with StderrMode::Suppress should drain child stdout, got: {stdout}" ); ensure!( !stderr.contains(STDOUT_MARKER) && !stderr.contains(STDERR_MARKER), - "{path_label} with StderrMode::Suppress should drain child stderr despite \ - cli.json={json}, got: {stderr}" + "{path_label} with StderrMode::Suppress should drain child stderr, got: {stderr}" ); } } Ok(()) } -/// A request whose `cli.json` and `stderr_mode` deliberately disagree: the -/// process layer must route streams by the explicit policy, not by `cli.json`. -#[cfg(unix)] +/// Worker body: build a request carrying the explicit `stderr_mode` selected +/// by the parent, run it against the fake Ninja, and let the parent assert the +/// stream routing from the captured child markers. #[test] #[ignore = "invoked as a stream-routing worker"] fn routing_worker() -> Result<()> { @@ -145,18 +138,13 @@ fn routing_worker() -> Result<()> { "suppress" => StderrMode::Suppress, other => bail!("unknown routing job {other:?}"), }; - // The JSON flag contradicts `stderr_mode` so routing can only come from - // the explicit policy field, not from the CLI JSON setting. - let cli = Cli { - json: !stderr_mode.is_suppress(), - ..Cli::default() - }; + let options = NinjaProcessOptions::default(); let targets = BuildTargets::default(); let env = CommandEnv::inherit(); let result = if tool { run_ninja_tool_with(&NinjaToolRequest { program: &ninja, - cli: &cli, + options: &options, build_file: Path::new("build.ninja"), tool: "clean", env: &env, @@ -165,7 +153,7 @@ fn routing_worker() -> Result<()> { } else { run_ninja_with(&NinjaBuildRequest { program: &ninja, - cli: &cli, + options: &options, build_file: Path::new("build.ninja"), targets: &targets, env: &env, @@ -175,36 +163,30 @@ fn routing_worker() -> Result<()> { result.context("run Ninja invocation in routing worker") } -/// cli.json=true with `stderr_mode=Forward` on a build request: the child -/// markers must reach the user's stdout and stderr, proving routing ignores -/// the JSON setting. -#[cfg(unix)] +/// A build request carrying `stderr_mode=Forward`: the child markers must +/// reach the user's stdout and stderr. #[test] -fn forward_request_routes_child_streams_despite_json_cli() -> Result<()> { +fn forward_request_routes_child_streams() -> Result<()> { assert_routing_case(StderrMode::Forward, false) } -/// cli.json=true with `stderr_mode=Forward` on the tool request: the child -/// markers must reach the user, proving the tool path also ignores cli.json. -#[cfg(unix)] +/// A tool request carrying `stderr_mode=Forward`: the child markers must +/// reach the user's stdout and stderr. #[test] -fn forward_tool_request_routes_child_streams_despite_json_cli() -> Result<()> { +fn forward_tool_request_routes_child_streams() -> Result<()> { assert_routing_case(StderrMode::Forward, true) } -/// cli.json=false with `stderr_mode=Suppress` on a build request: both child -/// markers must be drained even though the human CLI would otherwise forward -/// them, and the run marker proves the child really executed. -#[cfg(unix)] +/// A build request carrying `stderr_mode=Suppress`: both child markers must be +/// drained, and the run marker proves the child really executed. #[test] -fn suppress_request_drains_child_streams_despite_human_cli() -> Result<()> { +fn suppress_request_drains_child_streams() -> Result<()> { assert_routing_case(StderrMode::Suppress, false) } -/// cli.json=false with `stderr_mode=Suppress` on the tool request: both child -/// markers must be drained, with the run marker proving the child executed. -#[cfg(unix)] +/// A tool request carrying `stderr_mode=Suppress`: both child markers must be +/// drained, with the run marker proving the child executed. #[test] -fn suppress_tool_request_drains_child_streams_despite_human_cli() -> Result<()> { +fn suppress_tool_request_drains_child_streams() -> Result<()> { assert_routing_case(StderrMode::Suppress, true) } diff --git a/tests/ui/command_env_embedder_pass.rs b/tests/ui/command_env_embedder_pass.rs index 7f7e712c0..5d26d6b66 100644 --- a/tests/ui/command_env_embedder_pass.rs +++ b/tests/ui/command_env_embedder_pass.rs @@ -9,16 +9,15 @@ use std::io; use std::path::Path; -use netsuke::cli::Cli; use netsuke::runner::{ - BuildTargets, CommandEnv, NinjaBuildRequest, NinjaToolRequest, StderrMode, run_ninja_tool_with, - run_ninja_with, + BuildTargets, CommandEnv, NinjaBuildRequest, NinjaProcessOptions, NinjaToolRequest, + StderrMode, run_ninja_tool_with, run_ninja_with, }; /// The pieces an embedder would hold before building requests. struct Parts<'a> { program: &'a Path, - cli: &'a Cli, + options: &'a NinjaProcessOptions, build_file: &'a Path, targets: &'a BuildTargets<'a>, env: &'a CommandEnv, @@ -28,19 +27,19 @@ struct Parts<'a> { fn compose_requests<'a>(parts: &Parts<'a>) -> (NinjaBuildRequest<'a>, NinjaToolRequest<'a>) { let build = NinjaBuildRequest { program: parts.program, - cli: parts.cli, + options: parts.options, build_file: parts.build_file, targets: parts.targets, env: parts.env, - stderr_mode: StderrMode::from_json_enabled(parts.cli.json), + stderr_mode: StderrMode::Forward, }; let tool = NinjaToolRequest { program: parts.program, - cli: parts.cli, + options: parts.options, build_file: parts.build_file, tool: "clean", env: parts.env, - stderr_mode: StderrMode::from_json_enabled(parts.cli.json), + stderr_mode: StderrMode::Forward, }; (build, tool) } @@ -53,11 +52,11 @@ fn main() -> io::Result<()> { assert!(!env.is_empty()); assert!(env.get("PATH").is_some()); - let cli = Cli::default(); + let options = NinjaProcessOptions::default(); let targets = BuildTargets::default(); let parts = Parts { program: Path::new("ninja"), - cli: &cli, + options: &options, build_file: Path::new("build.ninja"), targets: &targets, env: &env,