Skip to content

[JSCOnly] Add Android cross-compile target - #196

Merged
Jarred-Sumner merged 10 commits into
mainfrom
claude/android-support
Apr 24, 2026
Merged

[JSCOnly] Add Android cross-compile target#196
Jarred-Sumner merged 10 commits into
mainfrom
claude/android-support

Conversation

@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

Adds aarch64-linux-android (and x86_64) as a JSCOnly target, built via Dockerfile.android (Ubuntu + LLVM 21 + NDK r27c).

Approach: Uses the same host clang as the rest of Bun's matrix (LLVM 21), passing --target/--sysroot at the NDK rather than using the NDK's bundled clang 18 — keeps codegen consistent across platforms. ICU is cross-built in two stages (host build for codegen tools, then --with-cross-build for the target).

WebKit source changes (3 lines):

  • Assertions.h — include <android/log.h> unconditionally on OS(ANDROID); LOG_ANDROID_SEND references it regardless of USE(BUN_JSC_ADDITIONS)
  • PlatformJSCOnly.cmakeif(ANDROID) source list (LoggingAndroid.cpp, RefPtrAndroid.cpp, liblog) + use MemoryPressureHandlerGeneric.cpp (the unix one references m_holdOffTimer which is !OS(ANDROID)-gated)
  • linux/RealTimeThreads.cpp#include <unistd.h> for getpid (bionic doesn't pull it transitively)

JIT: all tiers enabled (ENABLE_JIT/DFG_JIT/FTL_JIT/WEBASSEMBLY=ON, C_LOOP=OFF). Tested locally — jsc shell links as ARM aarch64 PIE.

CI: linux-android job added to build-reusable.yml — 4-artifact matrix ({arm64,amd64}-android{,-debug}, no LTO). Cross-compiles from x64 since the NDK only ships linux-x86_64 prebuilts.

CMake gotcha worth noting: CMAKE_SYSTEM_NAME=Android makes CMake's Android module silently override CMAKE_{C,CXX}_COMPILER with the NDK's clang. The Dockerfile uses CMAKE_SYSTEM_NAME=Linux + ANDROID=ON + explicit --target/--sysroot instead to keep our clang 21.

Companion Bun PR will follow once these prebuilts publish.

Dockerfile.android: Ubuntu + LLVM 21 + NDK r27c. Two-stage ICU
(host build for codegen tools, then --with-cross-build for target).
WebKit configured via CMake's native CMAKE_SYSTEM_NAME=Android while
overriding CMAKE_<LANG>_COMPILER to host clang.

PlatformJSCOnly.cmake: include wtf/android/{Logging,RefPtr}Android.cpp
and link liblog when ANDROID is set. WTF already has OS(ANDROID)
handling in ~25 places but JSCOnly never wired the source list.
- Assertions.h: include <android/log.h> unconditionally on OS(ANDROID);
  LOG_ANDROID_SEND at :760 references __android_log_print regardless of
  USE(BUN_JSC_ADDITIONS)
- PlatformJSCOnly.cmake: use generic/MemoryPressureHandlerGeneric.cpp on
  Android (the unix one references m_holdOffTimer which is gated
  !OS(ANDROID) in MemoryPressureHandler.h)
- linux/RealTimeThreads.cpp: include <unistd.h> for getpid (bionic
  doesn't pull it in transitively)
…droid

CMAKE_SYSTEM_NAME=Android force-selects the NDK's bundled clang and
silently ignores CMAKE_{C,CXX}_COMPILER, defeating the point of using
the same clang version as Bun. Treat as generic Linux cross-compile
with explicit --target/--sysroot in CFLAGS instead. Also pin
ICU_INCLUDE_DIR so FindICU doesn't pick up the NDK sysroot's
__INTRODUCED_IN(31)-annotated headers.
4-entry matrix ({arm64,amd64}-android × {Release,Debug}, no LTO/asan).
All run on linux-x64-gh — Google ships no linux-arm64 NDK, so Android
is always cross-compiled from x64.

android-release.sh wraps `docker buildx build -f Dockerfile.android
--target=artifact` following the musl-release.sh pattern. Wired into
the release job's needs/download/rename/upload.
… ld.lld)

