tinygo icon indicating copy to clipboard operation
tinygo copied to clipboard

add os.StartProcess

Open leongross opened this issue 1 year ago • 8 comments
trafficstars

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 :)

leongross avatar Jul 02 '24 14:07 leongross

@deadprogram @ayang64

leongross avatar Jul 09 '24 14:07 leongross

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!

leongross avatar Jul 09 '24 17:07 leongross

@ayang64 @aykevl @deadprogram @dgryski

leongross avatar Jul 16 '24 11:07 leongross

@leongross please rebase this branch and resolve merge conflicts. Thank you!

deadprogram avatar Jul 18 '24 15:07 deadprogram

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

leongross avatar Jul 23 '24 15:07 leongross

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

deadprogram avatar Jul 23 '24 15:07 deadprogram

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

leongross avatar Jul 29 '24 09:07 leongross

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.

leongross avatar Aug 02 '24 09:08 leongross

Closing since #4377 was merged.

aykevl avatar Nov 15 '24 10:11 aykevl