HeyPuter / phoenix

🔥 Puter's pure-javascript shell
https://puter.com/app/terminal
GNU Affero General Public License v3.0
88 stars 15 forks source link

WIP: Add a basic PathCommandProvider #26

Closed AtkinsSJ closed 3 months ago

AtkinsSJ commented 4 months ago

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. :sweat_smile:)

Slight progress towards #14.

KernelDeimos commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

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

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 commented 3 months ago

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 commented 3 months ago

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

AtkinsSJ commented 3 months ago

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

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

AtkinsSJ commented 3 months ago

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

AtkinsSJ commented 3 months ago

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.