apt.llvm.org packages install ld.lld-21 only; -fuse-ld=lld needs the
unversioned name. Add /usr/local/bin symlinks for the LLVM tools. Also:
make compiler-rt symlinks for both arches in base (layer is arch-agnostic
+ cacheable), and dump config.log on ICU configure failure.
…ler-rt

apt.llvm.org's clang uses the old-style flat runtime layout
(lib/linux/libclang_rt.builtins-<arch>-android.a) while tarball builds
use per-triple. Populate both, and pass --rtlib=compiler-rt explicitly
so clang doesn't fall back to -lgcc. Base stage now does a sanity link
so a misconfigured toolchain fails fast instead of inside ICU configure.
@coderabbitai

coderabbitai Bot commented Apr 24, 2026

Copy link
Copy Markdown

Walkthrough

Adds Android cross-compilation: new reusable GitHub Actions job, multi-stage Android Docker build to produce ICU and WebKit/JSC artifacts, Android-specific WTF CMake/platform integrations and header include adjustment, a small POSIX include fix, and a script to build/export Docker artifacts locally.

Changes

Cohort / File(s) Summary
GitHub Actions workflow
.github/workflows/build-reusable.yml
Adds a linux-android job that cross-compiles WebKit for Android (arm64/x86_64; Release/Debug), embeds BUN_WEBKIT_VERSION and Android bun-webkit/package.json metadata into artifacts, and updates the release job to download, move, and publish Android artifacts to GitHub Releases.
Android Docker build & helper script
Dockerfile.android, android-release.sh
Adds Dockerfile.android: multi-stage cross-compilation that pins LLVM/NDK/ICU, verifies downloads, builds host ICU tools then cross-compiles ICU and WebKit/JSC static libs/headers into /output. Adds android-release.sh to set defaults, compute arch flags, and run docker buildx build --target=artifact exporting results to a local directory.
WTF platform integration
Source/WTF/wtf/PlatformJSCOnly.cmake, Source/WTF/wtf/Assertions.h
PlatformJSCOnly.cmake: introduces Android-specific sources/headers (android/LoggingAndroid.cpp, android/RefPtrAndroid.*), exposes Android public header(s), links log, and appends Linux-based memory/process/thread implementations plus generic memory-pressure handler when targeting Android. Assertions.h: include of <android/log.h> now conditioned only on OS(ANDROID).
POSIX compatibility fix
Source/WTF/wtf/linux/RealTimeThreads.cpp
Adds #include <unistd.h> to ensure POSIX symbols used in the translation unit are available.
🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive The description provides comprehensive technical details about the Android cross-compile implementation, but does not follow the WebKit repository's required template format (missing Bugzilla bug link and component labels). Add a Bugzilla bug reference link and clarify whether this PR should be associated with a specific bug tracking entry per WebKit's contribution guidelines.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main change: adding Android as a JSCOnly cross-compilation target for the WebKit/JSC project.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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


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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@android-release.sh`:
- Around line 28-40: The script uses the variable temp without a default which
breaks under set -u; fix by ensuring temp is assigned a sensible default before
it's used (before mkdir -p "$temp"), e.g. assign temp to a newly created temp
directory or a fallback path using POSIX parameter expansion or an explicit
conditional assignment or mktemp -d so callers that don't export temp can run
the script locally; update android-release.sh to set temp when unset and then
proceed with mkdir/rm and the Docker build referencing that variable.

In `@Dockerfile.android`:
- Around line 25-28: The RUN step that pipes wget into bash (using LLVM_VERSION)
can hide download failures; change it to download the script to a file (e.g.
/tmp/llvm.sh) with wget or curl (using -f/--fail and retries), check the
download exit code, then run bash /tmp/llvm.sh ${LLVM_VERSION} only if the
download succeeded, and only then execute the symlink loop that creates links
for clang clang++ ld.lld lld llvm-ar llvm-ranlib; this ensures the install step
fails fast on truncated downloads and symlinks are created only after a
successful install.
- Line 80: The Dockerfile.android currently ADDs the ICU tarball from a remote
URL with no integrity checks; change the Dockerfile to fetch the tarball and its
accompanying checksum/signature and verify it before adding to the image:
replace the ADD https...icu4c-75_1-src.tgz line with a staged download step that
retrieves the tarball plus a .sha256 or .asc, verify the SHA256 (or GPG
signature) inside the build (or in a prior CI job) and only COPY the verified
local artifact into the image; reference the ICU tarball name icu4c-75_1-src.tgz
and add the checksum/signature file handling so the build fails if verification
does not succeed.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 29c77a8f-3fa4-4988-854a-f5c03380b995

📥 Commits

Reviewing files that changed from the base of the PR and between 7ab1748 and da0528b.

📒 Files selected for processing (6)
  • .github/workflows/build-reusable.yml
  • Dockerfile.android
  • Source/WTF/wtf/Assertions.h
  • Source/WTF/wtf/PlatformJSCOnly.cmake
  • Source/WTF/wtf/linux/RealTimeThreads.cpp
  • android-release.sh

Comment thread android-release.sh
Comment thread Dockerfile.android
Comment on lines +25 to +28
RUN wget -qO- https://apt.llvm.org/llvm.sh | bash -s -- ${LLVM_VERSION} && \
for t in clang clang++ ld.lld lld llvm-ar llvm-ranlib; do \
ln -sf /usr/bin/${t}-${LLVM_VERSION} /usr/local/bin/${t}; \
done

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Make the LLVM bootstrap fail when the download is incomplete.

Line 25 pipes wget into bash under Docker’s default /bin/sh, so a truncated/failed download can still exit successfully and leave dangling tool symlinks that only fail much later when clang is invoked. That makes toolchain breakage hard to diagnose.

Proposed fix
+SHELL ["/bin/bash", "-o", "pipefail", "-c"]
-RUN wget -qO- https://apt.llvm.org/llvm.sh | bash -s -- ${LLVM_VERSION} && \
+RUN wget -qO- https://apt.llvm.org/llvm.sh | bash -s -- "${LLVM_VERSION}" && \
+    test -x "/usr/bin/clang-${LLVM_VERSION}" && \
     for t in clang clang++ ld.lld lld llvm-ar llvm-ranlib; do \
       ln -sf /usr/bin/${t}-${LLVM_VERSION} /usr/local/bin/${t}; \
     done
🧰 Tools
🪛 Hadolint (2.14.0)

[warning] 25-25: Set the SHELL option -o pipefail before RUN with a pipe in it. If you are using /bin/sh in an alpine image or if your shell is symlinked to busybox then consider explicitly setting your SHELL to /bin/ash, or disable this check

(DL4006)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Dockerfile.android` around lines 25 - 28, The RUN step that pipes wget into
bash (using LLVM_VERSION) can hide download failures; change it to download the
script to a file (e.g. /tmp/llvm.sh) with wget or curl (using -f/--fail and
retries), check the download exit code, then run bash /tmp/llvm.sh
${LLVM_VERSION} only if the download succeeded, and only then execute the
symlink loop that creates links for clang clang++ ld.lld lld llvm-ar
llvm-ranlib; this ensures the install step fails fast on truncated downloads and
symlinks are created only after a successful install.

