Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add os.StartProcess #4323

Closed
wants to merge 3 commits into from
Closed

Conversation

leongross
Copy link
Contributor

@leongross leongross commented Jul 2, 2024

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 leongross marked this pull request as ready for review July 9, 2024 08:39
@leongross leongross force-pushed the os/StartProcess branch 2 times, most recently from 33a554e to e12c2de Compare July 9, 2024 12:05
@leongross
Copy link
Contributor Author

@deadprogram @ayang64

Copy link
Contributor

@ayang64 ayang64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/os/exec_posix.go Outdated Show resolved Hide resolved
src/os/exec_posix.go Outdated Show resolved Hide resolved
src/syscall/syscall.go Outdated Show resolved Hide resolved
@leongross
Copy link
Contributor Author

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 leongross changed the title add os.StartProcess implementation (hacky) add os.StartProcess implementation Jul 9, 2024
@leongross leongross requested a review from ayang64 July 9, 2024 18:43
@leongross
Copy link
Contributor Author

@leongross leongross force-pushed the os/StartProcess branch 4 times, most recently from 044812d to 33f688d Compare July 16, 2024 15:02
@deadprogram
Copy link
Member

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

@leongross leongross force-pushed the os/StartProcess branch 6 times, most recently from c949f60 to c5b9207 Compare July 23, 2024 09:07
@leongross leongross changed the title add os.StartProcess implementation add os.StartProcess Jul 23, 2024
@leongross
Copy link
Contributor Author

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

@deadprogram
Copy link
Member

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

@leongross leongross force-pushed the os/StartProcess branch 4 times, most recently from 455419f to cb2f4c3 Compare July 29, 2024 08:27
@leongross
Copy link
Contributor Author

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 leongross force-pushed the os/StartProcess branch 2 times, most recently from 4dcef18 to 9f94438 Compare July 31, 2024 13:09
src/os/exec_posix_test.go Outdated Show resolved Hide resolved
fail := int(libc_fork())
if fail < 0 {
// TODO: parse the syscall return codes
return errors.New("fork failed")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you can wrap some standard error in here, tests are easier.
syscall.EAGAIN is at least as good as this errors.New I think?

fail := int(libc_execve(&argv0[0], &argv1[0], &env1[0]))
if fail < 0 {
// TODO: parse the syscall return codes
return errors.New("fork failed")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here. fail can even be
fail := syscall.Errno(libc_execve(&argv0[0], &argv1[0], &env1[0]))

@leongross leongross force-pushed the os/StartProcess branch 2 times, most recently from aa6f630 to 2b0cd9e Compare August 2, 2024 09:24
@leongross
Copy link
Contributor Author

leongross commented Aug 2, 2024

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.

@aykevl
Copy link
Member

aykevl commented Nov 15, 2024

Closing since #4377 was merged.

@aykevl aykevl closed this Nov 15, 2024
@leongross leongross deleted the os/StartProcess branch November 15, 2024 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants