From 699f877bd51bc93717c76573a61a7b5598f1bd82 Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Fri, 19 May 2023 15:28:20 +0200 Subject: [PATCH 1/4] build: remove `ZiglingStep.eval` Remove the `ZiglingStep.eval` method and the src/ipc.zig source code. Use `Step.evalZigProcess`, instead. This greatly simplifies the code. Print the error messages and error bundle in `ZiglingStep.make`, both in case of error and success. Additionally, remove the `ZiglingStep.is_testing` field, since it is no longer necessary. --- build.zig | 180 +++++----------------------------------------------- src/ipc.zig | 68 -------------------- 2 files changed, 17 insertions(+), 231 deletions(-) delete mode 100644 src/ipc.zig diff --git a/build.zig b/build.zig index 46a260d..2e25217 100644 --- a/build.zig +++ b/build.zig @@ -1,7 +1,6 @@ const std = @import("std"); const builtin = @import("builtin"); const compat = @import("src/compat.zig"); -const ipc = @import("src/ipc.zig"); const tests = @import("test/tests.zig"); const Build = compat.Build; @@ -195,10 +194,6 @@ const ZiglingStep = struct { work_path: []const u8, mode: Mode, - is_testing: bool = false, - result_messages: []const u8 = "", - result_error_bundle: std.zig.ErrorBundle = std.zig.ErrorBundle.empty, - pub fn create( b: *Build, exercise: Exercise, @@ -232,6 +227,8 @@ const ZiglingStep = struct { } const exe_path = self.compile(prog_node) catch { + self.printErrors(); + if (self.exercise.hint) |hint| print("\n{s}Ziglings hint: {s}{s}", .{ bold_text, hint, reset_text }); @@ -240,12 +237,17 @@ const ZiglingStep = struct { }; self.run(exe_path, prog_node) catch { + self.printErrors(); + if (self.exercise.hint) |hint| print("\n{s}Ziglings hint: {s}{s}", .{ bold_text, hint, reset_text }); self.help(); std.os.exit(2); }; + + // Print possible warning/debug messages. + self.printErrors(); } fn run(self: *ZiglingStep, exe_path: []const u8, _: *std.Progress.Node) !void { @@ -381,153 +383,7 @@ const ZiglingStep = struct { zig_args.append("--listen=-") catch @panic("OOM"); - const argv = zig_args.items; - var code: u8 = undefined; - const exe_path = self.eval(argv, &code, prog_node) catch |err| { - self.printErrors(); - - 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, - }); - dumpArgs(argv); - }, - error.ExitCodeFailure => { - print("{s}{s}: The following command exited with error code {}:{s}\n", .{ - red_text, self.exercise.main_file, code, reset_text, - }); - dumpArgs(argv); - }, - error.ProcessTerminated => { - print("{s}{s}: The following command terminated unexpectedly:{s}\n", .{ - red_text, self.exercise.main_file, reset_text, - }); - dumpArgs(argv); - }, - error.ZigIPCError => { - // Commenting this out for now. It always shows up when compilation fails. - //print("{s}{s}: The following command failed to communicate the compilation result:{s}\n", .{ - // red_text, self.exercise.main_file, reset_text, - //}); - //dumpArgs(argv); - }, - else => { - print("{s}{s}: Unexpected error: {s}{s}\n", .{ - red_text, self.exercise.main_file, @errorName(err), reset_text, - }); - dumpArgs(argv); - }, - } - - return err; - }; - self.printErrors(); - - return exe_path; - } - - // Code adapted from `std.Build.execAllowFail and `std.Build.Step.evalZigProcess`. - pub fn eval( - self: *ZiglingStep, - argv: []const []const u8, - out_code: *u8, - prog_node: *std.Progress.Node, - ) ![]const u8 { - assert(argv.len != 0); - const b = self.step.owner; - const allocator = b.allocator; - - var child = Child.init(argv, allocator); - child.env_map = b.env_map; - child.stdin_behavior = .Pipe; - child.stdout_behavior = .Pipe; - child.stderr_behavior = .Pipe; - - try child.spawn(); - - var poller = std.io.poll(allocator, enum { stdout, stderr }, .{ - .stdout = child.stdout.?, - .stderr = child.stderr.?, - }); - defer poller.deinit(); - - try ipc.sendMessage(child.stdin.?, .update); - try ipc.sendMessage(child.stdin.?, .exit); - - const Header = std.zig.Server.Message.Header; - var result: ?[]const u8 = null; - - var node_name: std.ArrayListUnmanaged(u8) = .{}; - defer node_name.deinit(allocator); - var sub_prog_node = prog_node.start("", 0); - defer sub_prog_node.end(); - - const stdout = poller.fifo(.stdout); - - poll: while (true) { - while (stdout.readableLength() < @sizeOf(Header)) { - if (!(try poller.poll())) break :poll; - } - const header = stdout.reader().readStruct(Header) catch unreachable; - while (stdout.readableLength() < header.bytes_len) { - if (!(try poller.poll())) break :poll; - } - const body = stdout.readableSliceOfLen(header.bytes_len); - - switch (header.tag) { - .zig_version => { - if (!std.mem.eql(u8, builtin.zig_version_string, body)) - return error.ZigVersionMismatch; - }, - .error_bundle => { - self.result_error_bundle = try ipc.parseErrorBundle(allocator, body); - }, - .progress => { - node_name.clearRetainingCapacity(); - try node_name.appendSlice(allocator, body); - sub_prog_node.setName(node_name.items); - }, - .emit_bin_path => { - const emit_bin = try ipc.parseEmitBinPath(allocator, body); - result = emit_bin.path; - }, - else => {}, // ignore other messages - } - - stdout.discard(body.len); - } - - const stderr = poller.fifo(.stderr); - if (stderr.readableLength() > 0) { - self.result_messages = try stderr.toOwnedSlice(); - } - - // Send EOF to stdin. - child.stdin.?.close(); - child.stdin = null; - - // Keep the errors compatible with std.Build.execAllowFail. - const term = try child.wait(); - switch (term) { - .Exited => |code| { - if (code != 0) { - out_code.* = @truncate(u8, code); - - return error.ExitCodeFailure; - } - }, - .Signal, .Stopped, .Unknown => |code| { - out_code.* = @truncate(u8, code); - - return error.ProcessTerminated; - }, - } - - if (self.is_testing) { - return "ok"; - } - return result orelse return error.ZigIPCError; + return try self.step.evalZigProcess(zig_args.items, prog_node); } fn help(self: *ZiglingStep) void { @@ -548,27 +404,25 @@ const ZiglingStep = struct { fn printErrors(self: *ZiglingStep) void { resetLine(); - // Print the additional log and verbose messages. - // TODO: use colors? - if (self.result_messages.len > 0) print("{s}", .{self.result_messages}); + // Display error/warning messages. + if (self.step.result_error_msgs.items.len > 0) { + for (self.step.result_error_msgs.items) |msg| { + print("{s}error: {s}{s}\n", .{ red_text, reset_text, msg }); + } + } - // Print the compiler errors. + // Render compile errors at the bottom of the terminal. // TODO: use the same ttyconf from the builder. const ttyconf: std.debug.TTY.Config = if (use_color_escapes) .escape_codes else .no_color; - if (self.result_error_bundle.errorMessageCount() > 0) { - self.result_error_bundle.renderToStdErr(.{ .ttyconf = ttyconf }); + if (self.step.result_error_bundle.errorMessageCount() > 0) { + self.step.result_error_bundle.renderToStdErr(.{ .ttyconf = ttyconf }); } } }; -fn dumpArgs(args: []const []const u8) void { - for (args) |arg| print("{s} ", .{arg}); - print("\n", .{}); -} - /// Clears the entire line and move the cursor to column zero. /// Used for clearing the compiler and build_runner progress messages. fn resetLine() void { diff --git a/src/ipc.zig b/src/ipc.zig deleted file mode 100644 index 9eaaa13..0000000 --- a/src/ipc.zig +++ /dev/null @@ -1,68 +0,0 @@ -/// Client side support for Zig IPC. -const std = @import("std"); -const debug = std.debug; -const fs = std.fs; -const mem = std.mem; - -const Allocator = mem.Allocator; -const Client = std.zig.Client; -const ErrorBundle = std.zig.ErrorBundle; -const Server = std.zig.Server; - -/// This data structure must be kept in sync with zig.Server.Message.EmitBinPath. -const EmitBinPath = struct { - flags: Flags, - path: []const u8, - - pub const Flags = Server.Message.EmitBinPath.Flags; - - pub fn deinit(self: *EmitBinPath, allocator: Allocator) void { - allocator.free(self.path); - self.* = undefined; - } -}; - -pub fn parseErrorBundle(allocator: Allocator, data: []const u8) !ErrorBundle { - const EbHdr = Server.Message.ErrorBundle; - const eb_hdr = @ptrCast(*align(1) const EbHdr, data); - const extra_bytes = - data[@sizeOf(EbHdr)..][0 .. @sizeOf(u32) * eb_hdr.extra_len]; - const string_bytes = - data[@sizeOf(EbHdr) + extra_bytes.len ..][0..eb_hdr.string_bytes_len]; - - // TODO: use @ptrCast when the compiler supports it - const unaligned_extra = std.mem.bytesAsSlice(u32, extra_bytes); - const extra_array = try allocator.alloc(u32, unaligned_extra.len); - // TODO: use @memcpy when it supports slices - // - // Don't use the "multi-object for loop" syntax, in - // order to avoid a syntax error with old Zig compilers. - var i: usize = 0; - while (i < extra_array.len) : (i += 1) { - extra_array[i] = unaligned_extra[i]; - } - - return .{ - .string_bytes = try allocator.dupe(u8, string_bytes), - .extra = extra_array, - }; -} - -pub fn parseEmitBinPath(allocator: Allocator, data: []const u8) !EmitBinPath { - const EbpHdr = Server.Message.EmitBinPath; - const ebp_hdr = @ptrCast(*align(1) const EbpHdr, data); - const path = try allocator.dupe(u8, data[@sizeOf(EbpHdr)..]); - - return .{ - .flags = ebp_hdr.flags, - .path = path, - }; -} - -pub fn sendMessage(file: fs.File, tag: Client.Message.Tag) !void { - const header: Client.Message.Header = .{ - .tag = tag, - .bytes_len = 0, - }; - try file.writeAll(mem.asBytes(&header)); -} From d0de9e534898451016e8f72328e70d76452c7c15 Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Fri, 19 May 2023 19:21:21 +0200 Subject: [PATCH 2/4] build: use `std.Build.Step.fail` to report errors Have all error messages handled in a single place (printError), by using the `std.Build.Step.fail` method. Ensure that the first letter in the error message is lower case and remove coloring, since it is done in the `ZiglingStep.printError` method. Additionally, in the `ZiglingStep.check_test` method, remove trailing whitespace from stderr. --- build.zig | 42 +++++++++++++++--------------------------- 1 file changed, 15 insertions(+), 27 deletions(-) diff --git a/build.zig b/build.zig index 2e25217..1823919 100644 --- a/build.zig +++ b/build.zig @@ -266,10 +266,9 @@ const ZiglingStep = struct { .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 self.step.fail("unable to spawn {s}: {s}", .{ + exe_path, @errorName(err), }); - return err; }; switch (self.exercise.kind) { @@ -285,17 +284,15 @@ 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, + return self.step.fail("{s} exited with error code {d} (expected {})", .{ + self.exercise.main_file, code, 0, }); - return error.BadExitCode; } }, else => { - print("{s}{s} terminated unexpectedly{s}\n", .{ - red_text, self.exercise.main_file, reset_text, + return self.step.fail("{s} terminated unexpectedly", .{ + self.exercise.main_file, }); - return error.UnexpectedTermination; }, } @@ -310,19 +307,14 @@ const ZiglingStep = struct { const output = try trimLines(b.allocator, raw_output); const exercise_output = self.exercise.output; if (!std.mem.eql(u8, output, self.exercise.output)) { - const red = red_text; - const reset = reset_text; - - print( + return self.step.fail( \\ - \\{s}========= expected this output: =========={s} + \\========= expected this output: ========== \\{s} - \\{s}========= but found: ====================={s} + \\========= but found: ===================== \\{s} - \\{s}=========================================={s} - \\ - , .{ red, reset, exercise_output, red, reset, output, red, reset }); - return error.InvalidOutput; + \\========================================== + , .{ exercise_output, output }); } print("{s}PASSED:\n{s}{s}\n\n", .{ green_text, output, reset_text }); @@ -333,19 +325,15 @@ const ZiglingStep = struct { .Exited => |code| { if (code != 0) { // The test failed. - print("{s}{s}{s}\n", .{ - red_text, result.stderr, reset_text, - }); + const stderr = std.mem.trimRight(u8, result.stderr, " \r\n"); - return error.TestFailed; + return self.step.fail("\n{s}", .{stderr}); } }, else => { - print("{s}{s} terminated unexpectedly{s}\n", .{ - red_text, self.exercise.main_file, reset_text, + return self.step.fail("{s} terminated unexpectedly", .{ + self.exercise.main_file, }); - - return error.UnexpectedTermination; }, } From 3e38a4fc844e572ee0a8b5cb61558763de0f2157 Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Sat, 20 May 2023 07:21:11 +0200 Subject: [PATCH 3/4] build: in `ZiglingStep.check_output` panic in case of OOM This is necessary since, when trimLines returns `std.mem.Allocator.Error`, no error message will be displayed to the user. An alternative is to use `std.Build.Step.fail`, but using @panic("OOM") is simpler and consistent with existing code. --- build.zig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.zig b/build.zig index 1823919..9b8a1ee 100644 --- a/build.zig +++ b/build.zig @@ -304,7 +304,7 @@ const ZiglingStep = struct { // Validate the output. // NOTE: exercise.output can never contain a CR character. // See https://ziglang.org/documentation/master/#Source-Encoding. - const output = try trimLines(b.allocator, raw_output); + const output = trimLines(b.allocator, raw_output) catch @panic("OOM"); const exercise_output = self.exercise.output; if (!std.mem.eql(u8, output, self.exercise.output)) { return self.step.fail( From 0b4ad6e99e48a2e71a8361f14ba6ffcdfcccc51b Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Mon, 22 May 2023 11:44:52 +0200 Subject: [PATCH 4/4] build: use the old color style In the `Zigling.printError` method, use a bold red color for the "error:" string and a dim red color for the error message. In the `Zigling.check_output` method, use the old color style. --- build.zig | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/build.zig b/build.zig index 9b8a1ee..1cf4f22 100644 --- a/build.zig +++ b/build.zig @@ -119,6 +119,8 @@ pub fn build(b: *Build) !void { if (use_color_escapes) { red_text = "\x1b[31m"; + red_bold_text = "\x1b[31;1m"; + red_dim_text = "\x1b[31;2m"; green_text = "\x1b[32m"; bold_text = "\x1b[1m"; reset_text = "\x1b[0m"; @@ -184,6 +186,8 @@ pub fn build(b: *Build) !void { var use_color_escapes = false; var red_text: []const u8 = ""; +var red_bold_text: []const u8 = ""; +var red_dim_text: []const u8 = ""; var green_text: []const u8 = ""; var bold_text: []const u8 = ""; var reset_text: []const u8 = ""; @@ -307,14 +311,20 @@ const ZiglingStep = struct { const output = trimLines(b.allocator, raw_output) catch @panic("OOM"); const exercise_output = self.exercise.output; if (!std.mem.eql(u8, output, self.exercise.output)) { + const red = red_dim_text; + const reset = reset_text; + + // Override the coloring applied by the printError method. + // NOTE: the first red and the last reset are not necessary, they + // are here only for alignment. return self.step.fail( \\ - \\========= expected this output: ========== + \\{s}========= expected this output: =========={s} \\{s} - \\========= but found: ===================== + \\{s}========= but found: ====================={s} \\{s} - \\========================================== - , .{ exercise_output, output }); + \\{s}=========================================={s} + , .{ red, reset, exercise_output, red, reset, output, red, reset }); } print("{s}PASSED:\n{s}{s}\n\n", .{ green_text, output, reset_text }); @@ -395,7 +405,9 @@ const ZiglingStep = struct { // Display error/warning messages. if (self.step.result_error_msgs.items.len > 0) { for (self.step.result_error_msgs.items) |msg| { - print("{s}error: {s}{s}\n", .{ red_text, reset_text, msg }); + print("{s}error: {s}{s}{s}{s}\n", .{ + red_bold_text, reset_text, red_dim_text, msg, reset_text, + }); } }