Comment thread Dockerfile.android Outdated
…: temp default

ADD --checksum verifies the ICU tarball; SHELL pipefail catches a
truncated llvm.sh download. android-release.sh defaults $temp so it's
runnable standalone (workflow already exports it).

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@android-release.sh`:
- Around line 15-20: The current ANDROID_ARCH handling in the MARCH_FLAG
assignment treats any non-"aarch64" value as x86, which hides typos and causes
failures later; update the if-block that checks ANDROID_ARCH (the existing
conditional that sets MARCH_FLAG) to explicitly accept known architectures
(e.g., "aarch64" and "x86_64" or whichever exact names you support) and for any
other value print a clear error message including the invalid ANDROID_ARCH and
exit with a non-zero status so the script fails fast instead of falling back to
the wrong -march flags.

In `@Dockerfile.android`:
- Around line 172-174: The CMake flags currently use
CMAKE_FIND_ROOT_PATH_MODE_PACKAGE, CMAKE_FIND_ROOT_PATH_MODE_LIBRARY, and
CMAKE_FIND_ROOT_PATH_MODE_INCLUDE set to BOTH which allows host (Ubuntu) paths
to satisfy target lookups during cross-build; change each of those flags to ONLY
so CMake restricts searches to the Android sysroot/target paths (i.e., replace
BOTH with ONLY for CMAKE_FIND_ROOT_PATH_MODE_PACKAGE,
CMAKE_FIND_ROOT_PATH_MODE_LIBRARY, and CMAKE_FIND_ROOT_PATH_MODE_INCLUDE in the
Dockerfile.android CMake invocation).
- Around line 40-43: The Dockerfile currently downloads the Android NDK
(NDK_VERSION) to /tmp/ndk.zip and unpacks it without integrity checks; change
the RUN that handles /tmp/ndk.zip so the image verifies the archive before
extracting (e.g., add a build ARG like NDK_SHA256 or download an official
checksum file and compare via sha256sum -c) and aborts on mismatch, cleans up
the zip on failure, and only then unzips to /opt/android-ndk; reference the RUN
that uses NDK_VERSION, /tmp/ndk.zip and /opt/android-ndk to locate where to add
the verification and failure handling.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: e76944c6-5b46-4e7f-a78d-d200d3f1c57b

📥 Commits

Reviewing files that changed from the base of the PR and between da0528b and e9c2d9b.

📒 Files selected for processing (2)
  • Dockerfile.android
  • android-release.sh

Comment thread android-release.sh Outdated
Comment thread Dockerfile.android
Comment thread Dockerfile.android
Comment on lines +172 to +174
-DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=BOTH \
-DCMAKE_FIND_ROOT_PATH_MODE_LIBRARY=BOTH \
-DCMAKE_FIND_ROOT_PATH_MODE_INCLUDE=BOTH \

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

For a CMake cross-compilation build, what is the effect of setting CMAKE_FIND_ROOT_PATH_MODE_PACKAGE, CMAKE_FIND_ROOT_PATH_MODE_LIBRARY, and CMAKE_FIND_ROOT_PATH_MODE_INCLUDE to BOTH instead of ONLY? Can BOTH cause host headers or libraries to be selected instead of target sysroot paths?

💡 Result:

Setting CMAKE_FIND_ROOT_PATH_MODE_PACKAGE, CMAKE_FIND_ROOT_PATH_MODE_LIBRARY, or CMAKE_FIND_ROOT_PATH_MODE_INCLUDE to BOTH (instead of ONLY) causes CMake's corresponding find commands (find_package, find_library, find_path/find_file) to search both the paths prefixed by CMAKE_FIND_ROOT_PATH (and CMAKE_SYSROOT) and the standard host system paths. With ONLY, searches are restricted exclusively to the CMAKE_FIND_ROOT_PATH and CMAKE_SYSROOT prefixed locations. Yes, BOTH can cause host headers or libraries to be selected instead of those in the target sysroot paths. For these modes, CMake first searches the rooted paths (CMAKE_FIND_ROOT_PATH then CMAKE_SYSROOT), then the non-rooted host system paths. If an item is not found in the target paths but exists on the host, it will be picked up from the host, potentially leading to contamination in cross-compilation builds where host and target libraries/headers are incompatible.

Citations:


🏁 Script executed:

fd -type f -name "Dockerfile.android" | head -5

Repository: oven-sh/WebKit

Length of output: 229


🏁 Script executed:

fd Dockerfile.android

Repository: oven-sh/WebKit

Length of output: 77


🏁 Script executed:

sed -n '170,180p' Dockerfile.android

Repository: oven-sh/WebKit

Length of output: 513


Don't let CMake resolve host packages into the Android build.

CMAKE_FIND_ROOT_PATH_MODE_{PACKAGE,LIBRARY,INCLUDE}=BOTH allows the Ubuntu builder's host paths to satisfy target lookups. In a cross-build that can pull x86_64 headers/libs into the Android configure or link step.

Suggested change
-    -DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=BOTH \
-    -DCMAKE_FIND_ROOT_PATH_MODE_LIBRARY=BOTH \
-    -DCMAKE_FIND_ROOT_PATH_MODE_INCLUDE=BOTH \
+    -DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ONLY \
+    -DCMAKE_FIND_ROOT_PATH_MODE_LIBRARY=ONLY \
+    -DCMAKE_FIND_ROOT_PATH_MODE_INCLUDE=ONLY \
📝 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.

Suggested change
-DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=BOTH \
-DCMAKE_FIND_ROOT_PATH_MODE_LIBRARY=BOTH \
-DCMAKE_FIND_ROOT_PATH_MODE_INCLUDE=BOTH \
-DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ONLY \
-DCMAKE_FIND_ROOT_PATH_MODE_LIBRARY=ONLY \
-DCMAKE_FIND_ROOT_PATH_MODE_INCLUDE=ONLY \
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Dockerfile.android` around lines 172 - 174, The CMake flags currently use
CMAKE_FIND_ROOT_PATH_MODE_PACKAGE, CMAKE_FIND_ROOT_PATH_MODE_LIBRARY, and
CMAKE_FIND_ROOT_PATH_MODE_INCLUDE set to BOTH which allows host (Ubuntu) paths
to satisfy target lookups during cross-build; change each of those flags to ONLY
so CMake restricts searches to the Android sysroot/target paths (i.e., replace
BOTH with ONLY for CMAKE_FIND_ROOT_PATH_MODE_PACKAGE,
CMAKE_FIND_ROOT_PATH_MODE_LIBRARY, and CMAKE_FIND_ROOT_PATH_MODE_INCLUDE in the
Dockerfile.android CMake invocation).

