-
Notifications
You must be signed in to change notification settings - Fork 29
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
[MOB-7314] Update Gradle, AGP and Maven publishing #662
Conversation
8f0970e
to
69af480
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #662 +/- ##
==========================================
+ Coverage 62.50% 62.54% +0.04%
==========================================
Files 70 70
Lines 4163 4157 -6
Branches 479 479
==========================================
- Hits 2602 2600 -2
+ Misses 1297 1291 -6
- Partials 264 266 +2 ☔ View full report in Codecov by Sentry. |
f49518d
to
5605808
Compare
5605808
to
8fc9c29
Compare
@@ -18,7 +18,7 @@ jobs: | |||
- name: Configure JDK | |||
uses: actions/setup-java@v1 | |||
with: | |||
java-version: 1.8 | |||
java-version: 11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New Gradle/AGP require Java 11+, bumped it in CI config.
@@ -28,7 +31,7 @@ android { | |||
} | |||
|
|||
debug { | |||
testCoverageEnabled true | |||
enableAndroidTestCoverage true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They changed the behavior of testCoverageEnabled
in AGP 7.x, breaking code coverage tooling. I googled the issue and found an AOSP bug ticket for this where they said they deprecated it and added separate options for unit and integration tests. Switched to only use it for integration tests, so that the Android plugin doesn't mess up unit test coverage.
@@ -80,6 +83,7 @@ dependencies { | |||
|
|||
tasks.withType(Test) { | |||
jacoco.includeNoLocationClasses = true | |||
jacoco.excludes = ['jdk.internal.*'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throws exceptions without this.
@@ -1,6 +1,6 @@ | |||
// Top-level build file where you can add configuration options common to all sub-projects/modules. | |||
buildscript { | |||
ext.kotlin_version = '1.4.32' | |||
ext.kotlin_version = '1.6.21' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New AGP required a Kotlin upgrade too.
@@ -18,6 +18,7 @@ | |||
# org.gradle.parallel=true | |||
android.enableJetifier=true | |||
android.useAndroidX=true | |||
org.gradle.jvmargs=-Xmx2048m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build sometimes was throwing OOM errors, so I bumped up heap memory for Gradle.
@@ -2,25 +2,32 @@ apply plugin: 'com.android.library' | |||
apply plugin: 'kotlin-android' | |||
|
|||
android { | |||
compileSdkVersion 29 | |||
compileSdkVersion 33 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The crypto library we're using requires compileSdkVersion 33
. It was not enforced in older versions of AGP, but once I bumped it, it wouldn't build without updating compileSdkVersion
.
androidTestImplementation 'androidx.test.espresso:espresso-core:3.2.0' | ||
androidTestImplementation 'androidx.test:runner:1.2.0' | ||
androidTestImplementation 'androidx.test:rules:1.2.0' | ||
|
||
} | ||
|
||
ext { | ||
publishedGroupId = 'com.iterable' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these properties were not used.
} | ||
|
||
apply from: 'https://raw.githubusercontent.com/nuuneoi/JCenter/master/installv1.gradle' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also removed the JCenter script since we're not publishing there anymore.
@@ -14,7 +14,7 @@ | |||
* limitations under the License. | |||
*/ | |||
|
|||
apply plugin: 'maven' | |||
apply plugin: 'maven-publish' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the changes below are to use the new Maven Publishing plugin in Gradle 7+. The old plugin is not supported, and the new one required some changes to work.
} | ||
|
||
task uploadArchives { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the new plugin is using a different name for the Gradle task, and I didn't want to change our documentation on releases, I just aliased the old task name to the new task name.
@@ -167,5 +171,5 @@ task jacocoDebugAndroidTestReport(type: JacocoReport, dependsOn: ['connectedChec | |||
classDirectories.from = files([debugTree]) | |||
additionalSourceDirs.from = files([sdkSrc, sdkUiSrc]) | |||
additionalClassDirs.from = files([sdkTree, sdkUiTree]) | |||
executionData.from = fileTree(dir: "$buildDir", include: "outputs/code_coverage/debugAndroidTest/connected/*.ec") | |||
executionData.from = fileTree(dir: "$buildDir", include: "outputs/code_coverage/debugAndroidTest/connected/**/*.ec") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this path change signify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updated versions changed where it stores the code coverage data file, adding it to a folder with the name of the device, so I had to change the pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked the changes... Added some questions for clarification. Will test it locally now
@@ -118,7 +118,7 @@ jobs: | |||
|
|||
- run: bash <(curl -s https://codecov.io/bash) | |||
- run: ./cc-test-reporter format-coverage app/build/reports/jacoco/jacocoDebugTestReport/jacocoDebugTestReport.xml --input-type jacoco -d | |||
- run: ./cc-test-reporter format-coverage iterableapi/build/reports/coverage/debug/report.xml --input-type jacoco -d | |||
- run: ./cc-test-reporter format-coverage iterableapi/build/reports/coverage/androidTest/debug/connected/report.xml --input-type jacoco -d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a residue of Java 11 change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updated version of AGP changed the path to this report.
buildToolsVersion '33.0.2' | ||
|
||
namespace 'com.iterable.iterableapi.testapp' | ||
testNamespace 'iterable.com.iterableapi' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like all build.gradle files now have namespace. Is it for better separation of context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The migration assistant moved this stuff from AndroidManifest.xml
here, namespace
and testNamespace
are new properties, replacing applicationId
in the Gradle file and package
in the xml.
@@ -1,7 +1,6 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<manifest xmlns:android="http://schemas.android.com/apk/res/android" | |||
xmlns:tools="http://schemas.android.com/tools" | |||
package="com.iterable.iterableapi.testapp"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why the package existed before then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was moved/renamed to namespace
.
@@ -6,7 +6,7 @@ apply plugin: 'kotlin-android-extensions' | |||
|
|||
android { | |||
compileSdkVersion 33 | |||
buildToolsVersion "29.0.3" | |||
buildToolsVersion "33.0.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be specified? Is there a way its automatically predetermined using current Android gradle plugin as stated here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, we can remove that too. I'd leave it for now though, I think it's better if the compile and build tools versions match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A very important PR! Thanks a tonn!
Tester App and the SDK runs smoothly and unit tests are also easily runnable via Tester app project. The problems of de-linked files (showing red lines in test classes) are all gone as well.
👍🏼
🔹 Jira Ticket(s) if any
✏️ Description
I tried updating to next minor versions first, but that required an update to
compileSdkVersion
, etc. So the only way is to update everything at once.This updates Gradle, AGP versions, as well as changes Maven publishing to use the new
maven-publish
plugin.