Fallout-build / Fallout-build/Fallout

Put an ILogger abstraction over Serilog and unify the two console output paths

Open
#428 1 comment 0 reactions 0 assignees View on GitHub
enhancement needs-triage target/vCurrent
Dominant language
C#
Stars
154
Forks
19
Avg merge
1d 22h
Merged PRs (30d)
15

Description

## Description

Refactor Fallout's console/logging output to sit behind a clean abstraction and collapse the **two parallel output paths** that exist today.

### Problem

Serilog is already the core logger — `Log.*` runs a real Serilog pipeline (console / host-CI / in-memory / file sinks, configured in `src/Fallout.Build/Logging.cs::Configure`). But **all UI/presentation output bypasses Serilog**: the logo, the target-outcome table, `WriteBlock` banners, build success/fail lines, and the "Errors & Warnings" block call static `Host.Success/Information/...` (`src/Fallout.Build/Host.Theming.cs`) → `IHostTheme.Write*` → raw `Console.WriteLine(Format(text, ansiCode))` (`src/Fallout.Build/Theming/AnsiConsoleHostTheme.cs:106`).

The seam joining the two paths is `IHostTheme`, which does awkward double duty: it implements the direct-write methods **and** inherits Serilog's `ConsoleTheme` so it can be cast `(ConsoleTheme)Theme` and handed to the console sink (`Logging.cs:103`).

Resulting smells:
- `Host.WriteErrorsAndWarnings` (`Host.cs:85`) **rebuilds the global `Log.Logger`** just to flush in-memory events — there is a literal `// TODO: move to Logging`.
- Direct-write UI lines do **not** reach the file/CI sinks and are **not** subject to the level switch / verbosity, unlike `Log.*` output — inconsistent behaviour for "the same" output.
- Serilog types **leak into the public API**: `Logging.LevelSwitch` (public `LoggingLevelSwitch`), the `ConsoleTheme` base classes on the public theme types, and `LogEventLevel` in the public `LogLevelPattern` / `DefaultLogLevel` attributes (`src/Fallout.Tooling/ToolTasks.Logger.cs`).

### Colour requirement (resolved)

Colourful console output is a hard requirement. Findings:
- **`ILogger` loses no colour** — colour is a provider/formatter concern, not an `ILogger`-API concern.
- **Serilog's console sink already does rich per-token colour** (ANSI/256/truecolor) but **not layout** (tables/boxes/progress) — which is exactly why the rich UI bypasses it today.
- **A custom `ILoggerProvider` was rejected** — zero colour benefit, and it would reimplement everything Serilog already gives us (level switch, in-memory capture, CI-annotation sink, file sinks + rolling cleanup, enrichers, filters).
- **Spectre.Console** (already a pinned dependency, used by the CLI prompts) is the right tool for the rich-UI part.

### Target architecture (hybrid)

1. **Logging** — code against `Microsoft.Extensions.Logging.ILogger`, keeping **Serilog as the provider** behind it via `Serilog.Extensions.Logging` (`SerilogLoggerFactory`). The existing Serilog pipeline is unchanged.
2. **Theming** — keep an `IHostTheme`-style interface but **decouple it from Serilog** (stop inheriting `ConsoleTheme`); add an internal `SerilogConsoleThemeAdapter : ConsoleTheme` that wraps the theme's colour data for the sink.
3. **Rich UI** — move the logo / outcome table / banners to **Spectre.Console** behind a new internal `IHostOutput` presenter, eliminating the direct-`Console.WriteLine` bypass and fixing `WriteErrorsAndWarnings`.

### Key decisions

- **Façade-over-DI, not full constructor injection.** The `IFalloutCommand` + MS.DI container introduced in #d0452cae lives **only in `Fallout.Cli`** — the build engine (`BuildManager.Execute`) is fully static (`new T()`, static `Log` / `Host.Instance`, ~50 call sites). So: introduce a composition root inside `BuildManager.Execute` (a per-run `ServiceCollection` + a shared `AddFalloutLogging` extension, also registered in the CLI container), but **expose the resolved logger / theme / output through the existing static façades** so the ~50 call sites are unchanged.
- **Adopt Spectre.Console now** (not deferred) for the rich-UI rendering.

### Additive-now vs deferred-breaking

