Merge pull request #4342 from laytan/process-exec-improv

fix os2.process_exec on non-windows and add a smoke test

core/os/os2/allocators.odin

@@ -62,8 +62,8 @@ TEMP_ALLOCATOR_GUARD_END :: proc(temp: runtime.Arena_Temp, loc := #caller_locati
 
 @(deferred_out=TEMP_ALLOCATOR_GUARD_END)
 TEMP_ALLOCATOR_GUARD :: #force_inline proc(loc := #caller_location) -> (runtime.Arena_Temp, runtime.Source_Code_Location) {
-	tmp := temp_allocator_temp_begin(loc)
 	global_default_temp_allocator_index = (global_default_temp_allocator_index+1)%MAX_TEMP_ARENA_COUNT
+	tmp := temp_allocator_temp_begin(loc)
 	return tmp, loc
 }
 

core/os/os2/errors_linux.odin

@@ -162,6 +162,8 @@ _get_platform_error :: proc(errno: linux.Errno) -> Error {
 		return .Invalid_File
 	case .ENOMEM:
 		return .Out_Of_Memory
+	case .ENOSYS:
+		return .Unsupported
 	}
 
 	return Platform_Error(i32(errno))

core/os/os2/errors_posix.odin

@@ -26,6 +26,8 @@ _get_platform_error :: proc() -> Error {
 		return .Invalid_File
 	case .ENOMEM:
 		return .Out_Of_Memory
+	case .ENOSYS:
+		return .Unsupported
 	case:
 		return Platform_Error(errno)
 	}

core/os/os2/pipe_posix.odin

