Add aarch64-linux-android target#29675
Conversation
|
Updated 5:26 PM PT - Apr 25th, 2026
✅ @Jarred-Sumner, your commit a305e1fea7ec8120867b4cb7113c2bc54944e4c0 passed in 🧪 To try this PR locally: bunx bun-pr 29675That installs a local version of the PR into your bun-29675 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for addressing all the earlier feedback — nothing new from the bug hunt on b66ed66. Deferring to a human for final sign-off since this is a 52-file new-platform port touching build infra, TLS root-cert loading, DNS defaults, syscall fallbacks, and a couple of intentional non-Android behavior changes (timerfd, c-ares double-init).
Extended reasoning...
Overview
Adds aarch64-linux-android (and x64-android) as a Bun build target. 52 files: build system (scripts/build/, build.zig, .buildkite/, bootstrap.{sh,ps1}), vendored-dep config (cares, libarchive, lolhtml, webkit), and runtime plumbing across env.zig, sys.zig, dns.zig, c_ares.zig, node_os/node_fs, fs.zig, run_command/upgrade_command, compile_target.zig, crash_handler.zig, npm.zig, PackageManager.zig, BunProcess.cpp, bun-spawn.cpp, NodeVM.cpp, shell/{interpreter,subproc}.zig, open.zig, Global.zig, plus codegen TARGET_PLATFORM threading. Also includes a few deliberate non-Android changes called out in the description: timerfd_create CLOCK_REALTIME→CLOCK_MONOTONIC (all Linux), dropping a leaked ares_init() before ares_init_options() (all platforms), and a jsDynamicCast hardening in NodeVM.cpp.
Prior feedback — all addressed
I left seven inline comments across earlier revisions; all are resolved in the current diff:
- 🔴 UBSan disabled on macOS debug (flags.ts
=== "gnu") → fixed in ebd3dd4 to!== "musl" && !== "android". - 🟡 eventfd NULL-return → now
BUN_PANIC(andBun__panicextern hoisted to internal.h). - 🟡 root_certs c_rehash matching ungated → now
#ifdef __ANDROID__. - 🟡
os.machine()linux-arm64 change → scoped to Android only with a TODO for the separate fix. - 🟡 codegen.ts JSDoc misplacement →
codegenTarget()moved above theregisterCodegenRulesdoc. - 🟡 subproc.zig PATH fallback on Windows → now gated on
bun.Environment.isPosix, matchingBun.spawn(). - 🟡 timerfd scope → kept (correctly) and called out in the PR description.
Security risks
Touches TLS system-CA loading (root_certs_linux.cpp) — but the new code path is #ifdef __ANDROID__-gated and only adds well-known Android cert directories; the non-Android path is byte-identical except for a whitespace fix. NodeVM.cpp swaps a jsCast for jsDynamicCast on a user-influenced shape, which is a hardening, not a risk. The seccomp EACCES/PERM fallbacks in sys.zig widen the "disable this syscall" trigger on all Linux, which is benign (worst case: a one-time fallback to the older syscall on a box where the newer one would have worked with EPERM for some other reason). No injection/auth/data-exposure surface introduced.
Level of scrutiny
High. This is a new platform target with cross-cutting runtime changes, a WebKit version bump, CI/build-image changes, and a handful of intentional behavior changes on shipping platforms. None of it is mechanical. The author explicitly notes runtime is "not yet verified" on-device. This is exactly the kind of PR that benefits from a maintainer's read of the design choices (API level 28, host-clang+NDK-sysroot approach, process.platform === "android" threading, DNS default switch) even though no defects remain from automated review.
Other factors
No Android test platforms in CI yet (acknowledged in description). Depends on #29393. CI on 03a6b9c showed one unrelated aarch64-musl build failure. Given scope and the new-target nature, deferring rather than approving.
…ebuilt) Pre-existing from #29675: Android Release CI defaulted to lto=true because os==="linux", but no -android-lto WebKit prebuilt is published. (FreeBSD already excluded since linux is false.)
There was a problem hiding this comment.
All my earlier feedback has been addressed and I didn't find anything new, but this is a 53-file new-platform port (build system, NDK toolchain wiring, TLS root-cert loading, plus a few intentional non-Android behavior changes) with no on-device runtime verification yet — worth a human pass before merging.
Extended reasoning...
Overview
Adds aarch64-linux-android (and x64-android) as a Bun build target. Spans the build system (.buildkite/Dockerfile, bootstrap.sh, ci.mjs, build.zig, scripts/build/{config,flags,source,zig,codegen,bun,ci,profiles}.ts, dep specs for cares/libarchive/lolhtml/webkit), runtime platform plumbing (env.zig, Global.zig, BunProcess.cpp, compile_target.zig, npm.zig, upgrade_command.zig, crash_handler.zig, analytics), and Android-specific runtime fixes (sys.zig seccomp fallbacks, dns.zig/c_ares.zig getaddrinfo default, root_certs_linux.cpp cacerts dirs, node_os.zig/node_fs.zig/fs.zig/run_command.zig/open.zig/shell tmp/sh/home fallbacks, napi.zig __ndk1 mangling, bun-spawn.cpp pthread_cancel guard). Also bundled: a couple of called-out non-Android fixes (timerfd CLOCK_MONOTONIC on all Linux, c-ares double-ares_init leak removal) and an unrelated-looking NodeVM.cpp jsCast→jsDynamicCast hardening.
Security risks
The TLS system-CA loading change (root_certs_linux.cpp) is the most security-adjacent piece; the c_rehash filename matching is now #ifdef __ANDROID__-gated and the Android dir list (/apex/.../cacerts, /system/etc/security/cacerts, /data/misc/user/0/cacerts-added) is additive-only, so non-Android trust behavior is unchanged. The Android PIE/-fPIC switch, --sysroot/-nostdlibinc flag wiring, and NDK compiler-rt symlinking into host clang's resource dir are build-infra rather than runtime attack surface. I don't see injection/auth/data-exposure risks introduced here.
Level of scrutiny
High. This is a new platform target touching ~53 files across build infra and core runtime, with deliberate behavior changes on shipping Linux (timerfd clock, c-ares init), and the PR description itself says it's not yet verified at runtime (no device/emulator testing, no Android test platforms in CI). The cross-compile toolchain approach (host clang 21 + NDK sysroot, symlinking compiler-rt into clang's resource dir, CMAKE_SYSTEM_NAME=Linux instead of Android for WebKit) is a design choice that deserves a human sign-off.
Other factors
Over the course of this PR I raised 14 inline issues (UBSan predicate regression on macOS, Dockerfile LLVM_VERSION expansion, os.cpus() lazy-proxy interaction, c_rehash double-loading on Debian, --android-ndk CLI registration, JSDoc placement, etc.); every one has been addressed and the last two (bootstrap.ps1 v18 revert, source.ts androidNdkRuntimeDir reuse) were fixed in acc68d7. The current diff looks clean to me — I'm deferring purely on size/scope/design, not on outstanding defects.
Jarred-Sumner
force-pushed
the
claude/android-support
branch
from
acc68d7 to
ccf0b7f
Compare
There was a problem hiding this comment.
All earlier review feedback has been addressed and the bug hunt is now clean — but a 52-file new-platform port with several intentional non-Android behavior changes (timerfd CLOCK_MONOTONIC, c-ares double-init drop, posix _PATH_DEFPATH spawn fallback, the NodeVM dynamicDowncast fix) and self-declared "not yet verified at runtime" status is well outside what I'll auto-approve; ready for a human pass.
Extended reasoning...
Overview
Adds aarch64-linux-android (and x64-android) as a Bun build target. Touches the full stack: build system (scripts/build/**, build.zig, .buildkite/Dockerfile, bootstrap.sh, ci.mjs), vendored-dep configs (cares, libarchive, lolhtml, webkit), usockets eventing/TLS-CA loading, and ~25 runtime source files across Zig/C++/TS for process.platform === "android" plumbing (DNS backend default, temp dir, shell path, os.* fallbacks, npm OS matching, compile-target, crash handler, etc.). 52 files changed.
Security risks
Low but non-zero. root_certs_linux.cpp adds Android system-CA directories and a c_rehash filename matcher (now correctly #ifdef __ANDROID__-gated, so no change to glibc/musl trust-store loading). The NodeVM.cpp hunk replaces an uncheckedDowncast on a user-influenced Promise shape with dynamicDowncast — a hardening fix, not a regression, but it's an unrelated drive-by that isn't called out in the PR description. No auth/crypto/permission logic is weakened.
Level of scrutiny
High. This is a new shipping platform with no runtime test coverage (CI builds only; emulator testing is explicitly deferred). It also deliberately changes behavior on existing Linux builds (timerfd_create → CLOCK_MONOTONIC, dropping the leaked ares_init(), posix BUN_DEFAULT_PATH_FOR_SPAWN fallback in Bun.$). Those are correct fixes and now documented in the description, but they're the kind of cross-cutting changes a maintainer should consciously sign off on rather than have a bot rubber-stamp.
Other factors
The author iterated through ~14 inline findings from earlier bot runs and addressed every one (UBSan-on-macOS regression, Dockerfile clang-${LLVM_VERSION} bug, os.cpus() lazy-proxy mismatch, --android-ndk CLI registration, JSDoc/comment fixes, bootstrap.ps1 revert, source.ts dedup). The current bug-hunt pass found nothing new. Depends on PR #29393 (WebKit upgrade). Given the scope, the explicit "not yet verified at runtime" caveat, and the platform-level design decisions involved, this needs human review.
Jarred-Sumner
force-pushed
the
claude/webkit-upgrade-87fd0daba19a
branch
from
a23edb5 to
eeef3fe
Compare
|
|
||
| auto* entry = loader->registryEntry(key); | ||
| if (!entry || !entry->record()) [[unlikely]] | ||
| return throwVMTypeError(globalObject, scope, makeString("require() failed to evaluate module \""_s, keyString, "\". This is an internal consistentency error."_s)); |
There was a problem hiding this comment.
🟡 Typo: "consistentency" → "consistency" in the functionEsmLoadSync error message. This is a user-visible string (albeit on an [[unlikely]] should-never-happen path) — trivial 1-char fix.
Extended reasoning...
What the bug is. In the new functionEsmLoadSync() (the synchronous require(esm) path added as part of the C++ module-loader migration), the error message at ZigGlobalObject.cpp:793 reads:
return throwVMTypeError(globalObject, scope, makeString("require() failed to evaluate module \""_s, keyString, "\". This is an internal consistentency error."_s));"consistentency" is not a word — it should be "consistency".
The specific code path. functionEsmLoadSync() is the new C++ implementation of the synchronous require(esm) path. After loadModuleSync resolves, it calls loader->registryEntry(key) to fetch the registry entry that the load just populated. If that lookup somehow returns no entry or no record — which should be impossible given the load promise just resolved successfully — it throws this TypeError with the misspelled message.
Why nothing else prevents it. This is a string-literal typo in brand-new code added by this PR (part of the JSC C++ module-loader migration that came in alongside the WebKit upgrade this PR depends on). There's no spell-checking on string literals, and the [[unlikely]] branch means it almost certainly hasn't been exercised at runtime.
Step-by-step proof.
- Open ZigGlobalObject.cpp at line 793 in the PR head.
- The string literal reads
"\". This is an internal consistentency error."_s. - "consistentency" has an extra "ten" — the correct spelling is "consistency".
- This string is passed verbatim to
throwVMTypeError, so if the branch ever fires, users see the typo in their thrown error's.message.
Impact. Zero functional impact — the error semantics are unchanged. The branch is annotated [[unlikely]] and represents an internal invariant violation (registry entry missing immediately after a successful synchronous load), so it should never surface in practice. But it is a user-visible string in new code added by this PR, and it's a trivial fix.
How to fix. Change consistentency → consistency at ZigGlobalObject.cpp:793.
Adds --abi=android with generic crossTarget/sysroot plumbing. Uses host clang 21 with NDK sysroot via --target/--sysroot rather than NDK clang. bun-zig.o + all vendored deps now compile; WebKit local-mode wired for CMake's native Android support. tinycc/FFI-JIT off by default; smoke test skipped (host can't exec target binary). Zig: bionic headers come from NDK via --libc + -Dandroid_ndk_sysroot (Zig bundles glibc/musl headers but not bionic). lchmod stubs to EOPNOTSUPP; glibc-only EAI_* and pwrite64 gated on isGlibc. Deps: cares omits bionic-missing getservby*_r; libarchive picks up contrib/android/include for android_lf.h; cargo (lolhtml) gets CARGO_TARGET_*_LINKER + -L<ndk-clang-lib> for -lunwind.
Full bun-debug now links as ARM aarch64 PIE for /system/bin/linker64. config.ts: linkNdkRuntimesIntoClang() symlinks NDK compiler-rt builtins, libunwind, and ubsan into host clang's resource dir at configure time. clang's driver emits full <resource-dir>/lib/<triple>/libclang_rt.*.a paths with no -L fallback, so the files must exist there. flags.ts: -fPIC/-pie on Android (bionic loader rejects non-PIE), drop -fno-pic/-no-pie which were glibc-only. webkit.ts: local mode on Android uses CMAKE_SYSTEM_NAME=Linux+ANDROID=ON (CMake's Android module would force NDK clang). ICU from BUN_ANDROID_ICU_ROOT — provides libs+includes; NDK sysroot's ICU headers are __INTRODUCED_IN(31)-annotated and unusable at API 28. bun.ts: bionic folds pthread/dl into libc, no -lpthread/-ldl. bindings: TextEncodingRegistry — RELEASE_LOG_ERROR references PAL log channel not built here; redirect to LOG_ERROR. bun-spawn — bionic has no pthread cancellation. napi.zig — NDK libc++ mangles as std::__ndk1.
ASAN/UBSan on Android need wrap.sh + runtime .so deployment alongside the binary — not worth the matrix complexity. Force asan=false in resolveConfig() (not just profile default) and gate UBSan compile/link flags on abi==="gnu". Drops the libclang_rt.ubsan_standalone .so from NEEDED; binary now depends only on libc/libm/liblog/libdl.
…(gated)
bootstrap.sh + .buildkite/Dockerfile: install NDK r27c at /opt/android-ndk,
export ANDROID_NDK_ROOT, add aarch64/x86_64-linux-android rustup targets.
NDK goes on all Linux build images (shared with gnu — getImageKey skips abi
for android).
ci.mjs: linux-{aarch64,x64}-android buildPlatforms entries, commented out
until WebKit -android prebuilts are published. getBuildArgs passes explicit
--os/--arch/--abi for android cpp/link modes since host detection on the
amazonlinux runner would resolve to gnu. getTargetTriplet adds -android.
No test platforms — running android tests needs an emulator.
compile_target.zig: Libc.android — bun build --compile --target=
bun-linux-arm64-android resolves @oven/bun-linux-aarch64-android.
defineValues() emits process.platform="android" for that target.
upgrade_command.zig: -android suffix in release zip filename.
npm.zig: OperatingSystem.current is android (not linux) when
isAndroid, so package.json "os":["android"] matches.
BunProcess.cpp: process.platform returns "android" on __ANDROID__
(Node compat — Termux/device Node reports "android", not "linux").
bun-release/platform.ts: bun-linux-{aarch64,x64}-android entries with
os:"android" so npm's optionalDependency resolution picks them on
device. abi detection keys off process.platform==="android".
…rget, not host Codegen scripts (replacements.ts, bundle-modules.ts, create-hash-table.ts) inline process.platform into the bundled src/js/** modules at build time. Without TARGET_PLATFORM set, a cross-compiled Android binary ships with "linux" baked in, so every process.platform === "android" branch in the builtins is dead-code-eliminated. registerCodegenRules now passes both env vars derived from cfg; bundle-modules.ts eval pass honors them too.
Global.os_name and os_display now check isAndroid so npm_config_user_agent, publish/install user-agent strings, and crash banners say "android" — otherwise native-addon postinstalls fetch glibc binaries. Analytics schema gets a separate .android tag so device crashes are triageable. OperatingSystem enum itself stays .linux on Android (132 switch sites key on it for kernel-level dispatch, which is correct — Android IS Linux at the syscall layer).
…backs epoll_kqueue.c: epoll_pwait2 fallback now also catches -EACCES (Android seccomp denies the syscall with EACCES, not ENOSYS — without this the loop returns -EACCES immediately and busy-spins). timerfd_create now uses CLOCK_MONOTONIC: timers are relative (settime flags=0), so REALTIME let wall-clock steps (NTP/NITZ) skew firing. General fix, surfaced by Android where clock steps are frequent. fs.zig: platformTempDir() → /data/local/tmp on Android (/tmp doesn't exist; transpiler cache, Tmpfile, bundler temp all hit ENOENT). dns.zig: Backend.default → .system on Android. c-ares can't discover nameservers (no /etc/resolv.conf, no JNI for ares_library_init_android), so it silently uses 127.0.0.1 and every lookup times out. bionic getaddrinfo proxies through netd which knows the real resolvers. sys.zig: preadv2/pwritev2 RWF_NOWAIT fallback also latches on EPERM/EACCES (vendor seccomp), not just ENOSYS/EOPNOTSUPP.
Android stores system CAs as individual PEM files named <hash>.0
(OpenSSL c_rehash format) under /system/etc/security/cacerts/ and
/apex/com.android.conscrypt/cacerts/ — no bundle file. The directory
loader's .crt/.pem/.cer extension filter rejected those, so
--use-system-ca and tls.getCACertificates('system') returned []. Now
also accepts ^[0-9a-f]{8}\.[0-9]+$ (helps Debian's /etc/ssl/certs too).
…_ (avoid Debian double-load)
… change → separate PR)
- NodeVM.cpp: jsDynamicCast<JSPromise*> on then() result (was jsCast — type confusion if vm-context Promise[Symbol.species] returns non-Promise) - config.ts: linkNdkRuntimesIntoClang sudo hint emits explicit ln dst names - NodeModuleModule.cpp: drop dead resolverFunctionCallback - codegen.ts: move codegenTarget() above registerCodegenRules JSDoc - subproc.zig: gate Bun.$ PATH fallback on isPosix (not Windows)
…d time The build user can't write to clang's resource dir on CI (read-only /usr). Create the symlinks as root in bootstrap.sh:install_android_ndk() and .buildkite/Dockerfile during image build. linkNdkRuntimesIntoClang in config.ts now warns instead of throwing on EACCES — the actual link step will fail loudly if they're genuinely missing.
…'t find host libstdc++
…LLVM_VERSION ARG not in scope)
…e + review fixes - flags.ts/webkit.ts: -nostdlibinc + explicit NDK include chain (libc++, triple, bionic). -nostdinc++ alone leaks /usr/include/c++/N via apt.llvm.org clang's GCC detection → ldiv_t/cmath breakage. - source.ts: rustup target add before cargo build (idempotent) — image installs target as root, build runs as buildkite-agent. - node_os.zig: os.cpus() returns N stub entries on /proc EACCES (was empty array → TypeError in lazyCpus wrapper). - build.ts: register --android-ndk CLI flag (hint referenced it). - config.ts: JSDoc Throws→Warns. harness.ts: strip stray JSC_useJIT env.
…1GB unused NDK files
…ayouts apt.llvm.org clang uses old-style flat lib/linux/libclang_rt.builtins- <arch>-android.a; tarball clang uses per-triple. Symlink BOTH in Dockerfile/bootstrap.sh/config.ts. Add --rtlib=compiler-rt to android link flags so clang doesn't fall back to -lgcc. Also: webkit.ts comment points to oven-sh/WebKit:Dockerfile.android. (aarch64-android build-cpp + build-zig both passed in #47755 — only the link step left.)
…> for _Unwind_* symbols
…; source.ts use cfg.androidNdkRuntimeDir
Jarred-Sumner
force-pushed
the
claude/android-support
branch
from
e0dd789 to
8c1294c
Compare
WalkthroughThis pull request adds comprehensive Android build and runtime support to the Bun codebase. Changes include Android NDK infrastructure setup, cross-compilation configuration, platform detection logic, and Android-specific code paths in the runtime. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
| var am_args = [_]stringZ{ "/system/bin/am", "start", "-a", "android.intent.action.VIEW", "-d", url }; | ||
| var two_args = [_]stringZ{ opener, url }; | ||
| const args_buf: []const stringZ = if (comptime Environment.isAndroid) &am_args else &two_args; |
There was a problem hiding this comment.
🟡 openURL() was patched for Android but the public opener const at the top of this file still falls through to "xdg-open" (it switches on os.tag, which is .linux on Android). Two callers consume opener directly — bun publish web-login (publish_command.zig:748) and the terminal-editor wrapper at open.zig:239 — so both will silently no-op on Android since xdg-open doesn't exist there. Since am start needs 6 args, the cleanest fix is routing publish_command through Open.openURL() rather than adding an Android arm to the const.
Extended reasoning...
What the bug is. This PR adds Android handling to openURL() (open.zig:15-17) and to src/js/internal/html.ts, both of which now spawn /system/bin/am start -a android.intent.action.VIEW -d <url> instead of xdg-open. But the public opener constant at open.zig:1-5 was left untouched:
pub const opener = switch (@import("builtin").target.os.tag) {
.macos => "/usr/bin/open",
.windows => "start",
else => "xdg-open",
};Android is an ABI, not a separate os.tag — builtin.target.os.tag is .linux on Android — so this falls through to else => "xdg-open", which doesn't exist on Android. The PR's new openURL() body works around this by ignoring opener entirely on Android, but two other call sites still consume the constant directly.
The two affected call sites.
-
bun publishweb login — src/cli/publish_command.zig:748:var child = std.process.Child.init(&.{ Open.opener, auth_url }, bun.default_allocator); _ = child.spawnAndWait() catch return;
On Android this tries to exec
xdg-open <url>, fails with ENOENT, and thecatch returnswallows it. The auth URL is already printed to stdout before this spawn, so the user can still copy-paste — the browser just doesn't auto-open. -
Terminal-editor wrapper — src/open.zig:239: when opening vim/emacs/neovim from the error overlay,
args_buf[0] = openerwraps the editor invocation in the platform opener. On Android this becomesxdg-open vim ..., which fails atspawn() catch returninautoClose().
Step-by-step proof.
- Build for
aarch64-linux-android.builtin.target.os.tag == .linux,builtin.target.abi.isAndroid() == true. openerevaluates the switch on.linux→ no.macos/.windowsmatch →else => "xdg-open".- User runs
bun publishand reaches the interactive web-login prompt; presses Enter. std.process.Child.init(&.{ "xdg-open", auth_url }, ...)→spawnAndWait()→execve("xdg-open", ...)→ENOENT(Android has no/usr/bin, no xdg-utils).catch returnswallows the error; no browser opens. Same flow for the editor-open path at open.zig:333.
Why nothing else prevents it. openURL() was the only consumer that got an Android branch; the const itself is pub and the two other consumers go around openURL(). There's no compile-time check that opener is meaningful on the current target.
Impact (why this is a nit). Android-only, on a brand-new target the PR explicitly says is "not yet verified at runtime". Both failures degrade gracefully (errors swallowed; publish prints the URL for manual copy-paste anyway). bun publish web-login and editor-open-from-error-overlay are niche features on Android. This is new-target incompleteness on code the PR explicitly touched, not a regression on shipping platforms.
How to fix. Adding an Android arm to the opener switch won't work — am start -a android.intent.action.VIEW -d <url> needs 6 argv entries, not the {opener, url} shape both callers use. Better options:
- Have publish_command.zig call
Open.openURL(auth_url)instead of spawningOpen.openerdirectly (it already has the right Android handling and the same fallback-print behavior). - For open.zig:239, either gate the
opener-wrapping on!Environment.isAndroid(just exec the editor binary directly), or accept it as known-unsupported on Android.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/deps/c_ares.zig (1)
592-609:⚠️ Potential issue | 🟠 Major
ares_library_cleanup()now poisons later channel initialization on Android.With
ARES_FLAG_NO_DFLT_SVR,ares_init_options()is expected to returnENOSERVERuntil the caller seeds resolvers or falls back to the system backend. This error path still callsares_library_cleanup()but never clearsares_has_loaded, so the first expected failure leaves the global one-time init flag stuck attruewhile the library is actually cleaned up. Any laterChannel.init()in the same process can then skiplibraryInit()and fail withENOTINITIALIZED.Also applies to: 1910-1910
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/deps/c_ares.zig` around lines 592 - 609, The error path after Error.get(ares_init_options(&channel, &opts, optmask)) calls ares_library_cleanup() but fails to reset the global one-time init flag, so on Android the ENOSERVERS case leaves ares_has_loaded (the global library init marker used by Channel.init/libraryInit) true while the library is cleaned up; update the error handler to also clear/reset ares_has_loaded (or the equivalent one-time init state) before returning the error so subsequent Channel.init calls will attempt libraryInit again..buildkite/Dockerfile (1)
222-227:⚠️ Potential issue | 🟠 MajorMirror the NDK/sysroot setup into
bun-build-linux-local.This stage now installs the Android Rust stdlibs, but it still lacks
/opt/android-ndk,ANDROID_NDK_ROOT, and the clang resource-dir symlinks added in thebuildkitestage. Any Android build run insidebun-build-linux-localwill still fail at configure/link time even though the CI stage succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.buildkite/Dockerfile around lines 222 - 227, The bun-build-linux-local stage currently installs Rust nightly and Android targets but omits the Android NDK/sysroot setup present in the buildkite stage; update the bun-build-linux-local stage to mirror that setup by installing or copying /opt/android-ndk, setting ANDROID_NDK_ROOT (and any related env vars), and creating the same clang resource-dir symlinks so the Rust targets (aarch64-linux-android, x86_64-linux-android) can configure/link correctly; locate the bun-build-linux-local stage in the Dockerfile and add the same NDK installation/copy steps and symlink creation used in the buildkite stage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.buildkite/ci.mjs:
- Around line 132-135: The new android platform entries (objects with abi:
"android") are being picked up by the verify-baseline step because the
verification logic treats all linux aarch64 builds as QEMU-verifiable; update
the verify-baseline flow (the verify-baseline job/verifyBaseline function) to
explicitly skip platforms where platform.abi === "android" (or platform.os ===
"linux" && platform.abi === "android") so Android builds are not enrolled in
baseline verification, or alternatively add and honor a skip flag (e.g.,
platform.skipVerifyBaseline = true) on the Android platform objects and make
verify-baseline check that flag before queuing QEMU verification.
In `@packages/bun-usockets/src/crypto/root_certs_linux.cpp`:
- Around line 132-137: The hardcoded user path in dir_paths (the static const
char* dir_paths[] array in root_certs_linux.cpp) only covers UID 0 and misses
secondary-user stores; either document this limitation in a comment near
dir_paths or replace the static entry with runtime resolution: detect the
active/current user ID (or glob /data/misc/user/*/) and check
/data/misc/user/<uid>/cacerts-added (or enumerate matches) before loading certs,
updating the logic that iterates dir_paths to consult the resolved paths instead
of the literal "/data/misc/user/0/cacerts-added".
In `@packages/bun-usockets/src/internal/internal.h`:
- Line 65: The declaration for Bun__panic has a malformed attribute token;
update the attribute syntax on the Bun__panic prototype to use the correct
GCC/Clang form by changing the single-underscore token to a double-underscore
token so the attribute reads __attribute__((__noreturn__)); leave the function
name Bun__panic and its parameters unchanged.
In `@scripts/bootstrap.sh`:
- Around line 1276-1280: The current check returns early if
ndk_home="/opt/android-ndk" exists, skipping export of ANDROID_NDK_ROOT and
recreation of clang resource-dir symlinks; change this so presence of $ndk_home
prevents re-downloading/installation but does not "return" out of the NDK setup
block—keep exporting ANDROID_NDK_ROOT and always run the symlink recreation
logic (the clang resource-dir symlink steps) even when ndk_home already exists;
reference the ndk_version, ndk_home, ANDROID_NDK_ROOT variables and the clang
resource-dir symlink code to locate and adjust the flow.
In `@scripts/build/bun.ts`:
- Around line 59-62: The Android link step misses linking libdl; update the
branch that checks cfg.abi === "android" to include "-ldl" in the libs array so
dlopen/dlsym symbols resolve (i.e., add "-ldl" alongside the existing libs.push
call that currently adds "-lc", "-lm", "-llog" for the android ABI). Locate the
conditional that inspects cfg.abi and the libs.push invocation and append "-ldl"
to the list.
In `@scripts/build/codegen.ts`:
- Around line 96-106: The codegenTarget function introduces inline platform/arch
logic; instead, add derived fields to Config (e.g., targetPlatform and
targetArch) and consume those here: replace the conditional logic in
codegenTarget with returns that read cfg.targetPlatform and cfg.targetArch (or
remove codegenTarget and reference the new fields directly), update any call
sites that use codegenTarget to use the Config fields, and remove duplicated
platform-derivation branches so all target platform/arch logic is centralized in
the Config construction code.
In `@scripts/build/config.ts`:
- Around line 636-642: The code computing ndkClangLib, ndkClangVer and
androidNdkRuntimeDir is duplicated; extract a small helper function like
ndkClangVersion(ndkPrebuilt) that encapsulates reading join(ndkPrebuilt, "lib",
"clang"), calling readdirSync(...)[0], and throwing the same BuildError when
undefined, then replace the inline logic in the current block and in
linkNdkRuntimesIntoClang() to call ndkClangVersion(ndkPrebuilt) and compute
androidNdkRuntimeDir = join(ndkClangLib, ndkClangVersion, "lib", "linux") using
the returned version.
In `@scripts/build/zig.ts`:
- Around line 110-127: The generated android-libc.txt created by androidLibcArgs
is not declared as an explicit input to the Zig build steps, so changes to its
contents won't invalidate targets like bun-zig.o; update the build graph to list
the libcFile produced by androidLibcArgs as an input/dependency for the zig
build/check actions (e.g., the targets/functions named zig_build and zig_check
and the output bun-zig.o) so ninja sees changes to android-libc.txt and reruns
Zig when its contents change.
In `@src/bun.js/bindings/NodeVM.cpp`:
- Around line 347-355: The code currently wraps a non-JSPromise thenable with
JSPromise::resolvedPromise(globalObject, thenResult), which resolves to the
thenable object instead of chaining its settlement; instead detect non-JSPromise
thenResult from promise->then(...) and chain it into a new JSPromiseDeferred:
create a JSPromiseDeferred (or use performPromiseThenFunction with
globalObject->thenable()), attach fulfillment and rejection handlers that
resolve/reject the deferred when the thenable settles, and return the deferred's
promise (replacing the JSPromise::resolvedPromise call); reference thenResult,
promise->then, JSPromise::resolvedPromise, performPromiseThenFunction,
globalObject->thenable(), and JSPromiseDeferred to locate and implement the
change.
In `@src/install/npm.zig`:
- Around line 645-646: OperatingSystem.current was changed to return .android
when Environment.isAndroid is set, but OperatingSystem.current_name still
returns "linux"; update current_name to be derived from the same logic as
current (either by switching on Environment.os and Environment.isAndroid or by
switching on OperatingSystem.current) so the string name matches the enum value
(e.g., return "android" when current is .android and "linux" when current is
.linux); update the implementation for OperatingSystem.current_name to use the
same unique symbols Environment.os, Environment.isAndroid or
OperatingSystem.current to ensure consistency.
In `@src/install/PackageManager.zig`:
- Around line 498-503: The wrapper forwards arguments with unquoted $@ which
lets paths with spaces or globs split; update the generated script in
PackageManager.zig so the forwarded args are quoted as "$@" (i.e., ensure the
Zig string that contains the node-gyp forwarding uses a quoted "$@" token in
both the Android and non-Android branches) so the runtime script preserves each
original argument as a single word when calling node-gyp.
In `@src/shell/subproc.zig`:
- Around line 701-706: The code is replacing an explicitly empty PATH with
BUN_DEFAULT_PATH_FOR_SPAWN on POSIX, which changes caller behavior; instead,
treat an empty string from event_loop.env().get("PATH") as a valid explicit
value and only substitute the default when PATH is absent (i.e., get() returns
none) and bun.Environment.isPosix is true. Update the PATH assignment logic
around event_loop.env().get("PATH")/bun.Environment.isPosix/
BUN_DEFAULT_PATH_FOR_SPAWN so that the branch returns the fetched p whenever
get() yields a value (even if p.len == 0), and only uses
BUN_DEFAULT_PATH_FOR_SPAWN when get() yields no value and
bun.Environment.isPosix is true.
---
Outside diff comments:
In @.buildkite/Dockerfile:
- Around line 222-227: The bun-build-linux-local stage currently installs Rust
nightly and Android targets but omits the Android NDK/sysroot setup present in
the buildkite stage; update the bun-build-linux-local stage to mirror that setup
by installing or copying /opt/android-ndk, setting ANDROID_NDK_ROOT (and any
related env vars), and creating the same clang resource-dir symlinks so the Rust
targets (aarch64-linux-android, x86_64-linux-android) can configure/link
correctly; locate the bun-build-linux-local stage in the Dockerfile and add the
same NDK installation/copy steps and symlink creation used in the buildkite
stage.
In `@src/deps/c_ares.zig`:
- Around line 592-609: The error path after
Error.get(ares_init_options(&channel, &opts, optmask)) calls
ares_library_cleanup() but fails to reset the global one-time init flag, so on
Android the ENOSERVERS case leaves ares_has_loaded (the global library init
marker used by Channel.init/libraryInit) true while the library is cleaned up;
update the error handler to also clear/reset ares_has_loaded (or the equivalent
one-time init state) before returning the error so subsequent Channel.init calls
will attempt libraryInit again.
🪄 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: c7819520-f6e3-4605-9eae-48120a0eed57
📒 Files selected for processing (52)
.buildkite/Dockerfile.buildkite/ci.mjsbuild.zigpackages/bun-release/src/platform.tspackages/bun-usockets/src/crypto/root_certs_linux.cpppackages/bun-usockets/src/eventing/epoll_kqueue.cpackages/bun-usockets/src/internal/internal.hpackages/bun-usockets/src/loop.cscripts/bootstrap.shscripts/build.tsscripts/build/bun.tsscripts/build/ci.tsscripts/build/codegen.tsscripts/build/config.tsscripts/build/deps/cares.tsscripts/build/deps/libarchive.tsscripts/build/deps/lolhtml.tsscripts/build/deps/webkit.tsscripts/build/flags.tsscripts/build/profiles.tsscripts/build/source.tsscripts/build/zig.tssrc/Global.zigsrc/analytics.zigsrc/analytics/schema.zigsrc/bun.js/RuntimeTranspilerStore.zigsrc/bun.js/bindings/BunProcess.cppsrc/bun.js/bindings/NodeVM.cppsrc/bun.js/bindings/TextEncodingRegistry.cppsrc/bun.js/bindings/bun-spawn.cppsrc/bun.js/modules/NodeModuleModule.cppsrc/bun.js/node/node_fs.zigsrc/bun.js/node/node_os.zigsrc/cli/run_command.zigsrc/cli/upgrade_command.zigsrc/codegen/bundle-modules.tssrc/compile_target.zigsrc/crash_handler.zigsrc/deps/c_ares.zigsrc/dns.zigsrc/env.zigsrc/fs.zigsrc/install/PackageManager.zigsrc/install/npm.zigsrc/js/internal/html.tssrc/js/node/net.tssrc/js/node/os.tssrc/napi/napi.zigsrc/open.zigsrc/shell/interpreter.zigsrc/shell/subproc.zigsrc/sys.zig
💤 Files with no reviewable changes (2)
- src/bun.js/modules/NodeModuleModule.cpp
- packages/bun-usockets/src/loop.c
| // Android: cross-compiled from glibc amazonlinux via NDK sysroot. Host arch | ||
| // matches target arch so only --abi/--target/--sysroot are cross. | ||
| { os: "linux", arch: "aarch64", abi: "android", distro: "amazonlinux", release: "2023", features: ["docker"] }, | ||
| { os: "linux", arch: "x64", abi: "android", distro: "amazonlinux", release: "2023", features: ["docker"] }, |
There was a problem hiding this comment.
Exclude Android builds from the existing baseline-verification flow.
Adding linux-aarch64-android here also enrolls it in verify-baseline, because that path currently treats every Linux aarch64 build as QEMU-verifiable. That step runs qemu-aarch64-static against a bionic-linked binary without an Android userspace, which does not match the PR's “no emulator-based tests” plan and will fail as soon as this target starts building.
Suggested follow-up outside this hunk
function needsBaselineVerification(platform) {
- const { os, arch, baseline } = platform;
- if (os === "linux") return (arch === "x64" && baseline) || arch === "aarch64";
+ const { os, arch, abi, baseline } = platform;
+ if (os === "linux") return abi !== "android" && ((arch === "x64" && baseline) || arch === "aarch64");
if (os === "windows") return arch === "x64" && baseline;
return false;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.buildkite/ci.mjs around lines 132 - 135, The new android platform entries
(objects with abi: "android") are being picked up by the verify-baseline step
because the verification logic treats all linux aarch64 builds as
QEMU-verifiable; update the verify-baseline flow (the verify-baseline
job/verifyBaseline function) to explicitly skip platforms where platform.abi ===
"android" (or platform.os === "linux" && platform.abi === "android") so Android
builds are not enrolled in baseline verification, or alternatively add and honor
a skip flag (e.g., platform.skipVerifyBaseline = true) on the Android platform
objects and make verify-baseline check that flag before queuing QEMU
verification.
| static const char* dir_paths[] = { | ||
| "/apex/com.android.conscrypt/cacerts", // API 30+ (mainline updatable) | ||
| "/system/etc/security/cacerts", // base system store | ||
| "/data/misc/user/0/cacerts-added", // user-installed | ||
| NULL | ||
| }; |
There was a problem hiding this comment.
User-installed certificates path hardcodes user ID 0.
The path /data/misc/user/0/cacerts-added only loads user-installed CAs for the primary user. On multi-user Android devices, secondary users (uid 10+) have separate cert stores at /data/misc/user/<uid>/cacerts-added that won't be picked up.
This is acceptable for initial Android support since most devices are single-user, but worth documenting as a known limitation.
🔧 Optional: Dynamic user ID resolution
- "/data/misc/user/0/cacerts-added", // user-installed
+ // Note: hardcoded to user 0; multi-user devices would need
+ // runtime resolution via getuid() or directory enumeration
+ "/data/misc/user/0/cacerts-added", // user-installed (primary user only)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bun-usockets/src/crypto/root_certs_linux.cpp` around lines 132 -
137, The hardcoded user path in dir_paths (the static const char* dir_paths[]
array in root_certs_linux.cpp) only covers UID 0 and misses secondary-user
stores; either document this limitation in a comment near dir_paths or replace
the static entry with runtime resolution: detect the active/current user ID (or
glob /data/misc/user/*/) and check /data/misc/user/<uid>/cacerts-added (or
enumerate matches) before loading certs, updating the logic that iterates
dir_paths to consult the resolved paths instead of the literal
"/data/misc/user/0/cacerts-added".
| #define UNLIKELY(cond) __builtin_expect((_Bool)(cond), 0) | ||
| #endif | ||
|
|
||
| extern void __attribute((__noreturn__)) Bun__panic(const char *message, size_t length); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if __attribute (single underscore) is defined as a macro or used elsewhere in the codebase
# Check for a macro definition of __attribute
rg -n '#define\s+__attribute\b' --type=c --type=cpp
# Check usage patterns of __attribute vs __attribute__ in the repo
echo "=== Usage of __attribute (single underscore) ==="
rg -c '\b__attribute\s*\(' --type=c --type=cpp | head -20
echo "=== Usage of __attribute__ (double underscore) ==="
rg -c '\b__attribute__\s*\(' --type=c --type=cpp | head -20Repository: oven-sh/bun
Length of output: 1027
🏁 Script executed:
# First, read the actual file content around line 65
cat -n packages/bun-usockets/src/internal/internal.h | sed -n '60,70p'Repository: oven-sh/bun
Length of output: 551
🏁 Script executed:
# Check if there's any compatibility header or macro definition in bun-usockets
rg -n 'define.*__attribute' packages/bun-usockets/ -t c -t cpp -t h
# Also check the main libusockets.h file to see the pattern
echo "=== Checking libusockets.h ==="
head -20 packages/bun-usockets/src/libusockets.hRepository: oven-sh/bun
Length of output: 894
🏁 Script executed:
# Check for any include guards or compatibility definitions that might explain single underscore
rg -B2 -A2 '__attribute\s*\(' packages/bun-usockets/src/internal/internal.hRepository: oven-sh/bun
Length of output: 220
Fix the __attribute syntax error.
Line 65 uses __attribute((__noreturn__)) with a single underscore, but the correct GCC/Clang syntax requires double underscores: __attribute__((__noreturn__)). This will cause a compilation error. Change __attribute( to __attribute__(.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bun-usockets/src/internal/internal.h` at line 65, The declaration
for Bun__panic has a malformed attribute token; update the attribute syntax on
the Bun__panic prototype to use the correct GCC/Clang form by changing the
single-underscore token to a double-underscore token so the attribute reads
__attribute__((__noreturn__)); leave the function name Bun__panic and its
parameters unchanged.
| ndk_version="$(android_ndk_version)" | ||
| ndk_home="/opt/android-ndk" | ||
| if [ -d "$ndk_home" ]; then | ||
| return | ||
| fi |
There was a problem hiding this comment.
Don't return before reapplying the NDK environment/symlink setup.
If /opt/android-ndk already exists, this skips exporting ANDROID_NDK_ROOT and recreating the clang resource-dir symlinks. A rerun after an LLVM upgrade or on a partially prepared machine will keep a stale resource dir, and Android links will fail until the NDK block is forced to run again.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/bootstrap.sh` around lines 1276 - 1280, The current check returns
early if ndk_home="/opt/android-ndk" exists, skipping export of ANDROID_NDK_ROOT
and recreation of clang resource-dir symlinks; change this so presence of
$ndk_home prevents re-downloading/installation but does not "return" out of the
NDK setup block—keep exporting ANDROID_NDK_ROOT and always run the symlink
recreation logic (the clang resource-dir symlink steps) even when ndk_home
already exists; reference the ndk_version, ndk_home, ANDROID_NDK_ROOT variables
and the clang resource-dir symlink code to locate and adjust the flow.
| if (cfg.abi === "android") { | ||
| // bionic: pthread/dl/rt are folded into libc; no separate libatomic | ||
| // (compiler-rt builtins). -llog for __android_log_*. | ||
| libs.push("-lc", "-lm", "-llog"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file and surrounding context around lines 59-62
cd /repo && head -80 scripts/build/bun.ts | tail -30Repository: oven-sh/bun
Length of output: 111
🏁 Script executed:
# Look for Android-related build logic and any existing -ldl references
rg -i "libdl|dlopen|dlfcn" scripts/build/ -A 2 -B 2Repository: oven-sh/bun
Length of output: 3006
🏁 Script executed:
# Check the full systemLibs function and Android handling
rg -A 20 "if \(cfg\.abi === \"android\"\)" scripts/build/bun.tsRepository: oven-sh/bun
Length of output: 1101
🏁 Script executed:
# Search for any comments or documentation about Android library selection
rg "android|bionic" scripts/build/ -i -B 2 -A 2Repository: oven-sh/bun
Length of output: 29467
🏁 Script executed:
# Search for dlopen/dlsym usage in the codebase
rg "dlopen|dlsym|dlfcn" scripts/ src/ -t ts -t cpp -t c -B 2 -A 2 | head -100Repository: oven-sh/bun
Length of output: 5958
🏁 Script executed:
# Look for any PR description, CHANGELOG, or documentation about Android library needs
rg -i "needed|libdl|android" --glob="*.md" --glob="CHANGELOG*" --glob="*changes*"Repository: oven-sh/bun
Length of output: 4790
🏁 Script executed:
# Check if there are any test files or Android-specific documentation
fd -t f -i "android" . | grep -E "\.(md|txt|ts|test)" | head -20Repository: oven-sh/bun
Length of output: 37
🌐 Web query:
Android bionic dlopen dlsym libc or separate libdl symbol location
💡 Result:
In Android's Bionic libc, dlopen and dlsym are provided by the dynamic linker (/system/bin/linker or linker64), not directly from libc.so. There is no separate libdl.so file on the device; instead, the linker creates a fake in-memory "libdl.so" (via get_libdl_info) with a synthetic soinfo struct containing the real symbol table pointing to the linker's implementations of dlopen, dlsym, etc. This fake libdl.so is the first entry in the global solist, so when binaries list libdl.so in DT_NEEDED, the linker finds this pre-loaded fake and resolves symbols to its own code. libdl/ in Bionic source contains weak stubs that proxy to the linker's loader_* functions (e.g., libdl.cpp proxies dlopen to loader_dlopen). When linking native code, -ldl pulls these stubs from libdl.so, but at runtime they use the linker's real implementations via the fake soinfo mechanism. This design avoids a real libdl.so file while providing the POSIX dlfcn.h interface. Symbols are thus located in the linker's address space, accessible via dlsym(RTLD_DEFAULT, "dlopen") or similar.
Citations:
- 1: https://android.googlesource.com/platform/bionic/+/android-4.2_r1/linker/dlfcn.c
- 2: https://android.googlesource.com/platform/bionic/+/master/libdl/libdl.cpp
- 3: https://stackoverflow.com/questions/60916039/how-does-libdl-work-with-linker-in-android
- 4: https://android.googlesource.com/platform/bionic/+/b648a8a/linker/dlfcn.cpp
- 5: https://android.googlesource.com/platform/bionic.git/+/refs/heads/main/linker/dlfcn.cpp
- 6: https://android.googlesource.com/platform/bionic/+/refs/heads/main/libdl/
- 7: https://android.googlesource.com/platform/bionic/%2B/refs/heads/main/linker/linker.h
- 8: https://android.googlesource.com/platform/bionic/+/e0848bbf/
Add -ldl to Android system library linking.
Android's Bionic linker implements dlopen/dlsym via a synthetic libdl.so; proper symbol resolution requires -ldl at link time even though there is no real libdl.so on the device. Without it, the build relies on some other dependency pulling libdl transitively, which is fragile. The FFI module and native addon loading (process.dlopen) both depend on this.
Suggested fix
- libs.push("-lc", "-lm", "-llog");
+ libs.push("-lc", "-lm", "-llog", "-ldl");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (cfg.abi === "android") { | |
| // bionic: pthread/dl/rt are folded into libc; no separate libatomic | |
| // (compiler-rt builtins). -llog for __android_log_*. | |
| libs.push("-lc", "-lm", "-llog"); | |
| if (cfg.abi === "android") { | |
| // bionic: pthread/dl/rt are folded into libc; no separate libatomic | |
| // (compiler-rt builtins). -llog for __android_log_*. | |
| libs.push("-lc", "-lm", "-llog", "-ldl"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/build/bun.ts` around lines 59 - 62, The Android link step misses
linking libdl; update the branch that checks cfg.abi === "android" to include
"-ldl" in the libs array so dlopen/dlsym symbols resolve (i.e., add "-ldl"
alongside the existing libs.push call that currently adds "-lc", "-lm", "-llog"
for the android ABI). Locate the conditional that inspects cfg.abi and the
libs.push invocation and append "-ldl" to the list.
| JSObject* thenResult = promise->then(globalObject, transformer, globalObject->promiseEmptyOnRejectedFunction()); | ||
| RETURN_IF_EXCEPTION(scope, nullptr); | ||
|
|
||
| RELEASE_AND_RETURN(scope, uncheckedDowncast<JSPromise>(thenResult)); | ||
| // JSPromise::then() may return a non-JSPromise when Promise[Symbol.species] | ||
| // is overridden in the vm context — don't uncheckedDowncast (release-mode | ||
| // static_cast) on a user-influenced shape. | ||
| if (auto* thenPromise = dynamicDowncast<JSPromise>(thenResult)) | ||
| RELEASE_AND_RETURN(scope, thenPromise); | ||
| RELEASE_AND_RETURN(scope, JSPromise::resolvedPromise(globalObject, thenResult)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== Call site under review ==\n'
sed -n '321,356p' src/bun.js/bindings/NodeVM.cpp
printf '\n== JSPromise APIs ==\n'
fd -i 'JSPromise.*' . | xargs -r rg -n -C4 'resolvedPromise(Value)?\s*\(|\bthen\s*\('
printf '\n== Promise internals/helpers that may preserve native promise semantics ==\n'
fd -i '.*Promise.*\.(h|cpp)$' . | xargs -r rg -n -C3 'performPromiseThen|NewPromiseCapability|SpeciesConstructor|promiseEmptyOnRejectedFunction'Repository: oven-sh/bun
Length of output: 5735
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Looking for JSPromise C++ implementation files =="
find . -name "*.h" -o -name "*.cpp" | xargs grep -l "class JSPromise" | head -5
echo ""
echo "== Searching for resolvedPromise implementation in JSPromise headers =="
find . -name "JSPromise.h" -o -name "*Promise*.h" | head -10 | xargs -r cat | head -200
echo ""
echo "== Looking for thenable/promise-like value handling near then() =="
rg -A 10 'Promise\[Symbol\.species\]|SpeciesConstructor' --max-count 5Repository: oven-sh/bun
Length of output: 8017
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for JSPromise in all header files =="
rg -l "class JSPromise|struct JSPromise" --type h | head -10
echo ""
echo "== Search for resolvedPromise definitions =="
rg -B 2 -A 8 "resolvedPromise.*JSValue.*JSPromise" --type h --type cpp | head -60
echo ""
echo "== Check how Promise.then handles species-constructed objects =="
rg -B 3 -A 10 "Symbol\.species|SpeciesConstructor" --type cpp | head -80Repository: oven-sh/bun
Length of output: 251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full context of NodeVM.cpp around the changed code =="
sed -n '300,365p' src/bun.js/bindings/NodeVM.cpp
echo ""
echo "== Other uses of resolvedPromise in NodeVM.cpp =="
rg -n "resolvedPromise|JSPromise::then" src/bun.js/bindings/NodeVM.cpp
echo ""
echo "== Similar promise handling patterns in other binding files =="
rg -B 5 -A 5 "JSPromise::then.*transformer|promise->then" src/bun.js/bindings/*.cpp | head -100
echo ""
echo "== Check if there are any thenable/thenableize patterns =="
rg -n "thenable|thenableize|performPromiseThen" src/bun.js/bindings/*.cpp | head -20Repository: oven-sh/bun
Length of output: 6754
Chain the non-JSPromise return value instead of wrapping it.
When Promise[Symbol.species] is overridden, promise->then(...) returns a non-JSPromise thenable object. Wrapping it directly with JSPromise::resolvedPromise(globalObject, thenResult) creates a promise that resolves to that thenable object rather than waiting for it to settle. This breaks the promise chain and can cause importModuleInner() to fulfill with the wrong value or drop rejections.
Use the established pattern elsewhere in the codebase (see bindings.cpp and ModuleLoader.cpp): attach a reaction handler via performPromiseThenFunction with globalObject->thenable() wrappers, or create a JSPromiseDeferred and chain the thenable's settlement to it via explicit .then() callbacks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/bindings/NodeVM.cpp` around lines 347 - 355, The code currently
wraps a non-JSPromise thenable with JSPromise::resolvedPromise(globalObject,
thenResult), which resolves to the thenable object instead of chaining its
settlement; instead detect non-JSPromise thenResult from promise->then(...) and
chain it into a new JSPromiseDeferred: create a JSPromiseDeferred (or use
performPromiseThenFunction with globalObject->thenable()), attach fulfillment
and rejection handlers that resolve/reject the deferred when the thenable
settles, and return the deferred's promise (replacing the
JSPromise::resolvedPromise call); reference thenResult, promise->then,
JSPromise::resolvedPromise, performPromiseThenFunction,
globalObject->thenable(), and JSPromiseDeferred to locate and implement the
change.
| const file = std.fs.cwd().openFile("/proc/stat", .{}) catch { | ||
| // hidepid mounts (common on Android) deny /proc/stat. lazyCpus in os.ts | ||
| // pre-creates hostCpuCount lazy proxies, so return that many stub | ||
| // entries (zeroed times / unknown model / speed 0) — matches Node. | ||
| const count: u32 = @intCast(@max(1, bun_sysconf__SC_NPROCESSORS_ONLN())); | ||
| const stubs = try jsc.JSValue.createEmptyArray(globalThis, count); | ||
| var i: u32 = 0; | ||
| while (i < count) : (i += 1) { | ||
| const cpu = jsc.JSValue.createEmptyObject(globalThis, 3); | ||
| cpu.put(globalThis, jsc.ZigString.static("times"), (CPUTimes{}).toValue(globalThis)); | ||
| cpu.put(globalThis, jsc.ZigString.static("model"), jsc.ZigString.static("unknown").withEncoding().toJS(globalThis)); | ||
| cpu.put(globalThis, jsc.ZigString.static("speed"), jsc.JSValue.jsNumber(0)); | ||
| try stubs.putIndex(globalThis, i, cpu); | ||
| } | ||
| return stubs; | ||
| }; |
There was a problem hiding this comment.
Don’t convert every /proc/stat failure into fake CPU data.
This catch now returns stub CPUs for all open failures on every Linux target, not just the Android permission-denied cases mentioned in the comment. That masks real failures such as fd exhaustion or I/O errors and changes os.cpus() from “throw” to silently returning bogus data. Please gate the stub fallback to the Android access-denied path and rethrow everything else.
| pub const current: OperatingSystem = switch (Environment.os) { | ||
| .linux => @enumFromInt(linux), | ||
| .linux => @enumFromInt(if (Environment.isAndroid) android else linux), |
There was a problem hiding this comment.
Keep OperatingSystem.current_name aligned with the new Android mapping.
Line 646 now makes OperatingSystem.current evaluate to .android, but OperatingSystem.current_name below still resolves to "linux" for the same build. That leaves the enum and string representations of the active OS inconsistent.
Suggested fix
pub const current_name = switch (Environment.os) {
- .linux => "linux",
+ .linux => if (Environment.isAndroid) "android" else "linux",
.mac => "darwin",
.windows => "win32",
.wasm => `@compileError`("Unsupported operating system: " ++ `@tagName`(current)),
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub const current: OperatingSystem = switch (Environment.os) { | |
| .linux => @enumFromInt(linux), | |
| .linux => @enumFromInt(if (Environment.isAndroid) android else linux), | |
| pub const current_name = switch (Environment.os) { | |
| .linux => if (Environment.isAndroid) "android" else "linux", | |
| .mac => "darwin", | |
| .windows => "win32", | |
| .wasm => `@compileError`("Unsupported operating system: " ++ `@tagName`(current)), | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/install/npm.zig` around lines 645 - 646, OperatingSystem.current was
changed to return .android when Environment.isAndroid is set, but
OperatingSystem.current_name still returns "linux"; update current_name to be
derived from the same logic as current (either by switching on Environment.os
and Environment.isAndroid or by switching on OperatingSystem.current) so the
string name matches the enum value (e.g., return "android" when current is
.android and "linux" when current is .linux); update the implementation for
OperatingSystem.current_name to use the same unique symbols Environment.os,
Environment.isAndroid or OperatingSystem.current to ensure consistency.
| else => (if (Environment.isAndroid) "#!/system/bin/sh\n" else "#!/bin/sh\n") ++ | ||
| \\if [ "x$npm_config_node_gyp" = "x" ]; then | ||
| \\ bun x --silent node-gyp $@ | ||
| \\else | ||
| \\ "$npm_config_node_gyp" $@ | ||
| \\fi |
There was a problem hiding this comment.
Quote "$@" when forwarding node-gyp arguments.
The wrapper currently re-expands every argument with $@, so paths containing spaces or glob characters will be split incorrectly before node-gyp sees them.
Suggested fix
else => (if (Environment.isAndroid) "#!/system/bin/sh\n" else "#!/bin/sh\n") ++
\\if [ "x$npm_config_node_gyp" = "x" ]; then
- \\ bun x --silent node-gyp $@
+ \\ bun x --silent node-gyp "$@"
\\else
- \\ "$npm_config_node_gyp" $@
+ \\ "$npm_config_node_gyp" "$@"
\\fi
\\📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else => (if (Environment.isAndroid) "#!/system/bin/sh\n" else "#!/bin/sh\n") ++ | |
| \\if [ "x$npm_config_node_gyp" = "x" ]; then | |
| \\ bun x --silent node-gyp $@ | |
| \\else | |
| \\ "$npm_config_node_gyp" $@ | |
| \\fi | |
| else => (if (Environment.isAndroid) "#!/system/bin/sh\n" else "#!/bin/sh\n") ++ | |
| \\if [ "x$npm_config_node_gyp" = "x" ]; then | |
| \\ bun x --silent node-gyp "$@" | |
| \\else | |
| \\ "$npm_config_node_gyp" "$@" | |
| \\fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/install/PackageManager.zig` around lines 498 - 503, The wrapper forwards
arguments with unquoted $@ which lets paths with spaces or globs split; update
the generated script in PackageManager.zig so the forwarded args are quoted as
"$@" (i.e., ensure the Zig string that contains the node-gyp forwarding uses a
quoted "$@" token in both the Android and non-Android branches) so the runtime
script preserves each original argument as a single word when calling node-gyp.
| .PATH = if (event_loop.env().get("PATH")) |p| | ||
| if (p.len > 0 or !bun.Environment.isPosix) p else bun.sliceTo(BUN_DEFAULT_PATH_FOR_SPAWN, 0) | ||
| else if (bun.Environment.isPosix) | ||
| bun.sliceTo(BUN_DEFAULT_PATH_FOR_SPAWN, 0) | ||
| else | ||
| "", |
There was a problem hiding this comment.
Preserve explicit empty PATH on POSIX instead of forcing the default path.
When inherited PATH is explicitly set to "", this now substitutes BUN_DEFAULT_PATH_FOR_SPAWN, which re-enables argv[0] path lookup unexpectedly. That can change command-resolution/security behavior for callers intentionally using an empty path.
💡 Suggested fix
- .PATH = if (event_loop.env().get("PATH")) |p|
- if (p.len > 0 or !bun.Environment.isPosix) p else bun.sliceTo(BUN_DEFAULT_PATH_FOR_SPAWN, 0)
- else if (bun.Environment.isPosix)
- bun.sliceTo(BUN_DEFAULT_PATH_FOR_SPAWN, 0)
- else
- "",
+ .PATH = if (event_loop.env().get("PATH")) |p|
+ p
+ else if (bun.Environment.isPosix)
+ bun.sliceTo(BUN_DEFAULT_PATH_FOR_SPAWN, 0)
+ else
+ "",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .PATH = if (event_loop.env().get("PATH")) |p| | |
| if (p.len > 0 or !bun.Environment.isPosix) p else bun.sliceTo(BUN_DEFAULT_PATH_FOR_SPAWN, 0) | |
| else if (bun.Environment.isPosix) | |
| bun.sliceTo(BUN_DEFAULT_PATH_FOR_SPAWN, 0) | |
| else | |
| "", | |
| .PATH = if (event_loop.env().get("PATH")) |p| | |
| p | |
| else if (bun.Environment.isPosix) | |
| bun.sliceTo(BUN_DEFAULT_PATH_FOR_SPAWN, 0) | |
| else | |
| "", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shell/subproc.zig` around lines 701 - 706, The code is replacing an
explicitly empty PATH with BUN_DEFAULT_PATH_FOR_SPAWN on POSIX, which changes
caller behavior; instead, treat an empty string from
event_loop.env().get("PATH") as a valid explicit value and only substitute the
default when PATH is absent (i.e., get() returns none) and
bun.Environment.isPosix is true. Update the PATH assignment logic around
event_loop.env().get("PATH")/bun.Environment.isPosix/ BUN_DEFAULT_PATH_FOR_SPAWN
so that the branch returns the fetched p whenever get() yields a value (even if
p.len == 0), and only uses BUN_DEFAULT_PATH_FOR_SPAWN when get() yields no value
and bun.Environment.isPosix is true.
…#29676) Adds FreeBSD as a cross-compile target, following the same model as oven-sh#29675 (Android): host clang + `--target=x86_64-unknown-freebsd14.3 --sysroot=<base.txz>`. Closes oven-sh#29675 **Stacked on oven-sh#29675** — this PR includes the Android commits since both share the `crossTarget`/`sysroot` build infrastructure. The FreeBSD-specific diff starts at `d892fcae0a`. **Depends on oven-sh/WebKit#197** for `bun-webkit-freebsd-*` prebuilts. Closes oven-sh#1524, closes oven-sh#22991. ## Status - [x] `zig build check-freebsd` clean (x64+aarch64, Debug+ReleaseFast) - [x] All C/C++ compiles for FreeBSD - [x] `bun-debug` + `bun` (release) link as valid FreeBSD 14.3 ELF - [x] **Runs on real FreeBSD 14.3** ([smoke test](https://github.com/oven-sh/bun/actions/runs/24879912532)): `--revision`, `process.platform`, `os.*`, fs, **`Bun.serve()` + `fetch()`** all work - [x] **All 4 WebKit FreeBSD prebuilts published** (oven-sh/WebKit#197) - [x] **All 6 BuildKite FreeBSD jobs pass** (`{x64,aarch64}-build-{cpp,zig,bun}`) - [x] BuildKite v32 images baked with FreeBSD sysroot - [x] 10 bughunt findings fixed (copy_file_range loop, watcher registration, io.tick, futex wake, detached spawn, getRSS, blob read_len, os.machine, crash metadata, copyFile fast path) ## Approach FreeBSD is a separate **OS** (not a Linux abi like Android), so it goes in `type OS = ...|"freebsd"`, `Environment.isFreeBSD`, `OperatingSystem.freebsd`. It shares kqueue with macOS but uses plain `kevent`/`struct kevent` (not Darwin's `kevent64_s`), and FreeBSD 13+ has `eventfd(2)` and `copy_file_range(2)`. Builtins: FreeBSD ships compiler-rt as `/usr/lib/libgcc.a` and clang's freebsd driver finds it via `--sysroot` — no resource-dir symlinking needed (unlike NDK). aarch64-unknown-freebsd is a Rust Tier 3 target (no prebuilt std), so lolhtml uses `-Zbuild-std` for that arch. Host-GCC include leak: on amazonlinux, clang's driver injects `/usr/include/c++/N` even with `--sysroot`, breaking `#include_next` in the sysroot's libc++. Fixed with `-nostdlibinc` + explicit `-isystem` for the two sysroot dirs. ## Prior art Builds on lwhsu/bun `claude/freebsd-support` (Zig source changes, adapted from old CMake build to `scripts/build/*.ts`) and nektro's `af85c02f6d` (`zig build check-freebsd`). --------- Co-authored-by: Sosuke Suzuki <sosuke@bun.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
2 participants
Adds
--abi=androidas a Bun build target. Cross-compiles via host clang 21 +--target/--sysrootpointed at the NDK (r27c, API 28) — same compiler as every other platform, NDK only supplies bionic sysroot/libc++/compiler-rt.Depends on #29393 (WebKit upgrade) — based on that branch. WebKit android prebuilts are published as of oven-sh/WebKit#196 (
WEBKIT_VERSIONbumped toa4ddad89).What works:
bun-debuglinks as ARM aarch64 PIE, NEEDED =libc/libm/liblog/libdlonly. All vendored deps + Zig + WebKit compile. JIT (Baseline/DFG/FTL/Wasm) enabled.process.platform === "android",bun upgrade/bun build --compile --target=bun-linux-arm64-android, npmos:["android"]all wired.Runtime fixes (from a comprehensive bughunt — 47 findings):
TARGET_PLATFORM/ARCHso bundledsrc/js/**inlinesprocess.platformas"android"not the host's"linux"(was DCE-ing every android branch)epoll_pwait2/pwritev2/memfd_createfallbacks handle seccompEACCES(Android 14+ blocks several syscalls)getaddrinfo(c-ares can't discover servers without/etc/resolv.conf/JNI)/system/etc/security/cacerts/(<hash>.0files)platformTempDir→/data/local/tmp;/system/bin/shfor shell spawnos.homedir/userInfo/networkInterfaces/cpusreturn empty instead of throwing on bionic/SELinux denialsNon-Android changes included (correct fixes, called out for visibility):
timerfd_createswitched fromCLOCK_REALTIMEtoCLOCK_MONOTONICon all Linux — relative interval timers were always wrong with REALTIME (NTP/settimeofday steps skew them); now matches kqueue's monotonic behavior on macOSares_init()call beforeares_init_options()— both ran on every channel init on all platformsCI: matrix entries enabled, NDK + rustup android targets added to build images (bootstrap.sh v31 / .ps1 v18). No test platforms — running android tests needs an emulator (separate work).
Not yet verified at runtime — binary structure is correct but needs device/emulator testing for JIT W^X, mimalloc TLS, io_uring fallback.
Companion robobun PR adds
bun-linux-{aarch64,x64}-androidas optional release artifacts.