-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat(crashlytics): add firebase.json support for NDK crash reporting #8788
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
mikehardy
left a comment
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 believe this will be a little more involved than the current state of the PR. A couple things:
1- the initial issue was about adding configuration support to optionally enable GWP-ASan support, this PR does not address that at all
2- this PR does add configuration support to optionally enable NDK support, but does so incompletely
For item 1 - GWP-ASan support, the configured AndroidManifest item is this one https://developer.android.com/ndk/guides/gwp-asan#opt-in and assuming we can configure it using the manifest placeholder style I suggest having it be a string value in firebase.json with 'default' as the default value (since that's what it is if not specified - our current state) but never and always as possible other values. That would actually close the bug and warrant the closes #nnnn tag in the PR description
For item 2 - NDK symbol upload is a real need for developers as well but is distinct from the logged issue. Simply enabling it isn't so bad, it's a similar placeholder but is in addition to the current one - https://firebase.google.com/docs/crashlytics/android/get-started-ndk#add-extension
That is half the job with regard to config, but the second half will take most of the work - you have to configure automatic symbol uploading.
Auto upload is trickier though because it is linked to gradle's internal + dynamic model of the build variants for an app, which are app-specific and may be customized (e.g., playRelease, fdroidRelease or premiumRelease, liteRelease if you have build flavors for different app modes vs the more common release). So there needs to be some gradle logic that hooks the gradle build post-config to detect all variant names with case insensitive release in the name, and add the upload as a finalizes dependency to the build for those variants
If it is not feasible to get all the release variants of an app (it should be, but may be difficult, and in fact the variants API in gradle just changed so it is particularly annoying...) an attempt to simply hard-code the release variant and pull it from config directly in a try/catch with a warning if it failed could be a first attempt
And the task to hook after the build of the release type(s) is then https://firebase.google.com/docs/crashlytics/android/get-started-ndk#set-up-automatic-native-symbols-upload
38a52cd to
7af280b
Compare
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 15 days until this gets closed automatically |
Description
closes #8784
Related issues
Release Summary
Checklist
AndroidiOSOther(macOS, web)e2etests added or updated inpackages/\*\*/e2ejesttests added or updated inpackages/\*\*/__tests__Test Plan
Think
react-native-firebaseis great? Please consider supporting the project with any of the below:React Native FirebaseandInvertaseon Twitter