From 27b941fdaf5dbc1311e9ad344a16376bfbcf4013 Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Mon, 1 May 2023 17:12:05 +0200 Subject: [PATCH 1/8] build: remove the ZiglingStep.makeInternal method Rename the doCompile method to compile and add the run method. The two methods are now called from the make method. Add the help method, since the error handling of compile and run methods are now separate. Remove the obsolete comment for the compile method. --- build.zig | 50 ++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 14 deletions(-) diff --git a/build.zig b/build.zig index 3b59d70..519ab85 100644 --- a/build.zig +++ b/build.zig @@ -240,7 +240,7 @@ const ZiglingStep = struct { return self; } - fn make(step: *Step, prog_node: *std.Progress.Node) anyerror!void { + fn make(step: *Step, prog_node: *std.Progress.Node) !void { const self = @fieldParentPtr(@This(), "step", step); if (self.exercise.skip) { @@ -248,28 +248,33 @@ const ZiglingStep = struct { return; } - self.makeInternal(prog_node) catch { + + const exe_path = self.compile(prog_node) catch { if (self.exercise.hint.len > 0) { print("\n{s}HINT: {s}{s}", .{ bold_text, self.exercise.hint, reset_text }); } - print("\n{s}Edit exercises/{s} and run this again.{s}", .{ red_text, self.exercise.main_file, reset_text }); - print("\n{s}To continue from this zigling, use this command:{s}\n {s}zig build -Dn={s}{s}\n", .{ red_text, reset_text, bold_text, self.exercise.key(), reset_text }); + self.help(); std.os.exit(1); }; - } - fn makeInternal(self: *@This(), prog_node: *std.Progress.Node) !void { - print("Compiling {s}...\n", .{self.exercise.main_file}); + self.run(exe_path, prog_node) catch { + if (self.exercise.hint.len > 0) { + print("\n{s}HINT: {s}{s}", .{ bold_text, self.exercise.hint, reset_text }); + } - const exe_file = try self.doCompile(prog_node); + self.help(); + std.os.exit(1); + }; + } + fn run(self: *@This(), exe_path: []const u8, _: *std.Progress.Node) !void { resetLine(); print("Checking {s}...\n", .{self.exercise.main_file}); const cwd = self.builder.build_root.path.?; - const argv = [_][]const u8{exe_file}; + const argv = [_][]const u8{exe_path}; var child = std.ChildProcess.init(&argv, self.builder.allocator); @@ -336,9 +341,9 @@ const ZiglingStep = struct { print("{s}PASSED:\n{s}{s}\n\n", .{ green_text, trimOutput, reset_text }); } - // The normal compile step calls os.exit, so we can't use it as a library :( - // This is a stripped down copy of std.build.LibExeObjStep.make. - fn doCompile(self: *@This(), prog_node: *std.Progress.Node) ![]const u8 { + fn compile(self: *@This(), prog_node: *std.Progress.Node) ![]const u8 { + print("Compiling {s}...\n", .{self.exercise.main_file}); + const builder = self.builder; var zig_args = std.ArrayList([]const u8).init(builder.allocator); @@ -362,7 +367,7 @@ const ZiglingStep = struct { const argv = zig_args.items; var code: u8 = undefined; - const file_name = self.eval(argv, &code, prog_node) catch |err| { + const exe_path = self.eval(argv, &code, prog_node) catch |err| { self.printErrors(); switch (err) { @@ -397,7 +402,7 @@ const ZiglingStep = struct { }; self.printErrors(); - return file_name; + return exe_path; } // Code adapted from `std.Build.execAllowFail and `std.Build.Step.evalZigProcess`. @@ -500,6 +505,23 @@ const ZiglingStep = struct { return result orelse return error.ZigIPCError; } + fn help(self: *ZiglingStep) void { + const path = self.exercise.main_file; + const key = self.exercise.key(); + + print("\n{s}Edit exercises/{s} and run this again.{s}", .{ + red_text, path, reset_text, + }); + + const format = + \\ + \\{s}To continue from this zigling, use this command:{s} + \\ {s}zig build -Dn={s}{s} + \\ + ; + print(format, .{ red_text, reset_text, bold_text, key, reset_text }); + } + fn printErrors(self: *ZiglingStep) void { resetLine(); From 40cbee8fa207c4b2d5918741493e015078d9b5fb Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Mon, 1 May 2023 17:32:07 +0200 Subject: [PATCH 2/8] build: fix incorrect error handling in ZiglingStep.compile When handling the error from the eval method, some possible errors are ignored. The make method will only print the exercise hint and the help message. Print the unexpected error message, in the else prong. Note that FileNotFound can also be considered unexpected. --- build.zig | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/build.zig b/build.zig index 519ab85..03fad4c 100644 --- a/build.zig +++ b/build.zig @@ -395,7 +395,16 @@ const ZiglingStep = struct { for (argv) |v| print("{s} ", .{v}); print("\n", .{}); }, - else => {}, + else => { + print("{s}{s}: Unexpected error: {s}{s}\n", .{ + red_text, + self.exercise.main_file, + @errorName(err), + reset_text, + }); + for (argv) |v| print("{s} ", .{v}); + print("\n", .{}); + }, } return err; From 11d8468539f489d760098453810ed9b274813169 Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Mon, 1 May 2023 19:27:50 +0200 Subject: [PATCH 3/8] build: use Child.exec in ZiglingStep.run Update the run method to use Child.exec, instead of Child.spawn followed by Child.wait. This simplifies the code. --- build.zig | 47 +++++++++++++++++------------------------------ 1 file changed, 17 insertions(+), 30 deletions(-) diff --git a/build.zig b/build.zig index 03fad4c..234a58f 100644 --- a/build.zig +++ b/build.zig @@ -272,44 +272,31 @@ const ZiglingStep = struct { resetLine(); print("Checking {s}...\n", .{self.exercise.main_file}); - const cwd = self.builder.build_root.path.?; - - const argv = [_][]const u8{exe_path}; - - var child = std.ChildProcess.init(&argv, self.builder.allocator); - - child.cwd = cwd; - child.env_map = self.builder.env_map; - - child.stdin_behavior = .Inherit; - if (self.exercise.check_stdout) { - child.stdout_behavior = .Pipe; - child.stderr_behavior = .Inherit; - } else { - child.stdout_behavior = .Inherit; - child.stderr_behavior = .Pipe; - } + const b = self.step.owner; - child.spawn() catch |err| { - print("{s}Unable to spawn {s}: {s}{s}\n", .{ red_text, argv[0], @errorName(err), reset_text }); + // Allow up to 1 MB of stdout capture. + const max_output_bytes = 1 * 1024 * 1024; + + var result = Child.exec(.{ + .allocator = b.allocator, + .argv = &.{exe_path}, + .cwd = b.build_root.path.?, + .cwd_dir = b.build_root.handle, + .max_output_bytes = max_output_bytes, + }) catch |err| { + print("{s}Unable to spawn {s}: {s}{s}\n", .{ + red_text, exe_path, @errorName(err), reset_text, + }); return err; }; - // Allow up to 1 MB of stdout capture. - const max_output_len = 1 * 1024 * 1024; const output = if (self.exercise.check_stdout) - try child.stdout.?.reader().readAllAlloc(self.builder.allocator, max_output_len) + result.stdout else - try child.stderr.?.reader().readAllAlloc(self.builder.allocator, max_output_len); - - // At this point stdout is closed, wait for the process to terminate. - const term = child.wait() catch |err| { - print("{s}Unable to spawn {s}: {s}{s}\n", .{ red_text, argv[0], @errorName(err), reset_text }); - return err; - }; + result.stderr; // Make sure it exited cleanly. - switch (term) { + switch (result.term) { .Exited => |code| { if (code != 0) { print("{s}{s} exited with error code {d} (expected {d}){s}\n", .{ red_text, self.exercise.main_file, code, 0, reset_text }); From 3ec978d73faa9db0fdbb60e4a3c0631f0923be48 Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Wed, 3 May 2023 09:56:29 +0200 Subject: [PATCH 4/8] build: remove ZiglingStep.builder field It is not necessary, since the builder is available in self.step.owner. --- build.zig | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/build.zig b/build.zig index 234a58f..7eb45d2 100644 --- a/build.zig +++ b/build.zig @@ -223,7 +223,6 @@ var reset_text: []const u8 = ""; const ZiglingStep = struct { step: Step, exercise: Exercise, - builder: *Build, work_path: []const u8, result_messages: []const u8 = "", @@ -234,7 +233,6 @@ const ZiglingStep = struct { self.* = .{ .step = Step.init(Step.Options{ .id = .custom, .name = exercise.main_file, .owner = builder, .makeFn = make }), .exercise = exercise, - .builder = builder, .work_path = work_path, }; return self; @@ -331,12 +329,12 @@ const ZiglingStep = struct { fn compile(self: *@This(), prog_node: *std.Progress.Node) ![]const u8 { print("Compiling {s}...\n", .{self.exercise.main_file}); - const builder = self.builder; + const b = self.step.owner; - var zig_args = std.ArrayList([]const u8).init(builder.allocator); + var zig_args = std.ArrayList([]const u8).init(b.allocator); defer zig_args.deinit(); - zig_args.append(builder.zig_exe) catch unreachable; + zig_args.append(b.zig_exe) catch unreachable; zig_args.append("build-exe") catch unreachable; // Enable C support for exercises that use C functions @@ -344,11 +342,11 @@ const ZiglingStep = struct { zig_args.append("-lc") catch unreachable; } - const zig_file = join(builder.allocator, &.{ self.work_path, self.exercise.main_file }) catch unreachable; - zig_args.append(builder.pathFromRoot(zig_file)) catch unreachable; + const zig_file = join(b.allocator, &.{ self.work_path, self.exercise.main_file }) catch unreachable; + zig_args.append(b.pathFromRoot(zig_file)) catch unreachable; zig_args.append("--cache-dir") catch unreachable; - zig_args.append(builder.pathFromRoot(builder.cache_root.path.?)) catch unreachable; + zig_args.append(b.pathFromRoot(b.cache_root.path.?)) catch unreachable; zig_args.append("--listen=-") catch unreachable; From feeba51940fa52835c69f22b7694ffeea8f1cc08 Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Wed, 3 May 2023 12:34:33 +0200 Subject: [PATCH 5/8] build: don't use @This() in ZiglingStep Use ZiglingStep, instead. This is consistent with the coding style in std.Build. --- build.zig | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/build.zig b/build.zig index 7eb45d2..29d4487 100644 --- a/build.zig +++ b/build.zig @@ -228,8 +228,8 @@ const ZiglingStep = struct { result_messages: []const u8 = "", result_error_bundle: std.zig.ErrorBundle = std.zig.ErrorBundle.empty, - pub fn create(builder: *Build, exercise: Exercise, work_path: []const u8) *@This() { - const self = builder.allocator.create(@This()) catch unreachable; + pub fn create(builder: *Build, exercise: Exercise, work_path: []const u8) *ZiglingStep { + const self = builder.allocator.create(ZiglingStep) catch unreachable; self.* = .{ .step = Step.init(Step.Options{ .id = .custom, .name = exercise.main_file, .owner = builder, .makeFn = make }), .exercise = exercise, @@ -239,7 +239,7 @@ const ZiglingStep = struct { } fn make(step: *Step, prog_node: *std.Progress.Node) !void { - const self = @fieldParentPtr(@This(), "step", step); + const self = @fieldParentPtr(ZiglingStep, "step", step); if (self.exercise.skip) { print("Skipping {s}\n\n", .{self.exercise.main_file}); @@ -266,7 +266,7 @@ const ZiglingStep = struct { }; } - fn run(self: *@This(), exe_path: []const u8, _: *std.Progress.Node) !void { + fn run(self: *ZiglingStep, exe_path: []const u8, _: *std.Progress.Node) !void { resetLine(); print("Checking {s}...\n", .{self.exercise.main_file}); @@ -326,7 +326,7 @@ const ZiglingStep = struct { print("{s}PASSED:\n{s}{s}\n\n", .{ green_text, trimOutput, reset_text }); } - fn compile(self: *@This(), prog_node: *std.Progress.Node) ![]const u8 { + fn compile(self: *ZiglingStep, prog_node: *std.Progress.Node) ![]const u8 { print("Compiling {s}...\n", .{self.exercise.main_file}); const b = self.step.owner; From c6c6a32270d63a2efdedea02305bc284046a63af Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Wed, 3 May 2023 14:39:42 +0200 Subject: [PATCH 6/8] build: improve the exercise output check Make the error message consistent with the one in std.Build.RunStep, using the "=" character instead of "-" and correctly aligning the text. --- build.zig | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/build.zig b/build.zig index 29d4487..c9914e0 100644 --- a/build.zig +++ b/build.zig @@ -313,11 +313,11 @@ const ZiglingStep = struct { if (!std.mem.eql(u8, trimOutput, trimExerciseOutput)) { print( \\ - \\{s}----------- Expected this output -----------{s} - \\"{s}" - \\{s}----------- but found -----------{s} - \\"{s}" - \\{s}-----------{s} + \\{s}========= expected this output: =========={s} + \\{s} + \\{s}========= but found: ====================={s} + \\{s} + \\{s}=========================================={s} \\ , .{ red_text, reset_text, trimExerciseOutput, red_text, reset_text, trimOutput, red_text, reset_text }); return error.InvalidOutput; From 771b499cbc2d3519a663dc194023dfbfbf2dda42 Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Wed, 3 May 2023 15:25:55 +0200 Subject: [PATCH 7/8] build: use @panic("OOM") instead of unreachable The code in ZiglingStep copied the error handling used in std.Build in the past. Use @panic("OOM") when the error is caused by the allocator failing to allocate memory. --- build.zig | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/build.zig b/build.zig index c9914e0..4ca9275 100644 --- a/build.zig +++ b/build.zig @@ -229,7 +229,7 @@ const ZiglingStep = struct { result_error_bundle: std.zig.ErrorBundle = std.zig.ErrorBundle.empty, pub fn create(builder: *Build, exercise: Exercise, work_path: []const u8) *ZiglingStep { - const self = builder.allocator.create(ZiglingStep) catch unreachable; + const self = builder.allocator.create(ZiglingStep) catch @panic("OOM"); self.* = .{ .step = Step.init(Step.Options{ .id = .custom, .name = exercise.main_file, .owner = builder, .makeFn = make }), .exercise = exercise, @@ -334,21 +334,21 @@ const ZiglingStep = struct { var zig_args = std.ArrayList([]const u8).init(b.allocator); defer zig_args.deinit(); - zig_args.append(b.zig_exe) catch unreachable; - zig_args.append("build-exe") catch unreachable; + zig_args.append(b.zig_exe) catch @panic("OOM"); + zig_args.append("build-exe") catch @panic("OOM"); // Enable C support for exercises that use C functions if (self.exercise.link_libc) { - zig_args.append("-lc") catch unreachable; + zig_args.append("-lc") catch @panic("OOM"); } - const zig_file = join(b.allocator, &.{ self.work_path, self.exercise.main_file }) catch unreachable; - zig_args.append(b.pathFromRoot(zig_file)) catch unreachable; + const zig_file = join(b.allocator, &.{ self.work_path, self.exercise.main_file }) catch @panic("OOM"); + zig_args.append(b.pathFromRoot(zig_file)) catch @panic("OOM"); - zig_args.append("--cache-dir") catch unreachable; - zig_args.append(b.pathFromRoot(b.cache_root.path.?)) catch unreachable; + zig_args.append("--cache-dir") catch @panic("OOM"); + zig_args.append(b.pathFromRoot(b.cache_root.path.?)) catch @panic("OOM"); - zig_args.append("--listen=-") catch unreachable; + zig_args.append("--listen=-") catch @panic("OOM"); const argv = zig_args.items; var code: u8 = undefined; From a5d93c0b2035915dbceb778acb44cb08e5d76684 Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Wed, 3 May 2023 17:44:47 +0200 Subject: [PATCH 8/8] build: improve coding style in ZiglingStep - Use an anonymous struct when initializing std.Build.Step. - Rename the builder parameter in the create method to b - Avoid lines too long Additionally: - In the run method, rename output to raw_output in order to make the next variable names shorter. - In the compile method, rename zig_file to path. --- build.zig | 71 +++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 45 insertions(+), 26 deletions(-) diff --git a/build.zig b/build.zig index 4ca9275..64968b9 100644 --- a/build.zig +++ b/build.zig @@ -228,10 +228,15 @@ const ZiglingStep = struct { result_messages: []const u8 = "", result_error_bundle: std.zig.ErrorBundle = std.zig.ErrorBundle.empty, - pub fn create(builder: *Build, exercise: Exercise, work_path: []const u8) *ZiglingStep { - const self = builder.allocator.create(ZiglingStep) catch @panic("OOM"); + pub fn create(b: *Build, exercise: Exercise, work_path: []const u8) *ZiglingStep { + const self = b.allocator.create(ZiglingStep) catch @panic("OOM"); self.* = .{ - .step = Step.init(Step.Options{ .id = .custom, .name = exercise.main_file, .owner = builder, .makeFn = make }), + .step = Step.init(.{ + .id = .custom, + .name = exercise.main_file, + .owner = b, + .makeFn = make, + }), .exercise = exercise, .work_path = work_path, }; @@ -249,7 +254,9 @@ const ZiglingStep = struct { const exe_path = self.compile(prog_node) catch { if (self.exercise.hint.len > 0) { - print("\n{s}HINT: {s}{s}", .{ bold_text, self.exercise.hint, reset_text }); + print("\n{s}HINT: {s}{s}", .{ + bold_text, self.exercise.hint, reset_text, + }); } self.help(); @@ -258,7 +265,9 @@ const ZiglingStep = struct { self.run(exe_path, prog_node) catch { if (self.exercise.hint.len > 0) { - print("\n{s}HINT: {s}{s}", .{ bold_text, self.exercise.hint, reset_text }); + print("\n{s}HINT: {s}{s}", .{ + bold_text, self.exercise.hint, reset_text, + }); } self.help(); @@ -288,7 +297,7 @@ const ZiglingStep = struct { return err; }; - const output = if (self.exercise.check_stdout) + const raw_output = if (self.exercise.check_stdout) result.stdout else result.stderr; @@ -297,20 +306,27 @@ const ZiglingStep = struct { switch (result.term) { .Exited => |code| { if (code != 0) { - print("{s}{s} exited with error code {d} (expected {d}){s}\n", .{ red_text, self.exercise.main_file, code, 0, reset_text }); + print("{s}{s} exited with error code {d} (expected {d}){s}\n", .{ + red_text, self.exercise.main_file, code, 0, reset_text, + }); return error.BadExitCode; } }, else => { - print("{s}{s} terminated unexpectedly{s}\n", .{ red_text, self.exercise.main_file, reset_text }); + print("{s}{s} terminated unexpectedly{s}\n", .{ + red_text, self.exercise.main_file, reset_text, + }); return error.UnexpectedTermination; }, } // Validate the output. - const trimOutput = std.mem.trimRight(u8, output, " \r\n"); - const trimExerciseOutput = std.mem.trimRight(u8, self.exercise.output, " \r\n"); - if (!std.mem.eql(u8, trimOutput, trimExerciseOutput)) { + const output = std.mem.trimRight(u8, raw_output, " \r\n"); + const exercise_output = std.mem.trimRight(u8, self.exercise.output, " \r\n"); + if (!std.mem.eql(u8, output, exercise_output)) { + const red = red_text; + const reset = reset_text; + print( \\ \\{s}========= expected this output: =========={s} @@ -319,17 +335,20 @@ const ZiglingStep = struct { \\{s} \\{s}=========================================={s} \\ - , .{ red_text, reset_text, trimExerciseOutput, red_text, reset_text, trimOutput, red_text, reset_text }); + , .{ red, reset, exercise_output, red, reset, output, red, reset }); return error.InvalidOutput; } - print("{s}PASSED:\n{s}{s}\n\n", .{ green_text, trimOutput, reset_text }); + print("{s}PASSED:\n{s}{s}\n\n", .{ green_text, output, reset_text }); } fn compile(self: *ZiglingStep, prog_node: *std.Progress.Node) ![]const u8 { print("Compiling {s}...\n", .{self.exercise.main_file}); const b = self.step.owner; + const exercise_path = self.exercise.main_file; + const path = join(b.allocator, &.{ self.work_path, exercise_path }) catch + @panic("OOM"); var zig_args = std.ArrayList([]const u8).init(b.allocator); defer zig_args.deinit(); @@ -337,13 +356,12 @@ const ZiglingStep = struct { zig_args.append(b.zig_exe) catch @panic("OOM"); zig_args.append("build-exe") catch @panic("OOM"); - // Enable C support for exercises that use C functions + // Enable C support for exercises that use C functions. if (self.exercise.link_libc) { zig_args.append("-lc") catch @panic("OOM"); } - const zig_file = join(b.allocator, &.{ self.work_path, self.exercise.main_file }) catch @panic("OOM"); - zig_args.append(b.pathFromRoot(zig_file)) catch @panic("OOM"); + zig_args.append(b.pathFromRoot(path)) catch @panic("OOM"); zig_args.append("--cache-dir") catch @panic("OOM"); zig_args.append(b.pathFromRoot(b.cache_root.path.?)) catch @panic("OOM"); @@ -357,35 +375,36 @@ const ZiglingStep = struct { switch (err) { error.FileNotFound => { - print("{s}{s}: Unable to spawn the following command: file not found{s}\n", .{ red_text, self.exercise.main_file, reset_text }); + print("{s}{s}: Unable to spawn the following command: file not found{s}\n", .{ + red_text, self.exercise.main_file, reset_text, + }); for (argv) |v| print("{s} ", .{v}); print("\n", .{}); }, error.ExitCodeFailure => { - print("{s}{s}: The following command exited with error code {}:{s}\n", .{ red_text, self.exercise.main_file, code, reset_text }); + print("{s}{s}: The following command exited with error code {}:{s}\n", .{ + red_text, self.exercise.main_file, code, reset_text, + }); for (argv) |v| print("{s} ", .{v}); print("\n", .{}); }, error.ProcessTerminated => { - print("{s}{s}: The following command terminated unexpectedly:{s}\n", .{ red_text, self.exercise.main_file, reset_text }); + print("{s}{s}: The following command terminated unexpectedly:{s}\n", .{ + red_text, self.exercise.main_file, reset_text, + }); for (argv) |v| print("{s} ", .{v}); print("\n", .{}); }, error.ZigIPCError => { print("{s}{s}: The following command failed to communicate the compilation result:{s}\n", .{ - red_text, - self.exercise.main_file, - reset_text, + red_text, self.exercise.main_file, reset_text, }); for (argv) |v| print("{s} ", .{v}); print("\n", .{}); }, else => { print("{s}{s}: Unexpected error: {s}{s}\n", .{ - red_text, - self.exercise.main_file, - @errorName(err), - reset_text, + red_text, self.exercise.main_file, @errorName(err), reset_text, }); for (argv) |v| print("{s} ", .{v}); print("\n", .{});