…OID_ARCH

Addresses remaining review hardening: verify the NDK zip before
extraction, and reject unknown ANDROID_ARCH values up front instead
of silently falling through to x86 flags.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
Dockerfile.android (1)

175-177: ⚠️ Potential issue | 🟠 Major

Don't let CMake fall back to Ubuntu host headers/libs here.

With these modes set to BOTH, any missing Android dependency can silently resolve from the builder image instead of the NDK sysroot or /icu. That makes the cross-build non-hermetic and can surface later as link/runtime mismatches.

🔒 Suggested change
     -DCMAKE_SYSTEM_PROCESSOR=${ANDROID_ARCH} \
     -DCMAKE_SYSROOT=${NDK_SYSROOT} \
+    -DCMAKE_FIND_ROOT_PATH="/icu;${NDK_SYSROOT}" \
     -DANDROID=ON \
@@
     -DICU_ROOT=/icu \
     -DICU_INCLUDE_DIR=/icu/include \
-    -DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=BOTH \
-    -DCMAKE_FIND_ROOT_PATH_MODE_LIBRARY=BOTH \
-    -DCMAKE_FIND_ROOT_PATH_MODE_INCLUDE=BOTH \
+    -DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ONLY \
+    -DCMAKE_FIND_ROOT_PATH_MODE_LIBRARY=ONLY \
+    -DCMAKE_FIND_ROOT_PATH_MODE_INCLUDE=ONLY \
For CMake cross-compilation, do CMAKE_FIND_ROOT_PATH_MODE_PACKAGE/LIBRARY/INCLUDE=BOTH allow host headers or libraries to be selected instead of the target sysroot? If target libraries live in /icu outside the sysroot, is the correct pattern to add /icu and the sysroot to CMAKE_FIND_ROOT_PATH and then switch those modes to ONLY?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Dockerfile.android` around lines 175 - 177, The CMake settings currently use
CMAKE_FIND_ROOT_PATH_MODE_PACKAGE/LIBRARY/INCLUDE=BOTH which allows falling back
to host headers/libs; change them to ONLY and ensure you populate
CMAKE_FIND_ROOT_PATH with the Android NDK sysroot and any external target-only
paths (e.g. /icu) so CMake searches the target sysroot and /icu first and never
silently picks host headers/libraries; update the CMake invocation that sets
CMAKE_FIND_ROOT_PATH and replace the three CMAKE_FIND_ROOT_PATH_MODE_* symbols
to ONLY accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Dockerfile.android`:
- Around line 57-70: The Dockerfile sets up runtime bits for both architectures
via the loop (for A in aarch64 x86_64) but only smoke-tests aarch64 at the end
(the echo ... --target=aarch64-unknown-linux-android${ANDROID_API} ... file
/tmp/t), so add a second smoke-test (or a small loop) that compiles and checks a
tiny binary for x86_64 as well (using
--target=x86_64-unknown-linux-android${ANDROID_API} and a separate temp file or
sequential checks) so the RES/NDK_RT symlink layout for x86_64 is verified
immediately after creating the links.

