getsentry / sentry-android-gradle-plugin

Gradle plugin for Sentry Android. Upload proguard, debug files, and more.
https://docs.sentry.io/platforms/android/gradle/
MIT License
143 stars 32 forks source link

Auto-Installation version mismatch causes runtime crash #329

Open PaulWoitaschek opened 2 years ago

PaulWoitaschek commented 2 years ago

Integration

sentry-android

Build System

Gradle

AGP Version

7.2.1

Proguard

Enabled

Version

6.0.0, plugin=3.1.0

Steps to Reproduce

Build an obfuscated release build and call:

fun initSentry(application: Application) {
  val options = Sentry.OptionsConfiguration<SentryAndroidOptions> { options ->
    options.apply {
      isEnableNdk = false // <- this fixes the crash
      ...
    }
  }
  SentryAndroid.init(application, NoOpLogger.getInstance(), options)
}

Expected Result

It doesn't crash

Actual Result

Follow up of the discussion in https://github.com/getsentry/sentry-java/pull/2031

Calling options.isEnableNdk = false causes the crash to not appear.

Stacktrace

signal 6 (SIGABRT), code -1 (SI_QUEUE), fault addr --------
Abort message: 'Throwing new exception 'no non-static method "Lio/sentry/android/core/SentryAndroidOptions;.getDsn()Ljava/lang/String;"' with unexpected pending exception: java.lang.NoSuchMethodError: no non-static method "Lio/sentry/android/core/SentryAndroidOptions;.getOutboxPath()Ljava/lang/String;"
  at void io.sentry.android.ndk.SentryNdk.initSentryNative(io.sentry.android.core.SentryAndroidOptions) (SourceFile:-2)
  at void io.sentry.android.ndk.SentryNdk.init(io.sentry.android.core.SentryAndroidOptions) (SourceFile:7)
  at java.lang.Object java.lang.reflect.Method.invoke(java.lang.Object, java.lang.Object[]) (Method.java:-2)
  at void io.sentry.android.core.l0.register(kp.r, io.sentry.SentryOptions) (SourceFile:106)
  at void io.sentry.u.l(io.sentry.SentryOptions, boolean) (SourceFile:98)
  at void io.sentry.u.m(kp.r0, io.sentry.u$a, boolean) (SourceFile:9)
  at void io.sentry.android.core.s0.e(android.content.Context, kp.s, io.sentry.u$a) (SourceFile:26)
  at void rv.c.c(android.app.Application) (SourceFile:26)
  at void yazio.App.onCreate() (SourceFile:9)
  at void android.app.Instrumentation.callApplicationOnCreate(android.app.Application) (Instrumentation.java:1223)
  at void android.app.ActivityThread.handleBindApplication(android.app.ActivityThread$AppBindData) (ActivityThread.java:6734)
  at void android.app.ActivityThread.access$1500(android.app.ActivityThread, android.app.ActivityThread$AppBindData) (ActivityThread.java:256)
  at void android.app.ActivityThread$H.handleMessage(android.os.Message) (ActivityThread.java:2090)
  at void android.os.Handler.dispatchMessage(android.os.Message) (Handler.java:106)
  at boolean android.os.Looper.loopOnce(android.os.Looper, long, int) (Looper.java:201)
  at void android.os.Looper.loop() (Looper.java:288)
  at void android.app.ActivityThread.main(java.lang.String[]) (ActivityThread.java:7842)
  at java.lang.Object java.lang.reflect.Method.invoke(java.lang.Object, java.lang.Object[]) (Method.java:-2)
  at void com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run() (RuntimeInit.java:548)
  at void com.android.internal.os.ZygoteInit.main(java.lang.String[]) (ZygoteInit.java:1003)
marandaneto commented 2 years ago

@PaulWoitaschek are you using proguard or r8? are you using any special configuration in your proguard config?

Can you add to your app's proguard file:

// You can specify any path and filename.
-printconfiguration ~/tmp/full-r8-config.txt

And check if the rule to keep SentryAndroidOptions is there? https://github.com/getsentry/sentry-java/blob/e4a46aa6dfe09aa185f1aaf6bd31235c12b520db/sentry-android-ndk/proguard-rules.pro#L5-L7

Thanks.

PaulWoitaschek commented 2 years ago

It does not exist.

But wait; we're not using sentry-android-ndk. It seems the plugin adds that, where we add the 6.0.0 core dependency.

Maybe this is related to the plugin adding an outdated dependency?

https://github.com/getsentry/sentry-android-gradle-plugin/blob/1deb7a6da144fdf6e55c86634afa9387132ec80c/plugin-build/src/main/kotlin/io/sentry/android/gradle/SentryPlugin.kt#L327

PaulWoitaschek commented 2 years ago

Yes, confirmed. Setting the version in the autoInstallation block fixes it.

I think this is a dependency issue: In a gradle module we have a dependency on sentry-core using 6.0.0. Now the plugins in the app module adds 5.7.0.

In the final dependeny graph this leads to inconsistent dependencies:

