feat: add two last android targets #154
Merged
Conversation
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
WalkthroughThe changes introduce support for additional Android architectures by updating build targets and workflows. A new optional Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow
participant pnpmAction
participant BuildSystem
Workflow->>pnpmAction: Invoke with cpu input (optional)
pnpmAction->>BuildSystem: Set CPU_ARCH env using cpu input
BuildSystem->>BuildSystem: Build for specified architecture/target
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
Adds Android build target support to the CLI package and CI workflows, enabling aarch64-linux-android and armv7-linux-androideabi builds.
- Introduce two new Android targets in
package.json - Extend GitHub Actions matrix to build those Android targets
- Update the composite pnpm action to accept a
cpuinput
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| package.json | Added new Android targets aarch64-linux-android and armv7-linux-androideabi |
| .github/workflows/release-napi.yml | Extended the build matrix for Android targets and passed a cpu variable |
| .github/actions/pnpm/action.yml | Added optional cpu input and a conditional step to configure pnpm CPU settings |
Comments suppressed due to low confidence (3)
.github/workflows/release-napi.yml:67
- The matrix entry for
aarch64-linux-androidis missing anarchitecturefield. Specify the correct host architecture (e.g.,arm64) so caching and build steps receive the proper input.
- target: aarch64-linux-android
.github/workflows/release-napi.yml:81
- The
architectureforarmv7-linux-androideabiis set tox86, which doesn't match the ARM target. It should be set to an ARM architecture (e.g.,arm).
architecture: x86
.github/actions/pnpm/action.yml:24
- Make the
ifcondition more explicit (e.g.,${{ inputs.cpu != "" }}) and verify YAML indentation so the composite action step is parsed correctly and only runs whencpuis provided.
if: ${{ inputs.cpu }}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to bc63937 in 1 minute and 48 seconds. Click for details.
- Reviewed
82lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4draft 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.
1. .github/actions/pnpm/action.yml:9
- Draft comment:
Added a new 'cpu' input. Consider documenting its intended usage and any expected default value for clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. .github/workflows/release-napi.yml:80
- Draft comment:
The matrix row for 'armv7-linux-androideabi' sets architecture to 'x86'. Verify that this is intentional for an Android armv7 target. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This appears to be asking the author to verify their intention, which violates our rules. Additionally, cross-compilation setups often use x86 hosts to build for ARM targets, so this configuration could be completely valid. Without deep knowledge of Android NDK and cross-compilation setup, we can't be certain this is actually an error. The architecture mismatch could genuinely be a mistake that would cause build failures. Android builds have specific requirements that may make this configuration problematic. While there could be issues, the comment only asks for verification rather than pointing out a specific problem. If this was an actual error, it would fail during builds. The configuration may be intentional for cross-compilation. Delete this comment as it only asks for verification without providing evidence of an actual problem, and build failures would catch real architecture mismatches.
3. package.json:54
- Draft comment:
New Android targets 'aarch64-linux-android' and 'armv7-linux-androideabi' have been added. Confirm that these targets are fully supported in your build and test setups. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. .github/actions/pnpm/action.yml:10
- Draft comment:
Typographical suggestion: Consider capitalizing 'cpu' to 'CPU' for consistency and adherence to common naming conventions (e.g., 'The CPU architecture to use'). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is purely a stylistic suggestion about documentation. It doesn't affect functionality. While technically correct that "CPU" is a common capitalization, this kind of minor documentation nitpick doesn't warrant a PR comment. The lowercase version is perfectly readable and understood. The capitalization of "CPU" is indeed a widely accepted convention in technical documentation. The change would make the documentation more professional. While technically correct, this is too minor of an issue to warrant a PR comment. We should focus on substantive changes that affect code quality or functionality. Delete this comment as it's too minor and purely stylistic. It doesn't affect code quality or functionality.
Workflow ID: wflow_o1E8N9sC7ZYM0w1v
You can customize 
bc63937 to
a6863be
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed a6863be in 1 minute and 47 seconds. Click for details.
- Reviewed
82lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3draft 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.
1. .github/actions/pnpm/action.yml:24
- Draft comment:
Consider explicitly checking that 'inputs.cpu' is non-empty (e.g. using${{ inputs.cpu != '' }}) to avoid accidental misconfiguration if an empty string is passed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. .github/workflows/release-napi.yml:80
- Draft comment:
For target 'armv7-linux-androideabi', the 'architecture' is set to 'x86', which seems inconsistent with an ARM target. Please verify if the correct architecture and CPU values are provided. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a complex build configuration for cross-platform builds. The fact that an ARM target has x86 architecture seems counterintuitive, but in cross-compilation scenarios, the build architecture can be different from the target architecture. Without deep knowledge of the Android NDK and build system requirements, I cannot be certain that x86 is incorrect here. I might be missing specific Android NDK build requirements. The original author may have intentionally set x86 for the build environment while targeting ARM. Given the complexity of cross-compilation setups and without clear evidence that x86 is definitely wrong here, I should err on the side of assuming the author knows their build requirements. Delete the comment since we don't have strong evidence that the x86 architecture setting is incorrect for this build configuration.
3. package.json:61
- Draft comment:
New Android targets ('aarch64-linux-android' and 'armv7-linux-androideabi') are added. Ensure that the build system and environment dependencies are properly configured to support these targets. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_aNACtymqEotvCoKO
You can customize
a6863be to
8481d8e
Compare
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 8481d8e in 2 minutes and 8 seconds. Click for details.
- Reviewed
84lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3draft 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.
1. .github/actions/pnpm/action.yml:24
- Draft comment:
Consider providing a default or fallback check for the CPU architecture. Previously the condition checked if architecture=='x86'; now it runs only when 'cpu' is set. Ensure this change matches the intended behavior. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The change appears intentional - moving from a hardcoded x86->ia32 mapping to a flexible cpu architecture input. Making it optional (only run when cpu is set) seems like a valid design choice. The comment is essentially asking to verify the author's intention, which violates our rules. There's no clear evidence that a fallback is required. Maybe there are edge cases where certain CPU architectures need special handling that we're not aware of? The previous code specifically handled x86, suggesting there was a reason for it. Without strong evidence that specific CPU architectures require special handling, we should trust that the author's more flexible approach is valid. The old code's x86 handling could have been overly specific. Delete the comment. It's asking for verification of intention and suggesting speculative changes without clear evidence of necessity.
2. .github/workflows/release-napi.yml:80
- Draft comment:
Verify that using 'architecture: x86' for the 'armv7-linux-androideabi' target is intentional, as the target name implies an ARM build. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This comment violates the rule about asking for verification or confirmation ("Verify that..."). Additionally, cross-compilation setups can be complex and may have valid reasons for using seemingly mismatched architectures. Without deep knowledge of the Android NDK and cross-compilation toolchain, we can't be certain this is actually incorrect. The architecture mismatch could be a genuine issue that causes build failures. The comment points out something that seems logically inconsistent. However, the comment is phrased as a verification request rather than pointing out a definite issue. Cross-compilation toolchains often have non-obvious requirements that may explain this setup. Delete the comment because it violates the rule against asking for verification, and we don't have strong evidence that the architecture setting is actually incorrect.
3. package.json:55
- Draft comment:
Ensure that the addition of Android targets ('aarch64-linux-android' and 'armv7-linux-androideabi') doesn’t require extra configuration in build scripts or workflows. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_daz040sHkRGucMuG
You can customize
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
package.json (1)
54-74: Consider organizing the targets list.
As thenapi.targetsarray grows, grouping by platform or sorting alphabetically improves readability and maintainability..github/actions/pnpm/action.yml (1)
24-28:⚠️ Potential issueValidate
cpuinput to prevent injection.
Unvalidatedinputs.cpucould introduce arbitrary shell commands. Add a whitelist check before setting it:- if: ${{ inputs.cpu }} shell: bash - run: pnpm config set supportedArchitectures.cpu "${CPU_ARCH}" + run: | + case "${CPU_ARCH}" in + ia32|x64|arm64) ;; + *) echo "Invalid CPU arch: ${CPU_ARCH}" && exit 1 ;; + esac + pnpm config set supportedArchitectures.cpu "${CPU_ARCH}" env: CPU_ARCH: ${{ inputs.cpu }}
🧹 Nitpick comments (1)
.github/actions/pnpm/action.yml (1)
9-11: Add type and default forcpuinput.
To enhance schema clarity, specifytype: stringand include adefault(e.g.,default: "") under thecpuinput.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/actions/pnpm/action.yml(2 hunks).github/workflows/release-napi.yml(3 hunks)package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
package.json (1)
61-64:❓ Verification inconclusive
Verify Android target validity for Rust builds.
Ensure that the newly added targetsaarch64-linux-androidandarmv7-linux-androideabiare recognized by Rust’s supported target list before proceeding.
🏁 Script executed:
#!/bin/bash # Confirm Rust recognizes the new Android NAPI targets rustup target list | grep "^aarch64-linux-android" rustup target list | grep "^armv7-linux-androideabi"Length of output: 301
Verify Android Rust build targets
Please confirm that Rust recognizes the newly added target triples inpackage.json(lines 61–64):
- aarch64-linux-android
- armv7-linux-androideabi
Run:
rustup target list | grep -E "^(aarch64-linux-android|armv7-linux-androideabi)$"If either target is missing, install it:
rustup target add aarch64-linux-android armv7-linux-androideabi.github/workflows/release-napi.yml (3)
57-60: Windows i686 mapping looks correct.
The combination ofarchitecture: x86andcpu: ia32correctly configures Node and pnpm for the 32-bit Windows target.
67-69: Validate defaults foraarch64-linux-androidrun.
Since noarchitectureorcpuis specified, the action will default to x64 Node and skip CPU override. Confirm this yields correct binaries for Android ARM64, or explicitly setcpu: arm64if needed.
120-122: Confirm backward compatibility forcpuinput.
The newcpuinput is optional. Verify that workflows not passing this value continue to work and default correctly.
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.
https://github.com/napi-rs/napi-rs/blob/6f74589a252c4c9ca344f7b18a066906260c704a/cli/src/utils/target.ts#L9-L29
Important
Add support for
aarch64-linux-androidandarmv7-linux-androideabitargets in build configuration and workflows.aarch64-linux-androidandarmv7-linux-androideabitargets torelease-napi.ymlandpackage.json.action.ymlto includecpuinput for architecture specification.release-napi.ymlto include new Android targets in the build matrix.cpu: ia32fori686-pc-windows-msvctarget inrelease-napi.yml.package.jsonto list new Android targets undernapi.targets.This description was created by
for 8481d8e. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
aarch64-linux-androidandarmv7-linux-androideabi) for native addon binaries.Chores