Modernize build (Java 8 compatiblity, fix build warnings, remove dead code) #2297

mateuszkwiecinski · 2023-05-20T20:28:52Z

I thought it makes sense to go through the project and refresh it a bit, just to fix some of the warnings visible during the compilation.
If you prefer, I can split the changes into multiple PRs, drop the unnecessary changes or simply close the PR.

The biggest change I'm proposing is to ensure library bytecode is compatible with Java 8 and I'm proposing a centralized way to set Java version the project is compiled with.
My reasoning is we can't change java runtime version on the device, and given

Java 8 language features are available for everyone via desugaring - source
Java 11 features are now also available via desugaring, but some of the features requre additional setup via coreLibraryDesugaring - source
only Android API 33 introduces Java 11 runtime: https://developer.android.com/about/versions/13/features#core-libraries
:lottie project has minSdk 16 which means the library has to support Java 6 runtime (with subset of Java 8 & 11 features available via desugaring)

I couldn't find the reason why would one want to allow Java 17-compatible bytecode. Please do correct me if I'm wrong here. AFAIU current state allows using Java 11 or Java 17 language features, the app will compile just fine but will fail in runtime when installed on device with lower API (lower java runtime). This hasn't happened yet, mostly because the only offence was wrong generic type definition, which wasn't a problem since generics are not present in the bytecode (or at least that's what I remembered).

I tried to keep the project unchanged, I didn't bump any dependencies, didn't change what's being tested on CI, didn't change Java version the project is compiled with. I aimed to make sure the project gets more resilient in terms of the environment it is invoked in. All of these are things I'd love to change, but separately and later :)
The state in this PR makes the project compile on my machine (with Java 20 installed) and doesn't produce most of the warnings I saw originally.

mateuszkwiecinski · 2023-05-20T20:30:26Z

build.gradle

+  def compileJavaVersion = JavaVersion.VERSION_17
+  def targetJavaVersion = JavaVersion.VERSION_1_8


This is what I meant as centralized place to define Java compatibility.
Right now the project compiles with Java 17 (via toolchains), but produces artifacts compatible down to Java 8

mateuszkwiecinski · 2023-05-20T20:32:11Z

issue-repro-compose/src/main/AndroidManifest.xml

@@ -1,6 +1,5 @@
 <?xml version="1.0" encoding="utf-8"?>
-<manifest xmlns:android="http://schemas.android.com/apk/res/android"
-    package="com.airbnb.lottie.issues.compose">


setting package in AndroidManifest is now deprecated and produces a build warning

mateuszkwiecinski · 2023-05-20T20:34:07Z

lottie/build.gradle

-task sourcesJar(type: Jar) {
-  from android.sourceSets.main.java.srcDirs
-  archiveClassifier.set('sources')
-}
-
-task javadoc(type: Javadoc) {
-  source = android.sourceSets.main.java.srcDirs
-  configurations.implementation.setCanBeResolved(true)
-  classpath += project.files(android.getBootClasspath().join(File.pathSeparator)) + configurations.implementation
-}
-


I checked publish task dependencies, and it seemed like these two weren't actually used 🤷

There is sourceReleaseJar and javaDocReleaseJar used respectively instead. I assume they are configured by vanniktech's plugin?

mateuszkwiecinski · 2023-05-20T20:34:27Z

lottie/src/main/java/com/airbnb/lottie/LottieDrawable.java

@@ -1323,7 +1323,7 @@ public <T> void addValueCallback(
   */
  public <T> void addValueCallback(KeyPath keyPath, T property,
      final SimpleLottieValueCallback<T> callback) {
-    addValueCallback(keyPath, property, new LottieValueCallback<>() {
+    addValueCallback(keyPath, property, new LottieValueCallback<T>() {


This was the only compilation error after I enabled Java 8 compatiblity

mateuszkwiecinski · 2023-05-20T20:35:08Z

...enchmark/src/main/java/com/airbnb/lottie/sample/compose/benchmark/LottieBaselineBenchmark.kt

    private fun UiScrollable.waitUntilReady() {
        this.waitUntil {
            // We know that there are at least 9 children
            childCount >= EXPECTED_ITEM_INDEX_COUNT
        }
    }

-    @OptIn(ExperimentalTime::class)
-    private fun UiObject.clickAndWait(maxWaitTime: Duration = Duration.seconds(5)) {
+    private fun UiObject.clickAndWait(maxWaitTime: Duration = 5.seconds) {


It seems like these parts of the project aren't invoked on the CI that's why the main branch contains non-compiling code

mateuszkwiecinski · 2023-05-20T20:37:51Z

sample/build.gradle

+    if (requested.group == "androidx.lifecycle" && requested.name == "lifecycle-viewmodel") {
+      useVersion versionFor(project, AndroidX.lifecycle.viewModel)
+    }


AFAICS release artifacts are never built (which means it is not tested if library obfuscates correctly?). If you try to run assembleRelease you'll get Duplicate class androidx.lifecycle.ViewModelLazy found in modules (...) error.

I'm proposing to enforce single artifact version across all configurations

Is there an incompatibility between two androidx versions that we could solve by using the right dependencies?

It's not really incompatibility of libraries, but a result of the build setup. The build (transitively) pulls both

androidx.lifecycle:lifecycle-viewmodel:2.5.1 (by androidx.appcompat:appcompat)
and

androidx.lifecycle:lifecycle-viewmodel-ktx:2.2.0 (by i.e. androidx.paging:paging-runtime-ktx).

It works in debug variant because debugImplementation libs.androidx.fragment.testing depends on viewmodel-ktx in proper version, but in release mode there is no such dependency 🤷

I pushed an update, that adds the viewmodel-ktx dependency regardless of the build variant (so it's not a transitive dependency anymore)

mateuszkwiecinski · 2023-05-20T20:39:17Z

sample/src/androidTest/java/com/airbnb/lottie/samples/FragmentVisibilityTests.kt

@@ -158,7 +158,7 @@ class FragmentVisibilityTests {
            }
        }

-        val scenario = launchFragmentInContainer<TestFragment>(themeResId = R.style.Theme_AppCompat)
+        val scenario = launchFragmentInContainer<TestFragment>(themeResId = AppcompatR.style.Theme_AppCompat)


AGP starting from 8.0 has android.nonTransitiveRClass set to true by default https://developer.android.com/build/releases/gradle-plugin#default-changes

mateuszkwiecinski · 2023-05-20T20:39:47Z

snapshot-tests/build.gradle

@@ -2,8 +2,7 @@ import static de.fayard.refreshVersions.core.Versions.versionFor

 plugins {
  id 'com.android.application'
-  id "kotlin-android"
-  id 'kotlin-kapt'


I couldn't find a place where kapt is used

mateuszkwiecinski · 2023-05-20T20:41:32Z

lottie/build.gradle

-}
-
-tasks.withType(JavaCompile) {
+tasks.withType(JavaCompile).configureEach {


withType() {} configures all tasks eagerly: https://docs.gradle.org/current/userguide/task_configuration_avoidance.html

gpeal

Overall, this looks good to me. Thanks for doing this. Just one question about the duplicate androidx classes.

gpeal · 2023-06-03T22:05:27Z

sample/build.gradle

+    if (requested.group == "androidx.lifecycle" && requested.name == "lifecycle-viewmodel") {
+      useVersion versionFor(project, AndroidX.lifecycle.viewModel)
+    }


Is there an incompatibility between two androidx versions that we could solve by using the right dependencies?

gpeal

Overall, this looks good to me. Thanks for doing this. Just one question about the duplicate androidx classes.

mateuszkwiecinski · 2023-06-04T17:53:01Z

Please let me know if I should investigate CI failures, if you suspect they are caused by any of my changes 🤔
I'm fairly sure one failure was caused by using @master version of emulator-wtf instead of stable v0 tag, the other one looked mysterious to me 👀

gpeal · 2023-06-05T16:14:22Z

@mateuszkwiecinski Apparently the emulator wtf branch changed from master to main but you can change it to v0

mateuszkwiecinski · 2023-06-05T18:08:08Z

😞 That wasn't it :/ pinning the version to v0 wasn't enough. ~~Is the issue known to you? Or shall I report the issue upstream: https://github.com/emulator-wtf/invoke/blob/19541297ededf0812afa9cc061ee7f199ec23f44/src/index.ts#L169~~ I went ahead and reported here: emulator-wtf/run-tests#2

gpeal · 2023-06-05T19:04:06Z

Just reached out to Madis Pink from emulator.wtf. He'll fix this.

madisp · 2023-06-05T19:44:14Z

😞 That wasn't it :/ pinning the version to v0 wasn't enough. ~~Is the issue known to you? Or shall I report the issue upstream: https://github.com/emulator-wtf/invoke/blob/19541297ededf0812afa9cc061ee7f199ec23f44/src/index.ts#L169~~ I went ahead and reported here: emulator-wtf/run-tests#2

Hey, we're terribly sorry about this :( It should be fixed now.

In addition we changed our release process a bit so the full tags are 100% stable and deterministic. E.g. if you depend on run-tests@v0.9.5 then all of the versions (downstream GHA action versions, ew-cli version) will remain fixed so there won't be any surprise failures like this.

mateuszkwiecinski · 2023-06-13T20:05:47Z

@gpeal Please let me know if you want me to pin a specific version as Madis suggested. Otherwise, it should be enough to just restart the build 🤞

gpeal · 2023-06-13T20:25:23Z

@mateuszkwiecinski Go ahead an pin it!

google/dagger#3701

https://docs.emulator.wtf/integrations/github-action/

github-actions · 2023-06-13T20:52:53Z

Snapshot Tests
API 23: Report Diff
API 31: Report Diff

gpeal

Thanks for doing this!

mateuszkwiecinski commented May 20, 2023

View reviewed changes

mateuszkwiecinski marked this pull request as ready for review May 22, 2023 16:10

gpeal reviewed Jun 3, 2023

View reviewed changes

mateuszkwiecinski added 19 commits June 13, 2023 22:35

Prefer using plugins block everywhere

96b1562

Unify how org.jetbrains.kotlin.android is applied

a0f7cd9

Ensure each subproject is compiled with the same compile settings

0207750

Centralize Java target + bring back Java 8 compatibility

5c23872

Bump dagger to work around incompatible metadata issue

64d7e29

google/dagger#3701

Workaround for duplicated class issue

df5127e

Remove not needed sourcesets manipulation

8323bc5

Run ./gradlew refreshVersions

f6ac11a

Unify kotlin-parcelize plugin application

d0a9872

Remove package definition from AndroidManifest

a91438c

Move reproducer projects code to src/main/kotlin

244a824

Fix deprecation warning

2e7a536

Prefer lazy task configuration

1619129

Remove unused pieces of code

187377e

Remove kapt from snapshot-tests

fc67b49

Fix androidTest compilation

c6bfcb2

Bump androidx.paging & androidx.lifecycle to latest versions

e5e84fa

Run ./gradlew refreshVersions

6e95ebe

Use v0 version of emulator-wtf/run-test

e10941c

https://docs.emulator.wtf/integrations/github-action/

Use v0.9.6 version of emulator-wtf/run-test

0eafb44

https://docs.emulator.wtf/integrations/github-action/

mateuszkwiecinski force-pushed the update_build branch from 050f34e to 0eafb44 Compare June 13, 2023 20:38

gpeal approved these changes Jun 13, 2023

View reviewed changes

gpeal merged commit cc8fbca into airbnb:master Jun 13, 2023
6 checks passed

mateuszkwiecinski deleted the update_build branch June 14, 2023 07:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize build (Java 8 compatiblity, fix build warnings, remove dead code) #2297

Modernize build (Java 8 compatiblity, fix build warnings, remove dead code) #2297

mateuszkwiecinski commented May 20, 2023 •

edited

mateuszkwiecinski May 20, 2023

mateuszkwiecinski May 20, 2023

mateuszkwiecinski May 20, 2023

mateuszkwiecinski May 20, 2023

mateuszkwiecinski May 20, 2023

mateuszkwiecinski May 20, 2023

gpeal Jun 3, 2023

mateuszkwiecinski Jun 4, 2023

mateuszkwiecinski May 20, 2023

mateuszkwiecinski May 20, 2023

mateuszkwiecinski May 20, 2023

gpeal left a comment

gpeal Jun 3, 2023

gpeal left a comment

mateuszkwiecinski commented Jun 4, 2023

gpeal commented Jun 5, 2023

mateuszkwiecinski commented Jun 5, 2023 •

edited

gpeal commented Jun 5, 2023

madisp commented Jun 5, 2023

mateuszkwiecinski commented Jun 13, 2023

gpeal commented Jun 13, 2023

github-actions bot commented Jun 13, 2023

gpeal left a comment

		def compileJavaVersion = JavaVersion.VERSION_17
		def targetJavaVersion = JavaVersion.VERSION_1_8

Modernize build (Java 8 compatiblity, fix build warnings, remove dead code) #2297

Modernize build (Java 8 compatiblity, fix build warnings, remove dead code) #2297

Conversation

mateuszkwiecinski commented May 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpeal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpeal left a comment

Choose a reason for hiding this comment

mateuszkwiecinski commented Jun 4, 2023

gpeal commented Jun 5, 2023

mateuszkwiecinski commented Jun 5, 2023 • edited

gpeal commented Jun 5, 2023

madisp commented Jun 5, 2023

mateuszkwiecinski commented Jun 13, 2023

gpeal commented Jun 13, 2023

github-actions bot commented Jun 13, 2023

gpeal left a comment

Choose a reason for hiding this comment

mateuszkwiecinski commented May 20, 2023 •

edited

mateuszkwiecinski commented Jun 5, 2023 •

edited