search

natalie-lang / natalie

a work-in-progress Ruby compiler, written in Ruby and C++
https://natalie-lang.org
MIT License
931 stars 61 forks source link

Implement first version of IO.popen #2200

Closed herwinw closed 1 month ago

herwinw commented 1 month ago

For now its functionality is pretty limited, but a number of unrelated specs depend on this method.

seven1m commented 1 month ago

I wonder if there's a way to do popen without adding a FILE to our IO object. I don't think CRuby has a FILE on the IO struct. Maybe they implement their own popen instead of using the system's?

Not a blocker for me; progress is progress. I just worry about the juggling of both a fileno and FILE*.

herwinw commented 1 month ago

I expect MRI does this the other way around: save a FILE*, instead of an int fd. (related: #1395)

herwinw commented 1 month ago

https://github.com/ruby/ruby/blob/6f6aff56b11d7f3df9b446b9399cc546875e07f5/include/ruby/io.h#L143-L154

Looks like it has both

herwinw commented 1 month ago

Another issue: popen is unidirectional by design, so we probably can't use that anyway for the r+ access mode (we have a popen2 in src/natalie.cpp that could be adapted to become bidirectional and solve this issue). That one now returns a FILE*, but does this by converting a file description using fdopen(3). We can probably get around without the FILE* in the IO object (but we would still need to save the pid).

Marking this as draft for now, rewriting this to use a modified popen2 might solve the Macos issues I'm seeing in the CI that I can't debug.

seven1m commented 1 month ago

OK, thanks for explaining that to me and for linking to the CRuby source. I was looking at the wrong struct! Having both FILE* and fd makes sense now.

herwinw commented 1 month ago

This now supports the basic functionality. I think this is good enough to get merged now, the next two steps are getting the additional arguments/argument types to work, and change the pipe to allow bidirectional communication.