fix(ci): correct Android release packaging for x86_64 emulators#426
fix(ci): correct Android release packaging for x86_64 emulators#426Siddhesh2377 wants to merge 2 commits intomainfrom
Conversation
… dirs The publish step in commons-release.yml used a glob pattern `jniLibs/*/` that never matched the actual build output directories (`jni/`, `commons/`, `onnx/`, `llamacpp/`, `rag/`). This caused incorrect or empty packaging of the RACommons Android ZIP, resulting in ARM64 libc++_shared.so being placed in the x86_64 folder — breaking x86_64 emulators. Fixes #425
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to a6fa2c8 in 13 seconds. Click for details.
- Reviewed
40lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_HfsbBlNplcpsu5G0
You can customize 
📝 WalkthroughWalkthroughUpdates to CI and SDK build scripts to collect and package Android native libraries across multiple artifact subdirectories and to enable multiple default ABIs for local builds; adds non-destructive copying and per-ABI reporting during release artifact assembly. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if [ -d "$abi_dir" ]; then | ||
| abi=$(basename "$abi_dir") | ||
| mkdir -p "android-combined/jniLibs/${abi}" | ||
| cp -n "${abi_dir}"*.so "android-combined/jniLibs/${abi}/" 2>/dev/null || true |
There was a problem hiding this comment.
cp -n silently skips existing files without logging. If duplicate .so names exist across subdirectories, only the first copy succeeds. Add -v flag for visibility into what's being copied vs skipped.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/commons-release.yml
Line: 405
Comment:
`cp -n` silently skips existing files without logging. If duplicate `.so` names exist across subdirectories, only the first copy succeeds. Add `-v` flag for visibility into what's being copied vs skipped.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| if [ ! -d "$artifact_dir" ]; then | ||
| continue | ||
| fi | ||
| for sub in jni commons onnx llamacpp rag; do |
There was a problem hiding this comment.
Consider using for sub in jni commons onnx llamacpp rag; do instead of listing them without quotes for better shell safety and readability.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/commons-release.yml
Line: 400
Comment:
Consider using `for sub in jni commons onnx llamacpp rag; do` instead of listing them without quotes for better shell safety and readability.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/commons-release.yml:
- Around line 410-417: Replace the current informational loop (iterating
android-combined/jniLibs/*/ using abi_dir, abi, count) with a validation gate
that (1) asserts the presence of all required ABI subdirs
["arm64-v8a","x86_64","armeabi-v7a","x86"] and fails the workflow if any are
missing or contain zero .so files, and (2) performs an ELF sanity check on key
libraries (e.g., x86_64/libc++_shared.so) using readelf/file to verify the ELF
Machine matches the ABI (EM_X86_64 for x86_64, etc.) and validates PT_LOAD
p_vaddr alignment meets 16KB page alignment; on any missing/incorrect ABI, ELF
machine mismatch, or alignment failure, emit an error message and exit non‑zero
so the job fails instead of continuing to zip.
- Line 405: The cp line is swallowing all errors via `2>/dev/null || true` and
hiding real failures when copying `${abi_dir}*.so`; replace this with a safe
existence check using shell nullglob: enable nullglob (shopt -s nullglob), build
an array like files=( "${abi_dir}"*.so ), if the array is empty skip the copy,
otherwise run cp -n "${files[@]}" "android-combined/jniLibs/${abi}/" and leave
failures to surface; refer to the existing cp command and the variables abi_dir
and abi when making the change.
| if [ -d "$abi_dir" ]; then | ||
| abi=$(basename "$abi_dir") | ||
| mkdir -p "android-combined/jniLibs/${abi}" | ||
| cp -n "${abi_dir}"*.so "android-combined/jniLibs/${abi}/" 2>/dev/null || true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd .github/workflows && wc -l commons-release.ymlRepository: RunanywhereAI/runanywhere-sdks
Length of output: 97
🏁 Script executed:
cat -n .github/workflows/commons-release.yml | sed -n '395,415p'Repository: RunanywhereAI/runanywhere-sdks
Length of output: 1191
🏁 Script executed:
head -20 .github/workflows/commons-release.ymlRepository: RunanywhereAI/runanywhere-sdks
Length of output: 632
🏁 Script executed:
rg "nullglob|set -o" .github/workflows/commons-release.ymlRepository: RunanywhereAI/runanywhere-sdks
Length of output: 56
🏁 Script executed:
grep -n "#!/bin/bash\|set -e\|set -u\|set -o" .github/workflows/commons-release.yml | head -20Repository: RunanywhereAI/runanywhere-sdks
Length of output: 56
🏁 Script executed:
rg "2>/dev/null" .github/workflows/commons-release.ymlRepository: RunanywhereAI/runanywhere-sdks
Length of output: 275
Line 405: avoid swallowing all cp failures during release packaging.
2>/dev/null || true hides real copy errors (not just "no matches"), so a broken ZIP can still be published. The glob pattern "${abi_dir}"*.so without nullglob will fail with "cannot stat '*.so'" when no files match, and this error is suppressed along with any actual copy failures. Only skip when no .so exists; fail on actual cp errors.
Suggested hardening
- cp -n "${abi_dir}"*.so "android-combined/jniLibs/${abi}/" 2>/dev/null || true
+ shopt -s nullglob
+ so_files=("${abi_dir}"*.so)
+ shopt -u nullglob
+ if [ ${`#so_files`[@]} -gt 0 ]; then
+ cp -n "${so_files[@]}" "android-combined/jniLibs/${abi}/"
+ 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.
| cp -n "${abi_dir}"*.so "android-combined/jniLibs/${abi}/" 2>/dev/null || true | |
| shopt -s nullglob | |
| so_files=("${abi_dir}"*.so) | |
| shopt -u nullglob | |
| if [ ${`#so_files`[@]} -gt 0 ]; then | |
| cp -n "${so_files[@]}" "android-combined/jniLibs/${abi}/" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/commons-release.yml at line 405, The cp line is swallowing
all errors via `2>/dev/null || true` and hiding real failures when copying
`${abi_dir}*.so`; replace this with a safe existence check using shell nullglob:
enable nullglob (shopt -s nullglob), build an array like files=(
"${abi_dir}"*.so ), if the array is empty skip the copy, otherwise run cp -n
"${files[@]}" "android-combined/jniLibs/${abi}/" and leave failures to surface;
refer to the existing cp command and the variables abi_dir and abi when making
the change.
| # Log what was collected per ABI | ||
| for abi_dir in android-combined/jniLibs/*/; do | ||
| if [ -d "$abi_dir" ]; then | ||
| abi=$(basename "$abi_dir") | ||
| count=$(ls "$abi_dir"/*.so 2>/dev/null | wc -l) | ||
| echo "Collected ${abi}: ${count} .so files" | ||
| ls -la "$abi_dir" 2>/dev/null | ||
| fi |
There was a problem hiding this comment.
Line 410-417 should enforce ABI correctness, not only log it.
This block reports counts but never fails on missing/empty ABI folders. Add required-ABI assertions (and ideally an ELF machine check for key libs like x86_64/libc++_shared.so) before zipping.
Suggested validation gate
- # Log what was collected per ABI
- for abi_dir in android-combined/jniLibs/*/; do
- if [ -d "$abi_dir" ]; then
- abi=$(basename "$abi_dir")
- count=$(ls "$abi_dir"/*.so 2>/dev/null | wc -l)
- echo "Collected ${abi}: ${count} .so files"
- ls -la "$abi_dir" 2>/dev/null
- fi
- done
+ # Validate required ABIs and collected libs
+ required_abis=(arm64-v8a armeabi-v7a x86_64 x86)
+ for abi in "${required_abis[@]}"; do
+ abi_dir="android-combined/jniLibs/${abi}"
+ if [ ! -d "$abi_dir" ]; then
+ echo "❌ Missing ABI directory: ${abi}" >&2
+ exit 1
+ fi
+ count=$(find "$abi_dir" -maxdepth 1 -name '*.so' | wc -l)
+ if [ "$count" -eq 0 ]; then
+ echo "❌ No .so files collected for ABI: ${abi}" >&2
+ exit 1
+ fi
+ echo "Collected ${abi}: ${count} .so files"
+ ls -la "$abi_dir"
+ done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/commons-release.yml around lines 410 - 417, Replace the
current informational loop (iterating android-combined/jniLibs/*/ using abi_dir,
abi, count) with a validation gate that (1) asserts the presence of all required
ABI subdirs ["arm64-v8a","x86_64","armeabi-v7a","x86"] and fails the workflow if
any are missing or contain zero .so files, and (2) performs an ELF sanity check
on key libraries (e.g., x86_64/libc++_shared.so) using readelf/file to verify
the ELF Machine matches the ABI (EM_X86_64 for x86_64, etc.) and validates
PT_LOAD p_vaddr alignment meets 16KB page alignment; on any missing/incorrect
ABI, ELF machine mismatch, or alignment failure, emit an error message and exit
non‑zero so the job fails instead of continuing to zip.
…ulator support Changes default ABIs from arm64-v8a only to arm64-v8a,armeabi-v7a,x86_64 so manual builds always include emulator binaries, preventing architecture mismatch in release packages. Fixes #425
|
@shubhammalhotra28 Fix Added |
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 (2)
sdk/runanywhere-commons/scripts/build-android.sh (2)
265-265:⚠️ Potential issue | 🟡 MinorStale default in error-path usage message.
The error message printed for unknown backends still shows the old single-ABI default, so users who hit this path get misleading guidance.
📝 Proposed fix
- echo " abis: comma-separated list (default: arm64-v8a)" + echo " abis: comma-separated list (default: arm64-v8a,armeabi-v7a,x86_64)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/runanywhere-commons/scripts/build-android.sh` at line 265, The usage message printed in the unknown-backend error path contains a stale default for ABIs—update the echo line that currently reads " abis: comma-separated list (default: arm64-v8a)" to reflect the current default ABI list (e.g., include arm64-v8a, armeabi-v7a or the actual comma-separated defaults used elsewhere); locate the echo invocation for the unknown backend usage text (the line printing " abis: comma-separated list (default: arm64-v8a)") and replace the default value string to match the real default configured in the script.
13-13:⚠️ Potential issue | 🟡 MinorStale comment: update the documented default ABI.
Line 13 still documents the old single-ABI default. It now diverges from the actual default set at line 198.
📝 Proposed fix
-# abis: comma-separated list (default: arm64-v8a) +# abis: comma-separated list (default: arm64-v8a,armeabi-v7a,x86_64)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/runanywhere-commons/scripts/build-android.sh` at line 13, The inline help comment "# abis: comma-separated list (default: arm64-v8a)" is stale; update that comment to match the actual default ABIs the script assigns later (the default ABIs variable/assignment around the script's default ABIS section), e.g. replace the old single-ABI note with the exact comma-separated default used by the script so the in-file documentation and the actual default remain consistent.
🧹 Nitpick comments (2)
sdk/runanywhere-commons/scripts/build-android.sh (2)
805-830: Considercp -nhere to stay consistent with the no-clobber strategy applied in the CI workflow.The PR applies
cp -nincommons-release.ymlto prevent cross-ABI overwrites. In this local packaging loop (lines 805–830 and the equivalent ONNX/LlamaCPP/WhisperCPP loops below), multiple source directories (jni/,onnx/,llamacpp/, …) are merged into a single per-ABI temp dir with plaincp, so same-named files likelibc++_shared.soandlibomp.soare silently overwritten by the last source that contains them. Since all sources are already partitioned by$ABI, the copies are architecturally safe—but adoptingcp -nhere would make the intent explicit and guard against any future source ordering changes.♻️ Proposed change (unified loop; apply the same pattern to ONNX/LlamaCPP/WhisperCPP loops)
- if [ -d "${JNI_DIST_DIR}/${ABI}" ]; then - cp "${JNI_DIST_DIR}/${ABI}"/*.so "${UNIFIED_TEMP}/${ABI}/" 2>/dev/null || true + if [ -d "${JNI_DIST_DIR}/${ABI}" ]; then + cp -n "${JNI_DIST_DIR}/${ABI}"/*.so "${UNIFIED_TEMP}/${ABI}/" 2>/dev/null || true fi - if [ -d "${DIST_DIR}/onnx/${ABI}" ]; then - cp "${DIST_DIR}/onnx/${ABI}"/*.so "${UNIFIED_TEMP}/${ABI}/" 2>/dev/null || true + if [ -d "${DIST_DIR}/onnx/${ABI}" ]; then + cp -n "${DIST_DIR}/onnx/${ABI}"/*.so "${UNIFIED_TEMP}/${ABI}/" 2>/dev/null || true fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/runanywhere-commons/scripts/build-android.sh` around lines 805 - 830, Replace the plain cp calls in the packaging loop that copy into "${UNIFIED_TEMP}/${ABI}/" with no-clobber copies so same-named .so files from different sources don't overwrite each other; specifically update the cp invocations that reference "${JNI_DIST_DIR}/${ABI}" and each "${DIST_DIR}/onnx/${ABI}", "${DIST_DIR}/llamacpp/${ABI}", "${DIST_DIR}/whispercpp/${ABI}", and "${DIST_DIR}/tflite/${ABI}" to use the -n (no-clobber) flag (keeping the existing redirection/exit-suppression behavior), and apply the same change to any equivalent ONNX/LlamaCPP/WhisperCPP/TFLite loops elsewhere to make the intent explicit and consistent.
198-198: Advisory: the new default triples local build time.Previously
./build-android.shbuilt one ABI (~4 min); it now builds three (~10–12 min). The header comment at line 27–29 still advertises the single-ABI quick-start as./build-android.sh, which is no longer accurate. Consider updating the "Quick start" example or adding a note that bare invocation now covers emulators too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/runanywhere-commons/scripts/build-android.sh` at line 198, The header comment in build-android.sh is stale: the default ABIS variable (ABIS="${2:-arm64-v8a,armeabi-v7a,x86_64}") now builds three ABIs by default, but the "Quick start" example still claims a single-ABI quick build using ./build-android.sh; update that header to either (a) state the new default builds multiple ABIs and will take ~10–12 minutes and include emulators, or (b) show both examples — a quick single-ABI invocation (e.g., ./build-android.sh arm64-v8a) and the new multi-ABI default (./build-android.sh) — and add a short note that the default includes emulator ABIs. Ensure the comment references the ABIS default and the script name (build-android.sh) so readers understand how to override the behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@sdk/runanywhere-commons/scripts/build-android.sh`:
- Line 265: The usage message printed in the unknown-backend error path contains
a stale default for ABIs—update the echo line that currently reads " abis:
comma-separated list (default: arm64-v8a)" to reflect the current default ABI
list (e.g., include arm64-v8a, armeabi-v7a or the actual comma-separated
defaults used elsewhere); locate the echo invocation for the unknown backend
usage text (the line printing " abis: comma-separated list (default:
arm64-v8a)") and replace the default value string to match the real default
configured in the script.
- Line 13: The inline help comment "# abis: comma-separated list
(default: arm64-v8a)" is stale; update that comment to match the actual default
ABIs the script assigns later (the default ABIs variable/assignment around the
script's default ABIS section), e.g. replace the old single-ABI note with the
exact comma-separated default used by the script so the in-file documentation
and the actual default remain consistent.
---
Nitpick comments:
In `@sdk/runanywhere-commons/scripts/build-android.sh`:
- Around line 805-830: Replace the plain cp calls in the packaging loop that
copy into "${UNIFIED_TEMP}/${ABI}/" with no-clobber copies so same-named .so
files from different sources don't overwrite each other; specifically update the
cp invocations that reference "${JNI_DIST_DIR}/${ABI}" and each
"${DIST_DIR}/onnx/${ABI}", "${DIST_DIR}/llamacpp/${ABI}",
"${DIST_DIR}/whispercpp/${ABI}", and "${DIST_DIR}/tflite/${ABI}" to use the -n
(no-clobber) flag (keeping the existing redirection/exit-suppression behavior),
and apply the same change to any equivalent ONNX/LlamaCPP/WhisperCPP/TFLite
loops elsewhere to make the intent explicit and consistent.
- Line 198: The header comment in build-android.sh is stale: the default ABIS
variable (ABIS="${2:-arm64-v8a,armeabi-v7a,x86_64}") now builds three ABIs by
default, but the "Quick start" example still claims a single-ABI quick build
using ./build-android.sh; update that header to either (a) state the new default
builds multiple ABIs and will take ~10–12 minutes and include emulators, or (b)
show both examples — a quick single-ABI invocation (e.g., ./build-android.sh
arm64-v8a) and the new multi-ABI default (./build-android.sh) — and add a short
note that the default includes emulator ABIs. Ensure the comment references the
ABIS default and the script name (build-android.sh) so readers understand how to
override the behavior.
|
@shubhammalhotra28 or @sanchitmonga22 can anyone merge this ? |
1 participant
Summary
commons-release.ymlpublish step that was looking for a non-existentjniLibs/directory instead of the actual build output directories (jni/,commons/,onnx/,llamacpp/,rag/)libc++_shared.soto be placed in thex86_64/folder ofRACommons-android-v0.1.6.zip, crashing Flutter apps on x86_64 emulatorscp -n(no-clobber) to prevent cross-architecture overwrites when the same.soexists in multiple subdirectoriesFixes #425
Test plan
commons-releaseworkflow run (dry run) and verify CI logs show correct per-ABI.sofile countsRACommons-android-v{VERSION}.zipand verifyx86_64/libc++_shared.sois actually x86_64 (readelf -hshould showEM_X86_64)Important
Fixes Android release packaging in
commons-release.ymlby correcting directory paths and preventing cross-architecture file overwrites.commons-release.ymlto correctly targetjni/,commons/,onnx/,llamacpp/,rag/directories instead of non-existentjniLibs/.cp -nto prevent overwriting.sofiles across different architectures.libc++_shared.soinx86_64/folder, preventing crashes on x86_64 emulators..sofile counts in CI logs.x86_64/libc++_shared.sois x86_64 usingreadelf -h.This description was created by
for a6fa2c8. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Greptile Summary
Fixed the Android release packaging by correcting the glob pattern to match the actual build output directory structure. The previous code looked for a non-existent
jniLibs/directory, causing it to fail silently and resulting in ARM64 libraries being placed in thex86_64/folder. The fix iterates through the correct subdirectories (jni/,commons/,onnx/,llamacpp/,rag/) and usescp -n(no-clobber) to prevent architecture contamination when the same.sofile exists across multiple ABIs.Key Changes:
jni,commons,onnx,llamacpp,ragsubdirectoriescp -nflag to prevent overwrites and ensure correct per-ABI.sofiles.sofile counts and file listingsConfidence Score: 4/5
cp -nprevents overwrites, and improved logging aids verification. Score of 4 (not 5) due to the complexity of the nested loops and the need for thorough testing with actual CI runs to verify all ABIs are packaged correctly.Important Files Changed
.sofiles from correct build output directories (jni/,commons/,onnx/,llamacpp/,rag/) instead of non-existentjniLibs/, and addedcp -nto prevent cross-architecture overwritesLast reviewed commit: a6fa2c8