-
Notifications
You must be signed in to change notification settings - Fork 4
Add --android-sdk-version
option to cmake-rn
#131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds a new --android-sdk-version
flag to the cmake-rn
CLI so users can specify the Android platform API level for CMake-based React Native builds.
- Introduce
DEFAULT_ANDROID_SDK_VERSION
and wire up a new CLI option - Pass
androidSdkVersion
throughgetTripletConfigureCmakeArgs
andconfigureProject
- Update Android CMake args to use
-DANDROID_PLATFORM=<sdkVersion>
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
packages/cmake-rn/src/cli.ts | Define default SDK version constant, add --android-sdk-version option, and integrate it into the command pipeline |
packages/cmake-rn/src/android.ts | Extend getAndroidConfigureCmakeArgs signature to accept sdkVersion and set ANDROID_PLATFORM |
Comments suppressed due to low confidence (5)
packages/cmake-rn/src/cli.ts:42
- [nitpick] Add a comment explaining why 24 was chosen (e.g. matching the community template’s minSdkVersion) so future maintainers understand this default.
const DEFAULT_ANDROID_SDK_VERSION = "24";
packages/cmake-rn/src/cli.ts:94
- [nitpick] Clarify in the help text that this flag sets the Android platform API level (e.g. 'API level for Android builds'), not the SDK tools version.
"The Android SDK version to use for Android builds"
packages/cmake-rn/src/cli.ts:92
- Consider adding a unit or integration test to ensure the
--android-sdk-version
option is parsed and propagated correctly.
const androidSdkVersionOption = new Option(
packages/cmake-rn/src/android.ts:82
- [nitpick] Remove or document the commented-out CMake flags related to
ANDROID_NATIVE_API_LEVEL
if they’re no longer needed to keep the codebase clean.
`ANDROID_ABI=${architecture}`,
packages/cmake-rn/src/android.ts:43
- Also verify that the SDK platform directory (e.g.
${ANDROID_HOME}/platforms/android-${sdkVersion}
) exists and, if missing, suggest runningsdkmanager --install "platforms;android-${sdkVersion}"
.
fs.existsSync(ndkPath),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming this option is here so that higher-level tooling (RN CLI?) can pull the mininmum SDK version from the user's Gradle files, is that correct?
It's mostly for the library developer to pick, but it's only really needed if someone is calling into Android APIs from their native addon. The reason I added now is just to silence a warning - I should have probably made that clear from the description 😊 |
Stacked on #130, merging this PR will:
--android-sdk-version
option with a default value matching the minimum required Android SDK version in the React Native community template.