-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix the error that Zig build system does not fail when a test fails #15078
Conversation
Double with: #15064 |
lib/init-exe/build.zig
Outdated
@@ -63,5 +63,5 @@ pub fn build(b: *std.Build) void { | |||
// the `zig build --help` menu, providing a way for the user to request | |||
// running the unit tests. | |||
const test_step = b.step("test", "Run unit tests"); | |||
test_step.dependOn(&exe_tests.step); | |||
test_step.dependOn(&exe_tests.run().step); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think exe_tests.run()
is marked as deprecated.
zig/lib/std/Build/CompileStep.zig
Line 551 in 37f6f79
/// Deprecated: use `std.Build.addRunArtifact` |
I haven't tested the following, but I think it would be better to have something like
// Creates a step for unit testing.
const exe_tests = b.addTest(.{
.root_source_file = .{ .path = "src/main.zig" },
.target = target,
.optimize = optimize,
});
const exe_tests_run = b.addRunArtifact(exe_tests);
// Similar to creating the run step earlier, this exposes a `test` step to
// the `zig build --help` menu, providing a way for the user to request
// running the unit tests.
const test_step = b.step("test", "Run unit tests");
test_step.dependOn(&exe_tests_run.step);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right and it works. Should I create a new pull request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no reason to create a new one. just amend this commit and force push.
better way
better way
format
it would be better imo to do this all in one commit. and did you forgot to update the other file too? |
I changed it online because my branch did not longer work :-( |
oops nvm. i didn't notice you changed both. LGTM (minus the extra commits) |
If you are using an old version of the Zig compiler, you can just build the stage3 compiler, following https://github.com/ziglang/zig/wiki/Contributing#editing-source-code. |
@Vexu Is it possible to merge this so you can use init-exe/init-lib again? |
Not with failed checks, I don't know how to help you with that since I can't find a failure in those 5k lines of success. |
|
As you can see, only the missing lines are added to the two templates |
But I can start the test again, see if anything changes. |
I checked this, but does not reproduces the problem. When I run
No, because this solution is allready deprecated: |
Fixes for: #15009 and #15059