Implement React Native Noir bindings and enhance Android build process#721
Merged
Conversation
- Introduced a new module for React Native Noir bindings, integrating it into the build process for the React Native platform. - Enhanced the Android build logic to conditionally apply architecture-specific parameters based on the presence of iOS architectures and the Noir adapter. - Updated the `android_noir` module to utilize a custom Zig linker for resolving ABI compatibility issues with the barretenberg prebuilt. - Refactored the React Native project creation logic to ensure proper handling of architecture-specific Gradle properties and cleanup of stale files. - Improved the `package.json` template for React Native to include necessary configurations and dependencies for the new setup. These changes streamline the integration of Noir with React Native and improve the overall build process for Android.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReact Native builds now support Noir-specific Android generation, explicit linker paths, JSI bindings, architecture-specific configuration, CMake and Gradle patches, stale entrypoint cleanup, refreshed keys, and updated template dependencies. ChangesReact Native Noir integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as build_project
participant Bindings as ReactNativePlatform
participant Noir as react_native_noir
participant Cargo as cargo ndk
participant Generator as uniffi-bindgen-react-native
CLI->>Bindings: Set up bindings directory
CLI->>Noir: Build Android architectures with Noir parameters
Noir->>Cargo: Build isolated NDK-linked library
Noir->>Generator: Generate JSI bindings from library metadata
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Deploying mopro with
|
| Latest commit: |
10df2fb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e9167029.mopro.pages.dev |
| Branch Preview URL: | https://fix-noir-react-native.mopro.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cli/src/build/android_noir.rs (1)
193-214: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSupport the advertised Windows host path.
The Windows branch in
ndk_host_tagreaches this code, but it emits a.shwrapper and only sets executable permissions on Unix. Cargo then receives a linker path Windows cannot launch as a shell script. Generate a Windows-native wrapper or fail early with an explicit unsupported-host error. (learn.microsoft.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/build/android_noir.rs` around lines 193 - 214, Update the wrapper-generation flow around the visible wrapper path and `fs::write` call to handle Windows hosts explicitly: generate a Windows-native executable wrapper that Cargo can launch, or return a clear unsupported-host error before emitting the `.sh` script. Do not allow the Windows `ndk_host_tag` branch to proceed with the Unix shell wrapper and Unix-only permissions path.mopro-ffi/src/app_config/android.rs (1)
199-206: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the linker override out of
RUSTFLAGS. Cargo treatsRUSTFLAGSas the highest-priority rustflags source, so this bypasses anyCARGO_TARGET_<TRIPLE>_RUSTFLAGSthe caller set. Passarch_strintoapply_arch_configand setCARGO_TARGET_<TRIPLE>_LINKER(or--config target.<triple>.linker) instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mopro-ffi/src/app_config/android.rs` around lines 199 - 206, Update apply_arch_config to accept arch_str and stop appending the linker override to RUSTFLAGS. Configure the linker through the target-specific CARGO_TARGET_<TRIPLE>_LINKER environment variable, or the equivalent target.<triple>.linker Cargo setting, so caller-provided target rustflags remain effective.mopro-ffi/src/app_config/react_native.rs (1)
48-65: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin the generator revision before installing it
git cloneis pullingzkmopro/uniffi-bindgen-react-nativefrommain, thencargo install --pathexecutes that moving code directly. Pin a commit SHA or published release tag so builds don’t drift from the0.31.0-3contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mopro-ffi/src/app_config/react_native.rs` around lines 48 - 65, Update the generator download flow around the first Command::new("git") invocation to check out a fixed commit SHA or release tag matching the 0.31.0-3 contract before the subsequent cargo install command runs. Preserve the existing clone failure handling and ensure cargo install uses the pinned checkout rather than the moving main revision.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/src/build.rs`:
- Around line 197-240: Update the React Native build flow in
generate_react_native_bindings so mixed iOS/Android selections do not patch
Android Gradle architectures without producing the corresponding Android
libraries. Either split the iOS and Android build paths so Android targets still
invoke the required Android build, or reject mixed selections before
patch_gradle_properties_architectures runs; preserve existing behavior for
iOS-only and Android-only selections.
In `@cli/src/create/react_native.rs`:
- Around line 56-67: Update the configuration handling around read_config so
only a missing Config.toml is ignored; propagate read or parse errors for an
existing file instead of silently skipping
patch_gradle_properties_architectures. Preserve the current ABI filtering and
patching behavior when configuration loads successfully.
In `@mopro-ffi/src/app_config/react_native.rs`:
- Around line 220-222: Update the CMake and Gradle file-reading branches in the
surrounding React Native configuration patching function to handle read errors
explicitly: return success only for NotFound, and propagate permission, I/O, and
UTF-8 failures with contextual error information. Replace both let Ok(...) early
returns, including the branch near the Gradle handling, while preserving the
existing behavior for missing files.
---
Outside diff comments:
In `@cli/src/build/android_noir.rs`:
- Around line 193-214: Update the wrapper-generation flow around the visible
wrapper path and `fs::write` call to handle Windows hosts explicitly: generate a
Windows-native executable wrapper that Cargo can launch, or return a clear
unsupported-host error before emitting the `.sh` script. Do not allow the
Windows `ndk_host_tag` branch to proceed with the Unix shell wrapper and
Unix-only permissions path.
In `@mopro-ffi/src/app_config/android.rs`:
- Around line 199-206: Update apply_arch_config to accept arch_str and stop
appending the linker override to RUSTFLAGS. Configure the linker through the
target-specific CARGO_TARGET_<TRIPLE>_LINKER environment variable, or the
equivalent target.<triple>.linker Cargo setting, so caller-provided target
rustflags remain effective.
In `@mopro-ffi/src/app_config/react_native.rs`:
- Around line 48-65: Update the generator download flow around the first
Command::new("git") invocation to check out a fixed commit SHA or release tag
matching the 0.31.0-3 contract before the subsequent cargo install command runs.
Preserve the existing clone failure handling and ensure cargo install uses the
pinned checkout rather than the moving main revision.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e2bc498-d6f5-4c63-8e04-a6374e335815
📒 Files selected for processing (7)
cli/src/build.rscli/src/build/android_noir.rscli/src/create/react_native.rsmopro-ffi/src/app_config/android.rsmopro-ffi/src/app_config/constants.rsmopro-ffi/src/app_config/react_native.rsmopro-ffi/src/app_config/template/react_native/package.json
- Enhanced the build logic in `build.rs` to better handle architecture-specific parameters for iOS and Android, ensuring proper integration of Noir bindings. - Simplified the initialization message in `print.rs` for improved readability by using direct string interpolation. - Updated Android build configurations to streamline the generation of bindings and ensure compatibility with the latest architecture requirements. These changes improve the overall build process and user experience when initializing projects.
- Modified `.gitignore` to exclude the `cli/src/build/` directory from being ignored, allowing for better management of build artifacts. - Introduced a new file `react_native_noir.rs` in `cli/src/build/`, implementing a specialized build process for React Native Noir, which includes architecture-specific handling and binding generation from NDK-linked libraries. These changes enhance the build process for React Native projects utilizing Noir, ensuring proper integration and compatibility with architecture-specific requirements.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cli/src/build/react_native_noir.rs (1)
129-143: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet
current_dir(project_dir)on thecargo ndkcommand.
build_ndk_linked_lib_for_bindgenusesproject_dirfor the target directory and lib name but doesn't set it as the command's working directory.cargo ndk buildrelies on the process CWD to discoverCargo.toml. This works today only because the caller passesenv::current_dir()asproject_dir, but the function should be self-contained — contrast withbuild_android_oncewhich explicitly setscurrent_dir(bindings_dir).🔧 Proposed fix
let mut cmd = Command::new("cargo"); + cmd.current_dir(project_dir); cmd.arg("ndk").arg("-t").arg(arch);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/build/react_native_noir.rs` around lines 129 - 143, Set the working directory of the cargo Command in build_ndk_linked_lib_for_bindgen to project_dir before executing it, so cargo ndk build discovers the intended Cargo.toml independently of the process CWD. Keep the existing argument and environment setup unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cli/src/build/react_native_noir.rs`:
- Around line 129-143: Set the working directory of the cargo Command in
build_ndk_linked_lib_for_bindgen to project_dir before executing it, so cargo
ndk build discovers the intended Cargo.toml independently of the process CWD.
Keep the existing argument and environment setup unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6eda5d08-8137-4a6e-95fe-c0b23485b327
📒 Files selected for processing (2)
.gitignorecli/src/build/react_native_noir.rs
- Corrected the conditional checks in the GitHub Actions workflow to ensure proper execution during pull requests. - Introduced a new step to set up Zig for the Noir adapter, addressing ABI compatibility issues with the barretenberg prebuilt. These changes enhance the reliability of the CI process and improve the build environment for the Noir adapter.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/build-and-test.yml (1)
393-399: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the Zig setup action to an immutable commit.
mlugg/setup-zig@v2is a mutable third-party reference. Pin it to a full commit SHA while retainingversion: 0.16.0to reduce supply-chain risk and make workflow behavior reproducible. Verify that the selected SHA corresponds to the intendedv2release.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-and-test.yml around lines 393 - 399, Update the “Setup Zig (for noir/barretenberg libc++ ABI)” workflow step to pin mlugg/setup-zig to the full commit SHA corresponding to the intended v2 release, replacing the mutable `@v2` reference while retaining version: 0.16.0 and the existing matrix.adapter condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/build-and-test.yml:
- Around line 393-399: Update the “Setup Zig (for noir/barretenberg libc++ ABI)”
workflow step to pin mlugg/setup-zig to the full commit SHA corresponding to the
intended v2 release, replacing the mutable `@v2` reference while retaining
version: 0.16.0 and the existing matrix.adapter condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a84370c-619e-47a9-a973-f6e0909faa3c
📒 Files selected for processing (1)
.github/workflows/build-and-test.yml
- Updated the error handling in `react_native.rs` to provide more context when reading configuration files fails, specifically for missing files. - Refactored the logic for reading the Gradle properties in `react_native.rs` and `react_native.rs` to use a match statement, improving clarity and robustness. - Improved error reporting in `patch_android_cmake_lists_uniffi_bindgen_resolve` and `patch_gradle_properties_architectures` functions to handle file read errors gracefully. These changes improve the reliability of the project setup process and enhance the user experience by providing clearer error messages.
# Conflicts: # cli/src/print.rs # mopro-ffi/src/app_config/android.rs
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
1 participant
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.
android_noirmodule to utilize a custom Zig linker for resolving ABI compatibility issues with the barretenberg prebuilt.package.jsontemplate for React Native to include necessary configurations and dependencies for the new setup.These changes streamline the integration of Noir with React Native and improve the overall build process for Android.
Summary by CodeRabbit
index.web.tsentrypoint during React Native project setup.reactNativeArchitecturesto match the ABIs actually built.