draft: test android build #31442
draft: test android build #31442
Conversation
WalkthroughAdds a GitHub Actions workflow Pre-merge checks and finishing touches❌ Failed checks (2 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/android-build.yml (1)
46-46: Consider replacing third-party actions with official GitHub Actions.This workflow relies on three non-official third-party actions:
cirruslabs/cache,dsherret/rust-toolchain-file, andnttld/setup-ndk. While these are relatively established, they introduce supply chain risk. Since this is a draft, it's acceptable for now, but before merging:
- Replace
cirruslabs/cache@v4with the officialactions/cache@v4- Consider
dtolnay/rust-toolchain(widely adopted) instead ofdsherret/rust-toolchain-file@v1- The NDK setup action is reasonable, but verify it's actively maintained
Also applies to: 58-58, 61-61
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/android-build.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: bench release linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: build libs
- GitHub Check: build android x86_64
- GitHub Check: build android aarch64
🔇 Additional comments (3)
.github/workflows/android-build.yml (3)
71-74: Verify hardcoded linker paths resolve correctly.The linker paths are hardcoded with the assumption that
nttld/setup-ndk@v1setsANDROID_NDK_HOMEand that the subdirectory structure matches exactly. Confirm that:
ANDROID_NDK_HOMEis correctly exported by the NDK setup action- The NDK r27c release includes these exact paths:
toolchains/llvm/prebuilt/linux-x86_64/bin/aarch64-linux-android24-clangandx86_64-linux-android24-clang
69-74: Clarify build configuration: debug-only vs. release.The workflow builds only debug targets (
--lockedwithout--release). For Android, release builds are typically preferred for distribution. Clarify the intent:
- If this is purely for validation/testing, debug is fine.
- If this should produce production artifacts, add a release build variant (either as a separate matrix value or additional step).
1-78: Overall structure looks sound for Android cross-compilation testing.The workflow correctly:
- Sets up concurrency cancellation to avoid redundant builds
- Configures git and clones submodules appropriately
- Installs the NDK and Rust targets
- Caches Cargo artifacts for faster rebuilds
- Verifies the binary exists as a sanity check
For a draft PR exploring Android build automation, this is a good foundation. Address the supply chain and verification concerns above before production use.
Set up Android build environment variables and update linker paths.
Added installation steps for build dependencies and GN.
Added step to install Deno in the Android build workflow.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/android-build.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test release linux-aarch64
- GitHub Check: bench release linux-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test release macos-aarch64
- GitHub Check: test debug macos-x86_64
- GitHub Check: test release macos-x86_64
- GitHub Check: build libs
🔇 Additional comments (4)
.github/workflows/android-build.yml (4)
118-122: Clarify the debug build choice.The cargo build (line 118) doesn't specify
--release, so it produces a debug binary. This is slower to build and produces a larger binary. If this is intentional for testing purposes, add a comment explaining why. If it should be a release build, add the flag.Is the debug build intentional? If so, consider adding a comment:
- run: | + run: | + # Building debug binary for testing (not optimized for production) cargo build --target ${{ matrix.arch }}-linux-android --lockedOr if it should be release:
- cargo build --target ${{ matrix.arch }}-linux-android --locked + cargo build --target ${{ matrix.arch }}-linux-android --locked --release
1-28: Overall workflow structure looks solid.The workflow follows reasonable patterns: sensible caching strategy (Cargo home), matrix strategy for multiple architectures, selective submodule cloning, proper concurrency handling, and clear job naming. Good foundational CI/CD setup.
79-97: Unfortunately, I'm unable to directly verify the concern due to persistent repository access issues and insufficient public documentation about the specific implementation ofinstall_prebuilt.jsin the denoland/deno repository.Given these limitations, here is the rewritten review comment:
Verify the gn installation output path before merging.
Line 83 runs
./tools/install_prebuilt.js gn, then line 97 assumes the binary was placed at$GITHUB_WORKSPACE/third_party/prebuilt/linux64. Confirm thatinstall_prebuilt.jsoutputs gn to this exact location; if the script places it elsewhere or fails silently, the PATH modification will be ineffective and the build will fail later with a confusing error about missing gn.
60-63: Based on my web search verification, I can now provide a rewritten review comment:
Deno v2.x has no official prebuilt Android binaries; verify if the workflow requires Deno to run on Android or if it's only used as a build tool on Linux/macOS.
Deno v2.x official prebuilt binaries are only available for macOS (x64, arm64), Linux (x64), and Windows (x64). Android is not included in the official distribution. While Deno can theoretically be built for Android using Android NDK, gn, and ninja, this remains community-driven and experimental with no official support or maintenance. If this workflow is intended to run on Android or execute Deno on an Android device, the
setup-deno@v2action withdeno-version: v2.xwill fail. Clarify whether Deno runs on the build system (Linux/macOS) as a build tool, or whether it's expected to execute on Android.
.github/workflows/android-build.yml
Outdated
| # Build V8 from source since prebuilt binaries don't exist for Android | ||
| V8_FROM_SOURCE: 1 | ||
| # Set API level | ||
| ANDROID_NDK_API_LEVEL: '24' |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract the hardcoded API level to avoid duplication.
The Android NDK API level (24) is hardcoded in multiple places: environment variable (line 104), linker names (lines 106-107, 109-114), and GN args (line 116). If this needs to change, it requires updates in 5+ locations, increasing the risk of inconsistency.
Consider centralizing this as a workflow variable or a job-level env var at the top of the job, then reference it:
steps:
+ - name: Set Android API level
+ run: |
+ echo "ANDROID_API_LEVEL=24" >> $GITHUB_ENV
+
- name: Configure gitThen reference ${{ env.ANDROID_API_LEVEL }} in the linker names:
CARGO_TARGET_AARCH64_LINUX_ANDROID_LINKER: aarch64-linux-android24-clang
- CARGO_TARGET_X86_64_LINUX_ANDROID_LINKER: x86_64-linux-android24-clang
- # Compilers for build scripts
- CC_aarch64_linux_android: aarch64-linux-android24-clang
- CXX_aarch64_linux_android: aarch64-linux-android24-clang++
- AR_aarch64_linux_android: llvm-ar
- CC_x86_64_linux_android: x86_64-linux-android24-clang
- CXX_x86_64_linux_android: x86_64-linux-android24-clang++
+ CARGO_TARGET_AARCH64_LINUX_ANDROID_LINKER: aarch64-linux-android${{ env.ANDROID_API_LEVEL }}-clang
+ CARGO_TARGET_X86_64_LINUX_ANDROID_LINKER: x86_64-linux-android${{ env.ANDROID_API_LEVEL }}-clang
+ # Compilers for build scripts
+ CC_aarch64_linux_android: aarch64-linux-android${{ env.ANDROID_API_LEVEL }}-clang
+ CXX_aarch64_linux_android: aarch64-linux-android${{ env.ANDROID_API_LEVEL }}-clang++
+ AR_aarch64_linux_android: llvm-ar
+ CC_x86_64_linux_android: x86_64-linux-android${{ env.ANDROID_API_LEVEL }}-clang
+ CXX_x86_64_linux_android: x86_64-linux-android${{ env.ANDROID_API_LEVEL }}-clang++And update line 116:
- EXTRA_GN_ARGS: 'android_ndk_api_level=24 use_jumbo_build=true'
+ EXTRA_GN_ARGS: 'android_ndk_api_level=${{ env.ANDROID_API_LEVEL }} use_jumbo_build=true'Also applies to: 106-107, 109-114, 116-116
Updated Android build workflow to increase timeout and simplify GN arguments.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/android-build.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test release linux-aarch64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: bench release linux-x86_64
- GitHub Check: test release macos-aarch64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug macos-x86_64
- GitHub Check: build libs
- GitHub Check: build android aarch64
- GitHub Check: build android x86_64
.github/workflows/android-build.yml
Outdated
| - name: Install cargo-ndk | ||
| run: cargo install cargo-ndk | ||
|
|
||
| - name: Check compilation (without V8) | ||
| run: | | ||
| # We use `cargo ndk` to wrap the check command. | ||
| # It sets CC, CXX, and PATH so aws-lc-sys can find the NDK compiler. | ||
| cargo ndk -t ${{ matrix.arch }}-linux-android check --target ${{ matrix.arch }}-linux-android --locked --workspace --exclude v8 |
There was a problem hiding this comment.
Android NDK not installed—workflow will likely fail at build step.
The workflow installs cargo-ndk (line 66), but cargo-ndk is a wrapper that requires the Android NDK toolchain itself to be available. Without an explicit NDK installation step, the cargo ndk command on line 72 will fail trying to locate NDK compilers and tools.
Add an Android NDK setup step before running cargo ndk. Apply this diff to install the NDK:
- uses: dsherret/rust-toolchain-file@v1
+ - name: Install Android NDK
+ uses: nttld/setup-ndk@v1
+ with:
+ ndk-version: r27
+ add-to-path: true
+
- name: Add Android targetsAlternatively, if cargo-ndk has built-in NDK auto-download capability, explicitly configure it via the ANDROID_NDK_HOME or ANDROID_HOME environment variable to point to where it should download/locate the NDK.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .github/workflows/android-build.yml around lines 65 to 72, the workflow
installs cargo-ndk but does not ensure the Android NDK toolchain is present, so
the subsequent `cargo ndk` invocation will fail; add a step before running
`cargo ndk` to install or set up the Android NDK (for example use
actions/setup-android or download and extract the NDK and set
ANDROID_NDK_HOME/ANDROID_HOME), or configure cargo-ndk to auto-download by
exporting ANDROID_NDK_HOME/ANDROID_HOME to the intended path; ensure the NDK
install step runs before the cargo-ndk check and that environment variables
point to the installed NDK.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/android-build.yml (1)
145-159: Android NDK toolchain must be installed before building for Android targets.The workflow attempts to build rusty_v8 (and later Deno) for
aarch64-linux-androidandx86_64-linux-androidtargets, but the Android NDK is never installed. Without the NDK, the Rust compiler will fail to find the required C/C++ cross-compilation tools (clang, ar, linker, etc.).Add an NDK installation step before the rusty_v8 build:
- name: Add Android targets run: | rustup target add ${{ matrix.arch }}-linux-android + - name: Install Android NDK + uses: nttld/setup-ndk@v1 + with: + ndk-version: r27 + add-to-path: true + - name: Install build dependencies
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/android-build.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/android-build.yml
110-110: could not parse as YAML: could not find expected ':'
(syntax-check)
🪛 YAMLlint (1.37.1)
.github/workflows/android-build.yml
[error] 111-111: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test release linux-aarch64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug macos-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: build libs
🔇 Additional comments (3)
.github/workflows/android-build.yml (3)
1-28: Workflow trigger and matrix configuration look good.The concurrency settings and matrix strategy are correctly set up for parallel per-architecture builds.
29-86: Repository setup, caching, and patch download look reasonable.
160-176: Build and verification steps look good.The target-specific environment variable setup and binary verification are correctly implemented. This section will work once the Android NDK is installed (see prior issue).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/android-build.yml (1)
65-67: Android NDK toolchain is not installed—build will fail at compilation.Line 65-67 only adds Rust targets via
rustup target add, which provides the target specification but not the actual NDK toolchain (compilers, linker, sysroot). Without the NDK installed and available inPATH, the subsequentcargo buildsteps (lines 175, 192) will fail looking for compilers likeaarch64-linux-android24-clang.Add an NDK installation step before building:
- uses: dsherret/rust-toolchain-file@v1 + + - name: Install Android NDK + uses: nttld/setup-ndk@v1 + with: + ndk-version: r27 + add-to-path: true - name: Add Android targetsAlternatively, configure the NDK path explicitly via
ANDROID_NDK_HOMEorANDROID_HOMEenvironment variables if using a different setup method.
🧹 Nitpick comments (1)
.github/workflows/android-build.yml (1)
170-170: Centralize the hardcoded Android API level to reduce duplication.The API level
24appears in line 170 and is also baked into the linker names in the env vars suggested above. This creates maintenance burden if the API level needs to change.Define the API level as a workflow variable at the top of the job and reference it throughout:
steps: + - name: Set Android configuration + run: | + echo "ANDROID_API_LEVEL=24" >> $GITHUB_ENV - name: Configure gitThen use
${{ env.ANDROID_API_LEVEL }}in the env vars:env: ... EXTRA_GN_ARGS: 'android_ndk_api_level=${{ env.ANDROID_API_LEVEL }}' CARGO_TARGET_AARCH64_LINUX_ANDROID_LINKER: aarch64-linux-android${{ env.ANDROID_API_LEVEL }}-clang CARGO_TARGET_X86_64_LINUX_ANDROID_LINKER: x86_64-linux-android${{ env.ANDROID_API_LEVEL }}-clang
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/android-build.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug windows-x86_64
- GitHub Check: build libs
- GitHub Check: build android x86_64
- GitHub Check: build android aarch64
.github/workflows/android-build.yml
Outdated
| - name: Build rusty_v8 separately | ||
| env: | ||
| V8_FROM_SOURCE: 1 | ||
| PYTHON: python3 | ||
| # Don't use jumbo build if patches didn't apply | ||
| EXTRA_GN_ARGS: 'android_ndk_api_level=24' | ||
| run: | | ||
| cd /tmp/rusty_v8 | ||
|
|
||
| # Build rusty_v8 for Android | ||
| cargo build --release --target ${{ matrix.arch }}-linux-android -vv | ||
|
|
||
| # Save the built library location | ||
| echo "RUSTY_V8_ARCHIVE=/tmp/rusty_v8/target/${{ matrix.arch }}-linux-android/release/gn_out/obj/librusty_v8.a" >> $GITHUB_ENV | ||
| echo "RUSTY_V8_SRC_BINDING_PATH=/tmp/rusty_v8/target/${{ matrix.arch }}-linux-android/release/gn_out/src_binding.rs" >> $GITHUB_ENV | ||
|
|
||
| - name: Build Deno using prebuilt V8 | ||
| env: | ||
| RUSTY_V8_ARCHIVE: ${{ env.RUSTY_V8_ARCHIVE }} | ||
| RUSTY_V8_SRC_BINDING_PATH: ${{ env.RUSTY_V8_SRC_BINDING_PATH }} | ||
| run: | | ||
| # Set environment variables with target suffix (required by patched rusty_v8) | ||
| TARGET_UPPER=$(echo "${{ matrix.arch }}-linux-android" | tr '[:lower:]' '[:upper:]' | tr '-' '_') | ||
| export "RUSTY_V8_ARCHIVE_${TARGET_UPPER}=${RUSTY_V8_ARCHIVE}" | ||
| export "RUSTY_V8_SRC_BINDING_PATH_${TARGET_UPPER}=${RUSTY_V8_SRC_BINDING_PATH}" | ||
|
|
||
| # Build Deno | ||
| cargo build --target ${{ matrix.arch }}-linux-android --locked |
There was a problem hiding this comment.
Missing Cargo environment variables for Android cross-compilation.
The build steps (lines 175, 192) invoke cargo build --target ${{ matrix.arch }}-linux-android without setting required Cargo environment variables for the Android linker and compilers. This will cause linker errors during the build.
Add a step to configure Cargo environment variables before building rusty_v8 and Deno:
- name: Build rusty_v8 separately
env:
V8_FROM_SOURCE: 1
PYTHON: python3
# Don't use jumbo build if patches didn't apply
EXTRA_GN_ARGS: 'android_ndk_api_level=24'
+ CARGO_TARGET_AARCH64_LINUX_ANDROID_LINKER: aarch64-linux-android24-clang
+ CARGO_TARGET_X86_64_LINUX_ANDROID_LINKER: x86_64-linux-android24-clang
+ CC_aarch64_linux_android: aarch64-linux-android24-clang
+ CXX_aarch64_linux_android: aarch64-linux-android24-clang++
+ AR_aarch64_linux_android: llvm-ar
+ CC_x86_64_linux_android: x86_64-linux-android24-clang
+ CXX_x86_64_linux_android: x86_64-linux-android24-clang++
+ AR_x86_64_linux_android: llvm-ar
run: |Then apply the same env vars to the "Build Deno using prebuilt V8" step (lines 181-184).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
.github/workflows/android-build.yml lines 165-192: before invoking cargo build
for both the "Build rusty_v8 separately" and "Build Deno using prebuilt V8"
steps, set the required Cargo/compilation environment variables for the Android
target (use the target triple variable): export
CARGO_TARGET_${TARGET_UPPER}_LINKER to the Android NDK clang wrapper, set CC and
CXX to the corresponding clang/clang++ from ANDROID_NDK_HOME (or
ANDROID_NDK_ROOT), set AR to the ndk ar (or llvm-ar), and export any needed
cargo target vars such as CARGO_TARGET_${TARGET_UPPER}_AR and
CARGO_TARGET_${TARGET_UPPER}_RUNNER if applicable; apply the same exports in
both build steps (using the TARGET_UPPER derivation already present for the Deno
step) so the linker/compiler toolchain is correctly used during
cross-compilation.
|
TLDR not easy, here is a summary by Claude Summary: Building Deno for AndroidKey Findings
Building V8 from source for Android in GitHub Actions hits multiple issues:
Each fix reveals another layer of complexity RecommendationFor CI to catch Android-incompatible code:
For production Android builds:
The fundamental issue is that Android is not a first-class target for Deno/rusty_v8 yet, so it requires significant external infrastructure to build. |
|
But at least termux have automatic detection of deno builds failing, so that might be good enough |
1 participant
just testing if this is easy to add