Fix part of #2747: Disable Gradle in CI #5629
Conversation
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Self-reviewed, but this will be easiest verified from the CI run in this PR. The Gradle build and tests should not run (nor should they run once the PR is merged).
Coverage ReportResultsCoverage Analysis: SKIP ⏭️ This PR did not introduce any changes to Kotlin source or test files.
|
|
PTAL @adhiamboperes and @seanlip. Note that I will merge this on approval from either of you as it's blocking the SDK 34 PR (#5604) which is required for the upcoming 0.14 release of the app. |
APK & AAB differences analysisNote that this is a summarized snapshot. See the CI artifacts for detailed differences. DevExpand to see flavor specificsUniversal APKAPK file size: 19 MiB (old), 19 MiB (new), 0 bytes (No change) APK download size (estimated): 17 MiB (old), 17 MiB (new), 42 bytes (Added) Method count: 260202 (old), 260202 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6818 (old), 6818 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 18 MiB (old), 18 MiB (new), 0 bytes (No change) Configuration hdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 49 KiB (old), 49 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 86 KiB (old), 86 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 57 KiB (old), 57 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) AlphaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 4 bytes (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 6 bytes (Added) Method count: 116280 (old), 116280 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5786 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 4 bytes (Removed) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) BetaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 8 bytes (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 6 bytes (Added) Method count: 116286 (old), 116286 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5786 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 4 bytes (Removed) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) GaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 4 bytes (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 9 bytes (Added) Method count: 116286 (old), 116286 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5786 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 0 bytes (No change) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) |
adhiamboperes
left a comment
adhiamboperes
left a comment
There was a problem hiding this comment.
LGTM, thanks @BenHenning!
|
LGTM, thanks @BenHenning. Is there a tracking issue for the steps needed for Gradle removal? Might be worth linking to it here. |
|
Ah sorry ignore that, it's literally part of the title :P |
|
Thanks both. Going ahead and merging this to unblock downstream. |
## Explanation Fixes #5012 Removes references and code corresponding to KitKat (SDK version 19) since the app has been set to a minimum version of Lollipop (SDK 21) since #3910 (roughly--technically KitKat was still supported but we no longer shipped a KitKat version). Because of this, the changes in this PR are largely a no-op from a build perspective. More broadly, this change is motivated by a desire to decrease CI time (which was already reduce considerably in the recent merge of #5629) by removing extraneous app builds being done in CI: - ``//:oppia`` since we should really only use ``//:oppia_dev`` moving forward. - Dev and alpha KitKat builds (the latter of which takes a long time since only the alpha job was building 2 proguarded builds of the app and thus taking much more time than the beta and GA build jobs). Separately, ``build_tests.yml`` and ``unit_tests.yml`` were updated to no longer support caching since we disabled this behavior a while back (#2884) and are unlikely to reintroduce it due to high storage costs. Some other noteworthy changes: - The optional providing of ``Retrofit`` and Retrofit services has been reverted since it's no longer necessary (and corresponding tests have been removed since there's no longer any condition to verify). - Code that was gated to run below Lollipop has been removed since it can't execute except on KitKat devices. - Support for a main dex target list has been removed as we're unlikely to need it with native multidex support (which wasn't an option for KitKat builds), though we may choose to reintroduce it to speed up cold app starts since it _can potentially_ help improve time-to-splash app startup performance. - Updated manifest min SDK values to avoid potential developer confusion on the min SDK version supported (though, strictly speaking, this doesn't technically matter with the way the app builds). - This updates the default min version supported for OS deprecation. No additional work is needed to actually activate the deprecation notice on the KitKat version of the app since we no longer deploy it, and OS deprecation wasn't added to the last version that was deployed. - Three file content pattern failure messages were updated to no longer reference KitKat (though the value of these checks mostly still seems realized, so I think it's fine to keep them in). - Some additional licenses were added due to tooling around the dev AAB (even though these aren't dependencies that ship with the app). I think it's fine including extra licenses in this case. - The protobuf license was corrected to BSD 3-Clause as declared for the dependency and in the GitHub repository for the dependencies. - This bumps version codes due to removing two flavors (for KitKat dev and KitKat alpha). - ``NetworkModuleTest`` has had its tests redesigned and better filled in in order to pass the code coverage minimum requirement (since old tests were removed). Note that it's not quite perfect in what it's testing, but it is at least verifying the complete configuration of the ``Retrofit`` instance, and the singleton properties of the two services currently supported (verifying that the services are set up would require a lot more testing that seems outside the direct scope of this test, so it seems okay to ignore here). - As part of the previous item's work, ``NetworkLoggingInterceptor`` had an issue fixed where it could cause an OkHttp exception to be thrown when trying to process logs (due to reading the response body twice). Reverting this change will now cause breakage failures in ``NetworkModuleTest``. - ``NetworkModule``'s initialization order was changed for slightly better network request logging (see the new comment in that file for more context). Note that there are some workflow job removals and renames in this PR. Any required CI checks will be correspondingly updated once this PR is approved and ready to merge (to avoid blocking any other in-flight PRs). ## 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 This shouldn't have an side effects on non-KitKat UIs, and KitKat support itself is being removed in this PR (so there's nothing to demonstrate).
## Explanation Fixes oppia#5012 Removes references and code corresponding to KitKat (SDK version 19) since the app has been set to a minimum version of Lollipop (SDK 21) since oppia#3910 (roughly--technically KitKat was still supported but we no longer shipped a KitKat version). Because of this, the changes in this PR are largely a no-op from a build perspective. More broadly, this change is motivated by a desire to decrease CI time (which was already reduce considerably in the recent merge of oppia#5629) by removing extraneous app builds being done in CI: - ``//:oppia`` since we should really only use ``//:oppia_dev`` moving forward. - Dev and alpha KitKat builds (the latter of which takes a long time since only the alpha job was building 2 proguarded builds of the app and thus taking much more time than the beta and GA build jobs). Separately, ``build_tests.yml`` and ``unit_tests.yml`` were updated to no longer support caching since we disabled this behavior a while back (oppia#2884) and are unlikely to reintroduce it due to high storage costs. Some other noteworthy changes: - The optional providing of ``Retrofit`` and Retrofit services has been reverted since it's no longer necessary (and corresponding tests have been removed since there's no longer any condition to verify). - Code that was gated to run below Lollipop has been removed since it can't execute except on KitKat devices. - Support for a main dex target list has been removed as we're unlikely to need it with native multidex support (which wasn't an option for KitKat builds), though we may choose to reintroduce it to speed up cold app starts since it _can potentially_ help improve time-to-splash app startup performance. - Updated manifest min SDK values to avoid potential developer confusion on the min SDK version supported (though, strictly speaking, this doesn't technically matter with the way the app builds). - This updates the default min version supported for OS deprecation. No additional work is needed to actually activate the deprecation notice on the KitKat version of the app since we no longer deploy it, and OS deprecation wasn't added to the last version that was deployed. - Three file content pattern failure messages were updated to no longer reference KitKat (though the value of these checks mostly still seems realized, so I think it's fine to keep them in). - Some additional licenses were added due to tooling around the dev AAB (even though these aren't dependencies that ship with the app). I think it's fine including extra licenses in this case. - The protobuf license was corrected to BSD 3-Clause as declared for the dependency and in the GitHub repository for the dependencies. - This bumps version codes due to removing two flavors (for KitKat dev and KitKat alpha). - ``NetworkModuleTest`` has had its tests redesigned and better filled in in order to pass the code coverage minimum requirement (since old tests were removed). Note that it's not quite perfect in what it's testing, but it is at least verifying the complete configuration of the ``Retrofit`` instance, and the singleton properties of the two services currently supported (verifying that the services are set up would require a lot more testing that seems outside the direct scope of this test, so it seems okay to ignore here). - As part of the previous item's work, ``NetworkLoggingInterceptor`` had an issue fixed where it could cause an OkHttp exception to be thrown when trying to process logs (due to reading the response body twice). Reverting this change will now cause breakage failures in ``NetworkModuleTest``. - ``NetworkModule``'s initialization order was changed for slightly better network request logging (see the new comment in that file for more context). Note that there are some workflow job removals and renames in this PR. Any required CI checks will be correspondingly updated once this PR is approved and ready to merge (to avoid blocking any other in-flight PRs). ## 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 This shouldn't have an side effects on non-KitKat UIs, and KitKat support itself is being removed in this PR (so there's nothing to demonstrate).
## Explanation Fixes part of #2747 Fixes #1532 This PR is doing a lot to address the vast majority of #2747--removing support for Gradle. It doesn't address everything, however--some items will need to be addressed in future PRs (largely new cleanup opportunities that are unlocked with the removal of Gradle). However, it does remove the entirety of Gradle definition infrastructure. This PR specifically: - Removes all Gradle build definitions (``.gradle`` files, ``settings.gradle``, the ``gradlew`` wrappers, and the ``gradle/wrapper`` directory). As a side benefit, this removes some of the last binary files from the codebase. :) - Removes a bunch of Gradle-specific files not actually used by the Bazel builds, including proguard-rules.pro files (Bazel has separate definitions under ``config/proguard/``), some manifest files, and some Robolectric-specific definitions. - Deletes the ``main.yml`` workflow (which was disabled in #5629). This is entirely a Gradle-specific workflow, so there's nothing that needs to be moved over as all of what's being tested is covered more efficiently and thoroughly in other specific workflow files. - Updates to ``.gitignore`` per the Gradle file removals (fewer files/directories need to be ignored). - Removed all TODOs on #1532 and marking it as fixed as part of this PR rather than performing the file rename. See #1532 (comment) for an explanation on why. - Some comments have been updated rather than addressed where it made sense that addressing the problem was not exactly specific to Gradle support (there's a desire to remove all references to 'Gradle' as a clear way to ensure everything Gradle-specific has been successfully removed from the codebase--this has largely been done). - The infrastructure and definitions for segmenting tests between Bazel and Gradle has been completely removed (resulting in broad changes in the codebase) since Gradle-specific behaviors can't be tested anymore, and Bazel-specific behaviors are now the default. - Some of the waiting logic in ``StateFragmentTest`` and ``ExplorationActivityTest`` has been kept but had its removal TODO removed. This is because the code seems to have changed (or the original problem misunderstood). These waits seem actually necessary due to relying on real image loading, so the fix is more complex than simply "migrate to Bazel & move off of Gradle." There's real test infrastructure work needed to fix these tests properly, and it doesn't seem sufficiently important to prioritize (though note it did seem possible to remove the waits outright in ``ExplorationActivityTest`` without issue). The longer-term infrastructure work is tracked in #1523. - ``LoadLessonProtosFromAssets`` has been made default true (since it can't ever be false with Gradle removed). See the note below on removing JSON assets for the work to fully clean this up. - The removal of ``OppiaTestRunner`` as it was only used for Gradle builds to help it set up state correctly for orchestrated tests (i.e. Espresso). We may need to reintroduce something like this in the future, but for now there's no value in keeping an unused class. - Some simplifications in ``RepositoryFile`` and test removals in ``RepositoryFileTest`` now that Gradle build directories no longer need to be filtered. - ``TestEnvironmentConfig`` was removed since there's no longer a need to test which environment tests are currently running in. - Some Espresso documentation was removed since it's clearly no longer needed, but more documentation work is still needing to be completed (this was noticed due to the removal of ``OppiaTestRunner``). - ``ApiMockLoader`` was simplified now that test config artifacts are in a single location during testing time. - ``InitializeDefaultLocaleRule`` can now have nearly all of its reflection removed since it can be directly built to reference the classes that it's interacting with (which wasn't possible in the Gradle world, see the TODO in that class). Some new tests were added, as well, which brings its test coverage to 100%. - Updates CODEOWNERS to reflect all of the many file changes from the above. - Broad removal of the word 'module' in **many** locations and documentation. This term is generally replaced with the more architectural term 'layer' as to not overload 'module' from a Gradle, Bazel, and Dagger perspective (plus others). This required a lot of changes specifically in the Android Lint tool implementation and tests to try and keep the term 'module' specific to cases where it still makes sense (other than Dagger which is the bulk of the existing reference). - `TransformAndroidManifest` was updated to support adding top-level comments in manifest files (which are TODOs for #6010 to move or remove manifest files--see below). Tests were also added to test the new functionality. - A bunch of files were changed simply because my local editor removed trailing spaces and they were included in those various files that I happened to change (largely for 'module' reference renames and/or 'Gradle' reference removals/rewordings). Note also that this work opens up more things that still need to be, or can now be, addressed, including: - Removal of JSON lesson loading per #5663. We're no longer testing this behavior correctly as domain level tests would previously verify JSON loading logic in Gradle builds and proto logic in Bazel builds. This means there's a strong likelihood of breakages occurring if JSON loading isn't removed. - Ensuring Espresso tests can actually run with Bazel (see #4991). We no longer can verify Espresso behaviors now that Gradle is removed, which also subsequently means all test gating on Robolectric is redundant and all tests gated on Espresso never run. - It's possible #4748 is now made redundant with the removal of Gradle-specific ``robolectric.properties`` files, though I'm not entirely certain at this point which SDK version is used in Bazel tests for Robolectric (other than it being tied to Robolectric version and that we pull in all supported SDKs so that tests can selectively opt into specific SDKs for testing). - Possibly some of the language configuration bits can be simplified now that we don't need to gracefully fail if the configuration is missing (since it never should be), but there's no plan to audit or track work for this. - Reorganization of the codebase now that Gradle no longer imposes arbitrary modules (see #59 (comment)) as well as fixing testing artifact isolation (multiple issues tracking parts of this, and ideas possibly not yet captured in issues). - `genrule`s can now be removed to improve symbol analysis in Android Studio (and general build system complexity). See #1617. - Some documentation can potentially be improved, but with the replacement of the terms 'Gradle' and 'module' (plus past documentation changes that happened as a result of moving the team over to Bazel exclusively) much of the existing documentation seems solid. - It may now be possible to update to newer versions of Kotlin and Robolectric without worrying about AGP incompatibilities. - Moving the app `AndroidManifest.xml` from the app directory to the top-level directory to address #1640. Unfortunately this can't be done as part of this PR (as previously attempted) due to #6010 and required changes to the Android Lint script. Note that the addressing of #6010 also allows other manifests to potentially be removed and, hopefully, a reduction of manifests to the bare minimum (the top-level plus any asset-holding libraries--at least for now). ## 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 There are no known UI effects from removing Gradle support, but build infrastructure is complex and this is _not_ a no-op change. It's expected that the Bazel builds are relatively the same as before, however (see the APK & AAB differences analysis), so there shouldn't be an UI or UX changes as a result of this work.
Explanation
Fix part of #2747.
In order to unblock #5604, this PR disables Gradle in CI completely. This is an extreme mitigation to the issues discovered as part of that PR, but please see #5628 (comment) and that PR's main description for a detailed account of the attempt to migrate to the latest versions of Gradle. It seems that the best path forward both for this upcoming release and long-term is to outright remove Gradle, even though Bazel is not completely developer ready. The alternative (keeping a broken version of Gradle for several months as we migrate dependencies) seems far too disruptive, and not ideal in the long-term since we want to move away from having two build systems, anyway.
Please note that I will remove the required Gradle CI checks once the PR is approved (so that it can be merged).
This change and the eventual removal of Gradle has been announced in the CLaM chat. We'll need to reach out to specific newer team members in the new year once we actually perform the removal and update corresponding documentation.
Essential Checklist
For UI-specific PRs only
This is an infrastructure only change.