Prototype Compose on Bazel #5401
Conversation
Conflicts: app/src/main/java/org/oppia/android/app/home/HomeFragmentPresenter.kt third_party/maven_install.json
…se-bazel-kt1.6 Conflicts: third_party/maven_install.json
BenHenning
changed the base branch from
upgrade-to-kotlin1.6
to
migrate-to-newer-bazel-and-kotlin
|
NB: I updated this to be based on #4886 instead of the rules Kotlin branch since we need to make sure this specifically works with Bazel 6.x as that's the change that needs to get merged in to unblock the code coverage GSoC project. |
This puts our attempt in-line with the rules_kotlin example closest to our version.
|
Spent a bit of time investigating this, but will need to look more into it tomorrow. So far, it seems ComposeView does have setContentView but it takes a Function0 not a Function2 (there might be another one, I only checked classes.dex in the built oppia_dev.aab file). I still need to dig into HomeFragmentPresenter.class to see if there's actually Compose code being generated by the plugin (since it's hard to tell if it's actually running), and I need to verify that rules_kotlin's version works for us, too: https://github.com/bazelbuild/rules_kotlin/blob/f07c7687c6a63058c2910c91f9f976a468085cb9/examples/jetpack_compose. |
|
Pretty sure the plugin isn't running; I commented out the plugins line in app/BUILD.bazel and it didn't change the build graph at all. Will dig into what's going on. |
|
Aha, the issue is that the android_library() we're using to wrap all third-party dependencies seems incompatible with kt_compiler_plugin(). Using the Maven target directly actually activates the plugin, though I'm still working to find the correct version list that will build with our Bazel/Kotlin setup. This is a reasonable workaround to figure out whether it'll work. I've actually been thinking about removing our wrappers and using aliases instead. It should have better build performance and compatibility. |
|
Progress! Coursier (the utility that handles compiling the transitive closure of compatible Maven dependencies in rules_jvm_external) doesn't seem to handle beta/prerelease versions well, so we needed to find a non-prerelease version compatible with activity-compose version 1.4.0. This turned out to be 1.1.1. Unfortunately, the Compose compiler is very picky about the Kotlin compiler version (https://www.jetbrains.com/help/kotlin-multiplatform-dev/compose-compatibility-and-versioning.html#kotlin-compatibility though some of these are wrong), so we needed to downgrade to 1.6.10 from 1.6.21. From there, the Compose compiler seems to be running. Launching the app now results in a new failure: |
@theMr17 solved the issue on gradle this way. |
|
Ah yeah I just came across https://stackoverflow.com/q/66382502/3689782. Upgrading appcompat seems to work. I'm not keen at all these runtime dependency issues with Compose...hopefully the Robolectric tests can catch these problems otherwise it's going to introduce a lot of manual testing complexity for the team. That being said, it works!
Another issue I've noticed is that sticky header (https://developer.android.com/reference/kotlin/androidx/compose/foundation/lazy/LazyListScope#stickyHeader(kotlin.Any,kotlin.Any,kotlin.Function1)) requires Compose 1.2.0 (https://developer.android.com/jetpack/androidx/releases/compose-foundation#1.2.0) which will require Kotlin 1.7.x. Going to see if I can also get the sticky header working since all this effort won't really matter if we need to upgrade Kotlin right away, anyway. Edit: though maybe the documentation is wrong? |
|
Surprisingly, stickHeader seems to work just fine actuallyactually, without needing to update any versions. I expanded the prototype example to match more the example in the docs: jetpack_compose_sticky_header_bazel_demo.mp4 |
|
Also separately: using alias() seems to work fine. I'll pull that forward to one of the Bazel branch PRs (not sure which one yet). |
This fully demonstrates a working Bazel build, plus sticky headers working without issue.
|
I'll double check this branch once I pull some of the compatibility changes earlier into the chain, but everything seems to be working now. |
|
Also: wanted to mention that I ultimately found that android_library() was the culprit from bazelbuild/rules_kotlin#319 suggesting kt_jvm_import() and considering that it may behave differently than android_library(). That was also the only obvious difference from the example setup in the rules_kotlin repo. |
This looks pretty good! @theMr17, could you please pull these changes and verify your dummy UI in theMr17@673b75c? Please confirm that you can build with Bazel. |
…se-bazel-kt1.6 Conflicts: third_party/maven_install.json
|
Okay I pulled forward some of the extra changes needed here so now I think this PR is very close to the smallest number of changes needed to demonstrate working Jetpack Compose. I also verified that the functionality seems to build and work correctly with a Proguard build (oppia_beta); surprisingly no new Proguard exemptions are needed which is nice to see. De-assigning myself now since I don't think there's anything else for me to investigate at this point. |
@adhiamboperes I pulled these changes and added the dummy UI. I can confirm everything works fine. Screen-Recording.mp4 |
|
|
||
| # This exposes a patch that fixes an issue with duplicate processor arguments. See: | ||
| # github.com/bazelbuild/rules_kotlin/pull/940. | ||
| exports_files(["remove_processor_duplicates.patch"]) | ||
|
|
||
| kt_kotlinc_options( | ||
| name = "oppia_kotlinc_options", | ||
| warn = "error", | ||
| warn = "report", |
There was a problem hiding this comment.
@BenHenning, is it intentional changing the WError to report here?
There was a problem hiding this comment.
As discussed in the CLaM meeting, this was just to simplify this prototype PR though I've made a note in the Kotlin upgrade PR to make this a bit more lenient for local development (but still strict for CI so that we can continue to keep build warnings at a minimum).
|
Thanks @BenHenning, @theMr17! This is nice to see. |
|
Hi @adhiamboperes, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
oppiabot
bot
added
the
stale
## Explanation Fixes #4119 Fixes #4120 Fixes part of #59 This PR finishes the migration of the codebase to Kotlin 1.6 (addressing both #4119 and #4120). Kotlin 1.6 is needed as part of moving rules_kotlin to 1.7.x (which is, in turn, needed in conjunction with Bazel 6.x to enable strict dependency checking which significantly simplifies modularization which is planned for downstream PRs). This PR doesn't actually finish the movement to that version of rules_kotlin, but it does finish moving the codebase to a new enough (and no longer pre-release) version of rules_kotlin to allow using Kotlin 1.6 (over Kotlin 1.4 that the codebase currently uses): version 1.5.0. Previous PRs (#5400 and #5402) prepared for the changes here by addressing large categories of build warnings that have either arisen from this migration, or from past work. Note that another large category of warnings have also been addressed in this PR: by moving to Kotlin 1.6, there's no longer a runtime incompatibility between the Kotlin SDK and the reflection APIs (which was causing a _lot_ of warning output previously). Between all three PRs, the output is now very clean and free of nearly all build warnings. To try and keep the warnings clean long-term, this PR introduces warnings-as-errors for both Java and Kotlin code. However, please note some caveats: - Dagger generated code doesn't follow the Java warnings-as-error flag, so those warnings were cleaned up manually (and will need to be generally watched for, unfortunately). - The version of rules_kotlin used in this PR doesn't directly support turning on the functionality, but does internally (so a small patch file has been added to augment rules_kotlin). When the codebase is updated to rules_kotlin 1.7.x this patch will no longer be needed. - To ease development, a build configuration flag was added to disable failure upon encountering build warnings (per https://bazel.build/run/bazelrc and https://bazel.build/docs/configurable-attributes#query-and-cquery as an example), though this needs to be opted into: ```sh bazel build --config=ignore_build_warnings <target> ``` Some other details to note: - Version 1.6.10 is specifically picked in order to ensure Jetpack Compose compatibility (for preparation of the work being prototyped in #5401 to be compatible with the Oppia Android build environment). - The vast majority of code in this PR is updating parameterized tests to use a cleaner repeatable annotation pattern that wasn't available in Kotlin 1.4. - This upgrade absolutely does have runtime implications, but we're relying very heavily on existing automated tests to ensure correctness and no regressions. - This PR doesn't make an effort to move toward newer Kotlin language features except where forced (API deprecations) or largely wanted (the repeatable annotation change). - android-spotlight and kotlitex have been updated to support newer versions of Kotlin (as both are custom forks managed in the broader Oppia GitHub organization). - Gradle files have been updated to match the same dependency versions as Bazel (where it was obvious to make changes; some might still be a bit off). - The Gradle build configuration was also updated to use Kotlin 1.6.x (otherwise there would be build incompatibilities with Bazel). I think this is the last upgrade we can do for Gradle without upgrading AGP (which will cause us significant issues with the model module, so we're planning on instead dropping Gradle support). - API changes that needed to be addressed in this PR due to deprecations include: ``String.captialize``, ``String.toLowerCase``, ``String.toUpperCase``, ``SendChannel.offer``, and ``Char.toInt``. - New API changes that have been leveraged in this PR: ``Flow.lastOrNull`` and ``Deferred.asListenableFuture`` (to replace ``SettableFuture`` for safety; this also resulted in nice simplifications in ``CoroutineExecutorService``). - The JVM coroutines dependency needed to be split out from Maven and manually imported with some empty internal Java class files since it otherwise has some issues being desugared: bazelbuild/bazel#13553. This is a problem with the Desugarer used in Bazel 4.x (and maybe later versions, so this solution will probably need to kept for a while). - Some Proguard rule updates were needed due to Kotlin SDK changes--see the Proguard file & comments for specifics. - Due to dependency changes, the KitKat main dex file was also trimmed down. I'm fairly certain that it's already crashing on startup, so I don't care much about this change--it just needs to build. We plan to remove KitKat entirely eventually, anyway: #5012. - Jetifier (that is, automatic conversion from support libraries to Jetpack/AndroidX) support was disabled in Gradle. We don't have it enabled in Bazel, and it could potentially encourage strange one-version violations if it was ever actually needed. This is a safer (and likely more performant) change to make. - Moshi was updated to 1.13 to support the upgrade in Kotlin. This did result in a small configuration change due to its annotation processor being moved. Note that Moshi 1.14 couldn't be supported since it requires Kotlin 1.7+ which requires rules_kotlin 1.7+. This will be an option to upgrade in the future. - Some improvements and fixes were made in ``FilterPerLanguageResources`` (I think it was outputting something incorrectly before and that's now been fixed as part of a broader logical reworking of the filtering logic). - ``com.android.support:support-annotation`` was removed as a dependency since it was never used in Bazel, and shouldn't be used (since it's support library and not AndroidX). - The updates to Moshi and Kotlin dependencies resulted in a bunch of other transitive dependency updates. - Note that Gradle doesn't have ``allWarningsAsErrors`` enabled since it would require fixing more warnings than is exposed in Bazel, and we're using Bazel builds as the general source of truth for code quality. ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only N/A -- This is an infrastructural change. While it could inadvertently affect user-facing code, it shouldn't based on the current passing state of automated tests. --------- Co-authored-by: Adhiambo Peres <[email protected]> Co-authored-by: Sean Lip <[email protected]>
## Explanation Fixes #5370 Fixes part of #59 This PR updates the project to use Bazel 6.5.0 instead of 4.0.0. Note that most of the changes done so far in addressing #59 are centered around the concept of simplifying the Bazel maintenance as much as possible so that it's not too much more difficult than Gradle by the time we fully remove Gradle support from the project. While Bazel will always require more effort, there are many things that can be done to narrow the gap. This is a major step in that process since Bazel 4.x required using a custom Android toolchain (https://github.com/oppia/oppia-bazel-tools) which is not at all user friendly. Plus, there are many compatibility and performance improvements in later versions of Bazel that we want to be able to incorporate within the broader Oppia Android project. Bazel 6.x was specifically chosen because: - Bazel 4.x was missing support for the new D8 version which made it impossible to upgrade past 29.0.2 build tools bazelbuild/bazel#13989. - Bazel 5.x had some additional compatibility issues with the D8 change, so we weren't able to use it, either: bazelbuild/bazel#15957. - Bazel 7.x (which wasn't released when this work was originally done) introduces new bzlmod support that causes some additional build headaches that can be figured out later. - Bazel 6.5.0 specifically was chosen since it's the latest 6.x version (as of this edit) and seems to work correctly with existing unit tests. Some other important details to note: - rules_kotlin 1.7.x is needed at a minimum for Bazel 5.x+ support. However, an additional fix was needed (bazelbuild/rules_kotlin#940) in order to fix a deviation in functionality that occurred starting in Bazel 5.x's java_plugin support which led to some file duplication in rules_kotlin (that was fortunately easy to fix). Unfortunately, this change wasn't backported to 1.7.x so this PR makes use of a custom patch to rules_kotlin 1.7.1 (https://github.com/oppia/rules_kotlin) that includes the needed change. We'll get this change properly once we can upgrade to 1.8.x, though that will also require updating Kotlin itself to 1.8.x due to bazelbuild/rules_kotlin#1019. - Bazel 6.x (maybe 5.x) requires at least build tools 30.0.0 since it completely removed support for the old D8 compat dexer. 32.0.0 was chosen in this PR as it's simply a newer, more up-to-date build tools (and removes D8 completely). With this upgrade to Bazel 6.x we'll be able to update the build tools version more often (so long as it doesn't introduce AGP incompatibilities since we can't upgrade Gradle). - As of Bazel 6.x, we're able to reenable Java header compilation and incremental dexing, both of which should have _significant_ performance improvements for incremental builds of the app (and in fact we will have build errors if we disable incremental dexing). - In CI, we opted to **not** support build tools 29.0.2 or old builds of the app. Instead, we'll rely on build tools failing for certain PRs as an indicator that those PRs will require an update (once this PR is merged) in order to have CI run correctly. This is a lot easier than trying to figure out how to support before/after changes with some fairly complex environment differences. - There are a bunch of version updates that were needed to support the minimum version of Kotlin for rules 1.7.x (1.6 I think) as well as JDK 11 (which I think was needed for Bazel 5.x), and these have largely been taken care of in previous PRs to this one (though the JDK 11 update in CI was done in this PR, along with wiki documentation updates to address #5370). One such case of a necessary version upgrade: google/dagger#2511. - There was a change needed for the databinding java_plugin declaration to specify that it generates an API (in order for it to be used correctly in builds). - rules_java needed to be updated to support the newer version of Bazel. - The desugaring hack needed for kotlinx-coroutines-core-jvm was removed since it's no longer needed with the build tools & Bazel upgrade introduced in this PR. - This includes one small change in third-party to change all single-export wrappers that don't have additional plugins being enabled to aliases instead. This is more semantically correct as the wrappers may lose information (which caused problems when investigating adding Jetpack Compose support in #5401). While this isn't directly required for the Bazel upgrade, this is the last PR needed for Jetpack Compose support so it's being added here for simplicity. - ``.bazelrc`` was updated to configure tools, tests, and builds to all use the remote JDK 11 available via Bazel rather than ever using the user's local JDK. This should improve build hermeticity and consistency across different user environments (see https://bazel.build/docs/bazel-and-java). - Setup docs were updated to remove setting up JDK 11 (or Java at all for Linux & Mac) now that the user no longer needs to install Java (see previous point) except for Windows. The Python instructions were also removed since Bazel 6.x includes fixes for Android tools that previously depended on Python 2.x. - CI was unchanged for Java setup since, as far as I can tell, it's still needed for sdkmanager. There was also some small cleanup in unit_tests.yml that I noticed when updating CI versions. ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only N/A -- This is a build infrastructure change. It shouldn't impact the end user experience. --------- Co-authored-by: Adhiambo Peres <[email protected]> Co-authored-by: Sean Lip <[email protected]>
Explanation
Add compose dependencies.
The main problems with this PR are:
Additional notes:
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: