This could be a polymorphic variant (often the Stopped case isn't possible, e.g. for an exit status).

Would something like

type exit_status = [ `Exited of int | `Signaled of int ]

type status = [ exit_status | `Stopped of int ] 

be useful? If the Stopped case can sometimes happen in the exit case the exit_status method would still have to handle it. If it can never happen I suppose we can assert false it?

Yes, I think it can never happen and we should assert false for now.

We should be careful about the word "stopped" here. Normally a "stopped" process is one that has not exited but it just paused.

We could probably do with a helper function in lib_eio for checking the exit status. Process.await_exn or something?

Good idea -- do you think instead of the val exit_status we should provide val await : t -> exit_status and val await_exn : t -> unit ?

You can't ignore the base directory here (s is relative to that). It needs to use fchdir in that case.

Hmm, I've given it a quick go in 9f2a5ed which seems to be working but wasn't sure if that's right (or the cleanest way) to do it

Thanks, hopefully with the pipe changes I added in d553598 this can be completely removed as we check if there's an FD and if not we pipe into the sources and sinks (that code could do with a review I think)

While the low-level API should mimic the platform's native API, this high-level API is where we can add convenience features to make things easier to use.

For example, I think it should handle looking up the path for you. It could have an optional ?executable argument to take the path explicitly, but default to doing the sensible thing with argv[0] (i.e. look up implicit paths in $PATH). Note that this example wouldn't work on e.g. NixOS, where echo is at /run/current-system/sw/bin/echo.

stdin can default to /dev/null I think (we don't want to encourage passing it if it's not needed), and thinking about it further I'm OK with stderr being passed through by default. We already let all Eio code output traceln messages to stderr.

Not sure what's best for stdout. Making it optional and passing it though seems OK, as does making it a required argument. I think defaulting to /dev/null would be bad though as you wouldn't get any error indicating why it didn't work (unlike redirecting stdin, where you get end-of-file).

At some point we should add some other convenience functions like popen for cases like this, e.g.

Note: this will be a broken link until the next release. Might be OK, or we could link to the source code until then.

Not very convinced about this. I suspect that providing defaults for the standard streams would make this unnecessary.

Yes, I think it can never happen and we should assert false for now.

It's a shame we won't have the failing command in the error message.

We could have a Process.run for the common case of spawn followed by await. Then it would be easy to add run_exn and have it include the command in the error.

Accidental change?

If the cwd isn't a native directory then it should raise an exception instead of ignoring it.

You need to fork off a another fiber to handle this (I see you had it that way earlier and then removed it for some reason).

Failing to fork will work OK for short messages, where the kernel will buffer them, but will deadlock for longer ones. It's also a problem for interactive programs (e.g. hooked up to GUI). Imagine the program wants to output a long set of terms and conditions and then ask for confirmation. Once the stdout pipe is full, it will stop and wait for us to read it, but we're still here waiting for the user to finish sending the input (which they won't do because they haven't seen any output yet).

Very quickly, it seemed that there was a race between the process exiting and the standard input/output being read/written. This very reliably fails in the old fork version:

open Eio
open Eio.Std

let run env =
  let buf = Buffer.create 128 in
  Switch.run @@ fun sw ->
  let stdout = Flow.buffer_sink buf in
  let child =
    Process.spawn ~sw env#process_mgr ~stdin:env#stdin ~stdout ~stderr:env#stderr "/bin/echo" [ "echo"; "Hello" ]
  in
  Process.await_exn child;
  let contents = Buffer.contents buf in
  if contents = "Hello\n" then () else failwith contents

let () =
  Eio_main.run @@ fun env ->
  for i = 0 to 1000 do
    Eio.traceln "Process number %i" i;
    run env
  done

But your comment is right!

Yes, you need to wait for stdout to be copied to the buffer. Just waiting for the process to exit isn't sufficient. This works (but is easy to get wrong):

let run env =
  let buf = Buffer.create 128 in
  Switch.run (fun sw ->
      let stdout = Flow.buffer_sink buf in
      let child =
        Process.spawn ~sw env#process_mgr ~stdin:env#stdin ~stdout ~stderr:env#stderr "/bin/echo" [ "echo"; "Hello" ]
      in
      Process.await_exn child;
    );
  let contents = Buffer.contents buf in
  if contents = "Hello\n" then () else failwith contents

Perhaps we should just expose the pipes and let the user do the reading, as Lwt does? It's a bit of a pain with the types (which is why Lwt needs so many functions). Maybe GADTs would help (so if you pass ~stdout:Pipe then you can do Process.stdout child to get the flow).

Then we can add convenience functions on top (like popen) that collect the output for you, rather than hard-coding that behaviour in each backend.

https://docs.python.org/3/library/subprocess.html might be a good source of ideas.

Needs a test for the confined case too.