@@ -49,7 +49,7 @@ _pipe_has_data :: proc(r: ^File) -> (ok: bool, err: Error) {
 	if r == nil || r.impl == nil {
 		return false, nil
 	}
-	fd := posix.FD((^File_Impl)(r.impl).fd)
+	fd := __fd(r)
 	poll_fds := []posix.pollfd {
 		posix.pollfd {
 			fd = fd,
@@ -57,8 +57,10 @@ _pipe_has_data :: proc(r: ^File) -> (ok: bool, err: Error) {
 		},
 	}
 	n := posix.poll(raw_data(poll_fds), u32(len(poll_fds)), 0)
-	if n != 1 {
+	if n < 0 {
 		return false, _get_platform_error()
+	} else if n != 1 {
+		return false, nil
 	}
 	pipe_events := poll_fds[0].revents
 	if pipe_events >= {.IN} {
@@ -68,4 +70,4 @@ _pipe_has_data :: proc(r: ^File) -> (ok: bool, err: Error) {
 		return false, .Broken_Pipe
 	}
 	return false, nil
-}
+}

core/os/os2/process.odin

@@ -1,7 +1,7 @@
 package os2
 
 import "base:runtime"
-import "core:strings"
+
 import "core:time"
 
 /*
@@ -371,16 +371,18 @@ process_exec :: proc(
 	loc := #caller_location,
 ) -> (
 	state: Process_State,
-	stdout: []u8,
-	stderr: []u8,
+	stdout: []byte,
+	stderr: []byte,
 	err: Error,
 ) {
 	assert(desc.stdout == nil, "Cannot redirect stdout when it's being captured", loc)
 	assert(desc.stderr == nil, "Cannot redirect stderr when it's being captured", loc)
+
 	stdout_r, stdout_w := pipe() or_return
 	defer close(stdout_r)
 	stderr_r, stderr_w := pipe() or_return
-	defer close(stdout_w)
+	defer close(stderr_r)
+
 	process: Process
 	{
 		// NOTE(flysand): Make sure the write-ends are closed, regardless
@@ -392,45 +394,64 @@ process_exec :: proc(
 		desc.stderr = stderr_w
 		process = process_start(desc) or_return
 	}
-	stdout_builder := strings.builder_make(allocator) or_return
-	stderr_builder := strings.builder_make(allocator) or_return
-	read_data: for {
-		buf: [1024]u8
+
+	{
+		stdout_b: [dynamic]byte
+		stdout_b.allocator = allocator
+		defer stdout = stdout_b[:]
+
+		stderr_b: [dynamic]byte
+		stderr_b.allocator = allocator
+		defer stderr = stderr_b[:]
+
+		buf: [1024]u8 = ---
 		n: int
-		has_data: bool
-		hangup := false
-		has_data, err = pipe_has_data(stdout_r)
-		if has_data {
-			n, err = read(stdout_r, buf[:])
-			strings.write_bytes(&stdout_builder, buf[:n])
-		}
-		switch err {
-		case nil: // nothing
-		case .Broken_Pipe:
-			hangup = true
-		case:
-			return
-		}
-		has_data, err = pipe_has_data(stderr_r)
-		if has_data {
-			n, err = read(stderr_r, buf[:])
-			strings.write_bytes(&stderr_builder, buf[:n])
-		}
-		switch err {
-		case nil: // nothing
-		case .Broken_Pipe:
-			hangup = true
-		case:
-			return
+
+		stdout_done, stderr_done, has_data: bool
+		for err == nil && (!stdout_done || !stderr_done) {
+
+			if !stdout_done {
+				has_data, err = pipe_has_data(stdout_r)
+				if has_data {
+					n, err = read(stdout_r, buf[:])
+				}
+
+				switch err {
+				case nil:
+					_, err = append(&stdout_b, ..buf[:n])
+				case .EOF, .Broken_Pipe:
+					stdout_done = true
+					err = nil
+				}
+			}
+
+			if err == nil && !stderr_done {
+				has_data, err = pipe_has_data(stderr_r)
+				if has_data {
+					n, err = read(stderr_r, buf[:])
+				}
+
+				switch err {
+				case nil:
+					_, err = append(&stderr_b, ..buf[:n])
+				case .EOF, .Broken_Pipe:
+					stderr_done = true
+					err = nil
+				}
+			}
 		}
-		if hangup {
-			break read_data
+	}
+
+	if err != nil {
+		state, _ = process_wait(process, timeout=0)
+		if !state.exited {
+			_ = process_kill(process)
+			state, _ = process_wait(process)
 		}
+		return
 	}
-	err = nil
-	stdout = transmute([]u8) strings.to_string(stdout_builder)
-	stderr = transmute([]u8) strings.to_string(stderr_builder)
-	state = process_wait(process) or_return
+
+	state, err = process_wait(process)
 	return
 }
 

core/os/os2/process_linux.odin

@@ -523,7 +523,7 @@ _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) {
 		write_errno_to_parent_and_abort :: proc(parent_fd: linux.Fd, errno: linux.Errno) -> ! {
 			error_byte: [1]u8 = { u8(errno) }
 			linux.write(parent_fd, error_byte[:])
-			intrinsics.trap()
+			linux.exit(126)
 		}
 
 		stdin_fd: linux.Fd

core/os/os2/process_posix.odin

@@ -163,7 +163,7 @@ _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) {
 			#assert(len(posix.Errno) < max(u8))
 			errno := u8(posix.errno())
 			posix.write(parent_fd, &errno, 1)
-			runtime.trap()
+			posix.exit(126)
 		}
 
 		null := posix.open("/dev/null", {.RDWR})
@@ -223,7 +223,6 @@ _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) {
 			return
 		}
 
-		process.pid = int(pid)
 		process, _ = _process_open(int(pid), {})
 		return
 	}

core/os/os2/process_posix_other.odin

@@ -15,6 +15,7 @@ _process_list :: proc(allocator: runtime.Allocator) -> (list: []int, err: Error)
 }
 
 _process_open :: proc(pid: int, flags: Process_Open_Flags) -> (process: Process, err: Error) {
+	process.pid = pid
 	err = .Unsupported
 	return
 }

core/sys/posix/sys_wait.odin

@@ -124,11 +124,11 @@ WIFCONTINUED :: #force_inline proc "contextless" (x: c.int) -> bool {
 
 idtype_t :: enum c.int {
 	// Wait for any children and `id` is ignored.
-	P_ALL,
+	P_ALL  = _P_ALL,
 	// Wait for any child wiith a process group ID equal to `id`.
-	P_PID,
+	P_PID  = _P_PID,
 	// Wait for any child with a process group ID equal to `id`.
-	P_PGID,
+	P_PGID = _P_PGID,
 }
 
 Wait_Flag_Bits :: enum c.int {
@@ -166,6 +166,10 @@ when ODIN_OS == .Darwin {
 	WNOWAIT  :: 0x00000020
 	WSTOPPED :: 0x00000008
 
+	_P_ALL  :: 0
+	_P_PID  :: 1
+	_P_PGID :: 2
+
 	@(private)
 	_WSTATUS :: #force_inline proc "contextless" (x: c.int) -> c.int {
 		return x & 0o177
@@ -221,6 +225,10 @@ when ODIN_OS == .Darwin {
 	WNOWAIT  :: 8
 	WSTOPPED :: 2
 
+	_P_ALL  :: 7
+	_P_PID  :: 0
+	_P_PGID :: 2
+
 	@(private)
 	_WSTATUS :: #force_inline proc "contextless" (x: c.int) -> c.int {
 		return x & 0o177
@@ -275,6 +283,10 @@ when ODIN_OS == .Darwin {
 	WNOWAIT  :: 0x00010000
 	WSTOPPED :: 0x00000002
 
+	_P_ALL  :: 0
+	_P_PID  :: 1
+	_P_PGID :: 2
+
 	@(private)
 	_WSTATUS :: #force_inline proc "contextless" (x: c.int) -> c.int {
 		return x & 0o177
@@ -330,6 +342,10 @@ when ODIN_OS == .Darwin {
 	WNOWAIT  :: 0x00010000
 	WSTOPPED :: 0x00000002
 
+	_P_ALL  :: 0
+	_P_PID  :: 2
+	_P_PGID :: 1
+
 	@(private)
 	_WSTATUS :: #force_inline proc "contextless" (x: c.int) -> c.int {
 		return x & 0o177

tests/core/normal.odin

@@ -33,6 +33,7 @@ download_assets :: proc() {
 @(require) import "net"
 @(require) import "odin"
 @(require) import "os"
+@(require) import "os/os2"
 @(require) import "path/filepath"
 @(require) import "reflect"
 @(require) import "runtime"

tests/core/os/os2/process.odin

@@ -0,0 +1,24 @@
+package tests_core_os_os2
+
+import os "core:os/os2"
+import    "core:log"
+import    "core:testing"
+
+@(test)
+test_process_exec :: proc(t: ^testing.T) {
+	state, stdout, stderr, err := os.process_exec({
+		command = {"echo", "hellope"},
+	}, context.allocator)
+	defer delete(stdout)
+	defer delete(stderr)
+
+	if err == .Unsupported {
+		log.warn("process_exec unsupported")
+		return
+	}
+
+	testing.expect_value(t, state.exited,  true)
+	testing.expect_value(t, state.success, true)
+	testing.expect_value(t, err, nil)
+	testing.expect_value(t, string(stdout), "hellope\n")
+}

tests/core/sys/posix/posix.odin

@@ -1,8 +1,6 @@
 #+build darwin, freebsd, openbsd, netbsd
 package tests_core_posix
 
-import "base:runtime"
-
 import "core:log"
 import "core:path/filepath"
 import "core:strings"
@@ -217,52 +215,6 @@ test_termios :: proc(t: ^testing.T) {
 	testing.expect_value(t, transmute(posix.COutput_Flags)posix.tcflag_t(posix._FFDLY),  posix.FFDLY)
 }
 
-@(test)
-test_signal :: proc(t: ^testing.T) {
-	@static tt: ^testing.T
-	tt = t
-
-	@static ctx: runtime.Context
-	ctx = context
-
-	act: posix.sigaction_t
-	act.sa_flags = {.SIGINFO, .RESETHAND}
-	act.sa_sigaction = handler
-	testing.expect_value(t, posix.sigaction(.SIGCHLD, &act, nil), posix.result.OK)
-
-	handler :: proc "c" (sig: posix.Signal, info: ^posix.siginfo_t, address: rawptr) {
-		context = ctx
-		testing.expect_value(tt, sig, posix.Signal.SIGCHLD)
-		testing.expect_value(tt, info.si_signo, posix.Signal.SIGCHLD)
-		testing.expect_value(tt, info.si_status, 69)
-		testing.expect_value(tt, info.si_code.chld, posix.CLD_Code.EXITED)
-	}
-
-	switch pid := posix.fork(); pid {
-	case -1:
-		log.errorf("fork() failure: %v", posix.strerror())
-	case 0:
-		posix.exit(69)
-	case:
-		for {
-			status: i32
-			res := posix.waitpid(pid, &status, {})
-			if res == -1 {
-				if !testing.expect_value(t, posix.errno(), posix.Errno.EINTR) {
-					break
-				}
-				continue
-			}
-
-			if posix.WIFEXITED(status) || posix.WIFSIGNALED(status) {
-				testing.expect(t, posix.WIFEXITED(status))
-				testing.expect(t, posix.WEXITSTATUS(status) == 69)
-				break
-			}
-        }
-	}
-}
-
 @(test)
 test_pthreads :: proc(t: ^testing.T) {
 	testing.set_fail_timeout(t, time.Second)