Fix #1535, part of #4120: Upgrade to rules_kotlin v1.5.0 beta 3 #5400
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).
The new proto target isn't used anywhere so this was missed.
Conflicts: scripts/assets/todo_open_exemptions.textproto
…xternal Conflicts: scripts/assets/test_file_exemptions.textproto third_party/maven_install.json
Conflicts: app/src/main/java/org/oppia/android/app/translation/BUILD.bazel third_party/maven_install.json
These issues were found after I started using a new development environment.
ProfileAndDeviceIdFragmentTest had been updated to use a newer fragment initialization pattern, but that's no longer needed and seems to be causing what appears to be timing discrepancies between local dev and CI.
The issue ultimately arose from test parameters being initialized after they're needed in the launched UI. This type of change was tried earlier in the branch, but reverted since it didn't seem necessary. It is, however, necessary when there are environment differences (e.g. local vs. CI) or when running certain tests individually. Due to the difficulty in finding this issue, ActivityScenarioRule has been added as a prohibited pattern in the static regex checks (along with ActivityTestRule since that's deprecated and discouraged, anyway).
|
PTAL @adhiamboperes. Note that I suspect this will be a quicker PR to review compared to some of the others that I've sent recently. |
This was referenced May 23, 2024
BenHenning
changed the title
BenHenning
commented
May 27, 2024
adhiamboperes
approved these changes
May 28, 2024
|
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! |
|
Thanks @adhiamboperes and @seanlip! Going ahead and merging this. |
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>
adhiamboperes
added a commit
that referenced
this pull request
Jun 12, 2024
## 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 <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 #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:
BUILD.bazelfile 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.bazelchanges 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:
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
For UI-specific PRs only
N/A -- This should only affect build infrastructure, and barely impact resulting binary builds.