---

Duplicate comments:
In `@Dockerfile.android`:
- Around line 175-177: The CMake settings currently use
CMAKE_FIND_ROOT_PATH_MODE_PACKAGE/LIBRARY/INCLUDE=BOTH which allows falling back
to host headers/libs; change them to ONLY and ensure you populate
CMAKE_FIND_ROOT_PATH with the Android NDK sysroot and any external target-only
paths (e.g. /icu) so CMake searches the target sysroot and /icu first and never
silently picks host headers/libraries; update the CMake invocation that sets
CMAKE_FIND_ROOT_PATH and replace the three CMAKE_FIND_ROOT_PATH_MODE_* symbols
to ONLY accordingly.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: c9b5243d-67f3-46d1-abaf-c2021e05631c

📥 Commits

Reviewing files that changed from the base of the PR and between e9c2d9b and f94961e.

📒 Files selected for processing (2)
  • Dockerfile.android
  • android-release.sh

Comment thread Dockerfile.android
Comment on lines +57 to +70
RUN set -eux; \
RES=$($CC -print-resource-dir); \
NDK_RT=/opt/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/lib/clang/$(ls /opt/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/lib/clang/)/lib/linux; \
mkdir -p $RES/lib/linux; \
for A in aarch64 x86_64; do \
ln -sf $NDK_RT/libclang_rt.builtins-${A}-android.a $RES/lib/linux/; \
mkdir -p $RES/lib/linux/${A}; \
ln -sf $NDK_RT/${A}/libunwind.a $RES/lib/linux/${A}/; \
DIR=$RES/lib/${A}-unknown-linux-android${ANDROID_API}; \
mkdir -p $DIR; \
ln -sf $NDK_RT/libclang_rt.builtins-${A}-android.a $DIR/libclang_rt.builtins.a; \
ln -sf $NDK_RT/${A}/libunwind.a $DIR/libunwind.a; \
done; \
echo 'int main(){return 0;}' | $CC --target=aarch64-unknown-linux-android${ANDROID_API} --sysroot=$NDK_SYSROOT --rtlib=compiler-rt -fuse-ld=lld -xc - -o /tmp/t && file /tmp/t

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Smoke-test both supported Android triples.

