Fix Android build config for flutter_nfc_kit support#20
Fix Android build config for flutter_nfc_kit support#20Swastik-K-Sahu wants to merge 4 commits intofossasia:mainfrom
Conversation
Reviewer's Guide by SourceryThis pull request updates the Android build configuration to ensure compatibility with the No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @Swastik-K-Sahu - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a comment explaining why the NDK version is being explicitly set.
- It might be good to add a brief explanation in the PR description about why these changes are needed for
flutter_nfc_kitsupport.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@mariobehling @kienvo could you please review this? |
There was a problem hiding this comment.
@Swastik-K-Sahu What was the reason we required these changes again ? Was the build for flutter_nfc_kit failing ?
Also, please have a look at the failing Android build pipeline in the PR, looks like some compatibility issues.
| namespace = "org.fossasia.magic_epaper_app" | ||
| compileSdk = flutter.compileSdkVersion | ||
| ndkVersion = flutter.ndkVersion | ||
| ndkVersion = project.properties['android.ndkVersion'] |
There was a problem hiding this comment.
The ndkVersion was not getting updated on my local. It was being set to v26, but the minimum supported for some dependencies were v27.
| zipStoreBase=GRADLE_USER_HOME | ||
| zipStorePath=wrapper/dists | ||
| distributionUrl=https\://services.gradle.org/distributions/gradle-8.3-all.zip | ||
| distributionUrl=https\://services.gradle.org/distributions/gradle-8.9-all.zip |
There was a problem hiding this comment.
Such updates can usually be handled by dependabot. But till we set up that, this is fine.
| org.gradle.jvmargs=-Xmx4G -XX:MaxMetaspaceSize=2G -XX:+HeapDumpOnOutOfMemoryError | ||
| android.useAndroidX=true | ||
| android.enableJetifier=true | ||
| android.ndkVersion=27.0.12077973 No newline at end of file |
There was a problem hiding this comment.
Wouldn't this cause compatibility issues when we upgrade to higher versions of flutter ?
There was a problem hiding this comment.
Yes, I got that. I can remove this hardcoded version of ndk and let flutter choose the required ndkVersion.
Yes the build for |
|
@AsCress I have remove the hardcoded ndkVersion. Now can you check the android build in the pipeline? |
|
@mariobehling Could you please approve the workflows for this pull request ? |
|
Closing it as the project has progressed in the meantime. Thank you! |
Fixes #16
Updates the Android build setup to meet the requirements of the
flutter_nfc_kitplugin.✅ Changes
Java Versionto 17minSdk >= 268.9with AGP8.7.0android.permission.NFCinAndroidManifest.xmljvmTarget 17inbuild.gradlendkVersionto27.0.12077973Summary by Sourcery
Update Android build configuration to support flutter_nfc_kit plugin and improve build compatibility
New Features:
Enhancements:
Build: