First pass on running subprocesses

[piscisaureus]

No description provided.

[ry]

Cool! Needs tests

[kitsonk]

Couple comments. 🎉 was wanting this!

[piscisaureus]

https://code.visualstudio.com/docs/editor/tasks-appendix#_schema-for-tasksjson

^-- Also uses a CommandOptions interface

[ry]

https://code.visualstudio.com/docs/editor/tasks-appendix#_schema-for-tasksjson

^-- Also uses a CommandOptions interface

Very nice! Be sure to add that link in the source code so we maintain compatibility.

[ry]

I've refactored this API into two ops, so that child processes can be stored on the resources table. run() is now a sync op, which return a Child object.
await child.status() allows you to get the exit status
child.close() removes the resource.

I'm not so happy with all these different names "Command", "Run", "Child". I think we should use a single name for all these concepts. Does anyone have opinions on this?

[ry]

There is a lot to consider in this interface. We should name carefully. There are two distinct things to name in the low-level interface - the function which starts the subprocess - and the name of the subprocess object (a noun). Furthermore have the name of the command-line argument/permission. Ideally we want to introduce as few names as possible, we also want to be intelligible to people coming from different languages.

Here are a few options.

Option 1 Just call everything subprocess. Familiar to python people, correct terminology-wise. However it's very long.

--allow-subprocess   

let sp = deno.subprocess();
let status = await sp.status();
sp.close();

Option 2 Use "Subprocess" for the object but use "exec()" for the function. Exec isn't exactly right, as it has a fairly specific meaning - which we might want to expose at some point (but probably not). However it's short.

--allow-exec

let subprocess = deno.exec({ args: ["python"] });
let status = await subprocess.status();
subprocess.close();

Option 3 Use "Process" for the object but use "run()" for the function. run is short and doesn't overload concepts in Node. (Current naming)

--allow-run

let p = deno.run({ args: ["python"] });
let status = await p.status();
p.close();

[kitsonk]

We have compiler.run(), and while it is internal, it is what is used for executing a Deno programme at the moment. We don't really have the API's for running other JavaScript/Deno programmes yet, but it seems like something in the future.

I would vote for Option 1. It is clear in intent. The only other thought is going less functional and more class based and simply. I guess how much consistency do we want between constructors or factories for things?

const p = new Subprocess({ args: ["python"] });
let status = await p.status();
p.close();

[piscisaureus]

This type doesn't properly capture that either code or signal will have a value.
Thus the user won't be able to do:

declare function handleExitCode(code: number);
declare function handleExitSignal(sig: number);

if (status.code != null) {
  handleExitCode(status.code);
} else {
  handleExitSignal(status.signal);
}

[ry]

That code works currently.

[piscisaureus]

I concede -- It works because we using typescript in sloppy mode.
OK for now then.

[ry]

@piscisaureus Note that it is fundamentally sync. I don't think it blocks tho. Can you elaborate on your problem with it being sync?

[piscisaureus]

Okay if we hereby agree that RIDs should be assigned in on the JS side and not on the rust side and we'll make that in a follow-up patch.
Otherwise this adds a low level API that is fundamentally sync, which I will not approve.

[piscisaureus]

The issue is that run() cannot return until the rust side has assigned a RID to the to the SubProcess object.
So that gets in the way op queuing/batching, out-of-process backend etc. that we may want to do in the future.
I'm not against sync ops but I am against ops that can only be sync.

[ry]

Ok, I see what you mean. Yes, in order to support "dispatchv" we would need to allocate RIDs on the JS side. I'm ok with that. However there are many places currently where we're allocating RIDs and I think we should change them all to be allocated in JS at once. Therefore I'd argue in favor of keeping this as it is for now.

[piscisaureus]

Yes, in order to support "dispatchv" we would need to allocate RIDs on the JS side. I'm ok with that.

Ok, then we're golden.

[anjmao]

Other tests are using imported run function, but this one uses deno.run. I suggest using deno.run in all tests since it's better shows how deno's public api is used.

[piscisaureus]

LGTM. Can't approve since it's my own PR.

[ry]

I want to propose the following API. Which is based very closely on Go.

let cmd = deno.command("python", "-c", "print 'hi'");
let status = await cmd.run(); 
// run() starts the specified command and waits for it to complete.

Or if you want to start it and later wait for it to complete

let cmd = deno.command("python", "-c", "print 'hi'");
cmd.start();
let status = await cmd.exit(); 
// Go uses "Wait()" instead of "exit()". I replaced it because it sounds strange with await. 

Setting environmental variables (which won't be done in this patch)

let cmd = deno.command("prog");
cmd.env = Object.assign(deno.env(), {
  "FOO": "BAR"
});
let status = await cmd.run();

For stdin, stdout, stderr we would do the following to inherit from the parent process

let cmd = deno.command("prog");
cmd.stdout = deno.stdout;
cmd.stderr = deno.stderr;

Or to do the "piped" behavior

let cmd = deno.command("sh", "-c", "echo stdout; echo 1>&2 stderr");
let stderr = cmd.stderrPipe();
cmd.start();
let slurp = await deno.readAll(stderr); // Note readAll not implemented yet...

The Go API supports assigning an arbitrary File (in our case deno.File) to the stdio properties. We would not be able to achieve that with the current implementation of tokio_process.... But I think we should strive for ideal APIs and just die with notImplemented if someone supplies, for example, a File other than deno.stderr for cmd.stderr. Eventually we can replace tokio_process with something that matches these semantics better.

I think this API is straightforward and well thought-out. (Like most Go APIs.) It makes sense given how many other Go-inspired APIs we're using. It provides guidance in the future when expanding it (like in the case of env)

[ry]

Go also has a lower-level API: https://golang.org/pkg/os/#Process
Let me translate some of these:

let p = deno.startProcess("/usr/bin/python", ["python", "-c", "print 'hi'"], {
  // chdir into /tmp
  dir: "/tmp",
  // Inherit stdout from parent, stdin and stderr are null.
  files: [null, deno.stdout, null], 
});
// To kill a subprocess
p.kill();
// To detach a subprocess
p.release();
// Send a signal
p.signal("SIGINT");
// Wait for completion
let status = await p.exit();

[piscisaureus]

let p = deno.startProcess("/usr/bin/python", ["python", "-c", "print 'hi'"], { // chdir into /tmp dir: "/tmp", // Inherit stdout from parent, stdin and stderr are null. files: [null, deno.stdout, null], });

This is pretty similar to nodejs' spawn() api, except that here argv[0] is explicit.
That creates an awful mess when parsing the arguments. For a low level API we should really do just the options object:

startProcess({
  exe: "/usr/bin/python",  // Optional; if omitted, will be taken from args[0]
  args: ["python", "-c", "print 'hi'"],
  cwd: "/tmp"
});

[ry]

That creates an awful mess when parsing the arguments.

I would argue that the startProcess() with the three arguments (program, argv, options) is simpler than the options object alone - there isn't anything implicit in the former - nothing that has to be explained to the user like "Optional; if omitted, will be taken from args[0]"

Although it's not a very important use-case, it's worth noting that argv[0] can be different than the executed program. So it makes sense for the low-level API to separate the two concepts.

[ry]

LGTM - let's land this very useful patch and leave open the possibility for reworking these APIs later. I outlined my proposal in a comment above - after this is landed I will start an issue where we can discuss further.