[codex] Port remaining Android XR compatibility fixes#182
Open
matthargett wants to merge 3 commits into
Open
Conversation
matthargett
force-pushed
the
codex/android-xr-compat-remaining
branch
from
91cf1ad to
625af30
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the Android unit test harness and related dependencies to newer Android tooling (API 35 / newer NDK), improves Android-native logging, and adds compatibility for different V8 Inspector connect() signatures.
Changes:
- Bumped Android compile/target SDK to 35, updated NDK version handling, and improved ABI filter selection logic.
- Switched native test trace output to Logcat and updated Android emulator CI to API level 35.
- Updated JSC Android dependency and adjusted V8 Inspector agent for V8 version API differences.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/UnitTests/Android/app/src/main/cpp/JNI.cpp | Routes debug trace output to Android Logcat instead of stdout. |
| Tests/UnitTests/Android/app/build.gradle | Updates SDK/NDK configuration and refines ABI filter behavior. |
| README.md | Updates Android setup instructions to match new SDK/NDK/API level expectations. |
| Core/Node-API/package-jsc.json | Bumps jsc-android dependency version. |
| Core/AppRuntime/V8Inspector/Source/V8InspectorAgent.cpp | Adds V8 version-based connect() handling and adjusts string buffer creation. |
| .github/workflows/build-android.yml | Moves CI emulator image to API level 35. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Wrap the v8-version.h probe in `#if defined(__has_include)` so it's safe even on a preprocessor that doesn't provide __has_include (standard in C++17+, which this project uses, but the guarded form is the conventional portable idiom). When __has_include is unavailable, V8_MAJOR_VERSION stays undefined and ConnectFrontend() falls back to the 3-arg connect().
Document that the DebugTrace callback uses __android_log_print with a dedicated "JsRuntimeHost" tag rather than routing through AndroidExtensions StdoutLogger -- so connected-test diagnostics are filterable via `adb logcat -s JsRuntimeHost` instead of interleaving with all stdout under one tag.
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
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.
Summary
This ports the remaining relevant functional pieces from the closed Android XR compatibility work in #115 that are still not present on current
main.Changes included here:
250231.0.0to294992.0.0V8_MAJOR_VERSIONbasic_string<char16_t>touint16_t*inspector string-buffer path that newer toolchains rejectValidation
Ran gtest and JS unit tests:
Note
jsc-android is a dead package with a maintainer that's MIA. bumping to the latest (which is still 6 years old) helps, but I think the right longer-term move is to use bun's android builds of their JSC since they've already built a non-browser N-API v10+ compliant ecosystem around it.