WIP: Add a basic PathCommandProvider

[AtkinsSJ]

This checks for the given command name in each directory in $PATH, and returns an object for executing the first match it finds.

This executing is very basic: the command is run, and the arguments are passed to it, and we wait for it to exit before continuing. But we don't connect stdin, stdout, or stderr. (Because I couldn't figure out how to make a Node stream that works. 😅)

Slight progress towards #14.

[KernelDeimos]

This is my attempt so far to connect the streams. Unfortunately this is more complex than a call to .pipe() because of how node.js streams and the Streams API differ.

My attempt fails as soon as ctx.externs.out.write(chunk) is called on the callback passed to out.on('data', ...); the error says the stream is closed, but that shouldn't happen because I didn't see the output from console.log('DONE WAITING') (or even the output from console.log('WAITING')).

at the end of execute(ctx), after changing spawnSync to spawn;

const out = new stream.PassThrough();
const err = new stream.PassThrough();
const in_ = new stream.PassThrough();

out.on('data', (chunk) => {
    ctx.externs.out.write(chunk);
});
err.on('data', (chunk) => {
    ctx.externs.err.write(chunk);
});

const fn_err = label => err => {
    console.log(`ERR(${label})`, err);
};
out.on('error', fn_err('out'));
err.on('error', fn_err('err'));
child.stdout.on('error', fn_err('stdout'));
child.stderr.on('error', fn_err('stderr'));
child.stdin.on('error', fn_err('stdin'));


// pipe streams to child process
// out.pipe(child.stdin);
child.stdout.pipe(out);
child.stderr.pipe(err);

let rslv_sigint;
const p_int = new Promise(rslv => rslv_sigint = rslv);
ctx.externs.sig.on((signal) => {
    if ( signal === signals.SIGINT ) {
        rslv_sigint({ is_sigint: true });
    }
});

let data, done;
const next_data = async () => {
    let is_sigint = false;
    ({ value: data, done, is_sigint } = await Promise.race([
        p_int, in_.read(),
    ]));
    if ( is_sigint ) {
        console.log('SIGINT HAPPENED HERE');
        throw new Exit(130);
    }
    // ({ value: line, done } = await in_.read());
}

for ( await next_data() ; ! done ; await next_data() ) {
    console.log('WRITING SOMETHING')
    child.stdin.write(data);
}

// wait for the child to exit
console.log('WAITING');
await new Promise((resolve, reject) => {
    child.on('exit', (code) => {
        ctx.externs.out.write(`Exited with code ${code}`);
        if (code === 0) {
            resolve();
        } else {
            reject(new Error(`Exited with code ${code}`));
        }
    });
});
console.log('DONE WAITING');

[AtkinsSJ]

It complaining that the WritableStream is closed is a complete mystery to me. I bumped into that when I was first trying to make it work originally.

What confuses me is that passing a Node.js Stream object to stdio: [ ... ] makes it throw an error, saying that methods are missing. I think we do want to use spawnSync() so that the child can block, but that means having to provide the streams here instead of attaching callbacks after.

With some trial and error, I've found that it accepts passing in the process's streams. This isn't perfect, but it's a lot closer than what I had before. I've updated my code to do that (and to shrink the try block as you suggested).

[KernelDeimos]

I'll leave the PR open in the meantime. This is worth continued investigation because passing the streams on process directly will prevent command substitution and pipes from working here.

[AtkinsSJ]

Thought I was getting somewhere with this today, but I'm still a little stumped...

• If we spawn(), which is async, we close the stream here, because execution leaves the await execute(ctx); on line 287 there.
• If we spawnSync(), we can't pass the custom Stream objects in stdio: [ here ], unless we construct a file descriptor for them to use. From point 6 at https://nodejs.org/api/child_process.html#optionsstdio :

  The stream must have an underlying descriptor

I tried a hack, of using spawn() and ending the function with a spin-loop until the child process has exited. That works!... unless I include your code for handling stdin. My understanding of async JS needs some work, but it seems like await-ing anything makes us leave the function until the awaited thing completes?

Anyway, this feels promising.

[KernelDeimos]

Yep, await allows other tasks in a queue (managed but the runtime) to be executed - these can be other functions that finished awaiting something for example

[KernelDeimos]

Spawn should have an exit event so we can resolve a Promise object when the process is done

[AtkinsSJ]

Some progress, finally! It's still not perfect, as noted, but it's more correct than it was.

• stdin handling is wrong.
• The rejected-promise hack, which isn't making sense to me.

Actually testing SIGINT handling is also impossible until #65 is fixed.

[AtkinsSJ]

Another update, to make the code compatible with the upcoming which command, and make it a bit easier to follow.

[AtkinsSJ]

Changes: We now use node-pty where possible (aka, whenever we don't need to redirect outputs), which gives a much better experience, though not 100% there.

Long-term we'd need to properly handle terminal-related ioctls ourself, and stop needing node-pty, but it's nicer for now.

I've un-drafted in case you do feel like this is worth merging Eric, but fair enough if you don't yet.