Scripts: Use ZIG_EXE environment variable #1909
Conversation
sentientwaffle
force-pushed
the
dj-ci-zig-exe-env
branch
from
5cab4d2
to
b10c7aa
Compare
I use (many) git worktrees, and rather than giving each their own zig binary, I install it once in my PATH, so `./zig/zig` doesn't exist. `ZIG_EXE` is compatible with both workflows. (`ZIG_EXE` is set automatically for `zig run ...` – but not `zig build ...` with Run steps, so it needs to be explicitly set in `build.zig`.)
sentientwaffle
force-pushed
the
dj-ci-zig-exe-env
branch
from
b10c7aa
to
b23a338
Compare
There was a problem hiding this comment.
LGTM!
In general, I am a firm believer that it's better to say "this is the single configuration our project uses" rather than to try to support many different ways to do things. (and, for my worktrees, I actuall have a ./zig/zig symlink in each one of them).
However we don't actually make this more configurable really, we are just extracting a hard-coded constant into a variable, which is an improvement just along the readability axis!
LGTM with zig_exe_alloc replaced with a field --- I don't think we should have any "caller manages the memory" functions in shell.zig, it should figure out memory itself
src/shell.zig
Outdated
| @@ -596,12 +596,33 @@ pub fn spawn_options( | |||
| return child; | |||
| } | |||
|
|
|||
| pub fn zig_exe_alloc(shell: Shell, allocator: std.mem.Allocator) ![]const u8 { | |||
There was a problem hiding this comment.
For simplicity, let's rather do this during Shell.create and store the result as a field, like how we do for project_root.
I debated whether we should keep shell.zig as an independent re-usable utility, or whether to allow dependency on TigerBeetle repo specifically, and I figured that a bounded amount of dependency is probably better.k
As a bonus, this'll tell us eagarly if we miss the environmental variable in our build.zig!
There was a problem hiding this comment.
Ah, that is much better! We even already have the EnvMap in Shell.create, which simplifies the error handling.
Though, we do miss the environment variable often -- e.g. build.zig itself uses Shell. It just doesn't end up being needed in those cases.
| @@ -143,7 +144,7 @@ fn run_fuzzers( | |||
|
|
|||
| const random = std.crypto.random; | |||
|
|
|||
| try shell.exec("../zig/zig build -Drelease build_fuzz", .{}); | |||
| try shell.zig("build -Drelease build_fuzz", .{}); | |||
There was a problem hiding this comment.
👍 I actually forgot that we have shell.zig!
| @@ -186,8 +187,12 @@ fn run_fuzzers( | |||
| log.debug("will start '{s}'", .{command}); | |||
| const child = try shell.spawn_options( | |||
There was a problem hiding this comment.
but, sadly, we can't use it here, because we need a child...
It's on my todo list to re-visint shell.zig and try to come up with somewhat more orthogonal set of APIs. I like using shell.zig, but the implementaiton is almost 1000 lines of mostly copy-paste :(
| @@ -97,7 +97,7 @@ fn validate_release(shell: *Shell, gpa: std.mem.Allocator, language_requested: ? | |||
| .{}, | |||
| ); | |||
| const tag = stdx.cut(release_info, "\t").?.prefix; | |||
| log.info("validateing release {s}", .{tag}); | |||
src/shell.zig
Outdated
| // ZIG_EXE is set in `build.zig`, so it is available when using the run step for `zig build | ||
| // scripts`. | ||
| // ZIG_EXE is already an absolute path, but zig/zig is not. | ||
| const zig_exe_default = comptime "zig/zig" ++ builtin.target.exeFileExt(); |
There was a problem hiding this comment.
So, let's assume that it's just always set!
There was a problem hiding this comment.
As a bonus to this -- b.zig_exe (in build.zig) is already an absolute path!
Unfortunately `zig_exe` is optional – we can't assert that the `ZIG_EXE` environment variable is set. In particular, `build.zig` itself uses `Shell`.
I use (many) git worktrees, and rather than giving each their own zig binary, I install it once in my PATH, so
./zig/zigdoesn't exist.ZIG_EXEis compatible with both workflows.(
ZIG_EXEis set automatically forzig run ...– but notzig build ...with Run steps, so it needs to be explicitly set inbuild.zig.)