sentry-android-gradle-plugin icon indicating copy to clipboard operation
sentry-android-gradle-plugin copied to clipboard

Auto-Installation version mismatch causes runtime crash

Open PaulWoitaschek opened this issue 3 years ago • 15 comments

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)

PaulWoitaschek avatar Jun 11 '22 22:06 PaulWoitaschek

@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.

marandaneto avatar Jun 12 '22 08:06 marandaneto

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 avatar Jun 12 '22 09:06 PaulWoitaschek

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.

PaulWoitaschek avatar Jun 12 '22 09:06 PaulWoitaschek

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)

romtsn avatar Jun 12 '22 10:06 romtsn

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.

PaulWoitaschek avatar Jun 12 '22 10:06 PaulWoitaschek

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?

romtsn avatar Jun 12 '22 11:06 romtsn

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?

bruno-garcia avatar Jun 12 '22 11:06 bruno-garcia

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.

PaulWoitaschek avatar Jun 12 '22 11:06 PaulWoitaschek

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.

romtsn avatar Jun 12 '22 12:06 romtsn

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 avatar Jun 13 '22 07:06 marandaneto

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 avatar Jun 13 '22 07:06 marandaneto

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.

marandaneto avatar Jun 13 '22 07:06 marandaneto

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

romtsn avatar Jun 14 '22 18:06 romtsn

@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 avatar Jun 15 '22 12:06 romtsn

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.

romtsn avatar Feb 22 '23 15:02 romtsn