diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 2da36302e..25eab092a 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -3208,6 +3208,42 @@ context rather than resolving output or process configuration again; tests should inject the program through `run_with_ninja_program` when they need a deterministic child executable. +### Module: `runner::reporter` + +`src/runner/reporter.rs` owns construction of the run's `StatusReporter` from +resolved output settings. `ReporterOptions` bundles the resolved output mode, +progress preference, verbose preference, output preferences, and whether +standard output is a TTY. `make_reporter(options)` selects the base reporter, +`AccessibleReporter` or `IndicatifReporter` when progress is enabled and +`SilentReporter` otherwise, then wraps it in `VerboseTimingReporter` when +verbose mode is active. `should_force_text_task_updates` decides whether the +indicatif reporter emits textual task updates, forcing them for accessible +mode or non-TTY standard output. + +`run_with_ninja_program` (in `src/runner/mod.rs`) constructs the run's +`StatusReporter` through `reporter::make_reporter` after resolving output mode +and reporter settings, then shares it via the `ExecutionContext` it passes to +`dispatch::execute`. + +#### Reuse boundary + +- **Ownership:** `runner::reporter` is an internal, non-public submodule of + the runner; it owns all `StatusReporter` construction and the + concrete-reporter selection rules. Nothing outside it builds the run's + reporter, and it is not part of the crate's public API. +- **Permitted call-sites:** only the runner boundary in `src/runner/mod.rs` + may call `make_reporter` — today solely `run_with_ninja_program`. + `runner::process`, dispatch handlers, and external embedders never + construct reporters; handlers consume the already-built reporter through + the `ExecutionContext`/`&dyn StatusReporter` only. +- **Composition rules:** the caller must resolve all `ReporterOptions` inputs + (output mode, progress, verbose, output prefs, stdout TTY) from + CLI/environment state before calling `make_reporter`; the module performs + no such resolution itself. The reporter is composed once per run and + shared immutably. New reporter kinds or selection policies belong in this + module beside the mode-selection logic, colocated with the output-mode + policy. + ### Module: `runner::process::ninja_program` `src/runner/process/ninja_program.rs` owns the executable-resolution boundary. @@ -3235,8 +3271,9 @@ All command events share these structured fields: - `operation`: caller-provided operation label such as `"build"` or tool name. - `ninja_program`: command program after UTF-8 normalization. -- `suppress_stderr`: derived from `cli.json`, true when JSON output suppresses - direct child-process streams. +- `suppress_stderr`: bool derived from the `StderrMode` policy via + `stderr_mode.is_suppress()`, true when the policy suppresses direct + child-process streams. Phase-specific fields supplement that shared set. The informational execution event includes `arg_count`. Spawn- and exit-failure events instead set @@ -3259,10 +3296,15 @@ paths: 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, - `cli.json` as `suppress_stderr`, and the chosen `operation`. + the request's `stderr_mode` policy, and the chosen `operation`. 4. Let `run_command_and_stream_with_context` handle span creation, execution logging, failure logging, and exit-status enforcement via context helpers. +The `StderrMode` policy type is independent of `Cli`; the runner derives the +policy at request-build time with `StderrMode::from_json_enabled(cli.json)`, +while the process layer consumes the request's `stderr_mode` field and never +reads `cli.json` itself. + ### Module: `runner::process::redaction` `src/runner/process/redaction.rs` owns the argument-redaction boundary that @@ -3366,10 +3408,13 @@ through the child `PATH` is acceptable. What the injected `PATH` always 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 an `env: &CommandEnv` field 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` -call these with `CommandEnv::inherit()`, reproducing production behaviour; +`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 +`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 [design document](netsuke-design.md) records the same architecture from the diff --git a/docs/execplans/3-10-1-guarantee-status-message-ordering.md b/docs/execplans/3-10-1-guarantee-status-message-ordering.md index ed566706d..8f527ee2e 100644 --- a/docs/execplans/3-10-1-guarantee-status-message-ordering.md +++ b/docs/execplans/3-10-1-guarantee-status-message-ordering.md @@ -183,7 +183,8 @@ The output architecture spans several modules: - `report_pipeline_stage()` helper for stage transitions. 4. **Runner module** (`src/runner/mod.rs`): - - `make_reporter()`: Factory function for selecting reporter. + - `make_reporter()`: Factory function for selecting reporter, defined in + `src/runner/reporter.rs`. - `handle_build()` and `handle_ninja_tool()`: Orchestrate build execution. - `on_task_progress_callback()`: Bridges Ninja status parsing to reporter. diff --git a/docs/execplans/3-10-2-consistent-log-prefixes.md b/docs/execplans/3-10-2-consistent-log-prefixes.md index e05e6d8c2..df1c30794 100644 --- a/docs/execplans/3-10-2-consistent-log-prefixes.md +++ b/docs/execplans/3-10-2-consistent-log-prefixes.md @@ -121,7 +121,7 @@ without breaking any existing functionality. `IndicatifReporter` accepts `OutputPrefs` and uses success prefix - `src/status_timing.rs`: `VerboseTimingReporter` accepts `OutputPrefs` and uses timing prefix with indented detail lines -- `src/runner/mod.rs`: Updated reporter factory +- `src/runner/reporter.rs`: Updated reporter factory - `tests/features/progress_output.feature`: Updated and expanded scenarios - `docs/roadmap.md`: Marked 3.10.2 as complete @@ -138,7 +138,7 @@ without breaking any existing functionality. 5. `locales/en-US/messages.ftl` — English locale messages. 6. `locales/es-ES/messages.ftl` — Spanish locale messages. 7. `tests/features/progress_output.feature` — BDD progress scenarios. -8. `src/runner/mod.rs` — Reporter factory (`make_reporter()`). +8. `src/runner/reporter.rs` — Reporter factory (`make_reporter()`). ### New prefix design diff --git a/docs/execplans/3-10-3-json-diagnostics-mode.md b/docs/execplans/3-10-3-json-diagnostics-mode.md index 138e713d2..c9cedd897 100644 --- a/docs/execplans/3-10-3-json-diagnostics-mode.md +++ b/docs/execplans/3-10-3-json-diagnostics-mode.md @@ -300,8 +300,9 @@ The current diagnostics path is split across a small number of files: existing `Cli` struct and already derives `OrthoConfig`. - [src/cli_l10n.rs](../../src/cli_l10n.rs) maps clap argument identifiers to Fluent help keys. -- [src/runner/mod.rs](../../src/runner/mod.rs) constructs - status reporters and executes commands. +- [src/runner/reporter.rs](../../src/runner/reporter.rs) + constructs the status reporter, while + [src/runner/mod.rs](../../src/runner/mod.rs) executes commands. - [src/runner/error.rs](../../src/runner/error.rs) contains `RunnerError`, which already implements `miette::Diagnostic`. - [src/manifest/diagnostics/mod.rs](../../src/manifest/diagnostics/mod.rs) @@ -479,9 +480,10 @@ Changes: through CLI or environment hints - render runtime failures as JSON when the merged CLI enables it 2. Suppress `tracing_subscriber` stderr logging when JSON mode is active. -3. In [src/runner/mod.rs](../../src/runner/mod.rs), force - `SilentReporter` when JSON mode is active so no status lines or timing - summaries reach `stderr`. +3. In [src/runner/reporter.rs](../../src/runner/reporter.rs), + `make_reporter` selects `SilentReporter` when progress is disabled + in JSON mode, so no status lines or timing summaries reach + `stderr`. 4. Ensure success-path subcommand output remains unchanged on `stdout`. Acceptance for Stage C: diff --git a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md index 5479d59ff..d710f3eb8 100644 --- a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md +++ b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md @@ -107,8 +107,8 @@ Observable success: task-progress rendering in `IndicatifReporter` and `AccessibleReporter`. - [x] (2026-02-24) Stage C: Added centralized fallback behaviour in - `src/runner/mod.rs` (`should_force_text_task_updates`) to emit textual - task updates when stdout is non-TTY or accessible mode is enabled. + `src/runner/reporter.rs` (`should_force_text_task_updates`) to emit + textual task updates when stdout is non-TTY or accessible mode is enabled. - [x] (2026-02-24) Stage D: Added/updated unit and behavioural coverage using `rstest` and `rstest-bdd`: `src/runner/process/ninja_status.rs`, @@ -184,7 +184,8 @@ Validation summary: Primary implementation surfaces: - `src/runner/process/mod.rs`: child process spawning and output forwarding. -- `src/runner/mod.rs`: reporter construction and pipeline orchestration. +- `src/runner/reporter.rs`: reporter construction. +- `src/runner/mod.rs`: pipeline orchestration. - `src/status.rs`: reporter trait and implementations. - `src/status_pipeline.rs`: six-stage canonical ordering and labels. - `src/cli/mod.rs`: OrthoConfig-derived CLI configuration. diff --git a/docs/execplans/3-9-3-per-stage-timing-metrics.md b/docs/execplans/3-9-3-per-stage-timing-metrics.md index 48b2cecee..2fb6eec54 100644 --- a/docs/execplans/3-9-3-per-stage-timing-metrics.md +++ b/docs/execplans/3-9-3-per-stage-timing-metrics.md @@ -183,8 +183,8 @@ What shipped: - a deterministic stage-timing recorder, - a duration formatter (`ns`/`us`/`ms`/`s`), - `VerboseTimingReporter` wrapper with an injectable monotonic clock. -- Wired reporter selection in `src/runner/mod.rs` so verbose mode wraps the - resolved base reporter (including silent progress mode). +- Wired reporter selection in `src/runner/reporter.rs` so verbose mode + wraps the resolved base reporter (including silent progress mode). - Added localized timing summary runtime strings and updated verbose help copy in both locales. - Added `rstest` unit coverage for timing happy/unhappy/edge behaviour. @@ -213,7 +213,8 @@ Primary implementation surfaces: - `src/status.rs`: reporter trait and concrete status reporters. - `src/status_pipeline.rs`: canonical six-stage order and labels. -- `src/runner/mod.rs`: reporter construction and pipeline orchestration. +- `src/runner/reporter.rs`: reporter construction. +- `src/runner/mod.rs`: pipeline orchestration. - `src/cli/mod.rs`: OrthoConfig-derived `verbose` configuration. - `src/cli_l10n.rs`: localized clap help key mapping. - `src/localization/keys.rs`: Fluent key constants. @@ -268,7 +269,7 @@ silent by default. Planned changes: -- Update `runner::make_reporter(...)` to accept a verbose flag and wrap the +- Update `reporter::make_reporter(...)` to accept a verbose flag and wrap the selected base reporter with timing support when `cli.verbose` is true. - Ensure behaviour by mode: - non-verbose: unchanged output (no timing summary), diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 2b3c0c2fa..6fdc3d20f 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2132,10 +2132,13 @@ 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, and a `&CommandEnv` describing -the child's environment. `run_ninja_with` and `run_ninja_tool_with` consume -these; the convenience wrappers `run_ninja` and `run_ninja_tool` call them with -`CommandEnv::inherit()`, which is production behaviour. +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. The command construction follows this pattern: @@ -2171,9 +2174,20 @@ The command construction follows this pattern: to the user's console, potentially with additional formatting or status updates from Netsuke itself. -Standard output and error are piped and written back to Netsuke's own streams -so users see Ninja's messages in order. A non-zero exit status or failure to -spawn the process is reported as an `io::Error` for the CLI to surface. +Stream routing follows the `stderr_mode` policy carried by the request. +Outside JSON mode (`StderrMode::Forward`) the child's standard output and +error are piped and forwarded to Netsuke's own streams concurrently: stdout is +drained on the main thread while a separate thread forwards stderr, so users +see Ninja's messages as it produces them. No relative ordering is guaranteed +between the two streams. In JSON diagnostics mode the request +carries `StderrMode::Suppress`, which drains both streams to `io::sink()`: +stdout carries only the versioned result document and stderr only the +diagnostic document, keeping both machine-readable. The runner derives the +policy from the CLI's JSON setting via +`StderrMode::from_json_enabled(cli.json)`; the process layer consumes the +request's `stderr_mode` field and never re-derives it. A non-zero exit status +or failure to spawn the process is reported as an `io::Error` for the CLI to +surface. The `ninja_subprocess` span and its spawn and exit events carry `env_override_count` and `path_overridden`, derived from the prepared diff --git a/docs/users-guide.md b/docs/users-guide.md index 49c15d959..1c50a12a0 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -591,14 +591,17 @@ its own process environment. `netsuke::runner::CommandEnv` carries child environment overrides as data — `inherit()` changes nothing, `with_var` and `with_path` set variables for the spawned command only — and the explicit request forms `run_ninja_with` and `run_ninja_tool_with` accept a request -naming the program, build file, targets or tool, and that environment. The -convenience wrappers `run_ninja` and `run_ninja_tool` behave identically -with an inherited environment. Overrides are additive: variables not named -are inherited from the calling process, and the injected `PATH` governs -what commands Ninja launches will see. Relative program 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`. +naming the program, build file, targets or tool, that environment, and a +`stderr_mode: StderrMode` policy routing the child's standard streams: +`Suppress` drains both streams (keeping JSON diagnostics machine-readable), +while `Forward` relays them to the caller. The convenience wrappers +`run_ninja` and `run_ninja_tool` behave identically with an inherited +environment, deriving the policy from the CLI's JSON setting. Overrides are +additive: variables not named are inherited from the calling process, and +the injected `PATH` governs what commands Ninja launches will see. Relative +program 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`. The request itself is a named type: `netsuke::runner::NinjaBuildRequest` for a build and `netsuke::runner::NinjaToolRequest` for `ninja -t `. Both @@ -611,7 +614,7 @@ summarizes these additions and confirms the wrappers are unchanged. ```rust use netsuke::cli::Cli; use netsuke::runner::{ - BuildTargets, CommandEnv, NinjaBuildRequest, NinjaToolRequest, run_ninja_tool_with, + BuildTargets, CommandEnv, NinjaBuildRequest, NinjaToolRequest, StderrMode, run_ninja_tool_with, run_ninja_with, }; use std::path::Path; @@ -632,6 +635,9 @@ let build = NinjaBuildRequest { 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), }; let clean = NinjaToolRequest { program: Path::new("/usr/bin/ninja"), @@ -639,6 +645,7 @@ let clean = NinjaToolRequest { build_file: Path::new("build.ninja"), tool: "clean", env: &env, + stderr_mode: StderrMode::from_json_enabled(cli.json), }; if std::env::var_os("NETSUKE_GUIDE_RUN").is_some() { @@ -647,11 +654,13 @@ if std::env::var_os("NETSUKE_GUIDE_RUN").is_some() { } ``` -These types are additive: `run_ninja` and `run_ninja_tool` keep their existing -signatures and behaviour, so an existing caller needs no change. 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. +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. +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. ## Use the template standard library diff --git a/docs/v0-1-0-migration-guide.md b/docs/v0-1-0-migration-guide.md index e4675700e..6e537f2ef 100644 --- a/docs/v0-1-0-migration-guide.md +++ b/docs/v0-1-0-migration-guide.md @@ -22,7 +22,7 @@ Table: v0.1.0 child-environment API additions and their impact | --- | --- | --- | | 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, and targets or tool for the `*_with` run functions. | [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) | | 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) | @@ -30,7 +30,10 @@ Table: v0.1.0 child-environment API additions and their impact The convenience wrappers keep their signatures and their behaviour: the child inherits the calling process's environment, and Ninja is resolved -exactly as before. No caller needs to change to adopt this release. +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)`. ## Opting into ordered command lists diff --git a/src/runner/mod.rs b/src/runner/mod.rs index 3a3b43952..085df3233 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -8,21 +8,19 @@ mod dispatch; mod error; +mod reporter; pub use error::RunnerError; use crate::cli::{BuildArgs, Cli, Commands}; use crate::localization::{self, keys}; -use crate::output_mode::{self, OutputMode}; +use crate::output_mode; use crate::output_prefs::OutputPrefs; -use crate::status::{ - AccessibleReporter, IndicatifReporter, LocalizationKey, PipelineStage, SilentReporter, - StatusReporter, VerboseTimingReporter, report_pipeline_stage, -}; +use crate::status::{LocalizationKey, PipelineStage, StatusReporter, report_pipeline_stage}; use crate::{ir::BuildGraph, manifest, ninja_gen}; use anyhow::{Context, Result}; use camino::Utf8PathBuf; -use std::io::IsTerminal; +use std::io::{self, IsTerminal}; use std::path::Path; use tracing::{debug, info}; @@ -47,8 +45,8 @@ mod process; #[cfg(doctest)] pub use process::doc; pub use process::{ - CommandEnv, NinjaBuildRequest, NinjaToolRequest, run_ninja, run_ninja_tool, - run_ninja_tool_with, run_ninja_with, + CommandEnv, NinjaBuildRequest, NinjaToolRequest, StderrMode, run_ninja_tool_with, + run_ninja_with, }; use path_helpers::{ensure_manifest_exists_or_error, resolve_manifest_path, resolve_output_path}; @@ -115,43 +113,6 @@ impl Default for BuildTargets<'_> { } } -/// Build the appropriate [`StatusReporter`] for the resolved output mode, -/// progress preference, verbose preference, and output preferences. -#[derive(Debug, Clone, Copy)] -struct ReporterOptions { - mode: OutputMode, - progress_enabled: bool, - verbose: bool, - prefs: OutputPrefs, - stdout_is_tty: bool, -} - -fn make_reporter(options: ReporterOptions) -> Box { - let base: Box = if options.progress_enabled { - let force_text_task_updates = - should_force_text_task_updates(options.mode, options.stdout_is_tty); - match options.mode { - OutputMode::Accessible => Box::new(AccessibleReporter::new(options.prefs)), - OutputMode::Standard => Box::new(IndicatifReporter::with_force_text_task_updates( - options.prefs, - force_text_task_updates, - )), - } - } else { - Box::new(SilentReporter) - }; - - if options.verbose { - Box::new(VerboseTimingReporter::new(base, options.prefs)) - } else { - base - } -} - -const fn should_force_text_task_updates(mode: OutputMode, stdout_is_tty: bool) -> bool { - mode.is_accessible() || !stdout_is_tty -} - /// Execute the parsed [`Cli`] commands with the given output preferences. /// /// # Errors @@ -174,7 +135,7 @@ pub fn run_with_ninja_program(cli: &Cli, prefs: OutputPrefs, program: &Path) -> let mode = output_mode::resolve(cli.accessibility_override(), Some(cli.color)); let progress_enabled = cli.progress_enabled() && !cli.json; let stdout_is_tty = std::io::stdout().is_terminal(); - let reporter = make_reporter(ReporterOptions { + let reporter = reporter::make_reporter(reporter::ReporterOptions { mode, progress_enabled, verbose: cli.verbose && !cli.json, @@ -193,6 +154,56 @@ pub fn run_with_ninja_program(cli: &Cli, prefs: OutputPrefs, program: &Path) -> 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); @@ -231,6 +242,7 @@ fn handle_build(cli: &Cli, args: &BuildArgs, context: &ExecutionContext<'_>) -> build_file: build_path, targets: &targets, env: &CommandEnv::inherit(), + stderr_mode: StderrMode::from_json_enabled(cli.json), }, &mut on_task_progress, ) @@ -292,6 +304,7 @@ fn handle_ninja_tool( build_file: build_path, tool: tool.name, env: &CommandEnv::inherit(), + stderr_mode: StderrMode::from_json_enabled(cli.json), }, &mut on_task_progress, ) diff --git a/src/runner/process/child_exit.rs b/src/runner/process/child_exit.rs index 4f70da4fe..54a3a8695 100644 --- a/src/runner/process/child_exit.rs +++ b/src/runner/process/child_exit.rs @@ -9,7 +9,7 @@ use std::{ }; use super::{ - command_list_telemetry, + StderrMode, command_list_telemetry, command_logging::{CommandLogContext, log_command_exit_failure}, failure_attribution::CommandListFailure, streaming::ForwardStats, @@ -19,7 +19,7 @@ use super::{ #[derive(Clone, Copy)] pub(super) struct ExitFailureContext<'failure, 'clock, Clock> { pub(super) operation: &'failure str, - pub(super) suppress_stderr: bool, + pub(super) stderr_mode: StderrMode, pub(super) command_list_failure: Option<&'failure CommandListFailure>, pub(super) clock: &'clock Clock, pub(super) started_at: Instant, @@ -38,7 +38,7 @@ pub(super) fn check_exit_status_with_context( log_command_exit_failure( context, failure_context.operation, - failure_context.suppress_stderr, + failure_context.stderr_mode, status, ); if let Some(failure) = failure_context.command_list_failure { diff --git a/src/runner/process/command_logging.rs b/src/runner/process/command_logging.rs index d35501633..0cb78a68b 100644 --- a/src/runner/process/command_logging.rs +++ b/src/runner/process/command_logging.rs @@ -4,6 +4,7 @@ //! for operators, while also attaching stable tracing fields for tools that //! consume structured diagnostics. +use super::StderrMode; use super::command_env::env_names_eq; use super::redaction::{CommandArg, redact_sensitive_args}; use camino::Utf8PathBuf; @@ -89,7 +90,7 @@ impl CommandLogContext { pub(super) fn log_command_execution( context: &CommandLogContext, operation: &str, - suppress_stderr: bool, + stderr_mode: StderrMode, ) { info!( operation, @@ -97,7 +98,7 @@ pub(super) fn log_command_execution( arg_count = context.arg_count, env_override_count = context.env_override_count, path_overridden = context.is_path_overridden, - suppress_stderr, + suppress_stderr = stderr_mode.is_suppress(), "Executing command: {}", context.redacted_command, ); @@ -109,7 +110,7 @@ pub(super) fn log_command_execution( pub(super) fn log_command_spawn_failure( context: &CommandLogContext, operation: &str, - suppress_stderr: bool, + stderr_mode: StderrMode, err: &io::Error, ) { warn!( @@ -117,7 +118,7 @@ pub(super) fn log_command_spawn_failure( ninja_program = %context.program_display, env_override_count = context.env_override_count, path_overridden = context.is_path_overridden, - suppress_stderr, + suppress_stderr = stderr_mode.is_suppress(), failure_category = "spawn", error.kind = ?err.kind(), error = %err, @@ -132,7 +133,7 @@ pub(super) fn log_command_spawn_failure( pub(super) fn log_command_exit_failure( context: &CommandLogContext, operation: &str, - suppress_stderr: bool, + stderr_mode: StderrMode, status: ExitStatus, ) { warn!( @@ -140,7 +141,7 @@ pub(super) fn log_command_exit_failure( ninja_program = %context.program_display, env_override_count = context.env_override_count, path_overridden = context.is_path_overridden, - suppress_stderr, + suppress_stderr = stderr_mode.is_suppress(), failure_category = "exit_status", %status, "Ninja command exited unsuccessfully", @@ -155,7 +156,7 @@ pub(super) fn log_command_exit_failure( pub(super) fn command_span( context: &CommandLogContext, operation: &str, - suppress_stderr: bool, + stderr_mode: StderrMode, ) -> tracing::Span { info_span!( "ninja_subprocess", @@ -164,7 +165,7 @@ pub(super) fn command_span( arg_count = context.arg_count, env_override_count = context.env_override_count, path_overridden = context.is_path_overridden, - suppress_stderr, + suppress_stderr = stderr_mode.is_suppress(), failure_category = field::Empty, ) } diff --git a/src/runner/process/mod.rs b/src/runner/process/mod.rs index 81175c359..9ae69b962 100644 --- a/src/runner/process/mod.rs +++ b/src/runner/process/mod.rs @@ -35,15 +35,17 @@ use output_forwarding::{StatusObserver, spawn_and_stream_output}; mod command_env; mod configure; mod request; +mod stderr_mode; pub use command_env::CommandEnv; use configure::{configure_ninja_build_command, configure_ninja_tool_command}; pub use paths::*; pub use request::{NinjaBuildRequest, NinjaToolRequest}; +pub use stderr_mode::StderrMode; /// Per-invocation process settings passed only from Ninja setup to execution. struct CommandExecutionContext<'a, Clock> { operation: &'a str, - suppress_stderr: bool, + stderr_mode: StderrMode, captures_ninja_failure_output: bool, clock: &'a Clock, } @@ -74,29 +76,24 @@ fn run_command_and_stream_with_context( execution: &CommandExecutionContext<'_, Clock>, ) -> io::Result<()> { let context = CommandLogContext::from_command(&cmd); - let span = command_span(&context, execution.operation, execution.suppress_stderr); + let span = command_span(&context, execution.operation, execution.stderr_mode); let _entered = span.enter(); - log_command_execution(&context, execution.operation, execution.suppress_stderr); + log_command_execution(&context, execution.operation, execution.stderr_mode); let started_at = execution.clock.now(); let child = cmd.spawn().inspect_err(|err| { tracing::Span::current().record("failure_category", "spawn"); - log_command_spawn_failure( - &context, - execution.operation, - execution.suppress_stderr, - err, - ); + log_command_spawn_failure(&context, execution.operation, execution.stderr_mode, err); })?; let (status, command_list_failure) = spawn_and_stream_output( child, status_observer, - execution.suppress_stderr, + execution.stderr_mode, execution.captures_ninja_failure_output, )?; let failure_context = ExitFailureContext { operation: execution.operation, - suppress_stderr: execution.suppress_stderr, + stderr_mode: execution.stderr_mode, command_list_failure: command_list_failure.as_ref(), clock: execution.clock, started_at, @@ -104,43 +101,20 @@ fn run_command_and_stream_with_context( check_exit_status_with_context(status, &context, &failure_context) } -/// Invoke the Ninja executable with the provided CLI settings. -/// -/// The function forwards the job count and working directory to Ninja, -/// specifies the temporary build file, and streams its standard output and -/// error back 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(), - }) -} - /// Invoke Ninja with an explicit child-process environment. /// -/// Unlike [`run_ninja`], the caller supplies the environment applied to the -/// spawned command. Tests use this to place a fake Ninja on the child's `PATH` -/// without mutating the parent process, which would race every other test in -/// the same binary. +/// Unlike [`crate::runner::run_ninja`], the caller supplies the environment +/// applied to the spawned command. Tests use this to place a fake Ninja on the +/// child's `PATH` without mutating the parent process, which would race every +/// other test in the same binary. /// /// # Examples /// /// ```rust,no_run /// use netsuke::cli::Cli; -/// use netsuke::runner::{BuildTargets, CommandEnv, NinjaBuildRequest, run_ninja_with}; +/// use netsuke::runner::{ +/// BuildTargets, CommandEnv, NinjaBuildRequest, StderrMode, run_ninja_with, +/// }; /// use std::path::Path; /// /// let cli = Cli::default(); @@ -157,6 +131,7 @@ pub fn run_ninja( /// build_file: Path::new("build.ninja"), /// targets: &targets, /// env: &env, +/// stderr_mode: StderrMode::from_json_enabled(cli.json), /// })?; /// # Ok::<(), std::io::Error>(()) /// ``` @@ -176,33 +151,13 @@ fn run_ninja_with_clock( run_ninja_build_internal(*request, None, clock) } -/// Invoke a Ninja tool (e.g., `ninja -t clean`) with the provided CLI settings. -/// -/// The function forwards the job count and working directory to Ninja, -/// specifies the build file, and streams its standard output and error back 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(), - }) -} - /// Invoke a Ninja tool with an explicit child-process environment. /// /// # Examples /// /// ```rust,no_run /// use netsuke::cli::Cli; -/// use netsuke::runner::{CommandEnv, NinjaToolRequest, run_ninja_tool_with}; +/// use netsuke::runner::{CommandEnv, NinjaToolRequest, StderrMode, run_ninja_tool_with}; /// use std::path::Path; /// /// let cli = Cli::default(); @@ -212,6 +167,7 @@ pub fn run_ninja_tool(program: &Path, cli: &Cli, build_file: &Path, tool: &str) /// build_file: Path::new("build.ninja"), /// tool: "clean", /// env: &CommandEnv::inherit(), +/// stderr_mode: StderrMode::from_json_enabled(cli.json), /// })?; /// # Ok::<(), std::io::Error>(()) /// ``` @@ -226,7 +182,7 @@ pub fn run_ninja_tool_with(request: &NinjaToolRequest<'_>) -> io::Result<()> { struct NinjaInternalRequest<'request, 'observer> { program: &'request Path, - cli: &'request Cli, + stderr_mode: StderrMode, status_observer: Option>, operation: &'request str, captures_ninja_failure_output: bool, @@ -245,7 +201,7 @@ where configure(&mut cmd)?; let execution = CommandExecutionContext { operation: request.operation, - suppress_stderr: request.cli.json, + stderr_mode: request.stderr_mode, captures_ninja_failure_output: request.captures_ninja_failure_output, clock, }; @@ -259,7 +215,7 @@ fn run_ninja_build_internal( run_ninja_internal( NinjaInternalRequest { program: request.program, - cli: request.cli, + stderr_mode: request.stderr_mode, status_observer, operation: "build", captures_ninja_failure_output: true, @@ -277,7 +233,7 @@ fn run_ninja_tool_internal( run_ninja_internal( NinjaInternalRequest { program: request.program, - cli: request.cli, + stderr_mode: request.stderr_mode, status_observer, operation: request.tool, captures_ninja_failure_output: false, diff --git a/src/runner/process/output_forwarding.rs b/src/runner/process/output_forwarding.rs index ecc1ab2c9..9a7f7583a 100644 --- a/src/runner/process/output_forwarding.rs +++ b/src/runner/process/output_forwarding.rs @@ -1,6 +1,7 @@ //! Forward Ninja output while preserving bounded command-list attribution. use super::{ + StderrMode, child_exit::{finalize_streaming, terminate_child}, failure_attribution::{ CommandListFailure, NinjaFailureOutputTail, forward_stderr_with_attribution, @@ -60,7 +61,7 @@ where pub(super) fn spawn_and_stream_output( mut child: Child, status_observer: Option>, - suppress_stderr: bool, + stderr_mode: StderrMode, captures_ninja_failure_output: bool, ) -> io::Result<(ExitStatus, Option)> { let Some(stdout) = child.stdout.take() else { @@ -72,14 +73,13 @@ pub(super) fn spawn_and_stream_output( return Err(io::Error::other("child process missing stderr pipe")); }; - let err_handle = thread::spawn(move || { + let err_handle = thread::spawn(move || match stderr_mode { // Avoid a long-lived stderr lock: status observers invoked while // draining stdout may emit task updates to stderr, and that path must // not block behind stderr forwarding. In JSON diagnostics mode we still // drain child stderr, but discard it to keep stderr machine-readable. - if suppress_stderr { - forward_stderr_with_attribution(BufReader::new(stderr), io::sink()) - } else { + StderrMode::Suppress => forward_stderr_with_attribution(BufReader::new(stderr), io::sink()), + StderrMode::Forward => { forward_stderr_with_attribution(BufReader::new(stderr), io::stderr()) } }); @@ -87,22 +87,25 @@ pub(super) fn spawn_and_stream_output( // Intentionally drain stdout on the main thread when `status_observer` is // present so forwarding and callback-driven status updates keep a stable // ordering; moving this elsewhere can regress output timing/interleaving. - let (stdout_stats, stdout_failure) = if suppress_stderr { - let mut output = io::sink(); - forward_stdout( - stdout, - &mut output, - status_observer, - captures_ninja_failure_output, - ) - } else { - let mut output = io::stdout().lock(); - forward_stdout( - stdout, - &mut output, - status_observer, - captures_ninja_failure_output, - ) + let (stdout_stats, stdout_failure) = match stderr_mode { + StderrMode::Suppress => { + let mut output = io::sink(); + forward_stdout( + stdout, + &mut output, + status_observer, + captures_ninja_failure_output, + ) + } + StderrMode::Forward => { + let mut output = io::stdout().lock(); + forward_stdout( + stdout, + &mut output, + status_observer, + captures_ninja_failure_output, + ) + } }; // Capture the wait result without `?` so the stderr forwarding thread is diff --git a/src/runner/process/request.rs b/src/runner/process/request.rs index a516fdd54..27d31b146 100644 --- a/src/runner/process/request.rs +++ b/src/runner/process/request.rs @@ -4,7 +4,7 @@ //! 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}; +use super::{BuildTargets, CommandEnv, StderrMode}; use crate::cli::Cli; use std::path::Path; @@ -13,8 +13,7 @@ use std::path::Path; pub struct NinjaBuildRequest<'a> { /// Ninja executable to invoke. pub program: &'a Path, - /// Parsed CLI settings supplying the working directory, job count, and - /// diagnostics mode. + /// Parsed CLI settings supplying the working directory and job count. pub cli: &'a Cli, /// Generated build file passed with `-f`. pub build_file: &'a Path, @@ -23,6 +22,9 @@ pub struct NinjaBuildRequest<'a> { /// Environment overrides applied to the child process. Use /// [`CommandEnv::inherit`] to leave the parent environment in place. pub env: &'a CommandEnv, + /// Policy routing the child's standard streams. [`StderrMode::Suppress`] + /// keeps JSON diagnostics machine-readable by draining both streams. + pub stderr_mode: StderrMode, } /// Borrowed parameter bundle for `ninja -t` tool execution helpers. @@ -38,4 +40,7 @@ pub struct NinjaToolRequest<'a> { pub tool: &'a str, /// Environment overrides applied to the child process. pub env: &'a CommandEnv, + /// Policy routing the child's standard streams. [`StderrMode::Suppress`] + /// keeps JSON diagnostics machine-readable by draining both streams. + pub stderr_mode: StderrMode, } diff --git a/src/runner/process/stderr_mode.rs b/src/runner/process/stderr_mode.rs new file mode 100644 index 000000000..4799886f9 --- /dev/null +++ b/src/runner/process/stderr_mode.rs @@ -0,0 +1,86 @@ +//! Policy for routing a Ninja subprocess's standard streams. +//! +//! [`StderrMode`] carries the child-output routing decision explicitly instead +//! of burying it in a boolean re-derived inside the process layer. The runner +//! chooses the policy at the runner boundary when it builds a request: in JSON +//! diagnostics mode ([`crate::cli::Cli`]`::json`) the child's output must not +//! pollute the machine-readable streams, so the mode maps to [`Suppress`]; +//! otherwise it maps to [`Forward`]. The policy travels on +//! [`crate::runner::NinjaBuildRequest`] and [`crate::runner::NinjaToolRequest`] +//! as the `stderr_mode` field, and the process layer only consumes that field — +//! it never re-derives the policy from CLI state itself. +//! +//! [`Forward`] releases the child's stdout and stderr to the parent's +//! corresponding streams, preserving ordering for builds whose output the user +//! watches live. [`Suppress`] drains both streams to `io::sink()`, keeping JSON +//! diagnostics machine-readable: stdout carries only the versioned result +//! document and stderr only the diagnostic document, with no child output mixed +//! in. + +/// Policy for routing a Ninja subprocess's standard streams. +/// +/// Governs both child stdout and child stderr routing: [`StderrMode::Suppress`] +/// keeps JSON diagnostics machine-readable by draining both streams to +/// `io::sink()`, while [`StderrMode::Forward`] releases them to the parent's +/// corresponding streams. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum StderrMode { + /// Forward child stdout and stderr to the parent's streams. + Forward, + /// Drain both child streams, discarding their output. + Suppress, +} + +impl StderrMode { + /// Derive the policy from whether JSON diagnostics are enabled. + /// + /// # Examples + /// + /// ``` + /// use netsuke::runner::StderrMode; + /// + /// assert_eq!(StderrMode::from_json_enabled(true), StderrMode::Suppress); + /// assert_eq!(StderrMode::from_json_enabled(false), StderrMode::Forward); + /// ``` + #[must_use] + pub const fn from_json_enabled(json: bool) -> Self { + if json { Self::Suppress } else { Self::Forward } + } + + /// Return `true` when the policy drains child streams to `io::sink()`. + /// + /// # Examples + /// + /// ``` + /// use netsuke::runner::StderrMode; + /// + /// assert!(StderrMode::Suppress.is_suppress()); + /// assert!(!StderrMode::Forward.is_suppress()); + /// ``` + #[must_use] + pub const fn is_suppress(self) -> bool { + matches!(self, Self::Suppress) + } +} + +#[cfg(test)] +mod tests { + //! Unit tests for `StderrMode` policy derivation. + + use super::*; + use rstest::rstest; + + #[rstest] + #[case(true, StderrMode::Suppress)] + #[case(false, StderrMode::Forward)] + fn from_json_enabled_maps_boolean(#[case] json: bool, #[case] expected: StderrMode) { + assert_eq!(StderrMode::from_json_enabled(json), expected); + } + + #[rstest] + #[case(StderrMode::Suppress, true)] + #[case(StderrMode::Forward, false)] + fn is_suppress_reflects_variant(#[case] mode: StderrMode, #[case] expected: bool) { + assert_eq!(mode.is_suppress(), expected); + } +} diff --git a/src/runner/process/tests.rs b/src/runner/process/tests.rs index 9925f0230..8488bb1dd 100644 --- a/src/runner/process/tests.rs +++ b/src/runner/process/tests.rs @@ -6,6 +6,8 @@ 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)] use metrics_util::{ @@ -18,6 +20,7 @@ use monotony::{StdMonotonicClock, test_util::FixedMonotonicClock}; use proptest::prelude::*; use rstest::{fixture, rstest}; use std::ffi::OsString; +use std::path::Path; #[cfg(unix)] use std::path::PathBuf; #[cfg(unix)] @@ -25,6 +28,7 @@ use std::process::Stdio; use std::thread; #[cfg(unix)] use std::time::Duration; +use tracing_subscriber::filter::LevelFilter; /// A `MockEnv` answering exactly one `os_string` read of `NETSUKE_NINJA`. /// @@ -165,7 +169,7 @@ fn command_list_failure_duration_uses_the_injected_monotonic_clock() { let execution = CommandExecutionContext { operation: "build", - suppress_stderr: true, + stderr_mode: StderrMode::Suppress, captures_ninja_failure_output: false, clock: &clock, }; @@ -211,7 +215,7 @@ fn large_stdout_cannot_supply_command_list_attribution() -> anyhow::Result<()> { .stderr(Stdio::piped()); let execution = CommandExecutionContext { operation: "build", - suppress_stderr: true, + stderr_mode: StderrMode::Suppress, // Only a Ninja build can relay a subcommand's stderr through stdout. // An arbitrary command's large stdout must be forwarded untouched. captures_ninja_failure_output: false, @@ -254,3 +258,53 @@ proptest! { prop_assert_eq!(resolved, expected); } } + +/// 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. +#[test] +fn spawn_failure_logging_honours_explicit_stderr_mode() { + let cases = [ + (true, StderrMode::Forward, "suppress_stderr=false"), + (false, StderrMode::Suppress, "suppress_stderr=true"), + ]; + for (json, mode, expected_field) in cases { + let cli = Cli { + json, + ..Cli::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, + build_file: Path::new("build.ninja"), + targets: &targets, + env: &CommandEnv::inherit(), + stderr_mode: mode, + }); + assert!( + result.is_err(), + "spawning a missing Ninja should fail before any forwarding" + ); + captured.snapshot() + }); + assert_eq!( + events.len(), + 1, + "exactly one warning should be captured for {mode:?} with cli.json={json}, \ + got: {events:?}" + ); + // The single captured warning is the spawn failure; inspect it alone so a + // stray event carrying the expected field cannot mask a missing frame. + let event = events + .first() + .expect("the exactly-one-warning assertion above guarantees a first event"); + 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:?}" + ); + } +} diff --git a/src/runner/reporter.rs b/src/runner/reporter.rs new file mode 100644 index 000000000..d89d340c5 --- /dev/null +++ b/src/runner/reporter.rs @@ -0,0 +1,68 @@ +//! Construction of the run's [`StatusReporter`] from resolved output settings. + +use crate::output_mode::OutputMode; +use crate::output_prefs::OutputPrefs; +use crate::status::{ + AccessibleReporter, IndicatifReporter, SilentReporter, StatusReporter, VerboseTimingReporter, +}; + +/// Build the appropriate [`StatusReporter`] for the resolved output mode, +/// progress preference, verbose preference, and output preferences. +#[derive(Debug, Clone, Copy)] +pub(super) struct ReporterOptions { + pub(super) mode: OutputMode, + pub(super) progress_enabled: bool, + pub(super) verbose: bool, + pub(super) prefs: OutputPrefs, + pub(super) stdout_is_tty: bool, +} + +pub(super) fn make_reporter(options: ReporterOptions) -> Box { + let base: Box = if options.progress_enabled { + let force_text_task_updates = + should_force_text_task_updates(options.mode, options.stdout_is_tty); + match options.mode { + OutputMode::Accessible => Box::new(AccessibleReporter::new(options.prefs)), + OutputMode::Standard => Box::new(IndicatifReporter::with_force_text_task_updates( + options.prefs, + force_text_task_updates, + )), + } + } else { + Box::new(SilentReporter) + }; + + if options.verbose { + Box::new(VerboseTimingReporter::new(base, options.prefs)) + } else { + base + } +} + +const fn should_force_text_task_updates(mode: OutputMode, stdout_is_tty: bool) -> bool { + mode.is_accessible() || !stdout_is_tty +} + +#[cfg(test)] +mod tests { + //! Unit tests for the forced-text-update predicate. + + use super::*; + use rstest::rstest; + + #[rstest] + #[case(OutputMode::Standard, true, false)] + #[case(OutputMode::Standard, false, true)] + #[case(OutputMode::Accessible, true, true)] + #[case(OutputMode::Accessible, false, true)] + fn force_text_task_updates_when_required( + #[case] mode: OutputMode, + #[case] stdout_is_tty: bool, + #[case] expected: bool, + ) { + assert_eq!( + should_force_text_task_updates(mode, stdout_is_tty), + expected + ); + } +} diff --git a/src/runner/tests.rs b/src/runner/tests.rs index bc0b608db..7bd9c488a 100644 --- a/src/runner/tests.rs +++ b/src/runner/tests.rs @@ -21,19 +21,3 @@ fn resolve_output_path_respects_directory( let resolved = resolve_output_path(&cli, Path::new(input)); assert_eq!(resolved.as_ref(), Path::new(expected)); } - -#[rstest] -#[case(OutputMode::Standard, true, false)] -#[case(OutputMode::Standard, false, true)] -#[case(OutputMode::Accessible, true, true)] -#[case(OutputMode::Accessible, false, true)] -fn force_text_task_updates_when_required( - #[case] mode: OutputMode, - #[case] stdout_is_tty: bool, - #[case] expected: bool, -) { - assert_eq!( - should_force_text_task_updates(mode, stdout_is_tty), - expected - ); -} diff --git a/tests/bdd/steps/process.rs b/tests/bdd/steps/process.rs index cd0fbd945..0e9cae4ad 100644 --- a/tests/bdd/steps/process.rs +++ b/tests/bdd/steps/process.rs @@ -279,6 +279,7 @@ fn run(world: &TestWorld) -> Result<()> { build_file: Path::new("build.ninja"), targets: &targets, env: &world.command_env.borrow(), + stderr_mode: runner::StderrMode::from_json_enabled(cli.json), }) }) .ok_or_else(|| anyhow!("CLI configuration has not been initialised"))? diff --git a/tests/env_path_tests.rs b/tests/env_path_tests.rs index d34d7c118..d1e260982 100644 --- a/tests/env_path_tests.rs +++ b/tests/env_path_tests.rs @@ -185,7 +185,7 @@ fn composed_path_reaches_the_spawned_process( probe_fixture: Result<(tempfile::TempDir, PathBuf, PathBuf)>, ) -> Result<()> { use netsuke::cli::Cli; - use netsuke::runner::{BuildTargets, NinjaBuildRequest, run_ninja_with}; + use netsuke::runner::{BuildTargets, NinjaBuildRequest, StderrMode, run_ninja_with}; use std::path::Path; let (dir, probe, build_file) = probe_fixture?; let parent_before = std::env::var_os("PATH"); @@ -200,6 +200,7 @@ fn composed_path_reaches_the_spawned_process( build_file: build_file.as_path(), targets: &targets, env: &CommandEnv::inherit().with_path(&composed), + stderr_mode: StderrMode::from_json_enabled(cli.json), }) .context("run the probe")?; @@ -230,7 +231,7 @@ fn unoverridden_parent_variables_are_inherited( probe_fixture: Result<(tempfile::TempDir, PathBuf, PathBuf)>, ) -> Result<()> { use netsuke::cli::Cli; - use netsuke::runner::{BuildTargets, NinjaBuildRequest, run_ninja_with}; + use netsuke::runner::{BuildTargets, NinjaBuildRequest, StderrMode, run_ninja_with}; // Baseline: the probe spawned directly, outside `CommandEnv`, records // the PATH a plainly inherited child sees. Comparing child against child @@ -255,6 +256,7 @@ fn unoverridden_parent_variables_are_inherited( 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), }) .context("run the probe")?; @@ -282,7 +284,7 @@ fn general_overrides_reach_the_spawned_tool_process( )>, ) -> Result<()> { use netsuke::cli::Cli; - use netsuke::runner::{NinjaToolRequest, run_ninja_tool_with}; + use netsuke::runner::{NinjaToolRequest, StderrMode, run_ninja_tool_with}; let (dir, probe, build_file) = probe_fixture?; let cli = Cli::default(); @@ -293,6 +295,7 @@ fn general_overrides_reach_the_spawned_tool_process( 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), }) .context("run the probe")?; diff --git a/tests/logging_stderr/json.rs b/tests/logging_stderr/json.rs index df76259a5..1a2d0cb3c 100644 --- a/tests/logging_stderr/json.rs +++ b/tests/logging_stderr/json.rs @@ -1,7 +1,8 @@ //! JSON diagnostic, result-envelope, and standard stderr integration tests. -use super::support::{open_workspace, temp_with_minimal_manifest}; +use super::support::{open_workspace, temp_with_minimal_manifest, write_fake_ninja_script}; use anyhow::{Context, Result, ensure}; +use camino::Utf8Path; #[cfg(unix)] use netsuke::runner::NINJA_ENV; use predicates::prelude::*; @@ -286,3 +287,51 @@ fn json_success_graph_keeps_clean_stderr( ); Ok(()) } + +/// `--json build` with a fake Ninja that writes markers to both stdout and +/// stderr: JSON mode must suppress child output entirely, so stderr stays +/// empty and stdout holds exactly one result document with no child marker +/// leaking in. Guards the process layer against forwarding child streams in +/// JSON mode. +#[cfg(unix)] +#[test] +fn json_mode_suppresses_child_stdout_and_stderr_markers() -> Result<()> { + let temp = temp_with_minimal_manifest()?; + let workspace = open_workspace(&temp)?; + // The capability-scoped directory writes the script under a relative + // name; the absolute path is reserved for `NINJA_ENV`, since program + // resolution must not depend on the child's working directory. + write_fake_ninja_script( + &workspace, + Utf8Path::new("fake-ninja-markers"), + &["NINJA_STDOUT_MARKER_LINE_1", "NINJA_STDOUT_MARKER_LINE_2"], + Some("NINJA_STDERR_MARKER"), + )?; + let ninja_std = temp.path().join("fake-ninja-markers"); + let output = assert_cmd::cargo::cargo_bin_cmd!("netsuke") + .current_dir(temp.path()) + .env(NINJA_ENV, ninja_std.as_path()) + .arg("--json") + .arg("build") + .output() + .context("run netsuke --json build with marker-emitting fake ninja")?; + + ensure!(output.status.success(), "build should succeed"); + ensure!( + output.stderr.is_empty(), + "JSON mode should suppress child stderr: {:?}", + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8(output.stdout).context("stdout should be valid UTF-8")?; + ensure!( + !stdout.contains("NINJA_STDOUT_MARKER") && !stdout.contains("NINJA_STDERR_MARKER"), + "child markers must not leak into JSON output: {stdout}" + ); + let document: Value = + serde_json::from_str(&stdout).context("stdout should hold exactly one JSON document")?; + ensure!( + document.get("schema_version").and_then(Value::as_u64) == Some(1), + "build result should use schema version 1: {document}" + ); + Ok(()) +} diff --git a/tests/stderr_routing_tests.rs b/tests/stderr_routing_tests.rs new file mode 100644 index 000000000..1b513dc2d --- /dev/null +++ b/tests/stderr_routing_tests.rs @@ -0,0 +1,210 @@ +//! Request-level stream-routing tests with deliberately mismatched requests. +//! +//! 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. + +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, +}; +use std::path::{Path, PathBuf}; +use std::process::Command; +use tempfile::{TempDir, tempdir}; +use test_support::exec::write_exec_with_content; + +const NINJA_ENV: &str = "NETSUKE_TEST_ROUTING_NINJA"; +const JOB_ENV: &str = "NETSUKE_TEST_ROUTING_JOB"; +const TOOL_ENV: &str = "NETSUKE_TEST_ROUTING_TOOL"; +const RAN_FILE_ENV: &str = "NETSUKE_TEST_ROUTING_RAN_FILE"; +const WORKER_NAME: &str = "routing_worker"; + +const STDOUT_MARKER: &str = "NETSUKE_ROUTING_STDOUT_MARKER"; +const STDERR_MARKER: &str = "NETSUKE_ROUTING_STDERR_MARKER"; + +/// Write a fake Ninja that emits both stream markers and, separately, records +/// a run-marker file so the parent can tell whether the child ran at all. +/// +/// The run-marker path travels in `NETSUKE_TEST_ROUTING_RAN_FILE` (inherited by +/// the child through [`CommandEnv::inherit`]), so no path is interpolated into +/// 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!( + "#!/bin/sh\nprintf '%s\\n' '{STDOUT_MARKER}'\nprintf '%s\\n' '{STDERR_MARKER}' >&2\n\ + touch \"${RAN_FILE_ENV}\"\nexit 0\n", + ); + let path = write_exec_with_content(dir.path(), "fake-ninja", &script) + .context("write fake ninja executable")?; + Ok((dir, path)) +} + +/// 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 + .args(["--ignored", "--exact", WORKER_NAME, "--nocapture"]) + .env(NINJA_ENV, ninja) + .env(JOB_ENV, job) + .env(RAN_FILE_ENV, ran_file); + if tool { + command.env(TOOL_ENV, "1"); + } + Ok(command) +} + +/// 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)] +fn assert_routing_case(stderr_mode: StderrMode, tool: bool) -> Result<()> { + let job = match stderr_mode { + StderrMode::Forward => "forward", + StderrMode::Suppress => "suppress", + }; + let path_label = if tool { + "tool request" + } 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"); + let (_ninja_dir, ninja) = marker_emitting_ninja()?; + let output = run_routing_worker(job, tool, &ninja, &ran_file)? + .output() + .with_context(|| format!("run {stderr_mode:?} {path_label} worker"))?; + ensure!( + output.status.success(), + "{stderr_mode:?} {path_label} worker failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + let stdout = String::from_utf8(output.stdout).context("worker stdout should be UTF-8")?; + let stderr = String::from_utf8(output.stderr).context("worker stderr should be UTF-8")?; + match stderr_mode { + StderrMode::Forward => { + ensure!( + stdout.contains(STDOUT_MARKER), + "{path_label} with StderrMode::Forward should forward child stdout despite \ + cli.json={json}, got: {stdout}" + ); + ensure!( + stderr.contains(STDERR_MARKER), + "{path_label} with StderrMode::Forward should forward child stderr despite \ + cli.json={json}, 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}" + ); + ensure!( + !stderr.contains(STDOUT_MARKER) && !stderr.contains(STDERR_MARKER), + "{path_label} with StderrMode::Suppress should drain child stderr despite \ + cli.json={json}, 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)] +#[test] +#[ignore = "invoked as a stream-routing worker"] +fn routing_worker() -> Result<()> { + let process_env = DefaultEnv; + let job = process_env.raw(JOB_ENV).context("read routing job")?; + let tool = process_env.os_string(TOOL_ENV).is_some(); + let ninja = PathBuf::from( + process_env + .os_string(NINJA_ENV) + .context("read fake ninja path")?, + ); + let stderr_mode = match job.as_str() { + "forward" => StderrMode::Forward, + "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 targets = BuildTargets::default(); + let env = CommandEnv::inherit(); + let result = if tool { + run_ninja_tool_with(&NinjaToolRequest { + program: &ninja, + cli: &cli, + build_file: Path::new("build.ninja"), + tool: "clean", + env: &env, + stderr_mode, + }) + } else { + run_ninja_with(&NinjaBuildRequest { + program: &ninja, + cli: &cli, + build_file: Path::new("build.ninja"), + targets: &targets, + env: &env, + stderr_mode, + }) + }; + 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)] +#[test] +fn forward_request_routes_child_streams_despite_json_cli() -> 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)] +#[test] +fn forward_tool_request_routes_child_streams_despite_json_cli() -> 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)] +#[test] +fn suppress_request_drains_child_streams_despite_human_cli() -> 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)] +#[test] +fn suppress_tool_request_drains_child_streams_despite_human_cli() -> 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 7eb2bf723..7f7e712c0 100644 --- a/tests/ui/command_env_embedder_pass.rs +++ b/tests/ui/command_env_embedder_pass.rs @@ -11,7 +11,7 @@ use std::path::Path; use netsuke::cli::Cli; use netsuke::runner::{ - BuildTargets, CommandEnv, NinjaBuildRequest, NinjaToolRequest, run_ninja_tool_with, + BuildTargets, CommandEnv, NinjaBuildRequest, NinjaToolRequest, StderrMode, run_ninja_tool_with, run_ninja_with, }; @@ -32,6 +32,7 @@ fn compose_requests<'a>(parts: &Parts<'a>) -> (NinjaBuildRequest<'a>, NinjaToolR build_file: parts.build_file, targets: parts.targets, env: parts.env, + stderr_mode: StderrMode::from_json_enabled(parts.cli.json), }; let tool = NinjaToolRequest { program: parts.program, @@ -39,6 +40,7 @@ fn compose_requests<'a>(parts: &Parts<'a>) -> (NinjaBuildRequest<'a>, NinjaToolR build_file: parts.build_file, tool: "clean", env: parts.env, + stderr_mode: StderrMode::from_json_enabled(parts.cli.json), }; (build, tool) }