search

open-telemetry / opentelemetry-android

OpenTelemetry Tooling for Android
Apache License 2.0
156 stars 41 forks source link

Make coreLibraryDesugaring optional (when minSdk >= 26) #682

Open hannesbolmstedt opened 1 week ago

hannesbolmstedt commented 1 week ago

coreLibraryDesugaring should only be required for Android API levels lower than 26 (this is stated both in the README.md and VERSIONING.md).

However, isCoreLibraryDesugaringEnabled = true is currently set in both otel.android-app-conventions.gradle.kts and otel.android-library-conventions.gradle.kts making it impossible to import the project without enabling coreLibraryDesugaring, even when using a minSdk >= 26 (correct me if I am wrong).

My suggestion is to remove these two lines in order to make coreLibraryDesugaring optional.

LikeTheSalad commented 6 days ago

I just checked and our demo app does raise a compilation issue when isCoreLibraryDesugaringEnabled = false even if the minSdk version is set to 26.

I would think that the AGP should verify the minSdk version before raising the compilation issue, though that doesn't seem to be the case. One alternative might be to use the same animalsniffer check as the upstream Java repo does, to ensure we don't add code that only works on > 26 versions and remove the AGP check from our android lib conventions. Wdyt? @breedx-splk @bidetofevil @marandaneto

marandaneto commented 6 days ago

I use ru.vyarus.animalsniffer as well so it is def. a good thing, another option is to have a sample project (just a template) with low minSdk that compiles on CI, so if compilation breaks, you know you have used something you should not, it's not guaranteed that it won't break at runtime though.

bidetofevil commented 5 days ago

Yeah I don't think it's necessary - the app is responsible for deciding whether desugaring is necessary, so the SDK doing it is a bit of an over reach. I'd remove even the library dependency and let the app take care of that. A warning or failure during build time or in CI would be nice, but not strictly necessary IMO if we've documented this.

breedx-splk commented 4 days ago

I use ru.vyarus.animalsniffer as well so it is def. a good thing, another option is to have a sample project (just a template) with low minSdk that compiles on CI, so if compilation breaks, you know you have used something you should not, it's not guaranteed that it won't break at runtime though.

Yeah I also see the value in this. Maybe if it's slow we just do it nightly, but it seems like catching the obvious/easy stuff automatically is helpful.