-
Notifications
You must be signed in to change notification settings - Fork 250
fix: config ndkVersion #759
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
Open
ntcong91
wants to merge
1
commit into
hyochan:main
Choose a base branch
from
ntcong91:fix/config-ndk-version
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,8 @@ android { | |
| defaultConfig { | ||
| minSdk getExtOrIntegerDefault("minSdkVersion") | ||
| targetSdk getExtOrIntegerDefault("targetSdkVersion") | ||
| ndkVersion getExtOrDefault("ndkVersion") | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| externalNativeBuild { | ||
| cmake { | ||
|
|
||
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.
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.
🧩 Analysis chain
🏁 Script executed:
Repository: hyochan/react-native-nitro-sound
Length of output: 1997
🏁 Script executed:
Repository: hyochan/react-native-nitro-sound
Length of output: 290
🏁 Script executed:
Repository: hyochan/react-native-nitro-sound
Length of output: 58
🏁 Script executed:
Repository: hyochan/react-native-nitro-sound
Length of output: 1005
🏁 Script executed:
Repository: hyochan/react-native-nitro-sound
Length of output: 224
🏁 Script executed:
Repository: hyochan/react-native-nitro-sound
Length of output: 675
🏁 Script executed:
Repository: hyochan/react-native-nitro-sound
Length of output: 4763
🏁 Script executed:
Repository: hyochan/react-native-nitro-sound
Length of output: 769
🏁 Script executed:
Repository: hyochan/react-native-nitro-sound
Length of output: 2047
🏁 Script executed:
Repository: hyochan/react-native-nitro-sound
Length of output: 106
🏁 Script executed:
Repository: hyochan/react-native-nitro-sound
Length of output: 431
🏁 Script executed:
Repository: hyochan/react-native-nitro-sound
Length of output: 160
🌐 Web query:
Android Gradle Plugin ndkVersion property defaultConfig vs android block placement💡 Result:
ndkVersionis a property of the module’sandroid {}extension (i.e.,CommonExtension.ndkVersion). Set it directly insideandroid {}, not insidedefaultConfig {}. Example from the official DSL reference:android { ndkVersion '20.1.5948944' }. [1]ndkVersion. [1][2]defaultConfig { ndkVersion ... }sometimes “works”: in Groovy DSL, nested blocks can sometimes resolve unknown properties via dynamic delegation, so you may see Stack Overflow answers that putndkVersionunderdefaultConfig. But it’s not the documented location, is not how the DSL is defined, and is more likely to fail in Kotlin DSL (and/or be fragile across AGP versions). Preferandroid { ndkVersion ... }. [1][2]Recommended (Kotlin DSL):
android { ndkVersion = "26.1.10909125" }Recommended (Groovy DSL):
android { ndkVersion "26.1.10909125" }Sources: official AGP DSL (
CommonExtension.ndkVersion) [1], NDK/AGP behavior notes (default NDK +android.ndkVersion) [2].[1] https://developer.android.com/reference/tools/gradle-api/8.3/com/android/build/api/dsl/CommonExtension
[2] https://github.com/android/ndk-samples/wiki/Configure-NDK-Path
🌐 Web query:
AGP CommonExtension ndkVersion DefaultConfig API reference💡 Result:
ndkVersionis a property oncom.android.build.api.dsl.CommonExtension(i.e., the top-levelandroid { ... }block for app/library/etc.), not ondefaultConfig. It “requires the specified NDK version to be used.”[major.minor.build]format, and it’s mutually exclusive withndkPath. [1]defaultConfig { ... }is configured viaCommonExtension.defaultConfig(...)and usesDefaultConfig/BaseFlavorAPIs. NDK-related settings insidedefaultConfigare done via thendk { ... }block (typeNdk) for things like ABI filters (per-variant/per-flavor config), not the NDK version. [1][2]API reference pages (Android Developers):
Typical Kotlin DSL usage:
android { ndkVersion = "26.1.10909125" // CommonExtension.ndkVersion defaultConfig { ndk { abiFilters += setOf("arm64-v8a", "x86_64") } // DefaultConfig/BaseFlavor.ndk { ... } } }Sources: [1] [2]
Move
ndkVersionto theandroid {}block and guard against null valuesndkVersionis a property ofCommonExtension(theandroid {}block), notDefaultConfig. The Android Gradle Plugin documentation and API reference explicitly define it at the top-levelandroid {}block. Your example app correctly places it at line 76 inexample/android/app/build.gradle. Placing it insidedefaultConfig {}violates the DSL contract and will cause build failures.Additionally,
getExtOrDefault("ndkVersion")can returnnullwhen neitherrootProject.extnor theSound_ndkVersionproject property is set. AGP provides a default NDK version when none is specified, so the safest approach is to skip the assignment entirely when no value is configured—which preserves the pre-PR behavior.🐛 Proposed fix
android { namespace "com.margelo.nitro.sound" compileSdk getExtOrIntegerDefault("compileSdkVersion") + def ndkVer = getExtOrDefault("ndkVersion") + if (ndkVer) { + ndkVersion ndkVer + } + defaultConfig { minSdk getExtOrIntegerDefault("minSdkVersion") targetSdk getExtOrIntegerDefault("targetSdkVersion") - ndkVersion getExtOrDefault("ndkVersion") externalNativeBuild {📝 Committable suggestion
🤖 Prompt for AI Agents