feat: support profiles to conditional options #4
Conversation
Summary of ChangesHello @GrapeBaBa, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive build profile system, enabling users to define distinct configurations for their projects. It seamlessly integrates profile management into the existing configuration parsing and code generation workflows, allowing for dynamic and conditional application of build options based on the selected profile. This enhancement provides greater flexibility and control over project builds, catering to different development, testing, and production environments. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an excellent feature by adding support for build profiles. The implementation is comprehensive, covering configuration parsing in Config.zig and code generation in ConfigBuildgen.zig. I've identified a few critical issues and areas for improvement. There are a couple of bugs in the code generation logic within ConfigBuildgen.zig that would produce invalid Zig code. Additionally, I found some memory leaks in the new parsing logic in src/Config.zig that occur on error paths. I've also included a suggestion to reduce code duplication. After addressing these points, the pull request should be in great shape.
src/ConfigBuildgen.zig
Outdated
| \\ }}; | ||
| \\ const profile_tag = blk: {{ | ||
| \\{s} | ||
| \\ }} else {{ |
There was a problem hiding this comment.
There's a typo in the template string here. The }} else will produce invalid Zig syntax in the generated build.zig file. Assuming profileDispatchBody generates a complete if-else if chain ending with a }, this should be else to correctly append the final else block.
\ else {{
src/ConfigBuildgen.zig
Outdated
| fn profileDispatchBody(self: *ConfigBuildgen, profile_names: []const []const u8) ![]u8 { | ||
| var buffer = std.ArrayList(u8).init(self.allocator); | ||
| errdefer buffer.deinit(); | ||
| var writer = buffer.writer(); | ||
|
|
||
| for (profile_names, 0..) |name, index| { | ||
| const tag = try allocFmtId(self.allocator, "profile", name); | ||
| defer self.allocator.free(tag); | ||
|
|
||
| if (index == 0) { | ||
| try writer.print(" if (std.mem.eql(u8, profile, \"{s}\")) {{\n", .{name}); | ||
| } else { | ||
| try writer.print(" }} else if (std.mem.eql(u8, profile, \"{s}\")) {{\n", .{name}); | ||
| } | ||
|
|
||
| try writer.print(" break :blk ProfileTag.{s};\n", .{tag}); | ||
| } | ||
|
|
||
| return buffer.toOwnedSlice(); | ||
| } |
There was a problem hiding this comment.
The logic for generating the profile dispatch body is incorrect. It produces an extra closing brace in the else if condition and fails to close the final if block. This will cause a syntax error in the generated build.zig file. The function should generate a complete and valid if-else if chain.
fn profileDispatchBody(self: *ConfigBuildgen, profile_names: []const []const u8) ![]u8 {
var buffer = std.ArrayList(u8).init(self.allocator);
errdefer buffer.deinit();
var writer = buffer.writer();
for (profile_names, 0..) |name, index| {
const tag = try allocFmtId(self.allocator, "profile", name);
defer self.allocator.free(tag);
if (index == 0) {
try writer.print(" if (std.mem.eql(u8, profile, \"{s}\")) {{\n", .{name});
} else {
try writer.print(" }} else if (std.mem.eql(u8, profile, \"{s}\")) {{\n", .{name});
}
try writer.print(" break :blk ProfileTag.{s};\n", .{tag});
}
if (profile_names.len > 0) {
try writer.print(" }}", .{});
}
return buffer.toOwnedSlice();
}
There was a problem hiding this comment.
no issue here, the template has the final block
src/Config.zig
Outdated
| } | ||
|
|
||
| fn parseProfiles(self: *Self, index: std.zig.Zoir.Node.Index) Error!Profiles { | ||
| var profiles: Profiles = .{}; |
There was a problem hiding this comment.
This function allocates memory for profiles and its contents, but it doesn't deallocate this memory if an error occurs during parsing, leading to a memory leak. To prevent this, you can use errdefer to ensure profiles.deinit() is called on all error paths, and then cancel the errdefer before successful returns.
Here's an example of the pattern:
fn parseProfiles(...) !Profiles {
var profiles: Profiles = .{};
errdefer profiles.deinit(self.gpa);
// ... existing function body ...
if (successful_condition) {
errdefer (void)0; // Cancel cleanup on success
return profiles;
}
// ... more logic ...
errdefer (void)0; // Cancel cleanup on success
return profiles;
}| const profile_name = try self.gpa.dupe(u8, field_name); | ||
| var profile = try self.parseProfile(field_value); | ||
|
|
||
| const gop = try map.getOrPut(profile_name); | ||
| if (!gop.found_existing) { | ||
| gop.value_ptr.* = profile; | ||
| } else { | ||
| self.gpa.free(profile_name); | ||
| profile.deinit(self.gpa); | ||
| try self.returnParseErrorFmt("duplicate profile '{s}'", .{field_name}, field_value.getAstNode(self.zoir)); | ||
| } |
There was a problem hiding this comment.
There's a potential memory leak in this block. If map.getOrPut(profile_name) fails, the memory allocated for profile_name and profile will be leaked. Using errdefer ensures these resources are cleaned up correctly on error paths.
const profile_name = try self.gpa.dupe(u8, field_name);
errdefer self.gpa.free(profile_name);
var profile = try self.parseProfile(field_value);
errdefer profile.deinit(self.gpa);
const gop = try map.getOrPut(profile_name);
if (!gop.found_existing) {
gop.value_ptr.* = profile;
errdefer (void)0; // ownership of profile transferred, cancel errdefer
errdefer (void)0; // ownership of profile_name transferred, cancel errdefer
} else {
// `profile_name` and `profile` are not stored, so they need to be freed.
// The `errdefer`s would handle this if this function returned an error,
// but `returnParseErrorFmt` is `noreturn`. So we must clean up manually.
self.gpa.free(profile_name);
profile.deinit(self.gpa);
try self.returnParseErrorFmt("duplicate profile '{s}'", .{field_name}, field_value.getAstNode(self.zoir));
}
| if (self.options_modules) |*modules| { | ||
| for (modules.values()) |*module| { | ||
| for (module.values()) |*override| override.deinit(gpa); | ||
| for (module.keys()) |k| gpa.free(k); | ||
| module.deinit(); | ||
| } | ||
| for (modules.keys()) |k| gpa.free(k); | ||
| modules.deinit(); | ||
| } |
There was a problem hiding this comment.
This deinitialization logic is duplicated in the errdefer block of the parseProfileOptionsModules function (lines 1270-1278). To improve maintainability and avoid potential bugs from inconsistent updates, this logic should be extracted into a private helper function within the Profiles struct. This function can then be called from both places.
For example:
// Inside `pub const Profiles = struct {`
fn deinitOptionsModules(modules: *ArrayHashMap(OptionsModule), gpa: std.mem.Allocator) void {
for (modules.values()) |*module| {
for (module.values()) |*override| override.deinit(gpa);
for (module.keys()) |k| gpa.free(k);
module.deinit();
}
for (modules.keys()) |k| gpa.free(k);
modules.deinit();
}This can then be called from Profile.deinit and parseProfileOptionsModules.
There was a problem hiding this comment.
Pull request overview
This pull request adds support for build profiles to the configuration system, enabling users to define multiple named profiles with option-specific overrides. Users can specify a default profile and override option values on a per-profile basis.
- Introduces a new
Profilesstructure with profile management methods and option override capabilities - Extends the parser to handle profile definitions, option overrides, and type compatibility validation
- Updates code generation to emit profile selection logic with runtime dispatch based on build options
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/sync.zig | Added new test case for basic7.zbuild.zon fixture to test profile functionality |
| test/fixtures/basic7.zbuild.zon | New test fixture demonstrating profile configuration with dev/prod profiles and option overrides |
| src/Config.zig | Added Profiles struct with nested Profile and OptionOverride types, parser methods for profile parsing, and compatibility checking logic |
| src/ConfigBuildgen.zig | Updated build.zig generation to emit profile enum, dispatch logic, and per-profile switch statements for option overrides with proper type handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| for (n.names, 0..) |name, i| { | ||
| const module_name = try self.gpa.dupe(u8, name.get(self.zoir)); |
There was a problem hiding this comment.
Memory leak: The module_name allocated on line 1281 will leak if parseProfileModuleOverrides fails on line 1283, since it hasn't been added to the modules HashMap yet and won't be cleaned up by the errdefer block. Add errdefer self.gpa.free(module_name); after line 1281 to handle this case.
| const module_name = try self.gpa.dupe(u8, name.get(self.zoir)); | |
| const module_name = try self.gpa.dupe(u8, name.get(self.zoir)); | |
| errdefer self.gpa.free(module_name); |
| }, | ||
| .@"enum" => |e| { | ||
| // Define a named enum type so we can use it in runtime control flow | ||
| const enum_id = try allocFmtId(self.allocator, "Enum", name); |
There was a problem hiding this comment.
Memory leak: The enum_id allocated by allocFmtId is stored in the t variable and later used in self.options.put(name, .{ .type_name = t, ... }) at line 522, but it's never freed. Since the options HashMap only stores pointers and doesn't own the strings, this allocated memory should be freed after the function completes. However, since t is used in the generated output (line 507/514) and stored in the HashMap, the lifetime management is complex. Consider tracking these allocations for proper cleanup in the deinit method, or refactor to avoid the allocation altogether.
src/ConfigBuildgen.zig
Outdated
| var final_default_expr: ?[]const u8 = default; | ||
| var switch_id: ?[]const u8 = null; | ||
|
|
||
| if (override_count > 0) { | ||
| if (!all_profiles_covered and default == null) return error.MissingProfileFallback; | ||
|
|
||
| switch_id = try allocFmtId(self.allocator, "profile_default", name); | ||
|
|
||
| try self.writeLn( | ||
| "const {s} = switch (profile_tag) {{", | ||
| .{switch_id.?}, | ||
| .{}, | ||
| ); | ||
|
|
||
| for (override_list.items) |ov| { | ||
| try self.writeLn( | ||
| ".{s} => {s},", | ||
| .{ ov.tag, ov.expr }, | ||
| .{ .indent = 8 }, | ||
| ); | ||
| } | ||
|
|
||
| if (!all_profiles_covered) { | ||
| try self.writeLn( | ||
| "else => {s},", | ||
| .{default.?}, | ||
| .{ .indent = 8 }, | ||
| ); | ||
| } | ||
|
|
||
| try self.writeLn("}};", .{}, .{}); | ||
|
|
||
| final_default_expr = switch_id.?; | ||
| } |
There was a problem hiding this comment.
Memory leak: When profile overrides exist, the default value (which may be an allocated string from the switch block at line 365-441) is assigned to final_default_expr on line 467, but then final_default_expr is overwritten with switch_id on line 499. The original default string is never freed. You should free default before overwriting final_default_expr on line 499, but only if it was allocated (not for static strings like "bool" or type names from the config).
|
|
||
| const module_struct = try self.parseStructLiteral(index); | ||
| for (module_struct.names, 0..) |option_name_node, j| { | ||
| const option_name = try self.gpa.dupe(u8, option_name_node.get(self.zoir)); |
There was a problem hiding this comment.
Memory leak: The option_name allocated on line 1300 will leak if parseProfileOptionOverride fails on line 1302, since it hasn't been added to the overrides HashMap yet and won't be cleaned up by the errdefer block. Add errdefer self.gpa.free(option_name); after line 1300 to handle this case.
| const option_name = try self.gpa.dupe(u8, option_name_node.get(self.zoir)); | |
| const option_name = try self.gpa.dupe(u8, option_name_node.get(self.zoir)); | |
| errdefer self.gpa.free(option_name); |
Signed-off-by: grapebaba <grapebaba@grapebabadeMacBook-Pro.local>
Signed-off-by: grapebaba <grapebaba@grapebabadeMacBook-Pro.local>
Signed-off-by: grapebaba <grapebaba@grapebabadeMacBook-Pro.local>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, | ||
| .@"enum" => |e| { | ||
| // Define a named enum type so we can use it in runtime control flow | ||
| const enum_id = try allocFmtId(self.allocator, "Enum", name); |
There was a problem hiding this comment.
If append fails, enum_id will be leaked since it was allocated on line 350 but never freed. Add errdefer self.allocator.free(enum_id); after line 350.
| const enum_id = try allocFmtId(self.allocator, "Enum", name); | |
| const enum_id = try allocFmtId(self.allocator, "Enum", name); | |
| errdefer self.allocator.free(enum_id); |
| }; | ||
| }, | ||
| .enum_list => |e| { | ||
| const enum_id = try allocFmtId(self.allocator, "Enum", name); |
There was a problem hiding this comment.
If append on line 361 fails, enum_id will be leaked. Add errdefer self.allocator.free(enum_id); after line 360.
| const enum_id = try allocFmtId(self.allocator, "Enum", name); | |
| const enum_id = try allocFmtId(self.allocator, "Enum", name); | |
| errdefer self.allocator.free(enum_id); |
| defer if (switch_id) |id| self.allocator.free(id); | ||
|
|
||
| if (override_count > 0) { | ||
| if (!all_profiles_covered and init_default == null) return error.MissingProfileFallback; |
There was a problem hiding this comment.
This error is returned without providing context about which option is missing a fallback. Consider using returnParseErrorFmt or including the option name in the error for better debugging.
| if (!all_profiles_covered and init_default == null) return error.MissingProfileFallback; | |
| if (!all_profiles_covered and init_default == null) | |
| return returnParseErrorFmt("Missing profile fallback for option '{s}' in module '{s}'", .{name, module_name}); |
| const profiles = maybe_profiles.?; | ||
| if (profiles.getOverride(profile_name, module_name, name)) |override_ptr| { | ||
| if (!override_ptr.isCompatible(item)) { | ||
| return error.ProfileOverrideTypeMismatch; |
There was a problem hiding this comment.
This error doesn't indicate which profile, module, or option has the type mismatch. Consider providing more context in the error message to aid debugging.
| if (n.names.len != 1) { | ||
| try self.returnParseError("profile must specify exactly one field 'options_modules'", index.getAstNode(self.zoir)); | ||
| } | ||
|
|
||
| const field_name = n.names[0].get(self.zoir); | ||
| const field_value = n.vals.at(0); | ||
| if (std.mem.eql(u8, field_name, "options_modules")) { | ||
| profile.options_modules = try self.parseProfileOptionsModules(field_value); | ||
| } else { | ||
| try self.returnParseErrorFmt("unknown field '{s}' in profile", .{field_name}, field_value.getAstNode(self.zoir)); |
There was a problem hiding this comment.
The hardcoded requirement for exactly one field named 'options_modules' makes the profile structure inflexible for future extensions. Consider allowing additional optional fields or documenting why this strict limitation exists.
| if (n.names.len != 1) { | |
| try self.returnParseError("profile must specify exactly one field 'options_modules'", index.getAstNode(self.zoir)); | |
| } | |
| const field_name = n.names[0].get(self.zoir); | |
| const field_value = n.vals.at(0); | |
| if (std.mem.eql(u8, field_name, "options_modules")) { | |
| profile.options_modules = try self.parseProfileOptionsModules(field_value); | |
| } else { | |
| try self.returnParseErrorFmt("unknown field '{s}' in profile", .{field_name}, field_value.getAstNode(self.zoir)); | |
| var found_options_modules = false; | |
| for (n.names, 0..) |name, i| { | |
| const field_name = name.get(self.zoir); | |
| const field_value = n.vals.at(@intCast(i)); | |
| if (std.mem.eql(u8, field_name, "options_modules")) { | |
| profile.options_modules = try self.parseProfileOptionsModules(field_value); | |
| found_options_modules = true; | |
| } else { | |
| // Ignore unknown fields for forward compatibility. | |
| // Optionally, log a warning or handle as needed. | |
| // For now, do nothing. | |
| } | |
| } | |
| if (!found_options_modules) { | |
| try self.returnParseError("profile must specify 'options_modules'", index.getAstNode(self.zoir)); |
wemeetagain
left a comment
wemeetagain
left a comment
There was a problem hiding this comment.
should we enforce that only fields from the overridden options module are used?
likewise, should we enforce that only options modules that are already named may be overridden?
Motivation
This pull request adds support for build profiles to the configuration system, allowing users to define multiple profiles with module-specific option overrides and a default profile. It introduces a new
Profilesstructure, updates the parsing logic to handle profiles, and modifies code generation to emit profile-aware option values.Description
Profilesstruct toConfig.zig, supporting named profiles, a default profile, and per-module option overrides. Includes methods for deinitialization, querying profiles, and retrieving option overrides.profilesfield, including parsing of profiles, their options modules, and option overrides with type compatibility checks.ConfigBuildgen.zigto emit profile selection logic, including an enum of profile tags, dispatch code for selecting the active profile, and per-profile option overrides using a switch statement.