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

Make the Pty constructor take an object #10

Merged
merged 3 commits into from
May 6, 2024
Merged

Make the Pty constructor take an object #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
61c93ac
Fix a race between process death and stdio read
lhchavez May 3, 2024
a1eae7f
Make the Pty constructor take an object
lhchavez May 5, 2024
73aed4c
Merge branch 'main' into lh-pty-ctor-object
szymonkaliski May 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,15 @@

/* auto-generated by NAPI-RS */

/** The options that can be passed to the constructor of Pty. */
export interface PtyOptions {
command: string
args?: Array<string>
envs?: Record<string, string>
dir?: string
size?: Size
onExit: (err: null | Error, exitCode: number) => void
}
/** A size struct to pass to resize. */
export interface Size {
cols: number
Expand Down Expand Up @@ -57,7 +66,7 @@ export interface Size {
export class Pty {
/** The pid of the forked process. */
pid: number
constructor(command: string, args: Array<string>, envs: Record<string, string>, dir: string, size: Size, onExit: (err: null | Error, exitCode: number) => void)
constructor(opts: PtyOptions)
/** Resize the terminal. */
resize(size: Size): void
/**
Expand Down
106 changes: 50 additions & 56 deletions index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,21 +64,18 @@ describe('PTY', () => {
const message = 'hello from a pty';
let buffer = '';

const pty = new Pty(
'/bin/echo',
[message],
{},
CWD,
{ rows: 24, cols: 80 },
(err, exitCode) => {
const pty = new Pty({
command: '/bin/echo',
args: [message],
onExit: (err, exitCode) => {
expect(err).toBeNull();
expect(exitCode).toBe(0);
expect(buffer).toBe(message + '\r\n');
pty.close();

done();
},
);
});

const readStreamFd = pty.fd();
const readStream = fs.createReadStream('', { fd: readStreamFd });
Expand All @@ -96,20 +93,17 @@ describe('PTY', () => {
});

test('captures an exit code', (done) => {
let pty = new Pty(
'/bin/sh',
['-c', 'exit 17'],
{},
CWD,
{ rows: 24, cols: 80 },
(err, exitCode) => {
const pty = new Pty({
command: '/bin/sh',
args: ['-c', 'exit 17'],
onExit: (err, exitCode) => {
expect(err).toBeNull();
expect(exitCode).toBe(17);
pty.close();

done();
},
);
});
});

test('can be written to', (done) => {
Expand All @@ -123,13 +117,16 @@ describe('PTY', () => {
32, 99, 97, 116, 13, 10,
]);

const pty = new Pty('/bin/cat', [], {}, CWD, { rows: 24, cols: 80 }, () => {
// We have local echo enabled, so we'll read the message twice.
assert(buffer);
expect(macOSLinuxCatBufferCompare(buffer, result)).toBe(true);
pty.close();
const pty = new Pty({
command: '/bin/cat',
onExit: () => {
// We have local echo enabled, so we'll read the message twice.
assert(buffer);
expect(macOSLinuxCatBufferCompare(buffer, result)).toBe(true);
pty.close();

done();
done();
},
});

const readStream = fs.createReadStream('', { fd: pty.fd() });
Expand Down Expand Up @@ -157,10 +154,14 @@ describe('PTY', () => {
});

test('can be resized', (done) => {
const pty = new Pty('/bin/sh', [], {}, CWD, { rows: 24, cols: 80 }, () => {
pty.close();
const pty = new Pty({
command: '/bin/sh',
size: { rows: 24, cols: 80 },
onExit: () => {
pty.close();

done();
done();
},
});

const readStream = fs.createReadStream('', { fd: pty.fd() });
Expand Down Expand Up @@ -202,21 +203,17 @@ describe('PTY', () => {
test('respects working directory', (done) => {
let buffer = '';

const pty = new Pty(
'/bin/pwd',
[],
{},
CWD,
{ rows: 24, cols: 80 },
(err, exitCode) => {
const pty = new Pty({
command: '/bin/pwd',
onExit: (err, exitCode) => {
expect(err).toBeNull();
expect(exitCode).toBe(0);
expect(buffer).toBe(`${CWD}\r\n`);
pty.close();

done();
},
);
});

const readStream = fs.createReadStream('', { fd: pty.fd() });

Expand All @@ -235,15 +232,13 @@ describe('PTY', () => {
const message = 'hello from env';
let buffer: Buffer | undefined;

const pty = new Pty(
'/bin/sh',
['-c', 'echo $ENV_VARIABLE && exit'],
{
const pty = new Pty({
command: '/bin/sh',
args: ['-c', 'echo $ENV_VARIABLE && exit'],
envs: {
ENV_VARIABLE: message,
},
CWD,
{ rows: 24, cols: 80 },
(err, exitCode) => {
onExit: (err, exitCode) => {
expect(err).toBeNull();
expect(exitCode).toBe(0);
assert(buffer);
Expand All @@ -252,7 +247,7 @@ describe('PTY', () => {

done();
},
);
});

const readStream = fs.createReadStream('', { fd: pty.fd() });

Expand All @@ -277,14 +272,17 @@ describe('PTY', () => {
111, 32, 98, 117, 110, 13, 10,
]);

const pty = new Pty('/bin/cat', [], {}, CWD, { rows: 24, cols: 80 }, () => {
// We have local echo enabled, so we'll read the message twice. Furthermore, the newline
// is converted to `\r\n` in this method.
assert(buffer !== undefined);
expect(macOSLinuxCatBufferCompare(buffer, result)).toBe(true);
pty.close();
const pty = new Pty({
command: '/bin/cat',
onExit: () => {
// We have local echo enabled, so we'll read the message twice. Furthermore, the newline
// is converted to `\r\n` in this method.
assert(buffer !== undefined);
expect(macOSLinuxCatBufferCompare(buffer, result)).toBe(true);
pty.close();

done();
done();
},
});

const file = Bun.file(pty.fd());
Expand All @@ -305,14 +303,10 @@ describe('PTY', () => {

test("doesn't break when executing non-existing binary", (done) => {
try {
new Pty(
'/bin/this-does-not-exist',
[],
{},
CWD,
{ rows: 24, cols: 80 },
() => {},
);
new Pty({
command: '/bin/this-does-not-exist',
onExit: () => {},
});
} catch (e) {
expect(e.message).toContain('No such file or directory');

Expand Down
40 changes: 26 additions & 14 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,18 @@ struct Pty {
pub pid: u32,
}

/// The options that can be passed to the constructor of Pty.
#[napi(object)]
struct PtyOptions {
pub command: String,
pub args: Option<Vec<String>>,
pub envs: Option<HashMap<String, String>>,
pub dir: Option<String>,
pub size: Option<Size>,
#[napi(ts_type = "(err: null | Error, exitCode: number) => void")]
pub on_exit: JsFunction,
}

/// A size struct to pass to resize.
#[napi(object)]
struct Size {
Expand Down Expand Up @@ -121,25 +133,20 @@ fn set_nonblocking(fd: c_int) -> Result<(), napi::Error> {
impl Pty {
#[napi(constructor)]
#[allow(dead_code)]
pub fn new(
env: Env,
command: String,
args: Vec<String>,
envs: HashMap<String, String>,
dir: String,
size: Size,
#[napi(ts_arg_type = "(err: null | Error, exitCode: number) => void")] on_exit: JsFunction,
) -> Result<Self, napi::Error> {
pub fn new(env: Env, opts: PtyOptions) -> Result<Self, napi::Error> {
let should_dup_fds = env.get_node_version()?.release == "node";
let size = opts.size.unwrap_or(Size { cols: 80, rows: 24 });
let window_size = Winsize {
ws_col: size.cols,
ws_row: size.rows,
ws_xpixel: 0,
ws_ypixel: 0,
};

let mut cmd = Command::new(command);
cmd.args(args);
let mut cmd = Command::new(opts.command);
if let Some(args) = opts.args {
cmd.args(args);
}

let rustix_openpty::Pty {
controller: controller_fd,
Expand All @@ -161,8 +168,12 @@ impl Pty {
cmd.stderr(Stdio::from(user_fd.try_clone()?));
cmd.stdout(Stdio::from(user_fd.try_clone()?));

cmd.envs(envs);
cmd.current_dir(dir);
if let Some(envs) = opts.envs {
cmd.envs(envs);
}
if let Some(dir) = opts.dir {
cmd.current_dir(dir);
}

unsafe {
let raw_user_fd = user_fd.as_raw_fd();
Expand Down Expand Up @@ -212,7 +223,8 @@ impl Pty {
// they are ready to be `wait`'ed. This has the inconvenience that it consumes one FD per child.
//
// For discussion check out: https://github.com/replit/ruspty/pull/1#discussion_r1463672548
let ts_on_exit: ThreadsafeFunction<i32, ErrorStrategy::CalleeHandled> = on_exit
let ts_on_exit: ThreadsafeFunction<i32, ErrorStrategy::CalleeHandled> = opts
.on_exit
.create_threadsafe_function(0, |ctx| ctx.env.create_int32(ctx.value).map(|v| vec![v]))?;
thread::spawn(move || match child.wait() {
Ok(status) => {
Expand Down