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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to compileOnly dependencies for integrations #2125

Closed
romtsn opened this issue Jun 22, 2022 · 6 comments · Fixed by #2175
Closed

Switch to compileOnly dependencies for integrations #2125

romtsn opened this issue Jun 22, 2022 · 6 comments · Fixed by #2175

Comments

@romtsn
Copy link
Member

romtsn commented Jun 22, 2022

Description

At the moment we declare all integration dependencies (e.g. okhttp for sentry-okhttp-intreceptor) as implementation. This means we are potentially overriding user-defined dependencies, if they use an older version of okhttp. We should switch those dependencies to be compileOnly so at runtime we'd actually use what user has defined in their deps.

We should cautious about incompatible versions though, at best we could define version constraints for the min. supported version, at worst have a note for each integration/troubleshooting page on the docs

@marandaneto
Copy link
Contributor

We could also run a matrix test with all the supported major or minor versions, so we know that we don't introduce breaking changes as well.

@philipphofmann
Copy link
Member

We could also choose to ship with the lowest possible version of our integrations because Gradle will pick the highest one.

@marandaneto
Copy link
Contributor

We could also choose to ship with the lowest possible version of our integrations because Gradle will pick the highest one.

We already do that, we rarely upgrade our transitive dependencies unless it's needed due to security issues and so on.

@philipphofmann
Copy link
Member

We already do that, we rarely upgrade our transitive dependencies unless it's needed due to security issues and so on.

Switching to compileOnly and checking at runtime which version is available, could mean that we break the user at runtime though which is bad IMO. So I don't really get the benefit. Why not just keep it as is?

@marandaneto
Copy link
Contributor

marandaneto commented Jun 27, 2022

@philipphofmann the SDK should degrade gracefully if the desired lib isn't in the classpath.
We'd like to keep the App's direct dependency and not rely on Gradle's dependency resolution that can pick up the wrong version or override the user's defined version.
If let's say they want to use our okhttp integration, the minimum they have to do is to also have a dependency to the okhttp library themselves instead of relying on us as a transitive dependency.
By doing that, you avoid issues like #185, #2025 and getsentry/sentry-android-gradle-plugin#342
https://develop.sentry.dev/sdk/philosophy/#dependencies-cost

@romtsn
Copy link
Member Author

romtsn commented Jul 12, 2022

@philipphofmann the SDK should degrade gracefully if the desired lib isn't in the classpath.

Unfortunately, this is not always possible, because our integrations extend from the deps classes:

class SentryOkHttpInterceptor : Interceptor

So the class verification check will fail even before hitting our code, resulting in NoClassDefFoundError. I still think it should be fine though, because if they don't have an okhttp dependency, they don't have an OkHttpClient to attach our interceptor to.

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

Successfully merging a pull request may close this issue.

3 participants