From 1cf910fb51259e786e8964072eb98a17353bc3c8 Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Sat, 8 Apr 2023 09:02:38 +0200 Subject: [PATCH 1/8] build: enable full parallelism when -Dhealed is set The eowyn.sh script is used in a github workflow, but after commit 0d56ba3 (build: restore the exercise chain), the github action will take more time to complete. Enable full build parallelism, when -Dhealed is true and -Dn is null. Use the standard CompileStep and RunStep, instead of ZiglingStep. On my PC, this change reduces the build time by about 30%. --- build.zig | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/build.zig b/build.zig index d2702d0..6a0bcb8 100644 --- a/build.zig +++ b/build.zig @@ -593,6 +593,28 @@ pub fn build(b: *Build) !void { } start_step.dependOn(&prev_step.step); + return; + } else if (use_healed) { + const test_step = b.step("test", "Test the healed exercises"); + b.default_step = test_step; + + for (exercises) |ex| { + const base_name = ex.baseName(); + const file_path = std.fs.path.join(b.allocator, &[_][]const u8{ + "patches/healed", ex.main_file, + }) catch unreachable; + + const build_step = b.addExecutable(.{ .name = base_name, .root_source_file = .{ .path = file_path } }); + if (ex.C) { + build_step.linkLibC(); + } + build_step.install(); + + const run_step = build_step.run(); + + test_step.dependOn(&run_step.step); + } + return; } @@ -604,7 +626,7 @@ pub fn build(b: *Build) !void { for (exercises, 0..) |ex, i| { const base_name = ex.baseName(); const file_path = std.fs.path.join(b.allocator, &[_][]const u8{ - if (use_healed) "patches/healed" else "exercises", ex.main_file, + "exercises", ex.main_file, }) catch unreachable; const build_step = b.addExecutable(.{ .name = base_name, .root_source_file = .{ .path = file_path } }); From 67f369484cbba0be0c6dcb9b73cf7f872a25baf7 Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Sat, 8 Apr 2023 09:51:07 +0200 Subject: [PATCH 2/8] build: ensure the exercise links libc, when needed When running, as an example, `zig build -Dhealed -Dn=93 test`, the build fails, because the `test` step use the standard CompileStep, instead of ZiglingStep. Ensure that exercises using libc are correctly built. Closes #229 --- build.zig | 3 +++ 1 file changed, 3 insertions(+) diff --git a/build.zig b/build.zig index 6a0bcb8..ca76dab 100644 --- a/build.zig +++ b/build.zig @@ -560,6 +560,9 @@ pub fn build(b: *Build) !void { }) catch unreachable; const build_step = b.addExecutable(.{ .name = base_name, .root_source_file = .{ .path = file_path } }); + if (ex.C) { + build_step.linkLibC(); + } build_step.install(); const run_step = build_step.run(); From d43f19ded87b4419d5fd5845bc34a0a18819a599 Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Sat, 8 Apr 2023 12:56:51 +0200 Subject: [PATCH 3/8] build: add support for skipping exercises Currently, exercises not working with the current Zig compiler are commented. This is problematic, since the exercise key and exercise index are no longer consistent. Add the skip field to the Excercise struct. Update ZiglingStep to support it and add a new SkipStep step to support it when using the standard build step. --- build.zig | 148 +++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 103 insertions(+), 45 deletions(-) diff --git a/build.zig b/build.zig index ca76dab..dbda63e 100644 --- a/build.zig +++ b/build.zig @@ -33,6 +33,9 @@ const Exercise = struct { /// We need to keep track of this, so we compile with libc C: bool = false, + /// This exercise is not supported by the current Zig compiler. + skip: bool = false, + /// Returns the name of the main file with .zig stripped. pub fn baseName(self: Exercise) []const u8 { assert(std.mem.endsWith(u8, self.main_file, ".zig")); @@ -425,49 +428,59 @@ const exercises = [_]Exercise{ .main_file = "083_anonymous_lists.zig", .output = "I say hello!", }, - // disabled because of https://github.com/ratfactor/ziglings/issues/163 + + // Skipped because of https://github.com/ratfactor/ziglings/issues/163 // direct link: https://github.com/ziglang/zig/issues/6025 - // .{ - // .main_file = "084_async.zig", - // .output = "foo() A", - // .hint = "Read the facts. Use the facts.", - // .@"async" = true, - // }, - // .{ - // .main_file = "085_async2.zig", - // .output = "Hello async!", - // .@"async" = true, - // }, - // .{ - // .main_file = "086_async3.zig", - // .output = "5 4 3 2 1", - // .@"async" = true, - // }, - // .{ - // .main_file = "087_async4.zig", - // .output = "1 2 3 4 5", - // .@"async" = true, - // }, - // .{ - // .main_file = "088_async5.zig", - // .output = "Example Title.", - // .@"async" = true, - // }, - // .{ - // .main_file = "089_async6.zig", - // .output = ".com: Example Title, .org: Example Title.", - // .@"async" = true, - // }, - // .{ - // .main_file = "090_async7.zig", - // .output = "beef? BEEF!", - // .@"async" = true, - // }, - // .{ - // .main_file = "091_async8.zig", - // .output = "ABCDEF", - // .@"async" = true, - // }, + .{ + .main_file = "084_async.zig", + .output = "foo() A", + .hint = "Read the facts. Use the facts.", + .@"async" = true, + .skip = true, + }, + .{ + .main_file = "085_async2.zig", + .output = "Hello async!", + .@"async" = true, + .skip = true, + }, + .{ + .main_file = "086_async3.zig", + .output = "5 4 3 2 1", + .@"async" = true, + .skip = true, + }, + .{ + .main_file = "087_async4.zig", + .output = "1 2 3 4 5", + .@"async" = true, + .skip = true, + }, + .{ + .main_file = "088_async5.zig", + .output = "Example Title.", + .@"async" = true, + .skip = true, + }, + .{ + .main_file = "089_async6.zig", + .output = ".com: Example Title, .org: Example Title.", + .@"async" = true, + .skip = true, + }, + .{ + .main_file = "090_async7.zig", + .output = "beef? BEEF!", + .@"async" = true, + .skip = true, + }, + .{ + .main_file = "091_async8.zig", + .output = "ABCDEF", + .@"async" = true, + .skip = true, + }, + .{ .main_file = "092_interfaces.zig", .output = "Daily Insect Report:\nAnt is alive.\nBee visited 17 flowers.\nGrasshopper hopped 32 meters.", @@ -568,7 +581,12 @@ pub fn build(b: *Build) !void { const run_step = build_step.run(); const test_step = b.step("test", b.fmt("Run {s} without checking output", .{ex.main_file})); - test_step.dependOn(&run_step.step); + if (ex.skip) { + const skip_step = SkipStep.create(b, ex); + test_step.dependOn(&skip_step.step); + } else { + test_step.dependOn(&run_step.step); + } const install_step = b.step("install", b.fmt("Install {s} to prefix path", .{ex.main_file})); install_step.dependOn(b.getInstallStep()); @@ -614,8 +632,12 @@ pub fn build(b: *Build) !void { build_step.install(); const run_step = build_step.run(); - - test_step.dependOn(&run_step.step); + if (ex.skip) { + const skip_step = SkipStep.create(b, ex); + test_step.dependOn(&skip_step.step); + } else { + test_step.dependOn(&run_step.step); + } } return; @@ -673,6 +695,12 @@ const ZiglingStep = struct { fn make(step: *Step, prog_node: *std.Progress.Node) anyerror!void { _ = prog_node; const self = @fieldParentPtr(@This(), "step", step); + + if (self.exercise.skip) { + print("Skipping {s}\n\n", .{self.exercise.main_file}); + + return; + } self.makeInternal() catch { if (self.exercise.hint.len > 0) { print("\n{s}HINT: {s}{s}", .{ bold_text, self.exercise.hint, reset_text }); @@ -863,3 +891,33 @@ const PrintStep = struct { try p.file.writeAll(p.message); } }; + +// Skip an exercise. +const SkipStep = struct { + step: Step, + exercise: Exercise, + + pub fn create(owner: *Build, exercise: Exercise) *SkipStep { + const self = owner.allocator.create(SkipStep) catch @panic("OOM"); + self.* = .{ + .step = Step.init(.{ + .id = .custom, + .name = "Skip", + .owner = owner, + .makeFn = make, + }), + .exercise = exercise, + }; + + return self; + } + + fn make(step: *Step, prog_node: *std.Progress.Node) !void { + _ = prog_node; + const p = @fieldParentPtr(SkipStep, "step", step); + + if (p.exercise.skip) { + print("{s} skipped\n", .{p.exercise.main_file}); + } + } +}; From c656536d3f322be31fffd88daa9a43683b11f693 Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Sun, 9 Apr 2023 18:33:59 +0200 Subject: [PATCH 4/8] build: simplify code and add tests Simplify the code finding the exercise number from the exercise index, when the -Dn option is set. This is now possible since the exercise numbers have no holes. Add the validate_exercises function to check that exercise number are in the correct order, and call it at the start of the build function. Add tests, with 2 test cases. --- build.zig | 36 ++++++++--- test/tests.zig | 167 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 196 insertions(+), 7 deletions(-) create mode 100644 test/tests.zig diff --git a/build.zig b/build.zig index dbda63e..3281ed4 100644 --- a/build.zig +++ b/build.zig @@ -1,6 +1,7 @@ const std = @import("std"); const builtin = @import("builtin"); const compat = @import("src/compat.zig"); +const tests = @import("test/tests.zig"); const Build = compat.Build; const Step = compat.build.Step; @@ -8,7 +9,7 @@ const Step = compat.build.Step; const assert = std.debug.assert; const print = std.debug.print; -const Exercise = struct { +pub const Exercise = struct { /// main_file must have the format key_name.zig. /// The key will be used as a shorthand to build /// just one example. @@ -511,6 +512,7 @@ const exercises = [_]Exercise{ pub fn build(b: *Build) !void { if (!compat.is_compatible) compat.die(); + if (!validate_exercises()) std.os.exit(1); use_color_escapes = false; if (std.io.getStdErr().supportsAnsiEscapeCodes()) { @@ -558,15 +560,12 @@ pub fn build(b: *Build) !void { const header_step = PrintStep.create(b, logo, std.io.getStdErr()); if (exno) |i| { - const ex = blk: { - for (exercises) |ex| { - if (ex.number() == i) break :blk ex; - } - + if (i == 0 or i > exercises.len - 1) { print("unknown exercise number: {}\n", .{i}); std.os.exit(1); - }; + } + const ex = exercises[i - 1]; const base_name = ex.baseName(); const file_path = std.fs.path.join(b.allocator, &[_][]const u8{ if (use_healed) "patches/healed" else "exercises", ex.main_file, @@ -667,6 +666,9 @@ pub fn build(b: *Build) !void { } } ziglings_step.dependOn(prev_step); + + const test_step = b.step("test", "Run all the tests"); + test_step.dependOn(tests.addCliTests(b, &exercises)); } var use_color_escapes = false; @@ -921,3 +923,23 @@ const SkipStep = struct { } } }; + +// Check that each exercise number, excluding the last, forms the sequence `[1, exercise.len)`. +fn validate_exercises() bool { + // Don't use the "multi-object for loop" syntax, in order to avoid a syntax error with old Zig + // compilers. + var i: usize = 0; + for (exercises[0 .. exercises.len - 1]) |ex| { + i += 1; + if (ex.number() != i) { + print( + "exercise {s} has an incorrect number: expected {}, got {s}\n", + .{ ex.main_file, i, ex.key() }, + ); + + return false; + } + } + + return true; +} diff --git a/test/tests.zig b/test/tests.zig new file mode 100644 index 0000000..3de42db --- /dev/null +++ b/test/tests.zig @@ -0,0 +1,167 @@ +const std = @import("std"); +const root = @import("../build.zig"); + +const debug = std.debug; +const fs = std.fs; + +const Build = std.build; +const Step = Build.Step; +const RunStep = std.Build.RunStep; + +const Exercise = root.Exercise; + +pub fn addCliTests(b: *std.Build, exercises: []const Exercise) *Step { + const step = b.step("test-cli", "Test the command line interface"); + + // We should use a temporary path, but it will make the implementation of + // `build.zig` more complex. + const outdir = "patches/healed"; + + fs.cwd().makePath(outdir) catch |err| { + debug.print("unable to make '{s}': {s}\n", .{ outdir, @errorName(err) }); + + return step; + }; + + { + const case_step = createCase(b, "case-1"); + + // Test that `zig build -Dn=n -Dhealed test` selects the nth exercise. + var i: usize = 0; + for (exercises[0 .. exercises.len - 1]) |ex| { + i += 1; + if (ex.skip) continue; + + const patch = PatchStep.create(b, ex, outdir); + + const cmd = b.addSystemCommand( + &.{ b.zig_exe, "build", b.fmt("-Dn={}", .{i}), "-Dhealed", "test" }, + ); + cmd.setName(b.fmt("zig build -D={} -Dhealed test", .{i})); + cmd.expectExitCode(0); + cmd.step.dependOn(&patch.step); + + // Some exercise output has an extra space character. + if (ex.check_stdout) + expectStdOutMatch(cmd, ex.output) + else + expectStdErrMatch(cmd, ex.output); + + case_step.dependOn(&cmd.step); + } + + step.dependOn(case_step); + } + + { + const case_step = createCase(b, "case-2"); + + // Test that `zig build -Dn=n -Dhealed test` skips disabled esercises. + var i: usize = 0; + for (exercises[0 .. exercises.len - 1]) |ex| { + i += 1; + if (!ex.skip) continue; + + const cmd = b.addSystemCommand( + &.{ b.zig_exe, "build", b.fmt("-Dn={}", .{i}), "-Dhealed", "test" }, + ); + cmd.setName(b.fmt("zig build -D={} -Dhealed test", .{i})); + cmd.expectExitCode(0); + cmd.expectStdOutEqual(""); + expectStdErrMatch(cmd, b.fmt("{s} skipped", .{ex.main_file})); + + case_step.dependOn(&cmd.step); + } + + step.dependOn(case_step); + } + + const cleanup = b.addRemoveDirTree(outdir); + step.dependOn(&cleanup.step); + + return step; +} + +fn createCase(b: *Build, name: []const u8) *Step { + const case_step = b.allocator.create(Step) catch @panic("OOM"); + case_step.* = Step.init(.{ + .id = .custom, + .name = name, + .owner = b, + }); + + return case_step; +} + +// Apply a patch to the specified exercise. +const PatchStep = struct { + const join = fs.path.join; + + const exercises_path = "exercises"; + const patches_path = "patches/patches"; + + step: Step, + exercise: Exercise, + outdir: []const u8, + + pub fn create(owner: *Build, exercise: Exercise, outdir: []const u8) *PatchStep { + const self = owner.allocator.create(PatchStep) catch @panic("OOM"); + self.* = .{ + .step = Step.init(.{ + .id = .custom, + .name = "Patch", + .owner = owner, + .makeFn = make, + }), + .exercise = exercise, + .outdir = outdir, + }; + + return self; + } + + fn make(step: *Step, _: *std.Progress.Node) !void { + const b = step.owner; + const self = @fieldParentPtr(PatchStep, "step", step); + const exercise = self.exercise; + const name = exercise.baseName(); + + // Use the POSIX patch variant. + const file = join(b.allocator, &.{ exercises_path, exercise.main_file }) catch + @panic("OOM"); + const patch = join(b.allocator, &.{ patches_path, b.fmt("{s}.patch", .{name}) }) catch + @panic("OOM"); + const output = join(b.allocator, &.{ self.outdir, exercise.main_file }) catch + @panic("OOM"); + + const argv = &.{ "patch", "-i", patch, "-o", output, file }; + + var child = std.process.Child.init(argv, b.allocator); + child.stdout_behavior = .Ignore; // the POSIX standard says that stdout is not used + _ = try child.spawnAndWait(); + } +}; + +// +// Missing functions from std.Build.RunStep +// + +/// Adds a check for stderr match. Does not add any other checks. +pub fn expectStdErrMatch(self: *RunStep, bytes: []const u8) void { + const new_check: RunStep.StdIo.Check = .{ + .expect_stderr_match = self.step.owner.dupe(bytes), + }; + self.addCheck(new_check); +} + +/// Adds a check for stdout match as well as a check for exit code 0, if +/// there is not already an expected termination check. +pub fn expectStdOutMatch(self: *RunStep, bytes: []const u8) void { + const new_check: RunStep.StdIo.Check = .{ + .expect_stdout_match = self.step.owner.dupe(bytes), + }; + self.addCheck(new_check); + if (!self.hasTermCheck()) { + self.expectExitCode(0); + } +} From 647a24afae436d9d10ff731c7110448d6be00ccf Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Sun, 9 Apr 2023 18:48:19 +0200 Subject: [PATCH 5/8] build: improve PrintStep, SkipStep and PatchStep names Use lover case for the step names. Add the exercise name for the SkipStep and PatchStep step name. --- build.zig | 4 ++-- test/tests.zig | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/build.zig b/build.zig index 3281ed4..32ce74a 100644 --- a/build.zig +++ b/build.zig @@ -875,7 +875,7 @@ const PrintStep = struct { self.* = .{ .step = Step.init(.{ .id = .custom, - .name = "Print", + .name = "print", .owner = owner, .makeFn = make, }), @@ -904,7 +904,7 @@ const SkipStep = struct { self.* = .{ .step = Step.init(.{ .id = .custom, - .name = "Skip", + .name = owner.fmt("skip {s}", .{exercise.main_file}), .owner = owner, .makeFn = make, }), diff --git a/test/tests.zig b/test/tests.zig index 3de42db..c8f4af2 100644 --- a/test/tests.zig +++ b/test/tests.zig @@ -109,7 +109,7 @@ const PatchStep = struct { self.* = .{ .step = Step.init(.{ .id = .custom, - .name = "Patch", + .name = owner.fmt("patch {s}", .{exercise.main_file}), .owner = owner, .makeFn = make, }), From be782af7a30456990e7bcc58e40be3ac11e1ce17 Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Mon, 10 Apr 2023 18:14:58 +0200 Subject: [PATCH 6/8] build: restore compatibility support Commit 0d56ba3 (build: restore the exercise chain) broke the compatibility support for old compilers, due to the use of the multi-object for loop syntax. Use the normal for loop syntax; the change still keep the code readable. Use the variable `n`, instead of `i`, when referring to the exercise number; this will improve the readability. Closes #227 --- build.zig | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/build.zig b/build.zig index 32ce74a..39096fe 100644 --- a/build.zig +++ b/build.zig @@ -559,13 +559,13 @@ pub fn build(b: *Build) !void { const header_step = PrintStep.create(b, logo, std.io.getStdErr()); - if (exno) |i| { - if (i == 0 or i > exercises.len - 1) { - print("unknown exercise number: {}\n", .{i}); + if (exno) |n| { + if (n == 0 or n > exercises.len - 1) { + print("unknown exercise number: {}\n", .{n}); std.os.exit(1); } - const ex = exercises[i - 1]; + const ex = exercises[n - 1]; const base_name = ex.baseName(); const file_path = std.fs.path.join(b.allocator, &[_][]const u8{ if (use_healed) "patches/healed" else "exercises", ex.main_file, @@ -603,8 +603,8 @@ pub fn build(b: *Build) !void { var prev_step = verify_step; for (exercises) |exn| { - const n = exn.number(); - if (n > i) { + const nth = exn.number(); + if (nth > n) { const verify_stepn = ZiglingStep.create(b, exn, use_healed); verify_stepn.step.dependOn(&prev_step.step); @@ -646,8 +646,11 @@ pub fn build(b: *Build) !void { ziglings_step.dependOn(&header_step.step); b.default_step = ziglings_step; + // Don't use the "multi-object for loop" syntax, in order to avoid a syntax + // error with old Zig compilers. var prev_step: *Step = undefined; - for (exercises, 0..) |ex, i| { + for (exercises) |ex| { + const n = ex.number(); const base_name = ex.baseName(); const file_path = std.fs.path.join(b.allocator, &[_][]const u8{ "exercises", ex.main_file, @@ -657,7 +660,7 @@ pub fn build(b: *Build) !void { build_step.install(); const verify_stepn = ZiglingStep.create(b, ex, use_healed); - if (i == 0) { + if (n == 1) { prev_step = &verify_stepn.step; } else { verify_stepn.step.dependOn(prev_step); From 1ee8aabcf3457fe5951958adc11a7c8ca2f3476a Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Mon, 10 Apr 2023 21:02:54 +0200 Subject: [PATCH 7/8] build: simplify the code when solving all the exercises Initialize the first step in the chain to the header_step, instead of making the code more complex handling the first step as a special case in the for loop. --- build.zig | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/build.zig b/build.zig index 39096fe..8b93e2b 100644 --- a/build.zig +++ b/build.zig @@ -643,14 +643,12 @@ pub fn build(b: *Build) !void { } const ziglings_step = b.step("ziglings", "Check all ziglings"); - ziglings_step.dependOn(&header_step.step); b.default_step = ziglings_step; // Don't use the "multi-object for loop" syntax, in order to avoid a syntax // error with old Zig compilers. - var prev_step: *Step = undefined; + var prev_step = &header_step.step; for (exercises) |ex| { - const n = ex.number(); const base_name = ex.baseName(); const file_path = std.fs.path.join(b.allocator, &[_][]const u8{ "exercises", ex.main_file, @@ -660,13 +658,9 @@ pub fn build(b: *Build) !void { build_step.install(); const verify_stepn = ZiglingStep.create(b, ex, use_healed); - if (n == 1) { - prev_step = &verify_stepn.step; - } else { - verify_stepn.step.dependOn(prev_step); + verify_stepn.step.dependOn(prev_step); - prev_step = &verify_stepn.step; - } + prev_step = &verify_stepn.step; } ziglings_step.dependOn(prev_step); From 47876e637185e2194dde1b9245e1bc603a604d7e Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Mon, 10 Apr 2023 21:24:49 +0200 Subject: [PATCH 8/8] build: make PrintStep thread safe Update PrintStep to always printing to stderr, using std.debug.print, so that the message is written atomically. Note that currently it is not an issue, since PrintStep prints the message before everything else. --- build.zig | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/build.zig b/build.zig index 8b93e2b..84535d1 100644 --- a/build.zig +++ b/build.zig @@ -557,7 +557,7 @@ pub fn build(b: *Build) !void { const use_healed = b.option(bool, "healed", "Run exercises from patches/healed") orelse false; const exno: ?usize = b.option(usize, "n", "Select exercise"); - const header_step = PrintStep.create(b, logo, std.io.getStdErr()); + const header_step = PrintStep.create(b, logo); if (exno) |n| { if (n == 0 or n > exercises.len - 1) { @@ -861,13 +861,12 @@ const ZiglingStep = struct { } }; -// Print a message to a file. +// Print a message to stderr. const PrintStep = struct { step: Step, message: []const u8, - file: std.fs.File, - pub fn create(owner: *Build, message: []const u8, file: std.fs.File) *PrintStep { + pub fn create(owner: *Build, message: []const u8) *PrintStep { const self = owner.allocator.create(PrintStep) catch @panic("OOM"); self.* = .{ .step = Step.init(.{ @@ -877,7 +876,6 @@ const PrintStep = struct { .makeFn = make, }), .message = message, - .file = file, }; return self; @@ -887,7 +885,7 @@ const PrintStep = struct { _ = prog_node; const p = @fieldParentPtr(PrintStep, "step", step); - try p.file.writeAll(p.message); + print("{s}", .{p.message}); } };