Skip to content

sys: add bun.sys.termios wrapper for Android bionic#30391

Open
robobun wants to merge 6 commits into
mainfrom
farm/43e239ac/android-termios
Open

sys: add bun.sys.termios wrapper for Android bionic#30391
robobun wants to merge 6 commits into
mainfrom
farm/43e239ac/android-termios

Conversation

@robobun

@robobun robobun commented May 8, 2026

Copy link
Copy Markdown
Collaborator

Problem

std.c.termios for .linux assumes the glibc/musl shape (cc[32] + trailing c_ispeed/c_ospeed, ~60B). bionic uses the raw kernel asm-generic/termbits.h struct:

#define NCCS 19
struct termios {
  tcflag_t c_iflag, c_oflag, c_cflag, c_lflag;
  cc_t c_line;
  cc_t c_cc[NCCS];
};  // 36B — no c_ispeed/c_ospeed; baud is in c_cflag CBAUD bits

The first 36 bytes are layout-compatible so tcgetattr/tcsetattr mostly work, but:

  • writes to .ispeed/.ospeed land past bionic's struct and are silently ignored
  • Terminal.zig reinitialises c_cflag (t.cflag = .{ .CREAD = true, … }), zeroing the CBAUD bits → baud becomes B0

Fix

Same pattern as #30389:

  • src/sys/sys.zig: add bun.sys.termios — on Android an extern struct matching bionic (cc: [19]u8, no speed fields, reusing std's tc_*flag_t since the bit layouts come from the kernel UAPI and are identical); elsewhere = std.posix.termios. Add bun.sys.tcgetattr/tcsetattr that @extern libc directly on Android and forward to std.posix.* elsewhere. A comptime tripwire fires if std.posix.termios ever loses .ispeed and shrinks to 36B, so the workaround can be dropped once the Zig stdlib gains a bionic case.
  • src/runtime/api/bun/Terminal.zig: switch to bun.sys.termios/tcgetattr/tcsetattr; guard the .ispeed/.ospeed assignments with @hasField (it's a PTY, so baud is advisory only on bionic).
  • src/md/ansi_renderer.zig: same swap for the kitty-graphics probe's termios save/restore.

Verification

  • bun run zig:check-all — all 16 targets pass
  • standalone zig build-obj -target {aarch64,x86_64}-linux-android -lc on the wrapper — bionic struct is 36B as expected, tripwire doesn't fire, flag-type reuse compiles
  • bun bd test test/js/bun/terminal/ — all 121 tests pass
  • new test/internal/termios-layout.test.ts opens a PTY master via posix_openpt, writes a sentinel into c_cc[VLNEXT] and toggles lflag.ECHO via bun.sys.tcsetattr, reads both back via bun.sys.tcgetattr. The round-trip holds iff Zig and libc agree on the struct layout.

No Android runtime test is included because the misbehaviour only surfaces on a real Android libc, which CI does not run; zig build check-android-debug (from #30389) compile-checks the struct once that lands.

@robobun

robobun commented May 8, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 3:40 AM PT - May 8th, 2026

@robobun, your commit 5f2af29 has 3 failures in Build #52786 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 30391

That installs a local version of the PR into your bun-30391 executable, so you can run:

bun-30391 --bun

@coderabbitai

coderabbitai Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Adds bun.sys-backed termios support (Android bionic layout fallback), migrates terminal code to bun.sys tcgetattr/tcsetattr, exposes a JSC testing hook and JS binding termiosLayout(), and adds a POSIX test that validates termios struct round-trip and field preservation.

Changes

Android-compatible termios support

Layer / File(s) Summary
Core termios type and functions
src/sys/sys.zig
Added pub const termios (Android: bionic-compatible extern struct; otherwise alias to posix.termios) and implemented pub fn tcgetattr / pub fn tcsetattr with Android @extern calls, INTR retry, and errno-to-error mapping; non-Android delegates to posix.
Terminal.zig PTY setup
src/runtime/api/bun/Terminal.zig
Migrated createPtyPosix internals to use bun.sys.tcgetattr/bun.sys.tcsetattr; made baud-rate writes conditional on termios layout and updated internal getTermios/setTermios helpers.
ANSI renderer Kitty probe
src/md/ansi_renderer.zig
Updated probeKittyGraphics() to save/restore stdin termios via bun.sys.tcgetattr and bun.sys.tcsetattr instead of std.posix equivalents.
JSC testing hook
src/sys_jsc/error_jsc.zig
Added TestingAPIs.termiosLayout which opens a PTY, probes specific cc and lflag.ECHO entries, round-trips termios via bun.sys APIs, and returns { installed, readback, sizeof }.
JS test binding
src/js/internal-for-testing.ts
Exposed TestingAPIs.termiosLayout as export const termiosLayout() returning undefined or { installed, readback, sizeof }.
Tests and documentation
test/internal/termios-layout.test.ts
Added a POSIX-only test that calls termiosLayout(), asserts readback === installed, checks cc_lnext === 0x5a, and verifies sizeof > 0; includes explanatory comments about platform layout differences.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: adding a bun.sys.termios wrapper specifically for Android bionic compatibility.
Description check ✅ Passed The description fully addresses both template sections with clear problem statement, detailed fix explanation, and comprehensive verification steps including compile checks and test results.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

std.c.termios for .linux assumes the glibc/musl shape (cc[32] +
trailing c_ispeed/c_ospeed, ~60B). bionic uses the raw kernel
asm-generic/termbits.h struct (cc[19], no speed fields, 36B). The
first 36B are layout-compatible so tcgetattr/tcsetattr appear to
work, but writes to .ispeed/.ospeed land past bionic's struct and
reinitialising c_cflag zeroes CBAUD so baud becomes B0.

Add bun.sys.{termios,tcgetattr,tcsetattr}: on Android an extern
struct matching bionic and @extern'd libc calls that take it;
elsewhere transparent aliases of std.posix.*. Switch Terminal.zig
and md/ansi_renderer.zig to the wrappers, guard the .ispeed/.ospeed
assignments behind @Hasfield, and add a comptime tripwire so the
workaround can be dropped once std grows a bionic case.

A termiosLayout testing hook round-trips c_cc[VLNEXT] and
lflag.ECHO through libc on a posix_openpt fd; that property holds
iff Zig and libc agree on the struct layout.
@robobun robobun force-pushed the farm/43e239ac/android-termios branch from f31304b to 2cfb968 Compare May 8, 2026 08:11
Comment thread test/internal/termios-layout.test.ts Outdated
Comment thread src/sys_jsc/error_jsc.zig Outdated
robobun added 3 commits May 8, 2026 08:54
On BSD/macOS the master fd from posix_openpt() is not a terminal
(tcgetattr -> ENOTTY); only the slave side is. Open it via
grantpt/unlockpt/ptsname and run the round-trip there so the
layout test works on every POSIX target.
A static `import { termiosLayout }` throws SyntaxError at module
load when the binding is absent. bun:test's console output counts
that as '1 fail', but the JUnit reporter emits zero <testcase>
elements, so a gate that parses JUnit sees 0 failures. Requiring
inside the test body turns it into an assertion failure that JUnit
records. Matches sigaction-layout.test.ts.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/internal/termios-layout.test.ts`:
- Line 33: Replace the full-struct equality check between result!.readback and
result!.installed with targeted assertions that only verify intended invariants:
assert that readback.cc_lnext equals installed.cc_lnext and that the ECHO flag
bit in readback.c_lflag matches installed.c_lflag (use the ECHO
constant/bitmask), and remove the expect(result!.readback).toEqual(...) line;
this ensures the test verifies the round-trip of the specific control character
and preservation of the ECHO bit without relying on cross-libc-normalized
fields.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c455073e-aa0a-4d68-8b06-b5d957ca24d5

📥 Commits

Reviewing files that changed from the base of the PR and between 1c6f79a and 5f2af29.

📒 Files selected for processing (1)
  • test/internal/termios-layout.test.ts

Comment thread test/internal/termios-layout.test.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant