Skip to content

feat: support forwarding options#9

Open
spiral-ladder wants to merge 2 commits into
mainfrom
bing/forward-opts
Open

feat: support forwarding options#9
spiral-ladder wants to merge 2 commits into
mainfrom
bing/forward-opts

Conversation

@spiral-ladder

Copy link
Copy Markdown
Contributor

zbuild does not support partial arguments. For example, we would not be able to pass only optimize and expect the dependency to be built with the parent's target, which broke building lodestar-z since we were building with portable=true and optimize=.ReleaseFast.

This PR adds that support

@spiral-ladder spiral-ladder self-assigned this Jun 8, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the build runner to preserve user-supplied dependency arguments while automatically filling in missing target and optimize defaults. It also adds validation for the optimize field in module definitions and introduces test fixtures for these scenarios. The review feedback suggests improving compile-time safety by validating the type of the optimize parameter in validateOptimize to prevent cryptic compiler errors. Additionally, it recommends using std.Build.ResolvedTarget instead of std.Target.Query when forwarding the parent's target to maintain consistency and avoid unnecessary target re-resolution.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/build_runner.zig Outdated
Comment thread src/build_runner.zig
Comment on lines +2632 to +2637
if (!has_target) {
names[i] = "target";
types[i] = std.Target.Query;
attrs[i] = .{};
i += 1;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To ensure consistency with the default dependency configuration (which passes std.Build.ResolvedTarget directly) and to avoid unnecessary target re-resolution in the child dependency, we should use std.Build.ResolvedTarget instead of std.Target.Query when forwarding the parent's target.

    if (!has_target) {
        names[i] = "target";
        types[i] = std.Build.ResolvedTarget;
        attrs[i] = .{};
        i += 1;
    }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this case ResolvedTarget contains Target.Query, which is the native target, it's probably more correct to rely on that instead.

Comment thread src/build_runner.zig Outdated
result.optimize = optimize_parent;
}

if (!@hasField(Args, "target")) result.target = target_parent.query;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Following the change to use std.Build.ResolvedTarget in DependencyArgs, we should pass target_parent directly instead of its query representation target_parent.query.

    if (!@hasField(Args, "target")) result.target = target_parent;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above for reason to reject ResolvedTarget

zbuild does not support partial arguments. For example, we would
not be able to pass only `optimize` and expect the dependency to be
built with the parent's `target`, which broke building `lodestar-z`
since we were building with `portable=true` and `optimize=.ReleaseFast`.

This PR adds that support
@spiral-ladder spiral-ladder force-pushed the bing/forward-opts branch 2 times, most recently from 99d6525 to 5acb055 Compare June 9, 2026 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant