RUM-592: Support new Variant API #263
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #263 +/- ##
=============================================
- Coverage 80.56% 78.25% -2.30%
- Complexity 210 249 +39
=============================================
Files 26 31 +5
Lines 828 892 +64
Branches 110 114 +4
=============================================
+ Hits 667 698 +31
- Misses 116 144 +28
- Partials 45 50 +5
|
dd-sdk-android-gradle-plugin/src/main/kotlin/com/datadog/gradle/plugin/DdAndroidGradlePlugin.kt
Outdated
Show resolved
Hide resolved
0xnm
force-pushed
the
nogorodnikov/rum-592/support-new-variant-api
branch
from
eb41c15 to
7374873
Compare
0xnm
force-pushed
the
nogorodnikov/rum-592/support-new-variant-api
branch
from
7374873 to
70ba81c
Compare
| configureVariantForSdkCheck(target, variant, datadogExtension) | ||
| } | ||
| } else { | ||
| configureVariantForSdkCheck(target, variant, datadogExtension) |
There was a problem hiding this comment.
Shouldn't we have also the old variant based configureVariantForSdkCheck ? I can't see it in this PR anymore, just checking to make sure.
There was a problem hiding this comment.
this is right on this line. else here means it is old Variant API.
| versionName.set(variant.versionName) | ||
| versionCode.set(variant.versionCode) | ||
|
|
||
| if (extensionConfiguration.serviceName != null) { |
There was a problem hiding this comment.
why we are re - setting this here ? I saw that we already set this value when we configure the task. It makes it a bit weird to configure this property here at this level.
There was a problem hiding this comment.
That is for the NDK-based task, coming from the previous development. Maybe we can refactor this in the future.
|
|
||
| // can work probably even with lower versions, but legacy Variant API is working fine there as well | ||
| val CAN_ENABLE_NEW_VARIANT_API: Boolean | ||
| get() = TaskUtils.isAgpAbove(major = 8, minor = 4, patch = 0) |
There was a problem hiding this comment.
you could still extract those magic numbers in 3 constants here.
There was a problem hiding this comment.
What is the added value given that we use named arguments here and property name explains what we are looking for?
|
|
||
| @Suppress("UnstableApiUsage") | ||
| val allKotlin = variant.sources.kotlin?.all | ||
| return if (allJava != null) { |
There was a problem hiding this comment.
maybe you could define both kotlin and java as empty lists if they are null and then avoid the if/else below.
There was a problem hiding this comment.
allKotlin and allJava are not collections, but of Provider type.
| GitRepositoryDetector(execOps) | ||
| ).apply { | ||
| configure { uploadTask -> | ||
| @Suppress("MagicNumber") | ||
| if (DdTaskUtils.isGradleAbove(target, 7, 5)) { | ||
| if (TaskUtils.isGradleAbove(target, 7, 5)) { |
There was a problem hiding this comment.
we're still using some magic numbers here, maybe we should move this in a static constant in the CurrentAgpVersion ?
What does this PR do?
This PR brings support of new Variant API. It was originally added in AGP 7, but we didn't use it because it was missing some APIs from old Variant API.
Now we will use new Variant API for AGP 8.4.0 and above (because for the older AGP versions old Variant API works fine, but AGP 8.4.0 has a regression mentioned here).
Timeline of migration from old Variant API to the new one is here (new Variant API is completely stable with AGP 9, old Variant API is removed with AGP 10, mid-2025).
This PR should fix #242 and #257.
Review checklist (to be filled by reviewers)