+--- io.sentry:sentry-android:5.7.0
|    +--- io.sentry:sentry-android-core:5.7.0 -> 6.0.0
|    |    \--- io.sentry:sentry:6.0.0
|    \--- io.sentry:sentry-android-ndk:5.7.0
|         +--- io.sentry:sentry:5.7.0 -> 6.0.0
|         \--- io.sentry:sentry-android-core:5.7.0 -> 6.0.0 (*)

Disabling the auto installation also works.

From your side this coulde be mitigated by publishing a bom. The approach could look like this: You publish a bom. The plugin applies the bom. The plugin uses now either uses the plugin defined version for the bom or (with precedence) the user supplied sentry version and applies the bom using that version.

romtsn commented 2 years ago

We do publish a bom already -> https://docs.sentry.io/platforms/android/configuration/bill-of-materials/. I'm not sure the bom would help here though, because if we apply the bom with the version 5.7.0, this still would be a problem for your case, because you specify explicitly the -core version of 6.0.0, so the ndk would still be applied with 5.7.0, wouldn't it?

Ideally we'd look at your dependency tree and use the version that you've specified in the module, but that's unfortunately not possible without resolving the configuration, and this can be quite heavy for the build time (we have an improvement to at least warn you about that though (https://github.com/getsentry/sentry-android-gradle-plugin/issues/324).

There are also other multiple improvements to this:

  1. Bump the sentry SDK version in the plugin automatically and release a new version of the plugin together with the runtime sdk (https://github.com/getsentry/sentry-android-gradle-plugin/issues/322)
  2. Auto-installing NDK can be optional in the plugin (so only the -core package is installed by default)
PaulWoitaschek commented 2 years ago

From my understanding, an applied enforcedPlatform would do it.

Besides that, moving the plugin and the java library to the same repository and releasing them with the same version would mitigate these errors (where plugin and lib are out of sync) as well.

romtsn commented 2 years ago

Ah, I wasn't aware of enforcedPlatform, yeah it seems like that would do.

We actually wanted to be conservative here, and do not override any of the user-defined dependencies, but maybe it'd be better to have them enforced (e.g. by using a bom and giving user an option to specify a version) @bruno-garcia @marandaneto wdyt?

bruno-garcia commented 2 years ago

Ah, I wasn't aware of enforcedPlatform, yeah it seems like that would do.

We actually wanted to be conservative here, and do not override any of the user-defined dependencies, but maybe it'd be better to have them enforced (e.g. by using a bom and giving user an option to specify a version) @bruno-garcia @marandaneto wdyt?

Problem of us overriding the version is that user can't override our behavior it turns out to be wrong. Could we log a build warning instead? Or have a separate option to disable our overriding behavior if we add it?

PaulWoitaschek commented 2 years ago

In general, the approach to let users overwrite versions is good.

But in the case that you use auto-install, it is error prone and leads to inconsistent versions (which has caused the crash I'm reporting). And you can explicitly set the version in the auto install extension already so I expect this to solve more issues than it causes.

romtsn commented 2 years ago

Problem of us overriding the version is that user can't override our behavior it turns out to be wrong. Could we log a build warning instead?

It's kind of possible through autoInstallation.sentryVersion config, but it will set a single version for all integrations. The only problem I see with this, is if they want to keep an older version of, say, -timber, while updating -core to 6.0.0. But this still would be possible to enforce via gradle's dependency resolution.

Or have a separate option to disable our overriding behavior if we add it?

Having an option for that is also possible, yep.

marandaneto commented 2 years ago

Ah, I wasn't aware of enforcedPlatform, yeah it seems like that would do.

We actually wanted to be conservative here, and do not override any of the user-defined dependencies, but maybe it'd be better to have them enforced (e.g. by using a bom and giving user an option to specify a version) @bruno-garcia @marandaneto wdyt?

Yep, we should not override the user-defined dependencies to not force the need of upgrading the Sentry SDK nor the need of removing the Sentry Gradle plugin due to not being compatible.

marandaneto commented 2 years ago

Problem of us overriding the version is that user can't override our behavior it turns out to be wrong. Could we log a build warning instead?

It's kind of possible through autoInstallation.sentryVersion config, but it will set a single version for all integrations. The only problem I see with this, is if they want to keep an older version of, say, -timber, while updating -core to 6.0.0. But this still would be possible to enforce via gradle's dependency resolution.

Or have a separate option to disable our overriding behavior if we add it?

Having an option for that is also possible, yep.

All Sentry packages should be the very same version, we should not allow the mix of e.g. sentry-android-core 5.7 and sentry-android-timber 6.0, packages are published together with the very same version to avoid incompatibilities.

marandaneto commented 2 years ago

If the user defined 5.7.0 in its dependencies block (it doesn't matter if its the core package or not, versions should be aligned), we should keep using 5.7.0, if this is technically not possible or too expensive, I'd say we should turn off the auto-install for this use case. If the user defined the sentryVersion, then we respect that.

romtsn commented 2 years ago

Moved this to the SAGP repo, as it rather belongs here.

romtsn commented 2 years ago

@PaulWoitaschek we've released 3.1.1 which bumps the runtime sdk version to 6.1.0 , so this problem should be gone, but we'll look into a more future-proof solution. Thanks for your ideas and the bug report!

romtsn commented 1 year ago

We could have a gradle task which looks over the dependency tree and see if there are mismatched sentry packages' versions, fail the build or show a warning.