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

Setting minimum compileSdk version to 33 #234

Conversation

LikeTheSalad
Copy link
Contributor

Android's compileSdk version 34 uses Java 17 APIs which can cause compilation errors when using JDK 11 (at least it happened to me when using Robolectric in a project that depends on opentelemetry-android). I think the switch to a newer JDK version can be problematic for some projects, therefore I believe libs shouldn't enforce it (or at least not too soon), especially in this case where the AGP started to require Java 17 fairly recently.

Another, less problematic, side effect of using compileSdk 34 is that it requires upgrading Android Studio which I know several users avoid doing for a while until they have no other choice, and I wouldn't like OTel Android to be that annoying dependency...

I'm up for using the latest compileSdk version if it's necessary but at the moment that doesn't seem to be the case, so it's probably best to avoid causing these kinds of issues unless we have a good reason for it.

@LikeTheSalad LikeTheSalad requested a review from a team as a code owner January 29, 2024 16:41
@marandaneto
Copy link
Member

If you set sourceCompatibility and targetCompatibility to VERSION_1_8 or VERSION_11, it does not matter that you are compiling with Java 17, well, it shouldn't at least.

@LikeTheSalad
Copy link
Contributor Author

If you set sourceCompatibility and targetCompatibility to VERSION_1_8 or VERSION_11, it does not matter that you are compiling with Java 17, well, it shouldn't at least.

Those help with bytecode compatibility afaik, so technically speaking, it shouldn't matter at runtime but still the compileVersion 34 coming from a library when your project is on a lower one can get you these kinds of compilation errors:

* What went wrong:
Execution failed for task ':app:checkDebugAarMetadata'.
> A failure occurred while executing com.android.build.gradle.internal.tasks.CheckAarMetadataWorkAction
   > 3 issues were found when checking AAR metadata:
     
       1.  Dependency 'androidx.navigation:navigation-common:2.7.6' requires libraries and applications that
           depend on it to compile against version 34 or later of the
           Android APIs.
     
           :app is currently compiled against android-33.
     
           Also, the maximum recommended compile SDK version for Android Gradle
           plugin 7.4.0 is 33.
     
           Recommended action: Update this project's version of the Android Gradle
           plugin to one that supports 34, then update this project to use
           compileSdkVerion of at least 34.

Or this one if you happen to use Robolectric in your tests:

Failed to create a Robolectric sandbox: Android SDK 34 requires Java 17 (have Java 11)
java.lang.UnsupportedOperationException: Failed to create a Robolectric sandbox: Android SDK 34 requires Java 17 (have Java 11)
	at org.robolectric.plugins.DefaultSdkProvider$DefaultSdk.verifySupportedSdk(DefaultSdkProvider.java:172)

@marandaneto
Copy link
Member

AAR metadata

I see, target 34 then adds AAR metadata that isn't compatible, TIL, thanks.

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

I'm ok with this, but I'm not sure about the downgrades to core and navigation-fragment. As far as I can tell, those are still minSdkVersion = 14, so changing them shouldn't be part of this PR.

@LikeTheSalad this also needs a rebase. Thanks!

@marandaneto
Copy link
Member

marandaneto commented Mar 6, 2024

but I'm not sure about the downgrades to core and navigation-fragment. As far as I can tell, those are still minSdkVersion = 14, so changing them shouldn't be part of this PR.

@breedx-splk It's not about the min version but rather which version is compiled against, if any transitive dependency is compiled against 34, the problem will still exist, so you have to depend on the latest version compiled against 33 or lower.
if people use a newer version of the androidx.core compiled against 34, the gradle dependency resolver will pick the latest anyway, so it's not a biggie.

@LikeTheSalad
Copy link
Contributor Author

Yeah, as @marandaneto said, transitive dependencies can also cause this issue, which is the case with core and navigation-fragment libs. Although I also understand the concern about downgrading dependencies 😟

It's been a while since I created this PR though and I remember thinking at some point afterward that maybe we can get away with both things (i.e. avoiding enforcing the latest compileSdk version in the OTel Android SDK while also not having to downgrade any dependencies) by doing a proper separation between the "core" parts of the OTel Android SDK and its instrumentations (as we've been discussing here). For example, regarding the navigation-fragment lib, I'm guessing we only need it for the fragment lifecycle instrumentation, or I guess in other words, it's not needed for initializing the OpenTelemetry instance. If that's the case then we could extract the fragment instrumentation into its own module along with the latest version of navigation-fragment, and that way our users can pick the fragment instrumentation lib version that they'd like until they're ready to upgrade their project's compileSdk version with all of its implications, which can sometimes mean upgrading Android Studio too.

@marandaneto
Copy link
Member

That's what exactly I've done before, although I decided to keep androidx.lifecycle and androidx.core in the core package because they were needed for many default instrumentations that were part of the core package anyways.
The tradeoff was just we prefer better defaults even though it may cause a bit of a headache.
People could always have used an older version of the SDK anyways, or upgraded the tooling that would bring more benefits as well.

@LikeTheSalad
Copy link
Contributor Author

I see, thanks @marandaneto, it makes sense. So it seems like in this case if we decided to provide fragment lifecycle instrumentation as part of the "default ones" then the separation wouldn't make much difference as the latest version of the OTel Android SDK would also bring the latest version of the fragment lifecycle instrumentation with it.

Since you've dealt with this previously, I'm curious to know if you remember anybody complaining about having to upgrade their compileSdk version after upgrading to your library's latest version? Because I'm ultimately just trying to avoid unnecessary headaches for our users, but in the end, if this won't likely cause too many issues (because users can just avoid upgrading the whole SDK altogether), then we probably shouldn't scratch our heads too much about this.

@marandaneto
Copy link
Member

I see, thanks @marandaneto, it makes sense. So it seems like in this case if we decided to provide fragment lifecycle instrumentation as part of the "default ones" then the separation wouldn't make much difference as the latest version of the OTel Android SDK would also bring the latest version of the fragment lifecycle instrumentation with it.

Since you've dealt with this previously, I'm curious to know if you remember anybody complaining about having to upgrade their compileSdk version after upgrading to your library's latest version? Because I'm ultimately just trying to avoid unnecessary headaches for our users, but in the end, if this won't likely cause too many issues (because users can just avoid upgrading the whole SDK altogether), then we probably shouldn't scratch our heads too much about this.

yes, I do remember, Android native apps are usually fewer (they upgrade faster), but if Flutter or React native apps depend on this SDK, it will be a bigger problem.
I'd avoid the problem if possible, for sure, but it's also in preview state yet, so less of a headache right? unless there are users depending on it already and it'd be a deal breaker for them.

@LikeTheSalad
Copy link
Contributor Author

yes, I do remember, Android native apps are usually fewer (they upgrade faster), but if Flutter or React native apps depend on this SDK, it will be a bigger problem.
I'd avoid the problem if possible, for sure, but it's also in preview state yet, so less of a headache right? unless there are users depending on it already and it'd be a deal breaker for them.

Got it, thanks for your insights on this. I agree, we might still want to take a look at what options we have to minimize users' headaches on this, but probably for now it's quite soon to get to this point, considering that we're not stable yet and that most users should use this in android native apps, and I'm worried that if we treated this lib as stable (in the sense that we get hesitant about adding breaking changes) then that would defeat the purpose of having it marked as not-stable right now and most likely would make our progress to get to our first stable release much slower and painful 😅 So I think I'm just going to close this issue for now and take another look at it once we're done with the preview state. Cheers!

@marandaneto
Copy link
Member

Btw also this https://developer.android.com/google/play/requirements/target-sdk
The store is slowly forcing people to use the latest versions.

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