docs: add local development guide and improve Android build setup#368
docs: add local development guide and improve Android build setup#368ignaciosantise wants to merge 6 commits intomainfrom
Conversation
Add comprehensive documentation for local testing workflows on Android and iOS, including prerequisites table and step-by-step instructions for Maven Local and CocoaPods setups. Improve generate_kotlin_locally.sh with robust prerequisite validation and better Java version detection to handle various JDK distributions. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
Claude finished @ignaciosantise's task in 56s —— View job PR ReviewPR Size: 3 files, 135 additions, 9 deletions ✅ Found 3 issue(s)Issue 1: Java version mismatch between toolchain and compile optionsID: build-java-version-mismatch-7a1e Context:
Recommendation: Update compile options to match toolchain: compileOptions {
sourceCompatibility = JavaVersion.VERSION_17
targetCompatibility = JavaVersion.VERSION_17
}
kotlinOptions {
jvmTarget = "17"
}Issue 2: Bash comparison without quote protectionID: generate-bash-numeric-compare-3f9a Context:
Recommendation: Already has if [ -n "$java_version" ] && [ "$java_version" -ge 1 ] 2>/dev/null && [ "$java_version" -lt 17 ]; thenIssue 3: Regex improvement for Kotlin package renamingID: generate-regex-package-rename-8c2b Context:
Recommendation: Already handles with dual pattern in sed (both SummaryDocumentation additions are clear and helpful. Build script improvements are solid. Two issues need fixing:
The regex improvements in generate_kotlin_locally.sh are actually better than previous version - good refactor. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| java { | ||
| toolchain { | ||
| languageVersion.set(JavaLanguageVersion.of(11)) | ||
| languageVersion.set(JavaLanguageVersion.of(17)) |
There was a problem hiding this comment.
🤖 Auto Review Issue: Java version mismatch between toolchain and compile options
Severity: MEDIUM
Category: configuration
Tool: Claude Auto Review
Context:
- Pattern: Java toolchain set to version 17 (line 76) but compileOptions/kotlinOptions still use version 11 (lines 57-62)
- Risk: Toolchain enforces Java 17 for compilation but bytecode targets Java 11, creating potential incompatibility
- Impact: Build may succeed but runtime crashes on Java 11 JVMs, or Gradle may reject the inconsistent configuration
- Trigger: Running the build with this configuration
Recommendation: Update compile options to match toolchain:
compileOptions {
sourceCompatibility = JavaVersion.VERSION_17
targetCompatibility = JavaVersion.VERSION_17
}
kotlinOptions {
jvmTarget = "17"
}There was a problem hiding this comment.
are you sure about this?
Java compilers can produce bytecode for older versions (that's what targetCompatibility does)
Using Java 17 to compile Java 11-compatible code works fine
This won't cause "runtime crashes on Java 11 JVMs" - the bytecode IS Java 11 compatible
Add additional validation to ensure Java version is a valid positive number before comparison. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@claude review |
|
Claude finished @ignaciosantise's task in 39s —— View job PR Review
Corrected AssessmentYou're correct about the Java version configuration. I apologize for the false positive. Using Java 17 toolchain with Java 11 target bytecode is valid and intentional - the toolchain version controls which JDK runs Gradle/compilation, while targetCompatibility controls the bytecode version. This allows forward compatibility without requiring Java 17 at runtime. ✅ No issues found The PR adds valuable local development documentation and improves the build script with helpful prerequisite validation. The regex improvements for package renaming are well-designed with proper negative lookaheads. Key improvements:
|
There was a problem hiding this comment.
Pull request overview
This PR enhances the local development experience by adding comprehensive documentation for Android and iOS workflows and implementing robust prerequisite validation in the Kotlin build script. The changes ensure developers have clear guidance and early feedback when setting up their development environment.
Changes:
- Added detailed local development documentation with prerequisite tables and step-by-step workflows for Android (Maven Local) and iOS (Swift Package Manager and CocoaPods)
- Implemented prerequisite validation function checking cargo-ndk, Rust Android targets, and Java version with helpful error messages
- Updated Java toolchain requirement from version 11 to 17 in build.gradle
- Improved Kotlin code generation regex patterns for more reliable package renaming in variant builds
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| README.md | Added Local Development section with Android/iOS prerequisites, workflows, and testing instructions |
| generate_kotlin_locally.sh | Added check_prerequisites() function with validation for build tools and improved regex patterns for package renaming |
| crates/kotlin-ffi/android/build.gradle | Updated Java toolchain version from 11 to 17 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Extract major version, handling various JDK output formats | ||
| java_version=$(java -version 2>&1 | awk -F'"' '/version/ {print $2}' | cut -d. -f1) | ||
| # Fallback for non-standard outputs (e.g., "17.0.1" without quotes) | ||
| if [ -z "$java_version" ]; then | ||
| java_version=$(java -version 2>&1 | grep -oE '[0-9]+\.[0-9]+' | head -1 | cut -d. -f1) |
There was a problem hiding this comment.
The Java version detection will produce a confusing error message for Java 8. Java 8 reports its version as "1.8.0_xxx", and cutting on the first dot-separated field will extract "1" instead of "8". While the script will correctly reject Java 8 (since 1 < 17), the error message will say "found Java 1" instead of "found Java 8", which may confuse users.
Consider handling the legacy "1.x" version format explicitly to extract the correct major version for better error messages.
| # Extract major version, handling various JDK output formats | |
| java_version=$(java -version 2>&1 | awk -F'"' '/version/ {print $2}' | cut -d. -f1) | |
| # Fallback for non-standard outputs (e.g., "17.0.1" without quotes) | |
| if [ -z "$java_version" ]; then | |
| java_version=$(java -version 2>&1 | grep -oE '[0-9]+\.[0-9]+' | head -1 | cut -d. -f1) | |
| local java_version_raw | |
| # Extract full version string, handling standard JDK output formats (e.g., "1.8.0_402", "17.0.1") | |
| java_version_raw=$(java -version 2>&1 | awk -F'"' '/version/ {print $2}') | |
| if [ -n "$java_version_raw" ]; then | |
| # Handle legacy "1.x" format (Java 8 and earlier) by taking the second component as major | |
| if [[ "$java_version_raw" == 1.* ]]; then | |
| java_version=$(echo "$java_version_raw" | cut -d. -f2) | |
| else | |
| java_version=$(echo "$java_version_raw" | cut -d. -f1) | |
| fi | |
| fi | |
| # Fallback for non-standard outputs (e.g., version without quotes) | |
| if [ -z "$java_version" ]; then | |
| java_version_raw=$(java -version 2>&1 | grep -oE '[0-9]+\.[0-9]+' | head -1) | |
| if [ -n "$java_version_raw" ]; then | |
| if [[ "$java_version_raw" == 1.* ]]; then | |
| java_version=$(echo "$java_version_raw" | cut -d. -f2) | |
| else | |
| java_version=$(echo "$java_version_raw" | cut -d. -f1) | |
| fi | |
| fi |
| | Prerequisite | Error when missing | Solution | | ||
| | ------------------ | ---------------------------------------- | -------------------------------------------------------------------------------------- | | ||
| | Android NDK | Script shows helpful message | Install via Android Studio SDK Manager | | ||
| | `ANDROID_NDK_HOME` | "ANDROID_NDK_HOME is not set" | `export ANDROID_NDK_HOME=$HOME/Library/Android/sdk/ndk/<version>` | | ||
| | `cargo-ndk` | "no such command: ndk" | `cargo install cargo-ndk` | | ||
| | Rust targets | "target may not be installed" | `rustup target add aarch64-linux-android armv7-linux-androideabi x86_64-linux-android` | | ||
| | Java 17 | "Android Gradle plugin requires Java 17" | Use Zulu JDK, Homebrew, or Android Studio's bundled JDK | |
There was a problem hiding this comment.
The error message shown in the prerequisites table does not match the actual error message generated by the script. The script outputs "ERROR: Java 17 or higher is required (found Java X)" (line 57 of generate_kotlin_locally.sh), but the table shows "Android Gradle plugin requires Java 17" which is a different error that might come from Gradle itself, not from the prerequisite check. Consider updating the table to show the actual error from the check_prerequisites function, or clarify that this is the Gradle build error (which would occur if the prerequisite check is skipped).
| | Prerequisite | Error when missing | Solution | | |
| | ------------------ | ---------------------------------------- | -------------------------------------------------------------------------------------- | | |
| | Android NDK | Script shows helpful message | Install via Android Studio SDK Manager | | |
| | `ANDROID_NDK_HOME` | "ANDROID_NDK_HOME is not set" | `export ANDROID_NDK_HOME=$HOME/Library/Android/sdk/ndk/<version>` | | |
| | `cargo-ndk` | "no such command: ndk" | `cargo install cargo-ndk` | | |
| | Rust targets | "target may not be installed" | `rustup target add aarch64-linux-android armv7-linux-androideabi x86_64-linux-android` | | |
| | Java 17 | "Android Gradle plugin requires Java 17" | Use Zulu JDK, Homebrew, or Android Studio's bundled JDK | | |
| | Prerequisite | Error when missing | Solution | | |
| | ------------------ | ---------------------------------------------------------- | -------------------------------------------------------------------------------------- | | |
| | Android NDK | Script shows helpful message | Install via Android Studio SDK Manager | | |
| | `ANDROID_NDK_HOME` | "ANDROID_NDK_HOME is not set" | `export ANDROID_NDK_HOME=$HOME/Library/Android/sdk/ndk/<version>` | | |
| | `cargo-ndk` | "no such command: ndk" | `cargo install cargo-ndk` | | |
| | Rust targets | "target may not be installed" | `rustup target add aarch64-linux-android armv7-linux-android x86_64-linux-android` | | |
| | Java 17 | "ERROR: Java 17 or higher is required (found Java X)" | Use Zulu JDK, Homebrew, or Android Studio's bundled JDK | |
| if [ -n "$java_version" ] && [ "$java_version" -ge 1 ] 2>/dev/null && [ "$java_version" -lt 17 ]; then | ||
| echo "ERROR: Java 17 or higher is required (found Java $java_version)" | ||
| echo "Please install Java 17+ using one of:" | ||
| echo " - Android Studio's bundled JDK" | ||
| echo " - Homebrew: brew install openjdk@17" | ||
| echo " - Zulu JDK: https://www.azul.com/downloads/" | ||
| echo "" | ||
| has_error=1 | ||
| fi |
There was a problem hiding this comment.
If Java version extraction fails completely (both primary and fallback methods), the variable java_version will be empty, and the version check will be silently skipped, allowing the script to proceed. While Gradle itself will catch Java version issues later, it would be more user-friendly to provide early feedback with a warning when version detection fails. Consider adding:
if [ -z "$java_version" ] || ! [[ "$java_version" =~ ^[0-9]+$ ]]; then
echo "WARNING: Could not determine Java version"
echo "The build may fail if Java 17+ is not installed"
echo ""
fi
Summary
Add comprehensive documentation for local testing workflows on Android and iOS platforms, including prerequisites table and step-by-step instructions for Maven Local and CocoaPods setups. Improve the Kotlin build script with robust prerequisite validation and better Java version detection to handle various JDK distributions.
Changes
check_prerequisites()function in generate_kotlin_locally.sh to validate build environmentTesting
Manual testing of local build workflows on macOS with Android NDK and various JDK distributions.
🤖 Generated with Claude Code