Per the backwards-compatibility rules in `AGENTS.md` (#1/#2), removing the public Serilog types is a **breaking change** and must batch to the next yearly major on `experimental`.

**Additive now (`main`, `target/2026`):**
- ILogger seam + composition root + `AddFalloutLogging`; log façade re-pointed (Serilog `Log.*` keeps working).
- Decoupled `IHostTheme` colour model + `SerilogConsoleThemeAdapter` (keep the old `ConsoleTheme` base classes in place so the cast still compiles).
- Spectre `IHostOutput` presenter; fix `WriteErrorsAndWarnings` (no `Log.Logger` rebuild).
- `[Obsolete]` / `[Experimental("FALLOUT002")]` markers on the Serilog leaks; add `Fallout.Common.LogLevel` overloads to `LogLevelPattern` / `DefaultLogLevel`; register the experimental ID in `docs/experimental-apis.md`.

**Deferred to `experimental` (breaking, next yearly major):**
- Remove `ConsoleTheme` base classes from the `IHostTheme` impls; remove `public LevelSwitch`; remove `LogEventLevel` / `ConsoleThemeStyle` from public signatures. Each gets the `breaking-change` label + `⚠️ Breaking change` callout + CHANGELOG entry under the next major.

### Proposed PR sequence

1. **(main, additive)** Packages (`Serilog.Extensions.Logging`, `Microsoft.Extensions.Logging.Abstractions`); `AddFalloutLogging` + `ILoggerFactory` registration backed by the existing Serilog pipeline; composition root in `BuildManager.Execute`; static log façade re-pointed. No behaviour change. + bridge tests.
2. **(main, additive)** Decoupled `IHostTheme` colour model + `SerilogConsoleThemeAdapter`; rewire `Logging.cs:103` / `Host.cs:96` to use the adapter. Adapter parity tests + baseline Verify snapshots.
3. **(main, additive)** `IHostOutput` Spectre presenter for logo/table/banners/outcome; fix `WriteErrorsAndWarnings`; Spectre `PackageReference` on `Fallout.Build`; CI host overrides updated. Verify-snapshot review.
4. **(main, additive)** `[Obsolete]` / `[Experimental("FALLOUT002")]` markers + new `LogLevel` attribute overloads; register `FALLOUT002` in `docs/experimental-apis.md`; wire the CLI container to the same registrations.
5. **(experimental, breaking — next major)** Remove `ConsoleTheme` base classes, `public LevelSwitch`, and all `LogEventLevel` / `ConsoleThemeStyle` from public signatures.

### Packages (central — `Directory.Packages.props`)

- **New:** `Serilog.Extensions.Logging`, `Microsoft.Extensions.Logging.Abstractions`.
- **Already present (add refs to `Fallout.Build`):** `Microsoft.Extensions.DependencyInjection`, `Spectre.Console`.

### Test strategy

- **Verify snapshots are the regression contract** for every output-format change (logo / table / banner) in ANSI, plain, and redirected modes.
- `SerilogConsoleThemeAdapter` parity tests (same ANSI codes per level/token as today).
- `ILogger` → Serilog bridge tests (correct `LogEventLevel` mapping).
- `LogLevelPattern` / `DefaultLogLevel` overload parity tests.
- `WriteErrorsAndWarnings` test asserting no `Log.Logger` reset.

### Risks / open questions

- The composition root in `BuildManager.Execute` is **new architecture**, not just rewiring.
- Spectre truecolor **downgrade** for the Fallout-yellow logo (`#F5C800`, `Host.cs:36`) must match the current "older sinks ignore the escape bytes" intent.
- `applyThemeToRedirectedOutput: true` (`Logging.cs:104`, `Host.cs:97`) must be preserved — CI logs rely on it.
- 7 CI `*.Theming.cs` overrides (esp. GitHub Actions `::`-prefixed annotations) must not be stranded by the `IHostOutput` split.
- `InMemorySink` is a process-wide singleton (`Logging.cs:214`) — confirm lifetime under per-run providers so the end-of-build summary still captures everything.
- Confirm `FALLOUT002` is the next free experimental diagnostic ID before allocating.

## Usage Example

No change to the public logging surface in the additive phases — existing `Log.Information(...)` and the colored build UI keep working. Internally, components resolve logging through the abstraction instead of touching Serilog directly:

```csharp
// Composition root (BuildManager.Execute) — wired once, façades fed from it
services.AddFalloutLogging(build); // registers ILoggerFactory (Serilog provider), IHostTheme, IHostOutput

// Theme decoupled from Serilog; sink still gets colour via an adapter
configuration.WriteTo.Console(theme: new SerilogConsoleThemeAdapter(hostTheme), ...);

// Rich UI renders through the presenter instead of raw Console.WriteLine
hostOutput.WriteTargetOutcome(build); // Spectre table, not hand-rolled PadRight + ANSI
```

## Alternative

- **Status quo (the hybrid bypass):** keep two output paths — inconsistent sink coverage, the `Log.Logger`-rebuild hack, and the dual-hat `IHostTheme`/`ConsoleTheme`.
- **Push everything through a Serilog sink:** fights the log pipeline for non-log UI (tables/banners) and deepens the public Serilog coupling that we want to be able to unwind.
- **Write a fully custom `ILoggerProvider`:** rejected — reimplements Serilog's sink fan-out / enrichment / capture for no colour benefit.

Contributor guide

Open the contributing guide

Research direction

Start with AGENTS.md, then trace BuildManager.Execute, src/Fallout.Build/Logging.cs, Host.cs, and Host.Theming.cs to understand the existing composition and output paths. Work through the proposed phases, beginning with bridge tests and existing Verify snapshots; done means the listed tests and snapshots cover ILogger mapping, theme parity, rich output modes, and no Log.Logger reset.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
build-system, observability-sre, tooling
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.