Skip to content

Commit

Permalink
chore: fix pty support on Macs (denoland#20037)
Browse files Browse the repository at this point in the history
Many of the CI tests have been failing on my M2 Pro mac (Ventura 13.4)
when running inside of a vscode terminal (a strange `ENOTTY` error).
This modifies the pty-handling code to use libc directly rather than the
older pty library that appears mostly unmaintained (outside of
@littledivy's fork).

As a bonus, this should allow us to run pty tests on the mac CI runner.

After this PR, the tests now complete with 100% success on my local
machine. Before this PR, I needed to pass `CI=true` to get my local test
suite to pass.
  • Loading branch information
mmastrac committed Aug 3, 2023
1 parent 6ba245f commit 0f07dc9
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 44 deletions.
12 changes: 1 addition & 11 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 1 addition & 3 deletions test_util/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ futures.workspace = true
glob.workspace = true
hyper = { workspace = true, features = ["server", "http1", "http2", "runtime"] }
lazy-regex.workspace = true
libc.workspace = true
lsp-types.workspace = true
nix.workspace = true
once_cell.workspace = true
Expand All @@ -43,8 +44,5 @@ tokio.workspace = true
tokio-rustls.workspace = true
url.workspace = true

[target.'cfg(unix)'.dependencies]
pty2 = "0.1.0"

[target.'cfg(windows)'.dependencies]
winapi = { workspace = true, features = ["consoleapi", "synchapi", "handleapi", "namedpipeapi", "winbase", "winerror"] }
92 changes: 62 additions & 30 deletions test_util/src/pty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ impl Pty {
}

pub fn is_supported() -> bool {
let is_mac_or_windows = cfg!(target_os = "macos") || cfg!(windows);
if is_mac_or_windows && std::env::var("CI").is_ok() {
// the pty tests give a ENOTTY error for Mac and don't really start up
// on the windows CI for some reason so ignore them for now
let is_windows = cfg!(windows);
if is_windows && std::env::var("CI").is_ok() {
// the pty tests don't really start up on the windows CI for some reason
// so ignore them for now
eprintln!("Ignoring windows CI.");
false
} else {
Expand Down Expand Up @@ -216,18 +216,18 @@ impl Pty {

trait SystemPty: Read + Write {}

impl SystemPty for std::fs::File {}

#[cfg(unix)]
fn setup_pty(master: &pty2::fork::Master) {
fn setup_pty(fd: i32) {
use nix::fcntl::fcntl;
use nix::fcntl::FcntlArg;
use nix::fcntl::OFlag;
use nix::sys::termios;
use nix::sys::termios::tcgetattr;
use nix::sys::termios::tcsetattr;
use nix::sys::termios::SetArg;
use std::os::fd::AsRawFd;

let fd = master.as_raw_fd();
let mut term = tcgetattr(fd).unwrap();
// disable cooked mode
term.local_flags.remove(termios::LocalFlags::ICANON);
Expand All @@ -246,21 +246,60 @@ fn create_pty(
cwd: &Path,
env_vars: Option<HashMap<String, String>>,
) -> Box<dyn SystemPty> {
let fork = pty2::fork::Fork::from_ptmx().unwrap();
if fork.is_parent().is_ok() {
let master = fork.is_parent().unwrap();
setup_pty(&master);
Box::new(unix::UnixPty { fork })
} else {
std::process::Command::new(program)
use crate::pty::unix::UnixPty;
use std::os::unix::process::CommandExt;

// Manually open pty main/secondary sides in the test process. Since we're not actually
// changing uid/gid here, this is the easiest way to do it.

// SAFETY: Posix APIs
let (fdm, fds) = unsafe {
let fdm = libc::posix_openpt(libc::O_RDWR);
if fdm < 0 {
panic!("posix_openpt failed");
}
let res = libc::grantpt(fdm);
if res != 0 {
panic!("grantpt failed");
}
let res = libc::unlockpt(fdm);
if res != 0 {
panic!("unlockpt failed");
}
let fds = libc::open(libc::ptsname(fdm), libc::O_RDWR);
if fdm < 0 {
panic!("open(ptsname) failed");
}
(fdm, fds)
};

// SAFETY: Posix APIs
unsafe {
let cmd = std::process::Command::new(program)
.current_dir(cwd)
.args(args)
.envs(env_vars.unwrap_or_default())
.pre_exec(move || {
// Close parent's main handle
libc::close(fdm);
libc::dup2(fds, 0);
libc::dup2(fds, 1);
libc::dup2(fds, 2);
// Note that we could close `fds` here as well, but this is a short-lived process and
// we're just not going to worry about "leaking" it
Ok(())
})
.spawn()
.unwrap()
.wait()
.unwrap();
std::process::exit(0);

// Close child's secondary handle
libc::close(fds);
setup_pty(fdm);

use std::os::fd::FromRawFd;
let pid = nix::unistd::Pid::from_raw(cmd.id() as _);
let file = std::fs::File::from_raw_fd(fdm);
Box::new(UnixPty { pid, file })
}
}

Expand All @@ -272,40 +311,33 @@ mod unix {
use super::SystemPty;

pub struct UnixPty {
pub fork: pty2::fork::Fork,
pub pid: nix::unistd::Pid,
pub file: std::fs::File,
}

impl Drop for UnixPty {
fn drop(&mut self) {
use nix::sys::signal::kill;
use nix::sys::signal::Signal;
use nix::unistd::Pid;

if let pty2::fork::Fork::Parent(child_pid, _) = self.fork {
let pid = Pid::from_raw(child_pid);
kill(pid, Signal::SIGTERM).unwrap()
}
kill(self.pid, Signal::SIGTERM).unwrap()
}
}

impl SystemPty for UnixPty {}

impl Read for UnixPty {
fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> {
let mut master = self.fork.is_parent().unwrap();
master.read(buf)
self.file.read(buf)
}
}

impl Write for UnixPty {
fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
let mut master = self.fork.is_parent().unwrap();
master.write(buf)
self.file.write(buf)
}

fn flush(&mut self) -> std::io::Result<()> {
let mut master = self.fork.is_parent().unwrap();
master.flush()
self.file.flush()
}
}
}
Expand Down

0 comments on commit 0f07dc9

Please sign in to comment.