Fix #4119, #4120, and part of #59: Upgrade to Kotlin 1.6.10 [Blocked on #5402] #4937
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The check & related scripts required fairly involved reworking since the Maven install file (from which it sources Maven URL context) changed in format as part of the upgrade for rules_jvm_external. This has actually led to what seems to be more correct analysis of libraries that the build depends on, so more licenses have been added to the maven_dependencies.textproto tracking file. One unused Crashlytics dependency was removed since it was referencing an old license that doesn't exist anymore (and the artifact should be replaced in full by more recent Firebase Crashlytics dependencies that we are already using).
This addresses an underlying bug with the command executor that can, in some cases, break compute_affected_tests. It also refines some of its internal mechanisms for much better performance on expensive PRs. It also prepares the base support needed for merge queues, but the CI workflows aren't being updated in this change.
This prepares for merge queues (but doesn't quite configure the workflow for them--that will happen in a different PR), and improves how tests are computed for stacked PRs.
…xternal Conflicts: scripts/src/java/org/oppia/android/scripts/common/BUILD.bazel scripts/src/java/org/oppia/android/scripts/license/MavenDependenciesListCheck.kt scripts/src/java/org/oppia/android/scripts/license/MavenDependenciesRetriever.kt scripts/src/java/org/oppia/android/scripts/maven/GenerateMavenDependenciesList.kt scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesListCheckTest.kt scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesRetrieverTest.kt scripts/src/javatests/org/oppia/android/scripts/maven/GenerateMavenDependenciesListTest.kt
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).
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).
BenHenning
commented
Apr 1, 2023
BenHenning
commented
Apr 1, 2023
Specifically: - The TODO check was fixed by reformatting a TODO comment. - The Proguard build was fixed by upgrading kotlinx.coroutines to use a version compatible with Kotlin 1.6, as well as adding a Proguard dontwarn directive for one class that can't execute on Android.
BenHenning
added a commit
that referenced
this pull request
May 27, 2024
#4931) ## Explanation Fix part of #59 This PR introduces a simplification to Dagger setup throughout the Bazel build graph by defining a single top-level ``dagger_rules()`` invocation rather than introducing up to one per directory. The new approach actually matches Dagger documentation for how it should be setup: https://github.com/google/dagger/blob/f41033cc448eb7bdb83af2356c8802f1208d1824/examples/bazel/BUILD#L20. Semantically, this mainly serves to centralize the exported Dagger dependencies to a single library. Depending on this new top-level ``//:dagger`` library will still enable Dagger annotation processing and code generation for the library depending on it, so it's not expected to result in any significant performance improvements for the build graph. Note that this PR also makes some attempt at removing cases when Dagger isn't needed as a dependency at all (though this will be done in much more significant detail in downstream PRs), and in some cases Dagger was added just for implicitly depending on javax annotations (which can be depended on directly). There are also a number of version upgrades happening in this PR in preparation for upgrading to Kotlin 1.6 (in #4937) since older Dagger has some compatibility issues with newer Kotlin: - Dagger is being upgraded to 2.41 from the current 2.28.1. - The implicit Kotlin stdlib-jdk8 dependency is being bumped from 1.4.10 to 1.5.32 (this could be significant, but note that we are already using the 1.5.0 base Kotlin SDK). Note that this resulted in some additional Proguard exemptions for production builds. These *should* be okay to ignore, but they will be reevaluated in #4937. - Guava is being specially handled to force the Android version of Guava (and specifically 31.0.1 which is needed by the newer version of Dagger). Without this override the JRE version would be used which would cause Desugaring and runtime problems in production builds. - Other dependencies were updated or explicitly listed, as needed, for the Dagger & Guava changes: - com.google.errorprone:error_prone_annotations: 2.9.0 -> 2.11.0 - com.google.guava:failureaccess: 1.0.1 (currently used version) - com.google.j2objc:j2objc-annotations: 1.3 (currently used version) - org.checkerframework:checker-compat-qual: 2.5.5 (currently used version) - org.checkerframework:checker-qual: 3.13.0 -> 3.21.3 It was noticed that the tracking for Guava's third-party license was removed. This is due to Guava now being explicitly depended on rather than being managed by rules_jvm_external. #5397 was filed to track properly including all third-party licenses, not just those from Maven. Addressing that issue will result in Guava's license being included again in production builds. Finally, a ``ExplorationProgressListener`` binding was removed from ``ExplorationProgressModuleTest`` because it was noticed during development that it isn't actually used. ## 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 -- build infrastructure-only change. --------- Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com> Co-authored-by: Sean Lip <sean@seanlip.org>
BenHenning
changed the title
This is to ensure Jetpack Compose compatibility.
BenHenning
commented
May 27, 2024
There was a problem hiding this comment.
Self-reviewed latest changes.
FYI I downgraded Kotlin to 1.6.10 for Jetpack Compose support (per the findings in #5401).
BenHenning
added a commit
that referenced
this pull request
May 28, 2024
## Explanation Fixes #1535 Fixes part of #4120 This PR preps for the codebase-wide migration to Kotlin 1.6 (done in #4937) by first upgrading rules_kotlin from 1.5.0 alpha 2 to 1.5.0 beta 3. This upgrade, while small, produces a few nice benefits: - It removes the need for extra WORKSPACE dependency setup (thus addressing #1535). - It moves a bunch of rules_kotlin macros to new locations which results in changing nearly every ``BUILD.bazel`` file in the codebase. The changes are straightforward to review in isolation, so this PR acts as a means to help reduce the complexity of #4937 by pulling ahead these groups of ``BUILD.bazel`` changes while also moving rules_kotlin the smallest number of versions (ahead of the more major upgrades which will occur downstream). Note that this change is needed because: - We'll need to upgrade to a newer version of rules_kotlin in order to bring in Kotlin 1.6 support. - rules_kotlin moved these macros and using the old ones results in a _lot_ of build warnings that spam the local console when trying to build. Separately, this PR includes some minor trailing space cleanup in wiki/Oppia-Bazel-Setup-Instructions.md that was noticed when editing the file (since my local development environment auto-strips trailing spaces). ## 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 should only affect build infrastructure, and barely impact resulting binary builds. --------- Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com> Co-authored-by: Sean Lip <sean@seanlip.org>
BenHenning
commented
May 28, 2024
|
|
||
| kt_kotlinc_options( | ||
| name = "oppia_kotlinc_options", | ||
| warn = "error", |
There was a problem hiding this comment.
Reminder to update this to be 'report' for local builds and 'error' for CI builds.
There was a problem hiding this comment.
Is this to be done as part of this PR?
Conflicts: domain/src/main/java/org/oppia/android/domain/topic/PrimeTopicAssetsControllerImpl.kt
…tlin1.6 Conflicts: domain/BUILD.bazel third_party/versions.bzl
BenHenning
commented
May 29, 2024
There was a problem hiding this comment.
Thanks @BenHenning!
Apart from a few clarifications and a nit, everything looks good.
I did also wonder about the comment regarding FilterPerLanguageResources. If the changes fixed an error output, is it testable?
| * value via [Iteration]). | ||
| * annotating with [Iteration]. Such tests should not ever read from the [Parameter]-annotated | ||
| * fields since there's no way to ensure what values those fields will contain (thus they should be | ||
| * treated as undefined outside of tests that specific define their value via [Iteration]). |
There was a problem hiding this comment.
Nit:
Suggested change
| * treated as undefined outside of tests that specific define their value via [Iteration]). | |
| * treated as undefined outside of tests that specifically define their value via [Iteration]). |
|
|
||
| kt_kotlinc_options( | ||
| name = "oppia_kotlinc_options", | ||
| warn = "error", |
There was a problem hiding this comment.
Is this to be done as part of this PR?
| "text=(x^2-1)/(x+1)=y/2", | ||
| "expLatex=(x ^ {2} - 1) \\div (x + 1) = y \\div 2" | ||
| ) | ||
| @Iteration("numeric_-1", "type=NUMERIC_EXPRESSION", "text= - 1 ", "expLatex=-1") |
There was a problem hiding this comment.
For tests with these large spaces in the parameters, are they intentional? Ditto MathExpressionAccessibilityUtilTest.kt below.
| ) | ||
| @Iteration("-2==-2", "answer=-2", "input=-2") | ||
| @Iteration("1+3.14==1+3.14", "answer=1+3.14", "input=1+3.14") | ||
| @Iteration(" 1 + 3.14 ==1+3.14", "answer= 1 + 3.14 ", "input=1+3.14") |
There was a problem hiding this comment.
Same question about spaces.
BenHenning
added a commit
that referenced
this pull request
Jun 3, 2024
…5402) ## Explanation Fixes part of #4120 Fixes part of #1051 Similar to #5400, this brings forward changes that would otherwise go in #4937 to simplify the transition to Kotlin 1.6. Part of #4937 is introducing warnings-as-errors for both Kotlin and Java in order to reduce developer error, simplify the codebase, and minimize warnings when building (which can result in developer habits of ignoring warnings that might have real consequences to end users of the app). In order to keep the main migration PR smaller, this PR fixes all existing warnings and any new ones detected with the Kotlin 1.6 compiler that are not tied to Kotlin 1.5/1.6 API changes (those are part of #4937, instead). Fortunately, most of the changes could be brought forward into this PR. Specific things to note: - A few new issues were filed for SDK 33 deprecations caused, but not noted by, #5222): #5404, #5405, and #5406 and corresponding TODOs added. This PR opts for TODOs over actual fixes to minimize the amount of manual verification needed, and to try and keep the PR more focused on non-functional refactor changes (to reduce the risk as reverting this PR may be difficult if an issue is introduced). - A lot of the fixes were removing redundant casts or null checks. - The old mechanism we used for view models is deprecated, and had a lot of problems (partially documented in #1051). This PR moves the codebase over to directly injecting view models instead of using the view model provider (thus getting rid of true Jetpack view models entirely in the codebase). - We never used the Jetpack functionality, and we were leaking a lot of context objects that could theoretically result in memory leaks. - The migration of view models in this way has already been ongoing in the codebase; this PR just finishes moving the rest of them over to remove the deprecated JetPack view model reference. - Note that this doesn't actually change the scope of the view models, and in fact they should largely behave as they always have. - ``ObservableViewModel`` was subsequently updated, and may be something we could remove in the future now that it's no longer a Jetpack view model. - The old view model binding code was removed, along with its test file exemptions. It's no longer used now that the view models have been finished being migrated over to direct injection. - Some of the binding adapters didn't correctly correspond to their namespaced properties. I _think_ that the databinding compiler was still hooking them up correctly, but they produced build warnings that have now been addressed (specifically, 'app' is implied). Some other properties were using unusual namespaces, so these were replaced with 'app' versions for consistency & correctness. - Some cases where SAM interfaces could be converted to lambdas were also addressed (mainly for ``Observer`` callbacks in UI code). - ``DrawerLayout.setDrawerListener`` was replaced with calls to ``DrawerLayout.addDrawerListener`` since the former [is deprecated](https://developer.android.com/reference/androidx/drawerlayout/widget/DrawerLayout#setDrawerListener(androidx.drawerlayout.widget.DrawerLayout.DrawerListener)). This isn't expected to have a functional difference. - Some other minor control flow warnings were addressed (such as dead code paths). - ``when`` cases were updated to be comprehensive (as this is enforced starting in newer versions of Kotlin even for non-result based ``when`` statements). - Some unused variables were removed and/or replaced with ``_`` per Kotlin convention. - Some parameter names needed to be updated to match their override signatures. - One change in ``ExitSurveyConfirmationDialogFragment`` involved removing parsing a profile ID. Technically this is a semantic change since now a crash isn't going to happen if the profile ID is missing or incorrect, but that seems fine since the fragment doesn't even need a profile ID to be passed. - Some of the test activities were updated to bind a ``Runnable`` callback rather than binding to a method (just to avoid passing the unused ``View`` parameter and to keep things a bit simple binding-wise). - Some cases were fixed where variables were being shadowed due to reused names in deeper scopes. - There were some typing issues going on in tests with custom test application components. This has been fixed by explicitly declaring the application component types rather than them being implicit within the generated Dagger code. - ``getDrawable`` calls were updated to pass in a ``Theme`` since the non-theme version is deprecated. - Some Java property references were updated, too (i.e. using property syntax instead of Java getters when referencing Java code in Kotlin). - In some cases, deprecated APIs were suppressed since they're needed for testing purposes. - Mockito's ``verifyZeroInteractions`` has been deprecated in favor of ``verifyNoMoreInteractions``, so updates were made in tests accordingly. - ``ExperimentalCoroutinesApi`` and ``InternalCoroutinesApi`` have been deprecated in favor of a newer ``OptIn`` method (which can actually be done via kotlinc arguments, but not in this PR). Thus, they've been outright removed in cases where not needed, and otherwise migrated to the ``OptIn`` approach where they do need to be declared. - In some cases, Kotlin recommends using a ``toSet()`` conversion for iterable operations when it's more performant, so some of those cases (where noticed) have been addressed. - Some unused parameter cases needed to be suppressed for situations when Robolectric is using reflection to access them. - In some cases Android Studio would recommend transformation chain simplifications; these were adopted where obvious. - There are a few new TODOs added on #3616 as well, to clean up deprecated references that have been suppressed in this PR. - ``BundleExtensions`` was updated to implement its own version of the type-based ``getSerializable`` until such time as ``BundleCompat`` can be used, instead (per #5405). - A **lot** of nullability improvements needed to happen throughout the JSON asset loading process since there was a lot of loose typing happening there. - Some Kotlin & OkHttp deprecated API references were also updated to use their non-deprecated replacements. - ``NetworkLoggingInterceptorTest`` was majorly reworked to ensure that the assertions would actually run (``runBlockingTest`` was being used which is deprecated, and something I try to avoid since it's very difficult to write tests that use it correctly). My investigations showed that the assertions weren't being called, so these tests would never fail. The new versions will always run the assertions or fail before reaching them, and fortunately the code under test passes the assertions correctly. Ditto for ``ConsoleLoggerTest``. - Some parts of ``SurveyProgressController`` were reworked to have better typing management and to reduce the need for nullability management. - Some generic typing trickiness needed to be fixed ahead of the Kotlin version upgrade in ``UrlImageParser``. See file comments & links in those comments for more context. - ``BundleExtensionsTest`` had to be changed since ``getSerializableExtra`` is now deprecated. We also can't update the test to run SDK 33 since that requires upgrading Robolectric, and Robolectric can't be upgraded without upgrading other dependencies that eventually lead to needing to upgrade both Kotlin and Bazel (so it's a non-starter; this is a workaround until we can actually move to a newer version of Robolectric). - There was some minor code-deduplication & cleanup done in ``ClickableAreasImage``. - Some incorrect comments were removed in tests (to the effect of "using not-allowed-listed variables should result in a failure."). These seemed to have been copied from an earlier test, but the later tests weren't actually verifying that behavior so the comment wasn't correct. - An unused method was removed from ``ConceptCardRetriever`` (``createWrittenTranslationFromJson``) and some other small cleanup/consolidation work happened in that class. - Some stylistic changes were done in ``TopicController`` for JSON loading to better manage nullable situations, and to try and make the JSON loading code slightly more Kotlin idiomatic. Note that overall the PR has relied **heavily** on tooling to detect warnings to fix, and automated tests to verify that the changes have no side effects. Note also that this PR does not actually enable warnings-as-errors; that will happen in a downstream PR. ## 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 -- While this changes UI code, it should change very few UI behaviors and only failure cases for those it does affect. It's largely infrastructural-only and falls mainly under refactoring/cleanup work. --------- Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com> Co-authored-by: Sean Lip <sean@seanlip.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
Some other details to note:
String.captialize,String.toLowerCase,String.toUpperCase,SendChannel.offer, andChar.toInt.Flow.lastOrNullandDeferred.asListenableFuture(to replaceSettableFuturefor safety; this also resulted in nice simplifications inCoroutineExecutorService).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-annotationwas removed as a dependency since it was never used in Bazel, and shouldn't be used (since it's support library and not AndroidX).allWarningsAsErrorsenabled 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
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.