feat: add initial HarmonyOS (OHOS) support#30917
Open
springmin wants to merge 6 commits into
Open
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds HarmonyOS (OHOS) as a supported target: build/config/flags and Rust target, WebKit CMake/ICU wiring and zstd patching, OHOS cross-lib builders and packaging scripts, runtime/platform detection and syscall/spawn workarounds, installer/test-runner tweaks, and a small lolhtml crate-type change. ChangesOHOS Platform Support
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/bun-spawn.cpp (1)
371-396: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy liftConsider using self-pipe trick for fork fallback exec failure detection.
When
use_fork_fallbackis true, exec failures go undetected because fork() doesn't share memory. The macOS/FreeBSD path (lines 328-369) already implements a self-pipe trick that reliably detects exec failures via O_CLOEXEC on the write end.For consistency and reliability, the Linux fork fallback could reuse the same self-pipe pattern. Currently, if exec fails on the fallback path, the caller receives success (res=0) with a valid PID, but the child has already exited with status 127.
🤖 Prompt for 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. In `@src/jsc/bindings/bun-spawn.cpp` around lines 371 - 396, The fork-fallback branch (use_fork_fallback) currently assumes exec succeeded and returns res=0, which hides child exec failures; implement the same self-pipe pattern used in the macOS/FreeBSD path: create a pipe with O_CLOEXEC before fork, have the child write errno to the pipe if exec fails (and set CLOEXEC on the write end so a successful exec closes it), and in the parent after fork read from the pipe (or check EOF) to detect exec failure and set res to the reported errno (and reap the child if needed) while still populating *pid with child on success; update the code around the use_fork_fallback branch (the block referencing use_fork_fallback, child_errno, and fork_errpipe) to reuse the same logic and error-handling as the other platform path.
🤖 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 `@patches/zstd/ohos-qsort-r.patch`:
- Around line 15-23: The fallback qsort_r implementation uses a fixed stack
buffer tmp[256] and calls memcpy with the runtime size, which can overflow for
elements >256 bytes; replace the fixed buffer with a dynamically sized temporary
buffer (allocate with malloc/calloc or use alloca if acceptable) sized to 'size'
(or reuse a single malloc'd buffer for the whole sort) and ensure it is freed
after use; update the swap code in the qsort_r fallback (the block using tmp,
compar, nmemb, size) to allocate tmp_size = size, check allocation success, use
that buffer for the three memcpy calls, and free it before returning to
eliminate stack overflow.
In `@scripts/build/bun.ts`:
- Around line 79-86: The nested OHOS branch inside the Linux ICU handling is
dead code; inside the cfg.webkit === "local" && cfg.abi !== "android" block
remove the inner if (cfg.ohos && cfg.ohosIcuDir) check and its branch and always
push the ICU libs via libs.push("-licudata", "-licui18n", "-licuuc") (keep the
separate OHOS-specific ICU handling that exists later for cfg.ohos and
cfg.ohosIcuDir). This removes the unreachable cfg.ohos check while preserving
correct ICU linking for Linux and keeping the dedicated OHOS logic elsewhere.
In `@scripts/build/config.ts`:
- Around line 1187-1205: The comment on findHostCc is too terse—update the
comment above function findHostCc to briefly explain that OHOS cross-compilation
requires the system GCC (not LLVM clang) because GCC supplies
crtbegin.o/crtend.o and compatible crt startup files and link behavior that the
toolchain expects, and note circumstances when this might fail (e.g., systems
where /usr/bin/gcc is symlinked to clang). For findOhosSdkRoot, change the
lookup order in function findOhosSdkRoot to first check
process.env.OHOS_SDK_ROOT (and validate that resolve(<env>,
"ohos/native/sysroot") exists) before falling back to the existing candidates
list; keep the existing candidate checks and return undefined if none found.
Ensure references to findHostCc and findOhosSdkRoot are preserved.
- Around line 832-850: Make ohosCrossLibs and ohosIcuDir configurable instead of
hardcoding to cwd/build by adding optional fields (e.g. ohosCrossLibs,
ohosIcuDir) to PartialConfig and then, inside the OHOS branch in resolveConfig
(the block that sets ohosSdkRoot, ohosSysroot), set ohosCrossLibs =
partial.ohosCrossLibs ?? resolve(cwd, "build", "ohos-cross-libs") and ohosIcuDir
= partial.ohosIcuDir ?? resolve(cwd, "build", "ohos-icu", "target"); keep
existing validation (existsSync or other checks) as appropriate so existing
behavior is preserved when custom values are provided.
In `@scripts/build/deps/webkit.ts`:
- Around line 389-417: The code uses ohosCrossLibs without guarding and
constructs ICU tool paths fragily; update the block so you only reference
ohosCrossLibs when it is truthy (wrap assignments to args.CMAKE_EXE_LINKER_FLAGS
and args.CMAKE_SHARED_LINKER_FLAGS and the libcxx flags population inside the
existing ohosCrossLibs check or add an explicit if (ohosCrossLibs) guard) to
avoid undefined paths, and replace the brittle resolve(ohosIcuDir, "..", "..",
"ohos-icu", "host", "bin") logic used to set args.ICU_GENDATA_EXECUTABLE /
ICU_GENCCODE_EXECUTABLE / ICU_GENCMN_EXECUTABLE / ICU_PKGDATA_EXECUTABLE with a
more robust approach: derive a hostBin from a validated configuration value or
compute it with path.join/normalize relative to ohosIcuDir and verify existence
(fs.existsSync) before assigning each ICU_* arg, falling back or skipping
assignment if the executable is missing.
In `@scripts/build/flags.ts`:
- Around line 876-931: The -fPIE flag object currently has lang: "cxx" so it
only applies to C++; remove the lang restriction (or add a matching object for
C) so -fPIE is applied to all compilation units when c.ohos is true, keeping the
existing -pie linker entry as-is; update the object that defines flag: "-fPIE"
(remove or change its lang property) so C files also get PIE.
- Around line 149-176: The OHOS libc++ include flag uses a non-null assertion
c.ohosCrossLibs! but its when predicate only checks c.ohos, which can produce
invalid -I paths if ohosCrossLibs is undefined; update the flags entry that uses
c.ohosCrossLibs! (the object with lang:"cxx" and flag: c => [`-nostdinc++`,
`-I${c.ohosCrossLibs!}/libcxx/include/v1`,
`-I${c.ohosCrossLibs!}/libcxxabi/include`]) to guard against missing data by
changing its when predicate to c => c.ohos && !!c.ohosCrossLibs, or alternately
make the flag factory return an empty array when c.ohosCrossLibs is falsy so no
invalid include paths are emitted.
In `@scripts/build/source.ts`:
- Around line 1391-1398: The OHOS-specific env setup is duplicated and fragile:
instead of setting env.RUSTUP_TOOLCHAIN and CARGO_TARGET_<TRIPLE>_LINKER inside
the cfg.ohos branch, move the RUSTUP_TOOLCHAIN override and the envKey
assignment into the existing cross-compile block that handles cfg.crossTarget so
OHOS uses the same linker and link args (add the --target and --sysroot link
args there if missing); ensure you still set RUSTUP_TOOLCHAIN="stable" when
cfg.ohos is true, remove the separate cfg.ohos envKey assignment, and prefer
using resolved flags from resolveConfig() (cfg.ohos and cfg.crossTarget should
be derived booleans in Config) so the logic is not dependent on order of if
statements.
In `@scripts/ohos/build.sh`:
- Line 151: Update the command substitution that computes the package size so
the path is quoted to prevent word-splitting and globbing: modify the invocation
using du -sh /tmp/bun-release/${PKG_NAME}.tar.gz | awk ... to quote the path
inside the substitution (i.e., use "/tmp/bun-release/${PKG_NAME}.tar.gz") and
ensure the surrounding command that builds the info string (the info "包:
/tmp/bun-release/${PKG_NAME}.tar.gz ($(du -sh ... | awk '{print $1}'))") uses
the quoted form when calling du.
In `@scripts/ohos/prepare-cross-libs.sh`:
- Around line 61-70: The CMake command lines (the cmake -G Ninja block using
variables like CC, CXX, LLVM_DIR, CROSS_LIBS_DIR, LLVM_SRC_DIR, CFLAGS,
CXXFLAGS) are passing unquoted variable expansions which can break when paths
contain spaces; fix by quoting all variable expansions used as CMake argument
values (e.g., "${CC}", "${CXX}", "${LLVM_DIR}", "${CROSS_LIBS_DIR}",
"${LLVM_SRC_DIR}", "${CFLAGS}", "${CXXFLAGS}", etc.) in that cmake invocation
and apply the same quoting to the other cmake blocks for libc++abi and libc++ so
every path/flag variable is wrapped in double quotes.
In `@src/install/PackageInstall.rs`:
- Around line 1727-1732: The fallback path handling EPERM/EACCES leaks file
descriptors and drops source file mode bits: update the branch that currently
uses sys::File::openat, sys::File::create and sys::copy_file::copy_file so that
you wrap the returned handles in bun_sys::Fd (or otherwise obtain an Fd) and
call fd.close() for both source and destination after the copy completes (and on
error), and preserve the source permissions by reading the source mode (via
sys::fstat or equivalent on the sys::File handle) and applying it to the
destination (via sys::fchmod or equivalent) after copying; keep references to
entry, entry.basename, entry.path and destination_dir.fd() to locate the code to
change.
In `@src/jsc/bindings/bun-spawn.cpp`:
- Line 399: Remove the unused local variable debug_res: locate the assignment
"int debug_res = res;" and delete that declaration (and any other unused
debug_res occurrences) so the code no longer contains a dead variable; keep the
original 'res' usage intact and ensure no other references to debug_res remain
in the function/translation unit.
---
Outside diff comments:
In `@src/jsc/bindings/bun-spawn.cpp`:
- Around line 371-396: The fork-fallback branch (use_fork_fallback) currently
assumes exec succeeded and returns res=0, which hides child exec failures;
implement the same self-pipe pattern used in the macOS/FreeBSD path: create a
pipe with O_CLOEXEC before fork, have the child write errno to the pipe if exec
fails (and set CLOEXEC on the write end so a successful exec closes it), and in
the parent after fork read from the pipe (or check EOF) to detect exec failure
and set res to the reported errno (and reap the child if needed) while still
populating *pid with child on success; update the code around the
use_fork_fallback branch (the block referencing use_fork_fallback, child_errno,
and fork_errpipe) to reuse the same logic and error-handling as the other
platform path.
🪄 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: b1ade1c4-7f98-48aa-bc5f-cc3af35e7fb5
📒 Files selected for processing (22)
patches/lolhtml/crate-type.patchpatches/zstd/ohos-qsort-r.patchscripts/build/bun.tsscripts/build/config.tsscripts/build/deps/webkit.tsscripts/build/flags.tsscripts/build/rust.tsscripts/build/source.tsscripts/ohos/build.shscripts/ohos/prepare-cross-libs.shsrc/bun_core/Global.rssrc/bun_core/env.rssrc/crash_handler/lib.rssrc/install/PackageInstall.rssrc/install/PackageManager.rssrc/install/lib.rssrc/jsc/bindings/bun-spawn.cppsrc/jsc/bindings/c-bindings.cppsrc/runtime/cli/upgrade_command.rssrc/runtime/test_runner/harness/recover.rssrc/sys/lib.rssrc/sys/linux_syscall.rs
springmin
pushed a commit
to springmin/bun
that referenced
this pull request
May 17, 2026
springmin
pushed a commit
to springmin/bun
that referenced
this pull request
May 17, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@scripts/build/config.ts`:
- Around line 841-854: When OHOS is enabled, populate the generic cross-compile
fields so downstream plumbing sees the OHOS settings: after resolving
ohosSdkRoot/ohosSysroot/ohosCrossLibs, set the generic sysroot and crossTarget
(e.g. sysroot = ohosSysroot and crossTarget = partial.crossTarget ??
'aarch64-unknown-linux-gnu' or another OHOS target you use) so scripts that read
sysroot and crossTarget (instead of ohosSysroot/ohosCrossLibs) get the correct
values; update the block that defines ohosSysroot/ohosCrossLibs (symbols:
ohosSysroot, ohosCrossLibs, ohosSdkRoot, partial, crossTarget, sysroot) to
assign these generic fields accordingly.
In `@scripts/build/deps/webkit.ts`:
- Around line 394-408: The WebKit CMake override is rebuilding
args.CMAKE_CXX_FLAGS and args.CMAKE_C_FLAGS without the OHOS AArch64
compatibility flags, causing a mismatch; update the code in
scripts/build/deps/webkit.ts where ohosCrossLibs is handled (the
args.CMAKE_CXX_FLAGS and args.CMAKE_C_FLAGS assignments) to include the OHOS
AArch64 flags from scripts/build/flags.ts (specifically add
-mbranch-protection=none and -mno-outline-atomics when targeting OHOS AArch64)
so the same codegen flags are applied to both WebKit and the rest of the build.
In `@scripts/build/flags.ts`:
- Around line 915-920: The -fPIE entry currently placed with linkerFlags must be
moved into the compile-time flag table (e.g. globalFlags or bunOnlyFlags) so the
compiler sees it; update the flags data structure by removing the { flag:
"-fPIE", when: c => c.ohos, desc: ... } entry from the linkerFlags block and add
the same entry into globalFlags or bunOnlyFlags (keeping the when: c => c.ohos
predicate and description), so computeFlags will emit -fPIE into cflags/cxxflags
and the linker still uses -pie via ldflags.
In `@scripts/ohos/build.sh`:
- Around line 27-28: The SKIP_BUILD variable is defined but not used—make the
script respect SKIP_BUILD by guarding or short-circuiting Stages 4–7: either add
an early conditional that exits or returns when SKIP_BUILD=true, or wrap the
Stage 4–7 blocks in an if test that only executes when SKIP_BUILD is not "true";
reference the existing SKIP_BUILD variable and the stage blocks labeled "Stage
4" through "Stage 7" to locate where to add the check so
configuring/compiling/packaging is skipped when requested.
- Around line 67-69: The script masks failures for git operations so a failed
git fetch/checkout (in the BUILD_DIR and involving BUN_BRANCH) can be ignored;
change the block around git fetch and git checkout to detect and fail on errors
instead of using "|| true": run git fetch origin "$BUN_BRANCH" and git checkout
"$BUN_BRANCH" without swallowing errors, capture their exit statuses (or test
with git rev-parse --verify origin/"$BUN_BRANCH" or git rev-parse --verify
"$BUN_BRANCH") and if either command fails, emit a clear error (e.g., via echo
or logger) and exit 1 so the script stops rather than continuing to build an
unexpected tree; reference the BUILD_DIR and BUN_BRANCH variables and the git
fetch/git checkout calls when making this change.
- Around line 118-119: The script currently sets VERSION by executing the
cross-built binary ($BINARY --version) which may not run on the build host and
falls back to a stale hard-coded value; change VERSION assignment to read the
version field from package.json (or another canonical metadata file) instead of
executing the binary, then use that VERSION when composing PKG_NAME
("PKG_NAME=\"bun-ohos-aarch64-${VERSION}-${BUN_COMMIT}-${DATE_TAG}\""); update
any related logic that expects VERSION (e.g., variables BINARY, BUN_COMMIT,
DATE_TAG) to rely on the parsed package.json value so the package name and
README reflect the actual package.json version reliably.
In `@src/install/PackageInstall.rs`:
- Around line 1729-1738: The current code uses sys::File::openat and
sys::File::create then manually calls inf.close() and outf.close(), but a ?
early-return (e.g., from sys::File::create or sys::copy_file::copy_file) can
leak the source descriptor; wrap both opened handles with
sys::CloseOnDrop::file(...) immediately after obtaining them (replace direct
sys::File values with their CloseOnDrop wrappers), remove the explicit
inf.close()/outf.close() calls, and keep the permission-preserving logic using
sys::fstat and sys::fchmod by referencing the inner file handle when needed so
RAII always closes FDs on error or return.
In `@src/jsc/bindings/bun-spawn.cpp`:
- Around line 157-165: The OHOS branch currently calls fork() but still relies
on the parent reading child_errno as if vfork() semantics applied; change the
OHOS path to either set use_fork_fallback = true (so the parent skips
child_errno checks) or implement a proper exec-failure propagation (e.g.,
self-pipe like Darwin/FreeBSD) between child and parent; update the code around
the child = fork() in the block that now falls through for OHOS and ensure
use_fork_fallback is set when using fork() (or wire up a pipe and transfer exec
errno) so parent-side checks of child_errno (and logic downstream) behave
correctly.
🪄 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: 089b670f-b62f-40c5-83d1-0747c71d4e96
📒 Files selected for processing (11)
patches/zstd/ohos-qsort-r.patchscripts/build/bun.tsscripts/build/config.tsscripts/build/deps/webkit.tsscripts/build/deps/zstd.tsscripts/build/flags.tsscripts/build/source.tsscripts/ohos/build.shscripts/ohos/prepare-cross-libs.shsrc/install/PackageInstall.rssrc/jsc/bindings/bun-spawn.cpp
Author
CodeRabbit Review Response感谢 CodeRabbit 的详细审查。以下是对所有建议的处理说明。 Round 1 (12 comments) — Resolved in
|
| # | File | Issue | Action |
|---|---|---|---|
| 1 | ohos-qsort-r.patch |
tmp[256] stack buffer overflow |
✅ Fixed: malloc(size) + null check + free |
| 2 | bun.ts |
Dead OHOS ICU code inside Linux block | ✅ Fixed: removed unreachable if (cfg.ohos) |
| 3 | config.ts |
findOhosSdkRoot() env var priority + ohosCrossLibs/ohosIcuDir configurable |
✅ Fixed: OHOS_SDK_ROOT checked first; PartialConfig fields added |
| 4 | webkit.ts |
ohosCrossLibs referenced without guard |
✅ Fixed: wrapped in if (ohosCrossLibs) |
| 5 | flags.ts |
-fPIE had lang: "cxx" only; ohosCrossLibs! non-null assertion |
✅ Fixed: removed lang restriction; added !!c.ohosCrossLibs guard |
| 6 | source.ts |
Duplicate OHOS env setup | ✅ Fixed: merged into crossTarget block |
| 7 | build.sh |
Unquoted du path |
✅ Fixed: added quotes |
| 8 | prepare-cross-libs.sh |
Unquoted cmake variables | ✅ Fixed: all variables double-quoted |
| 9 | PackageInstall.rs |
FD leak + permission bits dropped | ✅ Fixed: close both FDs; preserve mode via fstat+fchmod |
| 10 | bun-spawn.cpp |
debug_res unused variable |
✅ Fixed: removed |
| — | zstd.ts |
Extra find: patches reference lost during merge | ✅ Fixed: restored patches: cfg => ... |
| — | Fork fallback self-pipe | Heavy lift requiring OHOS device testing | ⏸️ Skipped (see note below) |
Round 2 (8 comments) — Resolved in 63601f78
| # | File | Issue | Action |
|---|---|---|---|
| 1 | config.ts |
Generic cross-compile fields not set for OHOS | ✅ Fixed: sysroot = ohosSysroot; crossTarget = "aarch64-linux-ohos" |
| 2 | webkit.ts |
WebKit CMake missing OHOS AArch64 flags | ✅ Fixed: added -mbranch-protection=none -mno-outline-atomics |
| 3 | flags.ts |
-fPIE in linkerFlags (compile-time flag in wrong table) |
✅ Fixed: moved to globalFlags |
| 4 | build.sh |
SKIP_BUILD unused |
✅ Fixed: added if [ "$SKIP_BUILD" != "true" ] guard wrapping Stages 4–7 |
| 5 | build.sh |
git fetch/checkout || true swallows errors |
✅ Fixed: fetch warns on failure, checkout errors on failure |
| 6 | build.sh |
VERSION from cross-compiled binary (won't run on host) |
✅ Fixed: reads from package.json |
| 7 | PackageInstall.rs |
Manual close lacks RAII for ? early-return |
✅ Fixed: wrapped with CloseOnDrop::file() |
| 8 | bun-spawn.cpp |
OHOS fork() reads stale child_errno (vfork semantics) |
✅ Fixed: use_fork_fallback = true for __OHOS__ |
Note on self-pipe exec failure detection (skipped)
The fork-fallback self-pipe pattern suggested for bun-spawn.cpp (Darwin/FreeBSD path) is intentionally skipped in this round. It is a non-trivial refactor that requires testing on actual OHOS hardware to validate. The use_fork_fallback = true fix is the safe conservative approach — it avoids reading stale memory from the fork() copy-on-write child without introducing new IPC complexity.
Branch state
Both branches pushed with the same fix commits:
ohos-aarm64: f5be233b43pr/ohos-platform-support: 63601f785a(this PR)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
scripts/build/deps/webkit.ts (1)
413-417:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDerive the ICU host-tool directory from
ohosIcuDirinstead of re-hardcodingohos-icu.Line 413 reconstructs
.../ohos-icu/host/binby hand. That only works whencfg.ohosIcuDirhappens to be the defaultbuild/ohos-icu/targetlayout; any valid override to a different prefix makes all fourICU_*_EXECUTABLEpaths wrong even though the target ICU dir itself is correct. Compute the host bin as a sibling ofohosIcuDir(or pass it explicitly in config), and ideally only assign these variables when the binaries exist.💡 Minimal fix
- const hostBin = resolve(ohosIcuDir, "..", "..", "ohos-icu", "host", "bin"); + const hostBin = resolve(ohosIcuDir, "..", "host", "bin");🤖 Prompt for 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. In `@scripts/build/deps/webkit.ts` around lines 413 - 417, Derive the host tool directory from the configured ohosIcuDir instead of hardcoding "ohos-icu": compute hostBin as the sibling "host/bin" relative to ohosIcuDir's parent (e.g. parent-of-parent(ohosIcuDir) + "host/bin") using the same resolve helper, then set args.ICU_GENDATA_EXECUTABLE, args.ICU_GENCCODE_EXECUTABLE, args.ICU_GENCMN_EXECUTABLE and args.ICU_PKGDATA_EXECUTABLE from that hostBin only if the corresponding binaries actually exist (use fs.existsSync or equivalent) so overrides of cfg.ohosIcuDir still produce correct paths.
🤖 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 `@scripts/build/config.ts`:
- Around line 842-853: The OHOS path fields (ohosSdkRoot, ohosSysroot,
ohosCrossLibs, ohosIcuDir) are being stored verbatim from PartialConfig so
relative overrides break when run from a different cwd; update each assignment
to normalize relative overrides to absolute paths before storing: for
ohosSdkRoot, if partial.ohosSdkRoot is provided wrap it with resolve(cwd,
partial.ohosSdkRoot) (otherwise call findOhosSdkRoot()), for ohosSysroot,
ohosCrossLibs and ohosIcuDir do the same (use resolve(cwd, partial.<field>) when
partial value exists, otherwise fall back to the existing resolve(...) logic),
and keep the existsSync check using the resolved ohosSysroot value; ensure you
reference the symbols ohosSdkRoot, ohosSysroot, ohosCrossLibs and ohosIcuDir
when making these changes.
- Around line 331-338: PartialConfig is missing the crossTarget field that
resolveConfig() reads from partial.crossTarget; add crossTarget?: string to the
PartialConfig interface so it mirrors the Config type and can be resolved in
resolveConfig(). Update the interface declaration named PartialConfig (alongside
the existing Config interface entries like ohosSysroot, ohosSdkRoot,
ohosCrossLibs, ohosIcuDir) to include crossTarget?: string so resolveConfig()
can safely access partial.crossTarget.
---
Duplicate comments:
In `@scripts/build/deps/webkit.ts`:
- Around line 413-417: Derive the host tool directory from the configured
ohosIcuDir instead of hardcoding "ohos-icu": compute hostBin as the sibling
"host/bin" relative to ohosIcuDir's parent (e.g. parent-of-parent(ohosIcuDir) +
"host/bin") using the same resolve helper, then set args.ICU_GENDATA_EXECUTABLE,
args.ICU_GENCCODE_EXECUTABLE, args.ICU_GENCMN_EXECUTABLE and
args.ICU_PKGDATA_EXECUTABLE from that hostBin only if the corresponding binaries
actually exist (use fs.existsSync or equivalent) so overrides of cfg.ohosIcuDir
still produce correct paths.
🪄 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: 36432194-da5c-464d-9f58-2eaed9deaf91
📒 Files selected for processing (6)
scripts/build/config.tsscripts/build/deps/webkit.tsscripts/build/flags.tsscripts/ohos/build.shsrc/install/PackageInstall.rssrc/jsc/bindings/bun-spawn.cpp
Author
Round 3 (2 comments) — Resolved in
|
| # | File | Issue | Action |
|---|---|---|---|
| 1 | config.ts |
Partial override paths not resolved to absolute; PartialConfig missing crossTarget |
✅ Fixed: all partial OHOS paths wrapped with resolve(cwd, ...); crossTarget?: string added to interface |
| 2 | webkit.ts |
ICU host-bin path hardcodes ohos-icu dir name, breaks when ohosIcuDir is overridden |
✅ Fixed: derived as sibling of ohosIcuDir parent via resolve(ohosIcuDir, "..", "host", "bin") + guard with existsSync |
Branches updated:
ohos-aarm64: 387aca4c2fpr/ohos-platform-support: be20425af6
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/build/deps/webkit.ts (1)
42-47:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
existsSyncimport fromnode:fs.Line 417 uses
existsSync(exePath)in the OHOS WebKit configuration block, but the function is not imported. This will cause aReferenceError: existsSync is not definedat runtime when building WebKit for OHOS.Proposed fix
import { homedir } from "node:os"; +import { existsSync } from "node:fs"; import { join, resolve } from "node:path";🤖 Prompt for 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. In `@scripts/build/deps/webkit.ts` around lines 42 - 47, The runtime error comes from calling existsSync(exePath) without importing it; add the missing import for existsSync from "node:fs" at the top of the file so existsSync is defined where it's used (the OHOS WebKit configuration block that checks exePath). Update the existing import group (near computeCpuTargetFlags, slash, and source-related imports) to include existsSync so the check in the WebKit OHOS configuration can run without a ReferenceError.
🤖 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.
Outside diff comments:
In `@scripts/build/deps/webkit.ts`:
- Around line 42-47: The runtime error comes from calling existsSync(exePath)
without importing it; add the missing import for existsSync from "node:fs" at
the top of the file so existsSync is defined where it's used (the OHOS WebKit
configuration block that checks exePath). Update the existing import group (near
computeCpuTargetFlags, slash, and source-related imports) to include existsSync
so the check in the WebKit OHOS configuration can run without a ReferenceError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 542086c9-8286-4597-8fc3-2fad4fbf6fef
📒 Files selected for processing (2)
scripts/build/config.tsscripts/build/deps/webkit.ts
Author
Round 4 (1 comment) — Resolved in
|
| # | File | Issue | Action |
|---|---|---|---|
| 1 | webkit.ts |
🔴 Missing existsSync import — ReferenceError at runtime for OHOS WebKit builds |
✅ Fixed: added import { existsSync } from "node:fs" at top of file |
Branches updated:
ohos-aarm64: d6dc4ae068pr/ohos-platform-support: f323dbd42a
Contributor
There was a problem hiding this comment.
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 `@scripts/build/deps/webkit.ts`:
- Line 416: The loop mapping ICU tool environment variables assigns
ICU_GENDATA_EXECUTABLE to "genrb" incorrectly; update the mapping in the for
loop that iterates over [["ICU_GENDATA_EXECUTABLE", "genrb"], ...] so that
ICU_GENDATA_EXECUTABLE is paired with "gendata" (leave other entries unchanged),
ensuring the for loop (the const array used in the for (const [key, exe] of ...)
block) uses the correct executable name.
🪄 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: 7acefa9d-38bb-495b-a452-161b36e3433b
📒 Files selected for processing (1)
scripts/build/deps/webkit.ts
| if (ohosIcuDir) { | ||
| // hostBin is sibling of ohosIcuDir's parent: ohosIcuDir="<prefix>/target" → hostBin="<prefix>/host/bin" | ||
| const hostBin = resolve(ohosIcuDir, "..", "host", "bin"); | ||
| for (const [key, exe] of [["ICU_GENDATA_EXECUTABLE", "genrb"], ["ICU_GENCCODE_EXECUTABLE", "genccode"], ["ICU_GENCMN_EXECUTABLE", "gencmn"], ["ICU_PKGDATA_EXECUTABLE", "pkgdata"]] as const) { |
Contributor
There was a problem hiding this comment.
Point ICU_GENDATA_EXECUTABLE at gendata, not genrb.
ICU_GENDATA_EXECUTABLE is wired to the wrong ICU host tool here. genrb and gendata are separate executables, so this can break the OHOS local WebKit configure/build path when CMake passes gendata-specific arguments.
Proposed fix
- for (const [key, exe] of [["ICU_GENDATA_EXECUTABLE", "genrb"], ["ICU_GENCCODE_EXECUTABLE", "genccode"], ["ICU_GENCMN_EXECUTABLE", "gencmn"], ["ICU_PKGDATA_EXECUTABLE", "pkgdata"]] as const) {
+ for (const [key, exe] of [["ICU_GENDATA_EXECUTABLE", "gendata"], ["ICU_GENCCODE_EXECUTABLE", "genccode"], ["ICU_GENCMN_EXECUTABLE", "gencmn"], ["ICU_PKGDATA_EXECUTABLE", "pkgdata"]] as const) {For ICU/CMake integration, what executable should `ICU_GENDATA_EXECUTABLE` reference: `gendata` or `genrb`?
🤖 Prompt for 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.
In `@scripts/build/deps/webkit.ts` at line 416, The loop mapping ICU tool
environment variables assigns ICU_GENDATA_EXECUTABLE to "genrb" incorrectly;
update the mapping in the for loop that iterates over
[["ICU_GENDATA_EXECUTABLE", "genrb"], ...] so that ICU_GENDATA_EXECUTABLE is
paired with "gendata" (leave other entries unchanged), ensuring the for loop
(the const array used in the for (const [key, exe] of ...) block) uses the
correct executable name.
Adds the necessary platform detection, build configuration, and runtime adaptations to build and run Bun on HarmonyOS (OHOS) aarch64. - C++: pidfd_open/close_range bypass via BUN_OHOS_DISABLE_PIDFD - C++: current_max_fd=2, posix_spawn fallback, /system/bin PATH - Rust: IS_OHOS constant, os_name/os_display - Rust: memfd_create SIGSYS guard via BUN_OHOS_DISABLE_PIDFD - Rust: pidfd_open SIGSYS guard (both linux and android cfg paths) - Rust: linkat EPERM fallback to copy_file - Build: OHOS target triple in rustTarget(), C++ cmake config - Build: OHOS cross-compilation flags, sysroot, ICU linking - Build: WebKit CMake OHOS mode, smoke test skip - Build: zstd qsort_r compat patch - Scripts: build.sh and prepare-cross-libs.sh for the SDK
springmin
force-pushed
the
pr/ohos-platform-support
branch
from
7138e12 to
086a08e
Compare
Author
|
Rebased onto latest Branch: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds initial support for building Bun on HarmonyOS (OHOS) aarch64.
C++ Runtime Adaptations
Rust Runtime
Build System
Patches & Scripts
Tested: 53,391 test cases on OHOS device, 47,759 PASS (89.5%)