feat(contract): wire 1.6.0, build_options in env_file (cli#471 D3) - #522
Conversation
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<name>=<value> 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.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (17)
Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughWire version 1.6.0 adds validated ChangesProvider build-option contributions
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant ProviderHook
participant HookProcessing
participant OptionAccumulator
participant FingerprintAndBuild
participant Zig
ProviderHook->>HookProcessing: Submit env_file with build_options
HookProcessing->>OptionAccumulator: Validate and merge eligible options
FingerprintAndBuild->>OptionAccumulator: Compose Zig arguments
OptionAccumulator-->>FingerprintAndBuild: Return arguments with build options
FingerprintAndBuild->>Zig: Run fingerprint probe and core build
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No concrete merge-blocking issue is established. The build-option contribution rules and documented scope are consistent; merge after normal checks pass. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
User coderabbitai[bot] does not have write permissions |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c1481c250
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub const File = struct { | ||
| set: []const Var = &.{}, | ||
| path_prepend: []const []const u8 = &.{}, | ||
| build_options: ?[]const Var = null, |
There was a problem hiding this comment.
Declaring this field as an optional slice makes std.json.parseFromSliceLeaky map both an omitted key and an explicit "build_options": null to null. Consequently, the later file.build_options != null validation is bypassed: malformed null values are accepted, and even hooks on older wires or hooks that do not own the target can include the otherwise-forbidden key without rejection. Use a representation that tracks key presence separately from its required array value.
Useful? React with 👍 / 👎.
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.
2 participants
Part of #471 (§3 "Contract 1.4" row, decision D3). Wire numbers 1.4 and 1.5 went to other features, so this ships as 1.6.0, an additive minor.
What
build_options: a target owner'sbefore generate/before buildhook may putbuild_options: [{"name": "...", "value": "..."}]in itsenv_file.-D<name>=<value>to everyzig buildthe environment reaches: the fingerprint pass, the core compile and watched rebuilds. They come after its own arguments, in hook then list order. For example:zig build -Doptimize=ReleaseFast -Ddevice=true.-Ddevice=true) without replacing the whole build.Rules
[A-Za-z_][A-Za-z0-9_-]*, single-line values, no duplicate names.optimizeandtargetare refused as CLI-owned.before generate/before build, only on a negotiated wire of 1.6.0 or newer. Anything else is an invalidenv_fileerror naming the hook, even for an empty list.--docker: unchanged. It already refuses contributing hooks.Notes
replace buildhook stands in for the compile, so its owner's options reach only the fingerprint pass.Tests
provider_hooks_e2e.py: 81 invocations pass locally, with its pre-existing macOS--dockerblock skipped.zig build testpasses 13/13 steps (1160 tests).Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.