ci: more robust handling of directories in shell.zig #1275
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We use shell.zig for various "scripting" tasks. One problem there is managing environment --- current directory and environmental variables. So far, the strategy was to avoid doing anything special about it and just use std API to mutate global process state. That's not great, for two reasons: - First, mutating process state is inherently fragile. For example, calling setenv in a multithreaded program is almost always UB - Second, mutating global state might lead to logical errors. For example, we had a bug recently where `foo` called `bar`, `bar` changed process working directory, and, upon return, `foo` ended up in an unexpected state. (#1272) To fix the issues, this PR phases our global API in favor of `Shell.cwd` field. Trivia: in most other languages, adding a second cwd would be quite confusing, as that would make the `std` API behave in unexpected ways. However in Zig the cwd is not ambient, but rather passed explicitly to almost all APIs: std.fs.cwd().readFle("Readme.me") So there's no confusion!
matklad
commented
Nov 7, 2023
| @@ -9,6 +9,7 @@ const Shell = @import("../../shell.zig"); | |||
| const TmpTigerBeetle = @import("../../testing/tmp_tigerbeetle.zig"); | |||
|
|
|||
| pub fn tests(shell: *Shell, gpa: std.mem.Allocator) !void { | |||
| assert(shell.file_exists("TigerBeetle.sln")); | |||
There was a problem hiding this comment.
Fun fact: I've added these asserts as a drive by change, because why not, and they've already caught a bug in this PR!
| @@ -132,8 +132,6 @@ fn build_tigerbeetle(shell: *Shell, info: VersionInfo, dist_dir: std.fs.Dir) !vo | |||
| var section = try shell.open_section("build tigerbeetle"); | |||
| defer section.close(); | |||
|
|
|||
| try shell.project_root.setAsCwd(); | |||
There was a problem hiding this comment.
Before, we were defensively setting up cwd, because it could have been changed from under our feet. With pushd/popd we can rather assume that the cwd is reasonable, and just assert that when appropriate.
| cwd: std.fs.Dir, | ||
|
|
||
| // Stack of working directories backing pushd/popd. | ||
| cwd_stack: [cwd_stack_max]std.fs.Dir, |
There was a problem hiding this comment.
Open-coded bounded array here:
- std.BoundedArray is banned in favor of stdx.BoundedArray
- but, as we use shell.zig in build.zig, we don't want to drag any deps here
- and, also, std.BoundedArray doesn't really by us all that much.
| // TODO(Zig): use cwd_dir once that is available https://github.com/ziglang/zig/issues/5190 | ||
| var buffer: [std.fs.MAX_PATH_BYTES]u8 = undefined; | ||
| const cwd_path = try shell.cwd.realpath(".", &buffer); | ||
|
|
There was a problem hiding this comment.
Not happy about all these copy-paste here, but also not sure if its worth to try to dry it out -- it's hard to move stack-allocated bufer to a helper. Hopefully, cwd_dir will be available in the next release.
kprotty
approved these changes
Nov 8, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We use shell.zig for various "scripting" tasks. One problem there is
managing environment --- current directory and environmental variables.
So far, the strategy was to avoid doing anything special about it and
just use std API to mutate global process state. That's not great, for
two reasons:
First, mutating process state is inherently fragile. For example,
calling setenv in a multithreaded program is almost always UB
Second, mutating global state might lead to logical errors. For
example, we had a bug recently where
foocalledbar,barchangedprocess working directory, and, upon return,
fooended up in anunexpected state. (ci: fix wrong working directory when unmarking release as a draft #1272)
To fix the issues, this PR phases our global API in favor of
Shell.cwdfield.
Trivia: in most other languages, adding a second cwd would be quite
confusing, as that would make the
stdAPI behave in unexpected ways.However in Zig the cwd is not ambient, but rather passed explicitly to
almost all APIs:
So there's no confusion!