From 3c1481c25020aa971e2dd53434e6341c2901e7fa Mon Sep 17 00:00:00 2001 From: Daniel Murta Date: Tue, 29 Sep 2026 15:43:53 -0300 Subject: [PATCH 1/2] feat(contract): wire 1.6.0, build_options in env_file (cli#471 D3) A target owner's before-generate or before-build hook may put `build_options: [{name, value}]` in its env_file. The CLI appends them as -D= to every zig build the environment reaches: the fingerprint pass, the core compile and watched rebuilds. They come after its own arguments, in hook then list order. This is how labelle-ios asks for -Ddevice=true (the RFC's D3), with no full build replacement. - Shape: identifier names, single-line values, no duplicates. optimize and target are refused as CLI-owned. - Gating: only the target owner, only before generate/build, only on negotiated wire >= 1.6.0. Anything else is an invalid env_file error naming the hook. - Conflicts: two hooks giving different values, or an option the CLI's own argv already sets, is an error naming both. - --docker keeps refusing contributing hooks. The supported wire list gains 1.6.0. Fixtures, e2e version assertions and the contract docs are updated, and there is a new e2e build-options section. --- docs/provider-contract-v1.md | 27 ++++- docs/provider-hooks.md | 10 +- src/cli/pipeline/build.zig | 25 ++++- src/cli/pipeline/rebuild.zig | 9 +- src/cli/provider_contract.zig | 52 +++++++++- src/cli/provider_dispatch_test.zig | 2 +- src/cli/provider_env.zig | 155 +++++++++++++++++++++++++++- src/cli/provider_hooks.zig | 20 +++- src/cli/provider_hooks_env_test.zig | 49 +++++++++ src/cli/provider_manifest.zig | 11 +- src/cli/provider_run_outcome.zig | 2 +- src/cli/runner.zig | 12 ++- test/provider_android_like_e2e.py | 2 +- test/provider_dispatch_e2e.py | 2 +- test/provider_hooks_e2e.py | 44 +++++++- test/provider_run_outcome_e2e.py | 2 +- test/provider_targets_e2e.py | 4 +- 17 files changed, 392 insertions(+), 36 deletions(-) diff --git a/docs/provider-contract-v1.md b/docs/provider-contract-v1.md index bbacdca4..cc6787ad 100644 --- a/docs/provider-contract-v1.md +++ b/docs/provider-contract-v1.md @@ -6,7 +6,7 @@ Implementation progress: [project-local dispatch](provider-local-dispatch.md) implements the first executable slice of phase 2. Its explicit limitations do not weaken the normative contract below; full phase-2 acceptance is pending. -This document supplies normative v1 details for [the architecture RFC](rfc-package-commands.md). Where the illustrative RFC conflicts, this contract takes precedence. Migration is breaking: no legacy forwarding or implicit provider injection. Contract negotiation checks a provider's declared semver range against the wire versions the CLI speaks; it never warns and proceeds. See [Wire versions and negotiation](#wire-versions-and-negotiation) below: the CLI implements `1.5.0` and still speaks `1.4.0`, `1.3.0`, `1.2.0`, `1.1.0` and `1.0.0`, and every context carries the negotiated version. +This document supplies normative v1 details for [the architecture RFC](rfc-package-commands.md). Where the illustrative RFC conflicts, this contract takes precedence. Migration is breaking: no legacy forwarding or implicit provider injection. Contract negotiation checks a provider's declared semver range against the wire versions the CLI speaks; it never warns and proceeds. See [Wire versions and negotiation](#wire-versions-and-negotiation) below: the CLI implements `1.6.0` and still speaks `1.5.0`, `1.4.0`, `1.3.0`, `1.2.0`, `1.1.0` and `1.0.0`, and every context carries the negotiated version. ## 1. Package declarations and installed tools @@ -50,7 +50,7 @@ Every field below is required on the wires that define it, except the optional ` | Field | Type / rule | | --- | --- | -| `contract_version` | The negotiated wire version: `"1.0.0"`, `"1.1.0"`, `"1.2.0"`, `"1.3.0"`, `"1.4.0"` or `"1.5.0"` for this decoder | +| `contract_version` | The negotiated wire version: `"1.0.0"`, `"1.1.0"`, `"1.2.0"`, `"1.3.0"`, `"1.4.0"`, `"1.5.0"` or `"1.6.0"` for this decoder | | `invocation` | Object containing `kind`, `id`, `step`, `phase` | | `invocation.kind` | `"command"` or `"hook"` | | `invocation.id` | Command name or hook ID | @@ -111,7 +111,22 @@ A `before generate`, `after generate` or `before build` hook receives `env_file` - **Format.** Strict JSON; both keys are optional; unknown keys, duplicate keys and wrong types are errors. Each `set` entry has exactly `name` and `value`. - **Names** match `[A-Za-z_][A-Za-z0-9_]*`, appear once per file, and are not CLI-owned. The CLI-owned names are a fixed table (`config.reserved_env` in `src/cli/config.zig`), not a `LABELLE_*` prefix ban: `PATH` (extend it with `path_prepend`), `ZIG_GLOBAL_CACHE_DIR`, `ZIG_LOCAL_CACHE_DIR`, every `LABELLE_*` variable the CLI reads (`LABELLE_HOME`, `LABELLE_CONTEXT`, `LABELLE_OFFLINE`, `LABELLE_ZIG`, `LABELLE_ASSEMBLER`, …) and the `labelle run` options it sets for the game. Any other name may be set, `LABELLE_*` or not; a toolchain library directory such as `LABELLE_SDL2_LIB` stays settable. A test fails when the CLI source spells a `LABELLE_*` name the table does not classify. - **`path_prepend`** entries are absolute paths for the host (drive-qualified or UNC on Windows) and contain no PATH separator. -- **File states.** Missing: no contribution, which is normal. Empty, malformed (including a reserved name, a bad name or a relative PATH entry) or larger than 1 MiB: the command fails right after that hook, before any later zig invocation, with `labelle: hook '/' wrote an invalid env_file: `. Written by a hook that then failed: ignored; the hook's failure is the outcome. +- **`build_options`** (wire `1.6.0`+, cli#471 D3): `[{"name": ..., "value": ...}]`, each appended as `-D=` to the core `zig build`. See [Build options](#build-options). +- **File states.** Missing: no contribution, which is normal. Empty, malformed (including a reserved name, a bad name, a relative PATH entry or a refused build option) or larger than 1 MiB: the command fails right after that hook, before any later zig invocation, with `labelle: hook '/' wrote an invalid env_file: `. Written by a hook that then failed: ignored; the hook's failure is the outcome. + +#### Build options + +From wire `1.6.0` the file may also carry `build_options`, the Zig build options the target needs (a device build rather than a simulator one, say): + +```json +{ "set": [ { "name": "SDK_ROOT", "value": "/abs/sdk" } ], "build_options": [ { "name": "device", "value": "true" } ] } +``` + +- **Who.** Only the **target owner**'s `before generate` and `before build` hooks, on a negotiated wire of `1.6.0` or newer. The key from any other provider, from an `after generate` hook or from an owner capped below `1.6.0` makes the file invalid, even as an empty list, with `labelle: hook '/' wrote an invalid env_file: build_options may only come from the owner of target '', not package '

'` (or `... from a 'before generate' or 'before build' hook, not ' '`, or `build_options need provider contract >= 1.6.0; package '

' speaks `). +- **Shape.** Each entry has exactly `name` and `value`. Names match `[A-Za-z_][A-Za-z0-9_-]*` and appear once per file (case-sensitive, like Zig's own options); values are strings without a NUL byte or a line break. +- **CLI-owned options.** `optimize` (the CLI passes `-Doptimize` from `--optimize` or the owner's `target_defaults`) and `target` (`--docker --target`) are refused in the file: `build option 'optimize' is owned by the CLI (it passes -Doptimize itself)`. When the argv is assembled, a contributed option the CLI's own arguments already set fails the command before the zig invocation, naming both: `labelle: build option '-D=' from hook '/' conflicts with the CLI's '-D=...'`. +- **Merge.** As for `set`: hook execution order, then list order; two hooks giving one option different values is `labelle: hook environment conflict: build option '-D' is set to different values by hooks '' and ''`; the same value twice is fine. +- **Scope.** The options follow the CLI's own arguments, in that order, on every later `zig build` of the same build: the generation-time fingerprint pass (`zig build --list-steps -D...`, which a `before generate` contribution reaches and a `before build` one does not) and the core compile (`zig build -Doptimize=ReleaseFast -Ddevice=true`), watched rebuilds included (each starts over, like the environment). They never reach a hook's process, a provider's own tool build or the game. A `replace build` hook stands in for the compile, so there the options reach only the fingerprint pass. A watched rebuild whose options changed publishes normally: the running replacement's environment is what a session compares, not the options. **Merge.** Contributions apply in hook execution order (phases, `after_hooks` edges, then qualified ID) and accumulate across the phases of one build: @@ -169,10 +184,11 @@ The cold build is published as generation `0` before the replacement starts. A f ### Wire versions and negotiation -The CLI implements contract `1.5.0` and speaks every wire version listed here, newest first: +The CLI implements contract `1.6.0` and speaks every wire version listed here, newest first: | Wire | Adds | | --- | --- | +| `1.6.0` | `build_options` in env_file (additive minor, cli#471 D3). The context itself is unchanged; only the target owner's `before generate` / `before build` `env_file` may carry the key. | | `1.5.0` | `outcome_file` in the `run` object (additive minor, cli#473). | | `1.4.0` | `final_step` on every context (additive minor, cli#443). | | `1.3.0` | `cache_dir` and `env_file` on every context, and `watch` in the `run` object (additive minor, CLI 2.1.0). | @@ -182,7 +198,8 @@ The CLI implements contract `1.5.0` and speaks every wire version listed here, n For each invocation the CLI negotiates the **newest** wire version the provider's `command_contract` range admits and writes it as `contract_version`; a range that admits none of them is `UnsupportedContract` at discovery. Keys a wire version does not define are never emitted in it, so a provider decoding strictly (unknown fields are errors, as above) keeps working: -- `>=1.0.0 <2.0.0` admits every additive v1 minor, so it receives `1.5.0` and must accept the keys `1.1.0`, `1.2.0`, `1.3.0`, `1.4.0` and `1.5.0` add. A provider declaring such a range promises exactly that. +- `>=1.0.0 <2.0.0` admits every additive v1 minor, so it receives `1.6.0` and must accept the keys `1.1.0`, `1.2.0`, `1.3.0`, `1.4.0` and `1.5.0` add (`1.6.0` adds none to the context, only what an owner may write). A provider declaring such a range promises exactly that. +- `<1.6.0` (for example `>=1.3.0 <1.6.0`) receives the exact `1.5.0` wire: its `env_file` may not carry `build_options`. - `<1.5.0` (for example `>=1.3.0 <1.5.0`) receives the exact `1.4.0` wire, without `run.outcome_file`: its run replacement cannot report a timeout, so a status-0 exit is always a clean one. - `<1.4.0` (for example `>=1.0.0 <1.4.0`) receives the exact `1.3.0` wire, without `final_step`: its hooks run exactly as on `1.4.0` but cannot tell which command runs them. - `<1.3.0` (for example `>=1.0.0 <1.3.0`) receives the exact `1.2.0` wire, without `cache_dir`, `env_file` or `run.watch`: its hooks cannot contribute an environment and its run replacement cannot run a watch session. diff --git a/docs/provider-hooks.md b/docs/provider-hooks.md index 1bc79a10..ffca42d5 100644 --- a/docs/provider-hooks.md +++ b/docs/provider-hooks.md @@ -275,8 +275,16 @@ pass, which already configures the generated build; an `after generate` hook - The environment is rebuilt for every build, including every watched rebuild, so a hook that stops running leaves nothing behind. +- On wire `1.6.0` the **target owner**'s `before generate` and `before + build` hooks may also write `"build_options": [{"name": "device", + "value": "true"}]`: each becomes `-D=` after the CLI's own + arguments on the fingerprint pass and the core compile. Any other hook, + or an owner capped below `1.6.0`, may not send the key; `optimize` and + `target` are the CLI's. + The full rules are in the contract: -[environment contributions](provider-contract-v1.md#environment-contributions). +[environment contributions](provider-contract-v1.md#environment-contributions) +and [build options](provider-contract-v1.md#build-options). A provider capped below `1.3.0` gets neither key, so its hooks can't contribute. diff --git a/src/cli/pipeline/build.zig b/src/cli/pipeline/build.zig index bfdf0d1d..d6b800e1 100644 --- a/src/cli/pipeline/build.zig +++ b/src/cli/pipeline/build.zig @@ -7,6 +7,7 @@ const runner = @import("../runner.zig"); const bundle = @import("../bundle.zig"); const linux_desktop = @import("../linux_desktop.zig"); const provider_hooks = @import("../provider_hooks.zig"); +const provider_env = @import("../provider_env.zig"); const Context = @import("context.zig").Context; /// Provider hooks on `build` (contract §6) wrap the whole core build — @@ -46,6 +47,11 @@ pub fn run( null; defer if (composed_env) |*m| m.deinit(); const compile_env: ?*const std.process.Environ.Map = if (composed_env) |*m| m else zig_env_ptr; + // The target owner's `build_options` (wire 1.6.0+) follow the CLI's own + // arguments; one the CLI already passes is a conflict naming both. + var args_arena = std.heap.ArenaAllocator.init(allocator); + defer args_arena.deinit(); + const compile_args = try compileArgs(args_arena.allocator(), hook_site, zig_args, reporter); core_build: { if (hook_plans.build.replace) |replacement| { const code = try provider_hooks.runPhase(hook_site, &.{replacement}, .build, .replace, build_out); @@ -72,7 +78,7 @@ pub fn run( // terminal unaltered (nothing is captured or eaten). std.debug.print("labelle: building...\n", .{}); r.beginPhaseOrStep(.compile, "zig build"); - const build_code = try runner.runZigInheritProgress(allocator, target_dir, zig_args, compile_env, r); + const build_code = try runner.runZigInheritProgress(allocator, target_dir, compile_args, compile_env, r); // Wipe the spinner line before anything else prints on it. r.clearSpinner(); if (build_code != 0) { @@ -82,7 +88,7 @@ pub fn run( } } else { std.debug.print("labelle: building...\n", .{}); - const build_result = try runner.runZigWithEnv(allocator, target_dir, zig_args, compile_env); + const build_result = try runner.runZigWithEnv(allocator, target_dir, compile_args, compile_env); defer allocator.free(build_result.stdout); defer allocator.free(build_result.stderr); @@ -129,6 +135,21 @@ pub fn run( return null; } +/// `zig_args` plus the build options the hooks contributed so far (contract +/// §2 `build_options`), or the conflict with a CLI-owned argument reported +/// and `error.BuildOptionConflict`. +pub fn compileArgs(a: std.mem.Allocator, hook_site: *const provider_hooks.Site, zig_args: []const []const u8, reporter: anytype) ![]const []const u8 { + var diag: provider_env.Diagnostic = .{}; + return hook_site.env.zigArgs(a, zig_args, &diag) catch |err| switch (err) { + error.BuildOptionConflict => { + std.debug.print("labelle: {s}\n", .{diag.message}); + if (reporter) |r| r.finishFailed(1, "build option conflict"); + return err; + }, + else => return err, + }; +} + /// `labelle bundle` (cli#359): the exe is built; wrap it. Packaging /// runs AFTER the compile, so keep the progress feed open across it /// (a `run` phase) and only mark `done` once diff --git a/src/cli/pipeline/rebuild.zig b/src/cli/pipeline/rebuild.zig index a30f9000..19aede5e 100644 --- a/src/cli/pipeline/rebuild.zig +++ b/src/cli/pipeline/rebuild.zig @@ -437,7 +437,14 @@ pub const RebuildCtx = struct { null; defer if (composed) |*m| m.deinit(); const env: ?*const std.process.Environ.Map = if (composed) |*m| m else self.zig_env; - const res = runner.runZigWithEnv(a, self.target_dir, self.zig_args, env) catch |err| { + var args_arena = std.heap.ArenaAllocator.init(a); + defer args_arena.deinit(); + var diag: @import("../provider_env.zig").Diagnostic = .{}; + const args = self.hooks.env.zigArgs(args_arena.allocator(), self.zig_args, &diag) catch |err| { + std.debug.print("labelle: rebuild: {s}\n", .{if (err == error.BuildOptionConflict) diag.message else @errorName(err)}); + return error.BuildFailed; + }; + const res = runner.runZigWithEnv(a, self.target_dir, args, env) catch |err| { if (self.canceled()) return error.Canceled; std.debug.print("labelle: rebuild could not spawn zig ({s})\n", .{@errorName(err)}); return error.ZigSpawnFailed; diff --git a/src/cli/provider_contract.zig b/src/cli/provider_contract.zig index 13fce743..a838a7fa 100644 --- a/src/cli/provider_contract.zig +++ b/src/cli/provider_contract.zig @@ -2,18 +2,20 @@ const std = @import("std"); /// The contract version this CLI implements: the newest wire it speaks. -pub const version = "1.5.0"; +pub const version = "1.6.0"; /// Every wire version this CLI can speak, newest first. A minor is additive: /// `1.1.0` is `1.0.0` plus the optional `build_number` key, `1.2.0` is /// `1.1.0` plus `target_dir` and the `run` options, `1.3.0` is `1.2.0` /// plus `cache_dir` and `env_file`, `1.4.0` is `1.3.0` plus -/// `final_step`, and `1.5.0` is `1.4.0` plus `run.outcome_file` (§2). +/// `final_step`, `1.5.0` is `1.4.0` plus `run.outcome_file` (§2), and +/// `1.6.0` is `1.5.0` plus `build_options` in the target owner's +/// `env_file` (§2 "Environment contributions"; the context is unchanged). /// The version a provider receives is negotiated from its `command_contract` range /// (`provider_manifest.negotiate`), so a provider pinned to `<1.1.0` keeps /// receiving the exact `1.0.0` wire and never sees a key it would reject as /// unknown. -pub const supported_versions = [_][]const u8{ version, "1.4.0", "1.3.0", "1.2.0", "1.1.0", "1.0.0" }; +pub const supported_versions = [_][]const u8{ version, "1.5.0", "1.4.0", "1.3.0", "1.2.0", "1.1.0", "1.0.0" }; /// The first wire version that carries `build_number`. pub const build_number_since = "1.1.0"; @@ -33,6 +35,9 @@ pub const final_step_since = "1.4.0"; /// The first wire version whose `run` context carries `outcome_file`. pub const outcome_context_since = "1.5.0"; +/// The first wire version whose `env_file` may carry `build_options`. +pub const build_options_since = "1.6.0"; + fn atLeast(wire_version: []const u8, since: []const u8) bool { const wire = std.SemanticVersion.parse(wire_version) catch return false; const floor = std.SemanticVersion.parse(since) catch unreachable; @@ -74,6 +79,22 @@ pub fn carriesOutcomeContext(wire_version: []const u8) bool { return atLeast(wire_version, outcome_context_since); } +/// True when a provider on the wire `contract_version` may write +/// `build_options` to its `env_file` (the target owner's `before generate` +/// and `before build` hooks only, `buildOptionsSlot`). +pub fn carriesBuildOptions(wire_version: []const u8) bool { + return atLeast(wire_version, build_options_since); +} + +/// Whether the hook `invocation` may contribute `build_options` through its +/// `env_file`, ownership and wire aside: `before generate` and `before build` +/// only (an `after generate` hook's `env_file` may carry the environment but +/// not build options). +pub fn buildOptionsSlot(invocation: Invocation) bool { + if (!envFileSlot(invocation)) return false; + return invocation.phase.? == .before; +} + /// Whether a command whose last lifecycle step is `final` runs the hooks of /// `step` (contract §6): every command runs `generate`; `build`, `run` and /// `bundle` run `build` first; and `run` and `bundle` are alternatives, so @@ -767,7 +788,7 @@ test "build_number is optional on the wire and only for bundle hooks" { } test "a 1.0.0 context never carries build_number; every wire otherwise validates" { - try std.testing.expectEqualStrings("1.5.0", version); + try std.testing.expectEqualStrings("1.6.0", version); try std.testing.expect(carriesBuildNumber("1.1.0")); try std.testing.expect(carriesBuildNumber("1.2.0")); try std.testing.expect(carriesBuildNumber("1.3.0")); @@ -809,10 +830,33 @@ test "a 1.0.0 context never carries build_number; every wire otherwise validates try value.validate(true); value.contract_version = "1.5.0"; try value.validate(true); + // `1.6.0` adds no context key (its `build_options` live in the env_file). value.contract_version = "1.6.0"; + try value.validate(true); + value.contract_version = "1.7.0"; try std.testing.expectError(error.UnsupportedContract, value.validate(true)); } +test "1.6.0: build_options are wire-gated and slot-gated to before generate / before build" { + try std.testing.expect(carriesBuildOptions("1.6.0")); + try std.testing.expect(!carriesBuildOptions("1.5.0")); + try std.testing.expect(!carriesBuildOptions("1.0.0")); + const slot = struct { + fn of(step: Step, phase: Phase) bool { + return buildOptionsSlot(.{ .kind = .hook, .id = "h", .step = step, .phase = phase }); + } + }.of; + try std.testing.expect(slot(.generate, .before)); + try std.testing.expect(slot(.build, .before)); + // `after generate` contributes an environment, never build options. + try std.testing.expect(!slot(.generate, .after)); + try std.testing.expect(!slot(.build, .after)); + try std.testing.expect(!slot(.build, .replace)); + try std.testing.expect(!slot(.run, .before)); + try std.testing.expect(!slot(.bundle, .before)); + try std.testing.expect(!buildOptionsSlot(.{ .kind = .command, .id = "c", .step = null, .phase = null })); +} + /// A project hook context on `wire` for `step`, from the projectless fixture. pub fn hookContext(base: Context, wire: []const u8, step: Step) Context { var value = base; diff --git a/src/cli/provider_dispatch_test.zig b/src/cli/provider_dispatch_test.zig index 861d471a..ce70140e 100644 --- a/src/cli/provider_dispatch_test.zig +++ b/src/cli/provider_dispatch_test.zig @@ -101,7 +101,7 @@ test "provider dispatch: build_number reaches only a provider whose range admits try std.testing.expectEqualStrings("42", open.build_number.?); const open_wire = try std.json.Stringify.valueAlloc(a, open, .{}); try std.testing.expect(std.mem.indexOf(u8, open_wire, "\"build_number\":\"42\"") != null); - try std.testing.expect(std.mem.indexOf(u8, open_wire, "\"contract_version\":\"1.5.0\"") != null); + try std.testing.expect(std.mem.indexOf(u8, open_wire, "\"contract_version\":\"1.6.0\"") != null); // A range capped at the 1.1 wire still gets the key, and nothing newer. provider.meta.command_contract = ">=1.0.0 <1.2.0"; const mid = try wireContext(provider, host, abs, run, cache); diff --git a/src/cli/provider_env.zig b/src/cli/provider_env.zig index 0ec3d4fe..51a229a1 100644 --- a/src/cli/provider_env.zig +++ b/src/cli/provider_env.zig @@ -10,6 +10,12 @@ //! inherited environment; `apply`/`compose` make them on an //! `Environ.Map`. //! +//! - From wire `1.6.0` the target owner's `before generate` / `before build` +//! file may also carry `build_options`, appended as `-D=` to +//! the fingerprint pass and the core compile (`Accumulator.zigArgs`). The +//! owner, slot and wire gates need the hook, so `provider_hooks` applies +//! them; this file checks the shape. +//! //! The Windows rules (case-insensitive names that keep the inherited //! spelling) are a parameter rather than the build target, so they are //! exercised by the tests on every host. @@ -22,13 +28,37 @@ pub const native_windows = builtin.os.tag == .windows; pub const Var = struct { name: []const u8, value: []const u8 }; -/// The `env_file` document. Both keys may be omitted; any other key is an -/// error. +/// The `env_file` document. Every key may be omitted; any other key is an +/// error. `build_options` (wire `1.6.0`+) is null when absent, so the caller +/// can refuse the key itself from a hook that may not send it. pub const File = struct { set: []const Var = &.{}, path_prepend: []const []const u8 = &.{}, + build_options: ?[]const Var = null, }; +/// Zig build options the CLI passes (or may pass) itself: `-Doptimize` (from +/// `--optimize` or the target owner's `target_defaults`) and `-Dtarget` +/// (`--docker --target`). A provider may not contribute them. +pub const cli_owned_build_options = [_][]const u8{ "optimize", "target" }; + +/// A Zig build option name: `[A-Za-z_][A-Za-z0-9_-]*`. +pub fn buildOptionName(name: []const u8) bool { + if (name.len == 0 or !(std.ascii.isAlphabetic(name[0]) or name[0] == '_')) return false; + for (name) |c| { + if (!(std.ascii.isAlphanumeric(c) or c == '_' or c == '-')) return false; + } + return true; +} + +/// The `-D` argument `arg` sets `name` (`-Dname` or `-Dname=...`). +fn setsBuildOption(arg: []const u8, name: []const u8) bool { + if (!std.mem.startsWith(u8, arg, "-D")) return false; + const rest = arg[2..]; + if (!std.mem.startsWith(u8, rest, name)) return false; + return rest.len == name.len or rest[name.len] == '='; +} + /// Why a file or a merge was refused, as one line for the diagnostic that /// names the hook. Owned by the allocator the failing call received. pub const Diagnostic = struct { @@ -78,6 +108,16 @@ pub fn parseFile(a: std.mem.Allocator, bytes: []const u8, windows: bool, diag: * if (eqlName(previous.name, entry.name, windows)) return fail(a, diag, "'{s}' is set twice", .{entry.name}); } } + if (parsed.build_options) |options| for (options, 0..) |option, i| { + if (!buildOptionName(option.name)) return fail(a, diag, "invalid build option name '{s}' (names match [A-Za-z_][A-Za-z0-9_-]*)", .{option.name}); + for (cli_owned_build_options) |owned| { + if (std.mem.eql(u8, owned, option.name)) return fail(a, diag, "build option '{s}' is owned by the CLI (it passes -D{s} itself)", .{ option.name, owned }); + } + if (std.mem.indexOfAny(u8, option.value, "\x00\n\r") != null) return fail(a, diag, "the value of build option '{s}' contains a NUL byte or a line break", .{option.name}); + for (options[0..i]) |previous| { + if (std.mem.eql(u8, previous.name, option.name)) return fail(a, diag, "build option '{s}' is given twice", .{option.name}); + } + }; const sep = pathSeparator(windows); for (parsed.path_prepend) |dir| { if (dir.len == 0 or std.mem.indexOfScalar(u8, dir, 0) != null or !absoluteOn(dir, windows)) @@ -109,13 +149,16 @@ pub const Accumulator = struct { arena: ?std.heap.ArenaAllocator = null, vars: std.ArrayList(Entry) = .empty, path: std.ArrayList([]const u8) = .empty, + /// The target owner's `build_options` (wire `1.6.0`+), in hook order then + /// list order: `-D=` on every later `zig build`. + options: std.ArrayList(Entry) = .empty, /// The Windows name and path rules. The host's; a parameter for tests. windows: bool = native_windows, pub const Entry = struct { name: []const u8, value: []const u8, hook: []const u8 }; pub fn isEmpty(self: *const Accumulator) bool { - return self.vars.items.len == 0 and self.path.items.len == 0; + return self.vars.items.len == 0 and self.path.items.len == 0 and self.options.items.len == 0; } /// Whether a hook of this build contributed `name` (the host's name @@ -136,6 +179,7 @@ pub const Accumulator = struct { self.arena = null; self.vars = .empty; self.path = .empty; + self.options = .empty; } pub fn deinit(self: *Accumulator) void { @@ -144,7 +188,9 @@ pub const Accumulator = struct { /// Whether two accumulators make the same environment: the same /// variables with the same values, and the same PATH entries, in order - /// (names folded on Windows). Which hook contributed is not compared. + /// (names folded on Windows). Which hook contributed is not compared, + /// nor are the build options: they reach the compile, never the process + /// environment of a running replacement. pub fn sameAs(self: *const Accumulator, other: *const Accumulator) bool { if (self.vars.items.len != other.vars.items.len or self.path.items.len != other.path.items.len) return false; for (self.vars.items, other.vars.items) |x, y| { @@ -169,6 +215,9 @@ pub const Accumulator = struct { try copy.vars.append(a, .{ .name = try a.dupe(u8, entry.name), .value = try a.dupe(u8, entry.value), .hook = try a.dupe(u8, entry.hook) }); } for (self.path.items) |dir| try copy.path.append(a, try a.dupe(u8, dir)); + for (self.options.items) |entry| { + try copy.options.append(a, .{ .name = try a.dupe(u8, entry.name), .value = try a.dupe(u8, entry.value), .hook = try a.dupe(u8, entry.hook) }); + } return copy; } @@ -184,6 +233,14 @@ pub const Accumulator = struct { return fail(diag_a, diag, "'{s}' is set to different values by hooks '{s}' and '{s}'", .{ entry.name, existing.hook, hook }); } } + const options = file.build_options orelse &.{}; + for (options) |option| { + for (self.options.items) |existing| { + if (!std.mem.eql(u8, existing.name, option.name)) continue; + if (!std.mem.eql(u8, existing.value, option.value)) + return fail(diag_a, diag, "build option '-D{s}' is set to different values by hooks '{s}' and '{s}'", .{ option.name, existing.hook, hook }); + } + } if (self.arena == null) self.arena = std.heap.ArenaAllocator.init(backing); const a = self.arena.?.allocator(); const owned_hook = try a.dupe(u8, hook); @@ -199,6 +256,32 @@ pub const Accumulator = struct { } try self.path.append(a, try a.dupe(u8, dir)); } + outer: for (options) |option| { + for (self.options.items) |existing| { + if (std.mem.eql(u8, existing.name, option.name)) continue :outer; + } + try self.options.append(a, .{ .name = try a.dupe(u8, option.name), .value = try a.dupe(u8, option.value), .hook = owned_hook }); + } + } + + /// `base` (a `zig build` argv the CLI assembled) followed by one + /// `-D=` per contributed build option, in contribution + /// order. An option `base` already sets is a conflict naming the hook's + /// argument and the CLI's (`diag`, allocated with `a`): + /// `error.BuildOptionConflict`. With no options, `base` itself. + pub fn zigArgs(self: *const Accumulator, a: std.mem.Allocator, base: []const []const u8, diag: *Diagnostic) ![]const []const u8 { + if (self.options.items.len == 0) return base; + for (self.options.items) |option| { + for (base) |arg| { + if (!setsBuildOption(arg, option.name)) continue; + diag.message = try std.fmt.allocPrint(a, "build option '-D{s}={s}' from hook '{s}' conflicts with the CLI's '{s}'", .{ option.name, option.value, option.hook, arg }); + return error.BuildOptionConflict; + } + } + const out = try a.alloc([]const u8, base.len + self.options.items.len); + @memcpy(out[0..base.len], base); + for (self.options.items, out[base.len..]) |option, *arg| arg.* = try std.fmt.allocPrint(a, "-D{s}={s}", .{ option.name, option.value }); + return out; } /// The assignments that make the merged environment out of `inherited`: @@ -507,3 +590,67 @@ test "provider env: sets reports a contributed variable under the host's name ru try windows.add(a, a, "pkg/tc", .{ .set = &.{.{ .name = "Sdk_Root", .value = "C:\\one" }} }, &diag); try std.testing.expect(windows.sets("SDK_ROOT")); } + +test "provider env: build_options are parsed and validated (wire 1.6.0)" { + var arena = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena.deinit(); + const a = arena.allocator(); + const file = try parseOk(a, "{\"build_options\":[{\"name\":\"device\",\"value\":\"true\"},{\"name\":\"sdk-root_2\",\"value\":\"/abs sdk\"}]}", false); + try std.testing.expectEqual(@as(usize, 2), file.build_options.?.len); + try std.testing.expectEqualStrings("device", file.build_options.?[0].name); + // Absent is null (so the caller can refuse the key where it isn't + // allowed); an empty list is present. + try std.testing.expect((try parseOk(a, "{}", false)).build_options == null); + try std.testing.expectEqual(@as(usize, 0), (try parseOk(a, "{\"build_options\":[]}", false)).build_options.?.len); + for ([_][]const u8{ "", "1abc", "-dash", "has space", "a=b", "a.b" }) |name| { + const doc = try std.fmt.allocPrint(a, "{{\"build_options\":[{{\"name\":\"{s}\",\"value\":\"x\"}}]}}", .{name}); + try parseFails(a, doc, false, "invalid build option name"); + } + for (cli_owned_build_options) |name| { + const doc = try std.fmt.allocPrint(a, "{{\"build_options\":[{{\"name\":\"{s}\",\"value\":\"x\"}}]}}", .{name}); + try parseFails(a, doc, false, "owned by the CLI"); + } + try parseFails(a, "{\"build_options\":[{\"name\":\"a\",\"value\":\"x\\ny\"}]}", false, "line break"); + try parseFails(a, "{\"build_options\":[{\"name\":\"a\",\"value\":\"x\\ry\"}]}", false, "line break"); + try parseFails(a, "{\"build_options\":[{\"name\":\"a\",\"value\":\"x\\u0000\"}]}", false, "NUL"); + try parseFails(a, "{\"build_options\":[{\"name\":\"a\",\"value\":\"1\"},{\"name\":\"a\",\"value\":\"1\"}]}", false, "given twice"); + try parseFails(a, "{\"build_options\":[{\"name\":\"a\"}]}", false, "MissingField"); + try parseFails(a, "{\"build_options\":[{\"name\":\"a\",\"value\":true}]}", false, "not a valid env_file"); +} + +test "provider env: build_options merge in hook order and become -D arguments after the CLI's" { + var arena = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena.deinit(); + const a = arena.allocator(); + var acc: Accumulator = .{ .windows = false }; + defer acc.deinit(); + var diag: Diagnostic = .{}; + const base = [_][]const u8{ "/zig", "build", "-Doptimize=ReleaseFast" }; + // Nothing contributed: the CLI's argv unchanged. + try std.testing.expectEqual(@as(usize, 3), (try acc.zigArgs(a, &base, &diag)).len); + try acc.add(std.testing.allocator, a, "owner/toolchain", try parseOk(a, "{\"build_options\":[{\"name\":\"device\",\"value\":\"true\"},{\"name\":\"sdk\",\"value\":\"probe-device\"}]}", false), &diag); + try std.testing.expect(!acc.isEmpty()); + // The same value again is fine; a new one follows; a different value + // conflicts naming both hooks. + try acc.add(std.testing.allocator, a, "owner/sign", try parseOk(a, "{\"build_options\":[{\"name\":\"device\",\"value\":\"true\"},{\"name\":\"team\",\"value\":\"ABC\"}]}", false), &diag); + try std.testing.expectError(error.InvalidEnvFile, acc.add(std.testing.allocator, a, "owner/late", try parseOk(a, "{\"build_options\":[{\"name\":\"sdk\",\"value\":\"probe-sim\"}]}", false), &diag)); + try std.testing.expect(std.mem.indexOf(u8, diag.message, "'owner/toolchain' and 'owner/late'") != null); + const argv = try acc.zigArgs(a, &base, &diag); + const expected = [_][]const u8{ "/zig", "build", "-Doptimize=ReleaseFast", "-Ddevice=true", "-Dsdk=probe-device", "-Dteam=ABC" }; + try std.testing.expectEqual(expected.len, argv.len); + for (expected, argv) |want, got| try std.testing.expectEqualStrings(want, got); + // A build option the CLI's argv already sets: a conflict naming both. + const clashing = [_][]const u8{ "/zig", "build", "-Dsdk=macosx" }; + try std.testing.expectError(error.BuildOptionConflict, acc.zigArgs(a, &clashing, &diag)); + try std.testing.expect(std.mem.indexOf(u8, diag.message, "'-Dsdk=probe-device' from hook 'owner/toolchain'") != null); + try std.testing.expect(std.mem.indexOf(u8, diag.message, "the CLI's '-Dsdk=macosx'") != null); + // A longer name sharing the prefix is not the same option. + _ = try acc.zigArgs(a, &.{ "/zig", "build", "-Dsdk-root=/x", "-Ddevices" }, &diag); + // Clones carry them; the environment comparison ignores them. + var copy = try acc.clone(std.testing.allocator); + defer copy.deinit(); + try std.testing.expectEqual(@as(usize, 3), copy.options.items.len); + try std.testing.expect(copy.sameAs(&Accumulator{ .windows = false })); + acc.reset(); + try std.testing.expect(acc.isEmpty()); +} diff --git a/src/cli/provider_hooks.zig b/src/cli/provider_hooks.zig index d9fd3721..7a979a39 100644 --- a/src/cli/provider_hooks.zig +++ b/src/cli/provider_hooks.zig @@ -419,7 +419,7 @@ pub fn runPhaseReporting(site: *Site, list: []const Planned, step: contract.Step if (site.reporter) |r| r.finishFailed(code, "hook failed"); return code; } - if (env_file) |path| try absorbEnvFile(site, a, planned.qualified, path); + if (env_file) |path| try absorbEnvFile(site, a, planned, invocation, path); if (outcome_file) |path| reported.?.* = try run_outcome.absorb(site, a, planned.qualified, path); } return 0; @@ -448,7 +448,13 @@ fn privateDir(a: std.mem.Allocator, host: dispatch.Host, kind: []const u8) ![]co /// environment (contract §2): absent is no contribution; empty, malformed /// or conflicting fails the command here, before any later zig invocation, /// naming the hook. -fn absorbEnvFile(site: *Site, a: std.mem.Allocator, qualified: []const u8, path: []const u8) !void { +/// +/// `build_options` (wire `1.6.0`+) are accepted only from the target +/// owner's `before generate` / `before build` hooks on a negotiated wire of +/// `1.6.0` or newer: the key from anyone else is an invalid file, even +/// empty, since that wire or slot does not define it. +fn absorbEnvFile(site: *Site, a: std.mem.Allocator, planned: Planned, invocation: contract.Invocation, path: []const u8) !void { + const qualified = planned.qualified; const read = provider_env.readFile(a, path, site.env_file_cap) catch |err| switch (err) { error.StreamTooLong => return rejectEnvFile(site, qualified, try std.fmt.allocPrint(a, "the file is larger than the {d}-byte cap", .{site.env_file_cap})), else => return err, @@ -459,6 +465,16 @@ fn absorbEnvFile(site: *Site, a: std.mem.Allocator, qualified: []const u8, path: error.InvalidEnvFile => return rejectEnvFile(site, qualified, diag.message), else => return err, }; + if (file.build_options != null) { + const provider = planned.provider; + if (!provider.meta.ownsTarget(site.target)) + return rejectEnvFile(site, qualified, try std.fmt.allocPrint(a, "build_options may only come from the owner of target '{s}', not package '{s}'", .{ site.target, provider.meta.name })); + if (!contract.buildOptionsSlot(invocation)) + return rejectEnvFile(site, qualified, try std.fmt.allocPrint(a, "build_options may only come from a 'before generate' or 'before build' hook, not '{s} {s}'", .{ @tagName(invocation.phase.?), @tagName(invocation.step.?) })); + const wire = manifest.negotiate(provider.meta.command_contract orelse "") catch "none"; + if (!contract.carriesBuildOptions(wire)) + return rejectEnvFile(site, qualified, try std.fmt.allocPrint(a, "build_options need provider contract >= {s}; package '{s}' speaks {s}", .{ contract.build_options_since, provider.meta.name, wire })); + } site.env.add(site.backing, a, qualified, file, &diag) catch |err| switch (err) { error.InvalidEnvFile => { std.debug.print("labelle: hook environment conflict: {s}\n", .{diag.message}); diff --git a/src/cli/provider_hooks_env_test.zig b/src/cli/provider_hooks_env_test.zig index 525e8c40..ac98af01 100644 --- a/src/cli/provider_hooks_env_test.zig +++ b/src/cli/provider_hooks_env_test.zig @@ -230,3 +230,52 @@ test "provider hooks env: the contributing hook of a plan is found by slot and n provider.meta.command_contract = "<1.3.0"; try std.testing.expect(hooks.planContributor(try plan_for(a, &provider, .generate), try plan_for(a, &provider, .build)) == null); } + +test "provider hooks env: build_options come only from the target owner's before generate/build hooks on wire 1.6.0+" { + const list = [_]manifest.Hook{ hook("gen", .generate, .before), hook("post", .generate, .after), hook("pre", .build, .before) }; + var h: Harness = undefined; + try h.init(&list); + defer h.deinit(); + const opts = "{\"build_options\":[{\"name\":\"device\",\"value\":\"true\"}]}"; + // Not the owner of `desktop`: refused, even though the wire and slot fit. + Spy.writes = &.{.{ .id = "gen", .bytes = opts }}; + try std.testing.expectError(error.InvalidHookEnvFile, hooks.runPhase(&h.site, &.{h.planned(0)}, .generate, .before, h.out())); + try std.testing.expect(h.site.env.isEmpty()); + // An empty list is the key all the same. + Spy.writes = &.{.{ .id = "gen", .bytes = "{\"build_options\":[]}" }}; + try std.testing.expectError(error.InvalidHookEnvFile, hooks.runPhase(&h.site, &.{h.planned(0)}, .generate, .before, h.out())); + // The owner, in `before generate` and `before build`: accepted, in hook order. + h.provider.meta.targets = &.{"desktop"}; + Spy.writes = &.{ + .{ .id = "gen", .bytes = opts }, + .{ .id = "pre", .bytes = "{\"set\":[{\"name\":\"PROBE_VAR\",\"value\":\"v\"}],\"build_options\":[{\"name\":\"sdk\",\"value\":\"probe-device\"}]}" }, + }; + try std.testing.expectEqual(@as(u8, 0), try hooks.runPhase(&h.site, &.{h.planned(0)}, .generate, .before, h.out())); + try std.testing.expectEqual(@as(u8, 0), try hooks.runPhase(&h.site, &.{h.planned(2)}, .build, .before, h.out())); + try std.testing.expectEqual(@as(usize, 2), h.site.env.options.items.len); + try std.testing.expectEqualStrings("pkg/gen", h.site.env.options.items[0].hook); + try std.testing.expectEqualStrings("sdk", h.site.env.options.items[1].name); + var arena = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena.deinit(); + var diag: @import("provider_env.zig").Diagnostic = .{}; + const argv = try h.site.env.zigArgs(arena.allocator(), &.{ "zig", "build", "-Doptimize=Debug" }, &diag); + try std.testing.expectEqual(@as(usize, 5), argv.len); + try std.testing.expectEqualStrings("-Ddevice=true", argv[3]); + try std.testing.expectEqualStrings("-Dsdk=probe-device", argv[4]); + h.site.env.reset(); + // `after generate` may contribute an environment, never build options. + Spy.writes = &.{.{ .id = "post", .bytes = opts }}; + try std.testing.expectError(error.InvalidHookEnvFile, hooks.runPhase(&h.site, &.{h.planned(1)}, .generate, .after, h.out())); + Spy.writes = &.{.{ .id = "post", .bytes = "{\"set\":[{\"name\":\"PROBE_VAR\",\"value\":\"v\"}]}" }}; + try std.testing.expectEqual(@as(u8, 0), try hooks.runPhase(&h.site, &.{h.planned(1)}, .generate, .after, h.out())); + h.site.env.reset(); + // The owner on a 1.5.0 wire: the key doesn't exist there. + h.provider.meta.command_contract = ">=1.3.0 <1.6.0"; + Spy.writes = &.{.{ .id = "gen", .bytes = opts }}; + try std.testing.expectError(error.InvalidHookEnvFile, hooks.runPhase(&h.site, &.{h.planned(0)}, .generate, .before, h.out())); + try std.testing.expect(h.site.env.isEmpty()); + // Its plain environment still merges. + Spy.writes = &.{.{ .id = "gen", .bytes = "{\"set\":[{\"name\":\"PROBE_VAR\",\"value\":\"v\"}]}" }}; + try std.testing.expectEqual(@as(u8, 0), try hooks.runPhase(&h.site, &.{h.planned(0)}, .generate, .before, h.out())); + try h.envDirsGone(); +} diff --git a/src/cli/provider_manifest.zig b/src/cli/provider_manifest.zig index f36a2329..f700ccd9 100644 --- a/src/cli/provider_manifest.zig +++ b/src/cli/provider_manifest.zig @@ -329,11 +329,14 @@ test "provider manifest: a repeated top-level field is rejected, never last-wins } test "provider manifest: contract negotiation picks the newest wire the provider's range admits" { - try std.testing.expectEqualStrings("1.5.0", contract.version); + try std.testing.expectEqualStrings("1.6.0", contract.version); // An open v1 range admits every additive minor, so it gets the newest. - try std.testing.expectEqualStrings("1.5.0", try negotiate(">=1.0.0 <2.0.0")); - try std.testing.expectEqualStrings("1.5.0", try negotiate(">=1.1.0")); - try std.testing.expectEqualStrings("1.5.0", try negotiate(">=1.2.0")); + try std.testing.expectEqualStrings("1.6.0", try negotiate(">=1.0.0 <2.0.0")); + try std.testing.expectEqualStrings("1.6.0", try negotiate(">=1.1.0")); + try std.testing.expectEqualStrings("1.6.0", try negotiate(">=1.2.0")); + try std.testing.expectEqualStrings("1.6.0", try negotiate(">=1.3.0 <1.7.0")); + // A provider capped below 1.6.0 keeps the exact 1.5.0 wire: its + // env_file may not carry `build_options`. try std.testing.expectEqualStrings("1.5.0", try negotiate(">=1.3.0 <1.6.0")); // A provider capped below 1.5.0 keeps the exact 1.4.0 wire, without // `run.outcome_file`. diff --git a/src/cli/provider_run_outcome.zig b/src/cli/provider_run_outcome.zig index 6ead6dd9..41ea9d6a 100644 --- a/src/cli/provider_run_outcome.zig +++ b/src/cli/provider_run_outcome.zig @@ -161,7 +161,7 @@ test "provider run outcome: the wire context carries outcome_file from 1.5.0 onl provider.dir = try std.fs.path.join(a, &.{ abs, "pkg" }); const open = try dispatch.wireContext(provider, host, abs, run, cache); try open.validate(true); - try std.testing.expectEqualStrings("1.5.0", open.contract_version); + try std.testing.expectEqualStrings(contract.version, open.contract_version); try std.testing.expectEqualStrings(run.run_options.?.outcome_file.?, open.run.?.outcome_file.?); try std.testing.expect(carried(provider)); // Capped below 1.5.0: the run options without `outcome_file`. diff --git a/src/cli/runner.zig b/src/cli/runner.zig index 23f6e89a..4df90ef0 100644 --- a/src/cli/runner.zig +++ b/src/cli/runner.zig @@ -765,7 +765,17 @@ pub fn fixFingerprint(allocator: std.mem.Allocator, project_dir: []const u8, out // fingerprint — swallowing the real build into this probe (~0.15s vs // minutes; found while wiring the cli#284 progress feed, which showed // the cold compile landing inside the "generate" phase). - const result = try runZigWithEnv(allocator, output_dir, &.{ zig_exe, "build", "--list-steps" }, &zig_env); + // The target owner's `build_options` (wire 1.6.0+) reach the probe too: + // it configures the generated build, as the compile will. + var args_arena = std.heap.ArenaAllocator.init(allocator); + defer args_arena.deinit(); + const probe_args: []const []const u8 = &.{ zig_exe, "build", "--list-steps" }; + var diag: provider_env.Diagnostic = .{}; + const args = if (contributed) |env| env.zigArgs(args_arena.allocator(), probe_args, &diag) catch |err| { + if (err == error.BuildOptionConflict) std.debug.print("labelle: {s}\n", .{diag.message}); + return err; + } else probe_args; + const result = try runZigWithEnv(allocator, output_dir, args, &zig_env); defer allocator.free(result.stdout); defer allocator.free(result.stderr); diff --git a/test/provider_android_like_e2e.py b/test/provider_android_like_e2e.py index 05bd9d08..106f5663 100644 --- a/test/provider_android_like_e2e.py +++ b/test/provider_android_like_e2e.py @@ -321,7 +321,7 @@ def ordered(*step_dirs): ("bundle", "before", "pre-bundle"), ("bundle", "replace", "bundle"), ("bundle", "after", "post-bundle")], ran for e in entries: - assert e["context"]["contract_version"] == "1.5.0", e + assert e["context"]["contract_version"] == "1.6.0", e assert e["context"]["final_step"] == "bundle", e # The package hook saw the finished build. assert "lib" in hook_context(zig_out, "package")["output_entries"] diff --git a/test/provider_dispatch_e2e.py b/test/provider_dispatch_e2e.py index 5ee9ef2d..e7aa5d1b 100644 --- a/test/provider_dispatch_e2e.py +++ b/test/provider_dispatch_e2e.py @@ -71,7 +71,7 @@ def run(*args, code=0, cwd=nested): assert Path(first["cwd"]) == project ctx = first["context"] # `>=1.0.0 <2.0.0` admits every additive v1 minor: the newest wire. - assert ctx["contract_version"] == "1.5.0" and ctx["target"] == "desktop" + assert ctx["contract_version"] == "1.6.0" and ctx["target"] == "desktop" # A command's 1.2.0 `target_dir` is an explicit null; `run` is hook-only. assert "target_dir" in ctx and ctx["target_dir"] is None and "run" not in ctx, ctx # 1.3.0: a command gets its provider's persistent cache dir, created, diff --git a/test/provider_hooks_e2e.py b/test/provider_hooks_e2e.py index 55b9679c..c9bd390e 100644 --- a/test/provider_hooks_e2e.py +++ b/test/provider_hooks_e2e.py @@ -50,7 +50,7 @@ # `build.zig.zon` with a valid fingerprint, so the CLI's generation-time # fingerprint pass configures the build; PROBE_CONFIGURE_LOG=1 in a zig # invocation's environment makes the build's configure step append -# `||` to +# `|||<-Dprobe_opt or ->` to # `/configure.log`, one line per zig invocation that configured it. FAKE_ASSEMBLER = '''import os, shutil, sys, zlib from pathlib import Path @@ -85,13 +85,14 @@ def backend_of(root): 'const std = @import("std");\\n' 'pub fn build(b: *std.Build) void {\\n' ' const optimize = b.standardOptimizeOption(.{});\\n' + ' const probe_opt = b.option([]const u8, "probe_opt", "an e2e build option") orelse "-";\\n' ' if (b.graph.environ_map.get("PROBE_CONFIGURE_LOG") != null) {\\n' ' const log_path = b.pathFromRoot("configure.log");\\n' ' const previous = std.Io.Dir.cwd().readFileAlloc(b.graph.io, log_path, b.allocator, .limited(65536)) catch "";\\n' ' const value = b.graph.environ_map.get("PROBE_TOOLCHAIN") orelse "-";\\n' ' const path_env = b.graph.environ_map.get("PATH") orelse "";\\n' ' const head = path_env[0 .. std.mem.indexOfScalar(u8, path_env, std.fs.path.delimiter) orelse path_env.len];\\n' - ' const line = b.fmt("{s}{s}|{s}|{s}\\\\n", .{ previous, @tagName(optimize), value, head });\\n' + ' const line = b.fmt("{s}{s}|{s}|{s}|{s}\\\\n", .{ previous, @tagName(optimize), value, head, probe_opt });\\n' ' std.Io.Dir.cwd().writeFile(b.graph.io, .{ .sub_path = log_path, .data = line }) catch @panic("configure.log");\\n' ' }\\n' ' const exe = b.addExecutable(.{ .name = "game", .root_module = b.createModule(.{\\n' @@ -301,8 +302,8 @@ def broken(text, error): # Contract 1.2.0: every hook names the generated target dir; only # `run`-step hooks carry the run options. 1.3.0 adds the cache dir # everywhere and an env_file on `before build`; 1.4.0 the command's - # last step (1.5.0, run.outcome_file, is the negotiated wire). - assert e["context"]["contract_version"] == "1.5.0", e + # last step; 1.5.0 run.outcome_file (1.6.0, build_options, is the negotiated wire). + assert e["context"]["contract_version"] == "1.6.0", e assert e["context"]["final_step"] == "build", e assert (e["context"]["env_file"] is not None) == (e["invocation"]["phase"] == "before"), e assert Path(e["context"]["target_dir"]) == target_dir.resolve(), e @@ -744,7 +745,7 @@ def probe_context(step_dir, hook_id): replaced = run("run", *run_flags) assert not marker.exists(), "the core launch ran although a replace run hook stands in for it" ctx = probe_context(probe_out, "deploy") - assert ctx["contract_version"] == "1.5.0" and ctx["final_step"] == "run", ctx + assert ctx["contract_version"] == "1.6.0" and ctx["final_step"] == "run", ctx # Its outcome file (wire 1.5.0) is covered by test/provider_run_outcome_e2e.py. assert Path(ctx["run"].pop("outcome_file")).name == "outcome", ctx assert ctx["run"] == {"env": expected_env, "args": ["a", "b"], "timeout_ms": 30000, "watch": None}, ctx @@ -982,6 +983,39 @@ def owned_build(*flags, defaults=(("android", "ReleaseSafe"),)): assert "TargetDefaultRequiresOwnedTarget" in run("help").stderr a_manifest.write_text(manifest("fixture-a", OWNED, targets=["android"], defaults=(("android", "ReleaseSafe"), ("android", "ReleaseFast")))) assert "DuplicateTargetDefault" in run("help").stderr + + # ── contract 1.6.0: the target owner's build_options (cli#471 D3) ───── + # The owner's `before generate` hook contributes `-Dprobe_opt=on`: the + # fingerprint pass and the compile both configure with it, after the + # CLI's own `-Doptimize`. + options_file = base / "build-options.json" + options_file.write_text(json.dumps({"build_options": [{"name": "probe_opt", "value": "on"}]})) + OPTS = [hook("tc", "generate", "before", target="android"), hook("stamp-owned", "build", "after", target="android")] + opts_env = {"PROBE_CONFIGURE_LOG": "1", "FAKE_WITH_ZON": "1", "PROVIDER_PROBE_ENV": f"tc|{options_file}"} + a_manifest.write_text(manifest("fixture-a", OPTS, targets=["android"], defaults=(("android", "ReleaseSafe"),))) + reset() + run("build", "--platform=android", extra_env=opts_env) + lines = [line.split("|") for line in owned_log.read_text().splitlines()] + assert [(c[0], c[3]) for c in lines] == [("Debug", "on"), ("ReleaseSafe", "on")], lines + # A CLI-owned option is refused before generation, naming the hook. + options_file.write_text(json.dumps({"build_options": [{"name": "optimize", "value": "Debug"}]})) + reset() + bad = run("build", "--platform=android", code=1, extra_env=opts_env) + assert "labelle: hook 'fixture-a/tc' wrote an invalid env_file: build option 'optimize' is owned by the CLI" in bad.stderr, bad.stderr + assert "FIXTURE_GENERATE" not in bad.stderr, bad.stderr + # The owner capped below wire 1.6.0 can't send the key. + options_file.write_text(json.dumps({"build_options": [{"name": "probe_opt", "value": "on"}]})) + a_manifest.write_text(manifest("fixture-a", OPTS, targets=["android"], contract=">=1.3.0 <1.6.0")) + reset() + bad = run("build", "--platform=android", code=1, extra_env=opts_env) + assert "build_options need provider contract >= 1.6.0; package 'fixture-a' speaks 1.5.0" in bad.stderr, bad.stderr + # Nor can a provider that doesn't own the target (`desktop` is the core's). + a_manifest.write_text(manifest("fixture-a", ENV_HOOKS)) + reset() + bad = run("build", code=1, extra_env=dict(opts_env)) + assert "build_options may only come from the owner of target 'desktop', not package 'fixture-a'" in bad.stderr, bad.stderr + assert "build ok" not in bad.stderr, bad.stderr + a_manifest.write_text(manifest("fixture-a", A_HOOKS)) declare(dep_b, dep_a) diff --git a/test/provider_run_outcome_e2e.py b/test/provider_run_outcome_e2e.py index 1b4e9e69..5fb72bc3 100644 --- a/test/provider_run_outcome_e2e.py +++ b/test/provider_run_outcome_e2e.py @@ -142,7 +142,7 @@ def reset(): assert ran() == both, ran() assert "after-run hooks skipped" not in clean.stderr, clean.stderr ctx = deploy_context() - assert ctx["contract_version"] == "1.5.0", ctx + assert ctx["contract_version"] == "1.6.0", ctx outcome_file = Path(ctx["run"]["outcome_file"]) # Absolute, in a private directory the CLI removed once the replacement exited. assert outcome_file.is_absolute() and outcome_file.name == "outcome", ctx diff --git a/test/provider_targets_e2e.py b/test/provider_targets_e2e.py index d1f37dc2..046ea03c 100644 --- a/test/provider_targets_e2e.py +++ b/test/provider_targets_e2e.py @@ -492,8 +492,8 @@ def lookup(target, extra, requested): assert packed["context"]["invocation"]["step"] == "bundle", packed assert packed["context"]["build_number"] == "42", packed # `build_number` is a contract 1.1.0 key, negotiated from the range: an - # open v1 range gets the newest wire (1.5.0), which carries it... - assert packed["context"]["contract_version"] == "1.5.0", packed + # open v1 range gets the newest wire (1.6.0), which carries it... + assert packed["context"]["contract_version"] == "1.6.0", packed # ...while a provider capped below 1.1.0 gets the exact 1.0.0 wire with # no such key — a strict 1.0.0 decoder would reject it as unknown and fail # the bundle (Codex P2 on #421) — and the drop is said once, not silent. From 1ba80e9c96a1bce37b0ca98b03116f24f586fec6 Mon Sep 17 00:00:00 2001 From: Daniel Murta Date: Thu, 1 Oct 2026 08:48:10 -0300 Subject: [PATCH 2/2] fix(env_file): an explicit null build_options is an error, not "absent" std.json reads an omitted optional key and an explicit null alike, and only a present key is gated (owner, step, wire). So {"build_options": null} slipped past the gating. Refuse null at parse time: the key is either absent or an array. --- src/cli/provider_env.zig | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/cli/provider_env.zig b/src/cli/provider_env.zig index 51a229a1..834725d7 100644 --- a/src/cli/provider_env.zig +++ b/src/cli/provider_env.zig @@ -97,6 +97,14 @@ pub fn parseFile(a: std.mem.Allocator, bytes: []const u8, windows: bool, diag: * .duplicate_field_behavior = .@"error", .allocate = .alloc_always, }) catch |err| return fail(a, diag, "not a valid env_file document ({s}); expected {{\"set\":[{{\"name\":...,\"value\":...}}],\"path_prepend\":[...]}}", .{@errorName(err)}); + // An optional slice reads an omitted key and an explicit `null` alike, + // and only a present key is gated (owner, step, wire) by the caller. So + // `null` is refused here: the key is either absent or an array. + if (parsed.build_options == null) { + const raw = std.json.parseFromSliceLeaky(std.json.Value, a, bytes, .{}) catch null; + if (raw) |value| if (value == .object and value.object.contains("build_options")) + return fail(a, diag, "build_options must be an array (omit the key to contribute none)", .{}); + } for (parsed.set, 0..) |entry, i| { if (!contract.envName(entry.name)) return fail(a, diag, "invalid variable name '{s}' (names match [A-Za-z_][A-Za-z0-9_]*)", .{entry.name}); if (config.reservedEnvName(entry.name, windows)) { @@ -601,6 +609,11 @@ test "provider env: build_options are parsed and validated (wire 1.6.0)" { // Absent is null (so the caller can refuse the key where it isn't // allowed); an empty list is present. try std.testing.expect((try parseOk(a, "{}", false)).build_options == null); + // An explicit null is not "absent": it would dodge the caller's gating. + { + var diag: Diagnostic = .{}; + try std.testing.expectError(error.InvalidEnvFile, parseFile(a, "{\"build_options\":null}", false, &diag)); + } try std.testing.expectEqual(@as(usize, 0), (try parseOk(a, "{\"build_options\":[]}", false)).build_options.?.len); for ([_][]const u8{ "", "1abc", "-dash", "has space", "a=b", "a.b" }) |name| { const doc = try std.fmt.allocPrint(a, "{{\"build_options\":[{{\"name\":\"{s}\",\"value\":\"x\"}}]}}", .{name});