Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] feat(unstable): add Deno.openPty() #14535

Closed
wants to merge 9 commits into from
Closed

[WIP] feat(unstable): add Deno.openPty() #14535

wants to merge 9 commits into from

Conversation

imjamesb
Copy link
Contributor

@imjamesb imjamesb commented May 8, 2022 •
edited
Loading

Fixes #3994.

Todos:

  • Module (pty)
    • Unix
      • openpty(size) - Open/fork a PTY.
      • write(buf) - Synchronously write to the PTYs input.
      • read(buf) - Synchronously read from the PTYs output.
      • close() - Close and dispose of the PTY.
      • resize(size) - Set a new size on the pty.
      • size() - Get the current size known to the kernel. (Already taken care of by Deno's tty op).
    • Windows
      • openpty(size) - Open/fork a pseudoconsole.
      • write(buf) - Synchronously Write to the console.
      • read(buf) - Synchronously Read from the console.
      • close() - Close and dispose of the pseudoconsole.
      • resize(size) - Set a new size on the pseudoconsole.
      • size() - Get the current size known to the kernel. (Already taken care of by Deno's tty op).
  • Extension
    • Resource
      • Read from resource.
      • Write to resource.
      • Make resource supported by the following.
        • Deno.consoleSize(rid)
        • Deno.isatty(rid)
        • Deno.setRaw(rid, state, options?)
    • Ops
      • Unix
        • op_pty_open({size}) -> rid
        • op_pty_resize({rid, size})
      • Windows
        • op_pty_open({size}) -> rid
        • op_pty_resize({rid, size})
      • spawn.rs should support a pty property to spawn processes onto the PTY.
    • JS
      • A Pty class.
        • readonly rid getter.
        • With readable.
        • With writable.
        • With resize().
        • With close() aliased to Deno.close(this.#rid).
        • With consoleSize() aliased to Deno.consoleSize(this.#rid).
        • With isatty() aliased to Deno.isatty(this.#rid).
        • With setRaw(state, options?) aliased to Deno.setRaw(this.#rid, state, options)
    • dts
      • Deno.consoleSize(rid) - Make return type its own interface. interface ConsoleSize {rows, columns}...
      • Add Pty interface.
      • Add pty? option in SpawnOptions interface.
      • Add openPty(size) function.
        • Must work on op_spawn_sync
        • Must work on op_spawn_child

Interface(s):

declare namespace Deno {
  interface ConsoleSize {
    columns: number;
    rows: number;
  }
  interface Pty {
    readonly readable: ReadableStream<Uint8Array>;
    readonly writable: WritableStream<Uint8Array>;
    readonly rid: number;
    resize(size: ConsoleSize): void;
    close(): Promise<void>;
    consoleSize(): ConsoleSize;
    isatty(): boolean;
  }
  function openPty(size: ConsoleSize): Pty;
  function consoleSize(rid: number): ConsoleSize;
  
  interface SpawnOptions {
    args?: string[];
    cwd?: string | URL;
    clearEnv?: boolean;
    env?: Record<string, string>;
    uid?: number;
    gid?: number;
    stdin?: "piped" | "inherit" | "null";
    stdout?: "piped" | "inherit" | "null";
    stderr?: "piped" | "inherit" | "null";
    pty?: number;
  }

@imjamesb
Copy link
Contributor Author

imjamesb commented May 8, 2022

@kitsonk Any feedback to the current structure?

@nayeemrmn
Copy link
Collaborator

To be inline with the new spawn() API it should be more like:

declare namespace Deno {
  interface PtyChild {
    readonly readable: ReadableStream<Uint8Array>;
    readonly writable: WritableStream<Uint8Array>;
    readonly pid: number;
    readonly status: Promise<ChildStatus>;
    kill(signo: Signal): void;
    size(): ConsoleSize;
    resize(consoleSize: ConsoleSize): void;
  }
  interface ConsoleSize {
    columns: number;
    rows: number;
  }
  interface SpawnPtyOptions extends ConsoleSize {
    args: string[];
    clearEnv?: boolean;
    env?: Record<string, string>;
  }
  /** Requires --allow-run */
  function spawnPty(command: string | URL, options: SpawnPtyOptions): PtyChild;
}

@imjamesb
Copy link
Contributor Author

imjamesb commented May 8, 2022

To be inline with the new spawn() API it should be more like:

declare namespace Deno {
  interface PtyChild {
    readonly readable: ReadableStream<Uint8Array>;
    readonly writable: WritableStream<Uint8Array>;
    readonly pid: number;
    readonly status: Promise<ChildStatus>;
    kill(signo: Signal): void;
    size(): ConsoleSize;
    resize(consoleSize: ConsoleSize): void;
  }
  interface ConsoleSize {
    columns: number;
    rows: number;
  }
  interface SpawnPtyOptions extends ConsoleSize {
    args: string[];
    clearEnv?: boolean;
    env?: Record<string, string>;
  }
  /** Requires --allow-run */
  function spawnPty(command: string | URL, options: SpawnPtyOptions): PtyChild;
}

Agreed! @nayeemrmn

  1. AFAIK portable_pty doesn't offer signal arg on kill().
  2. portable_pty offers a way to wait for a pty to exit, and a way to get the return code, or none if the process hasn't exited yet. I think both should be available?

@nayeemrmn
Copy link
Collaborator

and a way to get the return code, or none if the process hasn't exited yet

A way to poll for the return code, yeah that was a discussion with spawn() as well and was decided as kind of an antipattern in JS.

@imjamesb
Copy link
Contributor Author

imjamesb commented May 8, 2022

and a way to get the return code, or none if the process hasn't exited yet

A way to poll for the return code, yeah that was a discussion with spawn() as well and was decided as kind of an antipattern in JS.

I see, I guess there's a point there! Let's keep the await .status syntax ;)

imjamesb added 4 commits May 9, 2022 10:43
@imjamesb
Cargo.lock
454836f 
Signed-off-by: James Bradlee <[email protected]>
@imjamesb
Revert "Cargo.lock"
0994b82 
This reverts commit 454836f.
@imjamesb
added op_pty_open
bf8b039 
Signed-off-by: James Bradlee <[email protected]>
@imjamesb
added op_pty_wait, note that it is currently not working
bff202e 
Signed-off-by: James Bradlee <[email protected]>
@imjamesb
Copy link
Contributor Author

imjamesb commented May 9, 2022

the usecase for this is when you need to start a process in a tty other than the currently running tty. Instead of output being sent to the main processes stdout and stderr it is sent to the PTY's stdout and stderr. And it won't receive any input unless you specifically write to the PTY. This can allow you to run two child processes at once in the same Deno session, showing two outputs, switching between which process get data from stdin. You can send the WINCH signal to resize the amount of rows and columns the PTY has and so on. With this you can essentially make tmux on Deno
https://discord.com/channels/684898665143206084/688040863313428503/973217262913867786

@imjamesb imjamesb mentioned this pull request May 9, 2022
Closed
dsherret
dsherret reviewed May 11, 2022
View reviewed changes
runtime/Cargo.toml Outdated
@@ -82,6 +82,7 @@ sys-info = "0.9.1"
termcolor = "1.1.3"
tokio = { version = "1.17", features = ["full"] }
uuid = { version = "1.0.0", features = ["v4"] }
portable-pty = "0.7.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could instead refactor and reuse the code in test_util/src/pty.rs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could instead refactor and reuse the code in test_util/src/pty.rs

Maybe, but it needs to be async on the input and output, so some tokio magic would have to be used. I'll take a look and see what I find out!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems doable, I'm gonna try to get some work done on it, but most likely won't really get to it until next week.

@imjamesb imjamesb mentioned this pull request Jun 20, 2022
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsherret dsherret dsherret left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support a pty interface
3 participants
@imjamesb @nayeemrmn @dsherret