Implement Progress IPC for Windows (#20105) #22000
Conversation
skyfex
force-pushed
the
progress-ipc-win
branch
from
cb1d968
to
5476460
Compare
| @@ -1831,7 +1831,7 @@ pub const CreateEnvironOptions = struct { | |||
| /// `null` means to leave the `ZIG_PROGRESS` environment variable unmodified. | |||
| /// If non-null, negative means to remove the environment variable, and >= 0 | |||
| /// means to provide it with the given integer. | |||
| zig_progress_fd: ?i32 = null, | |||
| zig_progress_fd: if (native_os == .windows) ?usize else ?i32 = null, | |||
There was a problem hiding this comment.
Using usize for windows excludes possibility of negative number here.. not sure about the consequences of that yet.
There was a problem hiding this comment.
Isn't it very sketchy to use just a negative number to mean something special for one particular function? Should probably be handled with an enum or something?
| if (fd >= 0) { | ||
| zig_progress_value = try std.fmt.allocPrintZ(allocator, "{d}", .{fd}); | ||
| break :a if (contains) .edit else .add; | ||
| } else { |
There was a problem hiding this comment.
the else case might never happen on Windows since we use "usize" for fd
| break :m try process.createWindowsEnvBlock(self.allocator, &env_map, .{ .zig_progress_fd = prog_fd }); | ||
| } | ||
| }; | ||
| // defer if (maybe_envp_buf) |envp_buf| self.allocator.free(envp_buf); |
There was a problem hiding this comment.
None of the cases above return null, so "maybe_envp_buf" ends up not being an optional...
I guess as an optimization we could return null here if prog_pipe_wr/prog_fd is null
| @@ -631,6 +631,7 @@ pub fn ReadFile(in_hFile: HANDLE, buffer: []u8, offset: ?u64) ReadFileError!usiz | |||
| .OPERATION_ABORTED => continue, | |||
| .BROKEN_PIPE => return 0, | |||
| .HANDLE_EOF => return 0, | |||
| .NO_DATA => return 0, | |||
There was a problem hiding this comment.
When windows.PIPE_NOWAIT was added to the pipe used for reading, I got the NO_DATA error here.
I was kind of guessing that it would be correct to return 0 here in this case..
| @@ -1394,6 +1411,70 @@ fn windowsMakeAsyncPipe(rd: *?windows.HANDLE, wr: *?windows.HANDLE, sattr: *cons | |||
| wr.* = write_handle; | |||
| } | |||
|
|
|||
| fn windowsMakeProgressPipe(rd: *?windows.HANDLE, wr: *?windows.HANDLE, sattr: *const windows.SECURITY_ATTRIBUTES) !void { | |||
There was a problem hiding this comment.
I tried to use windows.CreatePipe to avoid kernel32 calls (I saw #1840) but wasn't able to make that work in my testcase.
My first stab at solving a contributor friendly issue. I'm not very confident about the changes, so review carefully. I wouldn't be surprised if some rework is needed.
I wrote this before I discovered that the pull request #20114 existed. Maybe it's a better solution, I don't know. Should probably look at whether there's some things from there that can be adapted.