tinygo
tinygo copied to clipboard
add os.StartProcess
This PR adds a working implementation for os.StartProcess, just a concatenation of the fork and exec syscalls.
The upstream Golang implementation has a lot more safeguards with mutices etc but to keep this feasible I think this will do as well.
I would really appreciate some feedback on this to keep this code as bug-free as possible :)
@deadprogram @ayang64
In general I think this is good but I'd like more context and better comments before we give this a detailed review.
Can you reword some of your comments and maybe add additional commentary to describe some of the compromises you had to make without having to read the code in detail?
Not that I don't want to read the code but comments would help me understand some of the considerations you're talking about.
This is very reasonable, will do that!
@ayang64 @aykevl @deadprogram @dgryski
@leongross please rebase this branch and resolve merge conflicts. Thank you!
The CI errors are difficult for me to explain. When I run the test locally I get the following output:
$ make
$ ./build/tinygo test -v -run TestForkExec os
=== RUN TestForkExec
forkExec succeeded: new process has pid &{255262}
--- PASS: TestForkExec (0.00s)
PASS
ok os 0.001s
Looks to me like the problem is something to do with needing some additional build tags. The CI builds that fail are Windows and macOS. For example:
https://github.com/tinygo-org/tinygo/actions/runs/10061289668/job/27810982068?pr=4323#step:17:32
Internal test dependencies seem to be broken now, although this PR does not change something related to os.Interrupt.
main_test.go:572: test error: could not compile: /Users/runner/hostedtoolcache/go/1.22.5/arm64/src/testing/internal/testdeps/deps.go:149:63: undefined: os.Interrupt
I suspect that the import errors originate in this syscall package requirement code:
https://github.com/tinygo-org/tinygo/blob/release/loader/goroot.go#L219
The golang standard library does not provide implementations for Fork and Execve and it looks as if the custom syscall package for GOOS=linux is not loaded but the standard one which then makes sense.
A merge that just adds Frok and Execve to the syscall package could be the solution here.|
Alternatively, we could put Execve and Fork into another package such as exec or os.
Closing since #4377 was merged.