fix: init native libs if available on sdk init #461
Conversation
| // Integrations are registered in the same order. NDK before adding Watch outbox, | ||
| // because sentry-native move files around and we don't want to watch that. | ||
| options.addIntegration(new NdkIntegration()); | ||
| if (loadNdkIfAvailable(options, buildInfoProvider)) { |
There was a problem hiding this comment.
@bruno-garcia maybe I'd be good to get the Class<?> as a return type and pass to NdkIntegration as a ctor param, so we do the reflection only once on the class level, wdyt?
There was a problem hiding this comment.
That makes sense. So the return type would be @Nullable Class<?>.
It would also improve things imho if we didn't fiddle around with the options setEnableNdk value. Just leave it as-is.
When we check if it's true, if we did succeed in getting the class name through this method, we enable it. Otherwise we don't. It would just be nice of this method didn't have side effects. Simply get the class name if it's possible.
There was a problem hiding this comment.
yes, but on the NdkIntegration we do need to set setEnableNdk=false because we rely on that to add or not the sentry-android-ndk as a package on the sdk info
| val buildInfo = mock<IBuildInfoProvider>() | ||
| whenever(buildInfo.sdkInfoVersion).thenReturn(16) | ||
|
|
||
| // hard to test, lets just check that its not throwing anything |
There was a problem hiding this comment.
@bruno-garcia to be able to test this properly, I'd need to have an interface that hides the static call Class.forName, so I can mock and assert it, do you think it's worth it?
There was a problem hiding this comment.
With your suggested refactor of having loadNdk.. return the class, we could test this by making sure NdkIntegration initialized with null for Class<?> and with an instance it does?
I'd say it's your call. It's nice to have it testable and be able to assert this properly but on the other hand it bloats the SDK further with 1 more type just for this 1 test case.
There was a problem hiding this comment.
yes this is fully testable/tested now
There was a problem hiding this comment.
I've added a new type for the sake of testability, this is a critical part of the sdk, as it bundles other sdks and it'd be good to be sure that this is working properly, although those types are package-private anyway.
sentry-android-core/src/main/java/io/sentry/android/core/NdkIntegration.java
Show resolved
Hide resolved
2 participants
📢 Type of change
📜 Description
fix: init native libs if available on sdk init
💡 Motivation and Context
one could init the SDK with an empty DSN and still load a native lib that depends on the libsentry, the libsentry was never initialized, now it is and it'll be NoOp as the Android SDK.
💚 How did you test it?
running on API 16 and 28, with and without an empty DSN.
📝 Checklist
🔮 Next steps