Refactored Process.run

[ysbaddaden]

I needed Process.spawn, which executes a command, but returns its pid and won't wait for it. I eventually refactored all of Process.run, following the Ruby library (to some extent):

Process.spawn("make", output: "log/make.log")
Process.spawn("echo $var", env: { var: "value" })
Process.spawn({"cat", "-"}, input: "message", output: fd, error: fd)
Process.spawn("sleep 10", pgroup: true) # child process will outlive it's parent

I also implemented Open3, which helps to spawn processes with pipes and capture the output:

Open3.popen2("make") { |in, out, thr| ... }
output, error, status = Open3.capture3({"markdown", "--to", "html"}, md_text)
whole_output, status = Open3.capture2e({"make", "install"})

[ysbaddaden]

Damn, I didn't notice #379 earlier :/

[jhass]

Mmh, I actually like having a single unified interface for process spawning, that avoids having to learn a bunch of them which many have overlapping usecases. In Ruby I'm always not sure for a moment which one I should use, and I see a lot of people doing things like system("cmd 2>&1"), just because they didn't find the appropriate interfaces yet. To reduce everyday complexity in using that interface, I'd say we should have the common top level functions, like we already have andsystem, spawnwould indeed be nice. We also talked about not lettingsystemspawn a shell by default and addshellto do that instead. All those would do is invokingProcess.run` the right way.

[ysbaddaden]

I thought the following choices do what the programmer most certainly expects:

• system(String) => spawn in a shell
• system(Array | Tuple) => no shell

I personaly like a bare-metal Process.spawn with the niceties of Open3 on top of it, with 3 capture methods doing different things —documentation for system could point to it, or even rely on it.

I'd also favor dropping support for $? and Process::Status.last —not thread-safe, not even sure this is fiber-safe— to rely on Open3 instead, and have Open3 capture methods return a Process::Status (maybe).

[asterite]

@ysbaddaden If you and @jhass are sending pull requests for the same thing it only means it's time to celebrate: the community is growing! 😄🎉🎂🎈🎁

I'll review this with @waj today, if we have some time. There's a lot of functionality here, and I like it that it builds on top of other methods. And it doesn't segfault on travis :-) (but I'm almost sure the other PR segfaults because of a bug in Crystal, I still have to review that...)

Merci beaucoup!

[ysbaddaden]

I'm open for directions about system and the popen and capture methods.

Shall we strip system down to what the C method does? Ie. exec a command in a shell and return an exit status, or shall it have more bells and whistles like Ruby or even more like Open3? Or have it exec commands, and have a shell method instead... So many choices!

I like that Open3 has different methods for the 3 most common use cases, thought I agree the different names can be confusing, and require documentation. We could merge popen and capture into Process, and drop Open3. Also, I'm not sure if a Tuple or a struct is better. I know @asterite prefers structs.

[ysbaddaden]

To be honest, there was a Broken pipe: 13 in one Travis build: https://travis-ci.org/manastech/crystal/builds/49293470

[jhass]

I think making the top level functions abstractions with simple interfaces upon a more complex interface somewhere else is the way to go, there's no point in replicating a complex interface at the top level, if the convenience wrapper is not handling a usecase, one should just use the thing it wraps.

[asterite]

@ysbaddaden @jhass We are still reviewing both pull requests. It's very hard to take a decision :-)

We want to make the API very comfortable to use, simple and without redundancy. Of course this is hard because there's so many things you could want to do. There's for example this, which seems pretty confusing. I'm not sure why Ruby has so many ways to deal with this, I guess maybe it was an evolutionary process and they needed to keep backwards compatibility.

@ysbaddaden We really like that you copied Ruby's semantic here, but we might try to do something different (we don't know what, yet). Another thing is that your Process#spawn seems to only work with FileDescriptorIO. Then there's Open3, which we didn't know it existed and we are not sure either about that strange name (we could merge it into Process). But, overall the Pull Request is very clean and complete, we just don't know if it's the Crystal Way ® (mostly because we don't know whay Crystal Way means yet 😛)

So... patience :-)

But let's use this space to discuss about the design we want, and all the possibilities we want to cover.

[ysbaddaden]

Maybe I can have another PR with only Process.spawn (and Process.kill)? That would help not rushing a decision, and that's all I need for Prax 😁

Process.spawn and Process.exec should accept any IO. I modeled their implementation after Process.run and just permitted to pass a String (filename) for output or error. I didn't specify types, because I don't want to have to deal with them in Crystal code (unless calling C).

My moto is usually: don't bother reinventing the wheel if Ruby did. Yet I agree that:

• capturing process output/error isn't covered by Ruby's core, apart from system (totally different from C), the thread unsafe $? global, and backtick commands; that's a bit confusing;
• Open3 isn't easy to find in Ruby's stdlib, and has an peculiar naming scheme, but I like how simple it makes dealing with process pipes and to capture output/error. Especially now that I know it better!

[asterite]

@ysbaddaden Thank you for taking the time to do this.

We finally did it in bbf2036 . Before this we looked at every pull request about this big issue and we combined all that knowledge into a single, unified interface.

I'm sure there are a lot of thing to improve (for example Process::Status doesn't have the pid of the process, or there's no env or working_dir options) but now it should be much easier to implement these.