Fix #5370, part of #59: Migrate to Bazel 6.5.0 #4886
Conversation
|
Hi @BenHenning, 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
stale
|
Hi @BenHenning, 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
stale
BenHenning
changed the title
Also, update TODO check script to have nicer output, and support generating the exemption textproto file for easier updates in the future.
This moves the codebase to using the recommended single top-level Dagger library rather than replicating it in a bunch of different places.
This is needed for downstream work. It also includes ensuring that Guava JRE can never be used (since only Android should ever be referenced by the production app build).
There's some cleanup work needed beyond this, but this is the core change to introduce Kotlin 1.6 support (at least for dev builds). Moshi needed to be upgraded due to a metadata incompatibility when moving over to Kotlin 1.6.
rules_kotlin moved its imports into more structured bzl files to load, so this updates all references in the codebase to point to the correct locations (which removes debug warnings that show up on the CLI).
Moshi 1.14 pulls in kotlin-reflect 1.7.0 which isn't compatible with the rules_kotlin version we need for Bazel 4.x. Relatedly, this downgrades rules_kotlin to 1.5.0, but it fortunately keeps all other changes needed for 1.7.1 (which will be used in a later PR). Some code fixes were needed, too, for unknown reasons (since the build should've been using Kotlin 1.6 before). Either way, these fixes seem reasonable.
Fixed all warnings that the compiler warned about. Removed ViewModelProvider & fixed state leaking entirely by moving away from Jetpack's ViewModel as a base class (since we aren't correctly using that correctly).
|
Hi @BenHenning, 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
Enables Java warnings-as-errors, though this doesn't yet apply to kapt-generated code (such as the code from Dagger), but those warnings were still manually fixed. This also fixes a small import warning in a proto file, and warnings when building oppia_dev_kitkat (by updating the main dex list, but it's likely the build doesn't work anymore, anyway, and it's hard to test locally).
There's a race condition when building large numbers of app tests simultaneously that can lead to build failures. While the CI runs are resilient now to this failure, it'd be better to try and fix it. This removes the last non-AndroidX dependency from the codebase with hopes that it helps reduce the likelihood of the error (though there are no dependencies on it, so it's unlikely).
Conflicts: WORKSPACE app/build.gradle app/src/main/java/org/oppia/android/app/viewmodel/ViewModelBridgeFactory.kt config/proguard/kotlin-proguard-rules.pro domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/AnalyticsController.kt scripts/assets/maven_dependencies.textproto scripts/src/java/org/oppia/android/scripts/common/BUILD.bazel scripts/src/java/org/oppia/android/scripts/common/CommandExecutorImpl.kt scripts/src/java/org/oppia/android/scripts/license/MavenDependenciesRetriever.kt scripts/src/java/org/oppia/android/scripts/maven/model/MavenListDependencies.kt scripts/src/java/org/oppia/android/scripts/maven/model/MavenListDependency.kt scripts/src/java/org/oppia/android/scripts/maven/model/MavenListDependencyTree.kt third_party/BUILD.bazel third_party/maven_install.json third_party/versions.bzl tools/kotlin/BUILD.bazel utility/build.gradle utility/src/main/java/org/oppia/android/util/parser/image/UrlImageParser.kt
Got it, @BenHenning. Would that require making changes to Bazel Instructions with Windows wiki to use |
There was a problem hiding this comment.
Setting aside the existing //:oppia_dev issue on Windows, and the coverage issues with a few test targets, everything else LGTM. Thanks @BenHenning!
|
Unassigning @Rd4dev since they have already approved the PR. |
|
Hi @BenHenning, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks! |
|
@Rd4dev that's probably a good idea (the suggested Windows doc change). Will make that and then enable auto-merge. Separately, I realized that build stats won't run for this branch since it's using a newer version of Bazel. :) |
Revert changes to Windows doc.
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:
Some other important details to note:
.bazelrcwas 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).There was also some small cleanup in unit_tests.yml that I noticed when updating CI versions.
Essential Checklist
For UI-specific PRs only
N/A -- This is a build infrastructure change. It shouldn't impact the end user experience.