This layer wires runtime bits for aarch64 and x86_64, but Line 70 only verifies the aarch64 toolchain path. A broken x86_64 resource-dir layout will fail much later in the ICU/WebKit stages.

🧪 Suggested change
-    echo 'int main(){return 0;}' | $CC --target=aarch64-unknown-linux-android${ANDROID_API} --sysroot=$NDK_SYSROOT --rtlib=compiler-rt -fuse-ld=lld -xc - -o /tmp/t && file /tmp/t
+    for A in aarch64 x86_64; do \
+      echo 'int main(){return 0;}' | $CC --target=${A}-unknown-linux-android${ANDROID_API} --sysroot=$NDK_SYSROOT --rtlib=compiler-rt -fuse-ld=lld -xc - -o /tmp/t-${A}; \
+      file /tmp/t-${A}; \
+    done
🧰 Tools
🪛 Hadolint (2.14.0)

[info] 57-57: Double quote to prevent globbing and word splitting.

(SC2086)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Dockerfile.android` around lines 57 - 70, The Dockerfile sets up runtime bits
for both architectures via the loop (for A in aarch64 x86_64) but only
smoke-tests aarch64 at the end (the echo ...
--target=aarch64-unknown-linux-android${ANDROID_API} ... file /tmp/t), so add a
second smoke-test (or a small loop) that compiles and checks a tiny binary for
x86_64 as well (using --target=x86_64-unknown-linux-android${ANDROID_API} and a
separate temp file or sequential checks) so the RES/NDK_RT symlink layout for
x86_64 is verified immediately after creating the links.

@github-actions

Copy link
Copy Markdown

Preview Builds

Commit Release Date
f94961ec autobuild-preview-pr-196-f94961ec 2026-04-24 06:16:23 UTC

@Jarred-Sumner
Jarred-Sumner merged commit a4ddad8 into main Apr 24, 2026
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant