Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add sentry-android sdk version check #243

Merged
merged 10 commits into from
Jan 5, 2022

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented Dec 24, 2021

#skip-changelog

馃摐 Description

Add a sentry-android SDK version check in the projects.afterevaluate step

馃挕 Motivation and Context

This is necessary to avoid breaking customers' code at runtime, for instance, if they are using an older version of sentry-android which does not contain file I/O runtime classes. So we just disable the auto-instrumentation features based on the main SDK version

馃挌 How did you test it?

馃摑 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

馃敭 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Dec 25, 2021

Messages
馃摉 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 馃毇 dangerJS against 422a02c

@romtsn romtsn force-pushed the feat/sentry-android-sdk-version-check branch from 5ac2a5f to 8e666ae Compare December 25, 2021 11:16
@romtsn romtsn merged commit e536888 into feat/file-io Jan 5, 2022
@romtsn romtsn deleted the feat/sentry-android-sdk-version-check branch January 5, 2022 09:48
Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I came a bit late to the review :) Anyway I left some comments.

I'm also curious to know why is it needed to resolve a configuration to search for "sentry-android-core"? Is there a scenario where the Gradle Plugin can be applied to a project that doesn't also import the Android SDK? If so, why is this the case?

It would be way less error prone to just offer the possibility to add an implementation dependency to the Sentry Android SDK. Maybe this can be an opt-out feature, so the user could manually configure it if they wish.

This will simplify also the SentryAndroidSdkState and the isAtLeast as you already know the version of the Android SDK that is shipped with the Gradle Plugin.

@romtsn
Copy link
Member Author

romtsn commented Jan 5, 2022

Sorry I came a bit late to the review :) Anyway I left some comments.

I'm also curious to know why is it needed to resolve a configuration to search for "sentry-android-core"? Is there a scenario where the Gradle Plugin can be applied to a project that doesn't also import the Android SDK? If so, why is this the case?

It would be way less error prone to just offer the possibility to add an implementation dependency to the Sentry Android SDK. Maybe this can be an opt-out feature, so the user could manually configure it if they wish.

This will simplify also the SentryAndroidSdkState and the isAtLeast as you already know the version of the Android SDK that is shipped with the Gradle Plugin.

Yeah, that's unlikely that SAGP would be used without the main Android SDK, but we are not solely checking for presence of the SDK here, but also the version. Based on the version we enable/disable bytecode manipulation features, which otherwise would crash the app at runtime, if we would be calling to some non-existing classes.

We thought of having SAGP applying the main SDK as a dependency, but that still cannot guarantee that the version won't be resolved by Gradle to a higher one (if there's one in the dependency tree), or that it can even be forced by the users to something else. This approach may be brittle, but seems the most failsafe one at least. We can't afford this on the SDK I believe as it will be crashing the end-users apps.

This can be a nice feature though, I think we will eventually end up having SAGP as a single integration point

@romtsn
Copy link
Member Author

romtsn commented Jan 5, 2022

Submitted #250 to address your review @cortinico

@cortinico
Copy link
Contributor

Thanks for the clarifications @romtsn 馃憤

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants