Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade Gradle to 8.9 and AGP to 8.7.3 #5628

Closed
wants to merge 5 commits into from

Conversation

BenHenning
Copy link
Member

@BenHenning BenHenning commented Dec 28, 2024

NB: This intentionally not using the PR template since I'm using this PR to document the attempt, not actually submit it.

This PR contains several changes meant to bring AGP (and Gradle) up to the latest necessary for Android Studio Ladybug. The requirement here is not the Android Studio support but, rather, the fact that there seems to be some specific incompatibility with older AGP versions and SDK 34 support (see #5604).

Here's essentially the steps that went into trying to make this work:

  1. Upgrade all Gradle files except app/build.gradle to the Kotlin DSL from Groovy for better IDE integration and support (since each major version migration has caused a breakage that's very difficult to fix without proper syntax highlighting and API integration--Kotlin is much better for this than Groovy). See ce6db1e. Specific notes:
    • I primarily used https://gradle-kotlin-converter.vercel.app with custom fixes afterwards (which was needed in all of the build.gradle files except the top-level one I think).
    • This also involved a lot of other removals, including some protobuf specific filtering happening in each module, the build tools declaration, and some other things that didn't actually seem necessary for successful syncing, builds, or tests (though I never checked app module tests throughout this migration).
    • This was also difficult because the first migration failed to provide syntax highlighting without a working sync (i.e. no Gradle KSL issues) and an IDE restart and sync, per https://stackoverflow.com/q/77360783. Once I had one working file, however, I could use it to migrate other files by copying in parts of those migrated files into the worked & syncing one in order to get code analysis and error highlighting.
    • https://developer.android.com/build/build-variants and https://stackoverflow.com/a/72069228 helped with some of the migration, too, as a reference.
    • Documentation unclear on groovy/kotlin-stl distinction google/protobuf-gradle-plugin#383 was exceedingly helpful in getting the model module Gradle file migrated since it's quite different than the others, and there weren't great examples available for interacting with the proto plugin with KSL.
  2. Migrate app/build.gradle to Kotlin, upgrade Gradle from 6.8 to 7.2, and upgrade AGP from 4.2.2 to 7.1.3. This also upgraded the Google Play Services and Firebase Crashlytics plugins, required updating the build to use Java 11 instead of 1.8, required a small change in the model package build to avoid duplicates (per https://stackoverflow.com/q/67265308 and Gradle 7.2 support google/protobuf-gradle-plugin#522 (comment); I struggled a lot with getting this bit working since I couldn't figure out exactly how to customize the processResources step of the module's build pipeline), and some small manifest changes per AGP deprecations. See 058030e. Note that this also disables several required custom work for app builds (such as test sharding) to speed along the attempt to migrate--the intent was to bring this back later in the PR if the migration was successful.
  3. Upgrade Gradle from 7.2 to 7.5 and AGP from 7.1.3 to 7.4.2. This caused an irrecoverable version conflict in transitive dependencies that needed to be explicitly and manually overwritten (https://stackoverflow.com/a/69832319). It also exposed an incompatibility with the frontend IR compiler (which we enabled for Jetpack Compose, see https://youtrack.jetbrains.com/issue/KT-51717 for the issue)--I disabled it in order to get a successful sync. See f16b432.
  4. Upgrade the protobuf Gradle plugin from 0.8.17 to 0.9.4 since it's needed for the next Gradle/AGP upgrade. Some weird import issues started here--I assume there was something moved around. Switching to a wildcard import fixed the problems, though it would be nice to properly fix it. See 9d30487.
  5. Upgrade Gradle from 7.5 to 8.9 and AGP from 7.4.2 to 8.7.3. See 70f4119. Specific details:
    • This had a bunch more automated migrations including moving some manifest details to Gradle files (similar to Bazel) and a stricter requirement to use kapt over annotationProcessor.
    • This upgrade unfortunately ran into a compiler incompatibility with Kotlin 1.6.10 that didn't seem like it was fixed until 1.9.x, so this PR also upgraded Kotlin for Gradle to 1.9.24 and that's where the migration attempt ended. See https://stackoverflow.com/a/78506238 and https://youtrack.jetbrains.com/issue/KT-69541 for the specific issue.

NB: One or more of these versions also required upgrading to even newer Java versions on the host side for Gradle (which I'm not a fan of--I don't like that the host Java can be different than the build system's configuration since that can result in substantial behavioral skew for team members). I don't remember which one, but I ended up using the embedded JetBrains Java 21 JRE that comes bundled with Android Studio Ladybug.

Note that each Gradle & AGP upgrade was done initially automatically through the Android Studio upgrade plugin (https://developer.android.com/build/agp-upgrade-assistant#run-upgrade-assistant). I started with Bumblebee (the version we require for Gradle work) and later moved to Ladybug once the project was sufficiently updated. The specific Gradle/AGP version bumps were direct recommendations from the upgrader (hence why not every upgrade attempt was a major version). I'm not sure how the upgrader decides on the next version, but I essentially upgraded to the latest that Bumblebee would allow before moving over to Ladybug.

Originally, this migration was thought to be impractical due to a fundamental incompatibility with either the protobuf generation in model or the multi-test configuration being done in app module (I can't remember which, and I couldn't find the old discussions when this was analyzed several years ago). I didn't run into unavoidable problems with either module (though I did not actually verify that the app module tests were working, only non-app tests).

The migration attempt above can be summarized as such:

  • Attempt to upgrade to SDK 34 (in Fix #5535: Upgrade builds to target SDK 34 #5604). Observe an incompatibility with AGP that suggests a need to upgrade to a very new version of AGP.
  • Attempt to use Ladybug--failed. Attempt to use Bumblebee and upgrade--failed with difficult debugging options.
  • Attempt to upgrade to Kotlin DSL--success (mostly since a lot of the custom Gradle work was temporarily removed).
  • Attempt to upgrade Gradle from 6.8 to 7.2, then 7.5, and finally 8.9. Correspondingly, attempt to upgrade AGP from 4.2.2 to 7.1.3, then 7.4.2, and finally 8.7.3. All but the last upgrade synced successfully with other fixes, including disabling frontend IR in the Kotlin compiler.
  • Final upgrade resulted in a Kotlin compiler bug that requires upgrading to Kotlin 1.9.24.
  • Upgrading to Kotlin 1.9.24 introduces significant incompatibility with the app's old dependencies which would likely require significant version upgrades across the app.

For context, the upgrade to SDK 34 worked with very little trouble on Bazel despite using the same version of Kotlin and (roughly) the same version dependencies. Also, this upgrade is necessary since the current beta version of the app (0.12) will expire after 31 December 2024, and per https://developer.android.com/google/play/requirements/target-sdk we are required to upgrade to SDK 34 in order to deploy new versions of the app.

This moves all Gradle files except app/build.gradle from Groovy to the
Kotlin DSL.
The build does not currently work, but it does actually sync. Hopefully
this will provide a foundation for upgrading to something that AS
Ladybug can support an upgrade with.
This also disables frontend IR (which might break Compose) since it has
an incompatibility with the older Kotlin compiler the project is using.
It's not obvious why the AGP upgrade triggered this, but the entire
build system is quite complex to follow.
This is needed for the next Gradle/AGP upgrade, so doing it a commit
early to make it a bit easier.
The build is even more broken at the moment as it's running into a
Kotlin compiler issue that may require either upgrading to Kotlin 1.9.x
(which would result in a lot of dependency updates) or downgrading AGP
(if that's even an option with SDK 34).
@BenHenning
Copy link
Member Author

BenHenning commented Dec 28, 2024

I've opened this PR and written up the main PR description comment to try and capture the entirety of:

  • What we tried to do here (upgrade to a very new version of Gradle and AGP).
  • How we did that (the steps of the migrations and concessions/temporary removals).
  • Why that needs to happen now (cannot upgrade to SDK 34 without it).
  • Why we cannot mitigate the previous option (would break Gradle builds for team members).
  • Why we can't finish this migration (would require upgrading to Kotlin 1.9.x and like updating dozens or hundreds of our dependencies, resulting in potentially substantial failures and/or behavioral skew in the app).

It seems to me that the only option right now is actually to go ahead with #2747. The cost to migrate Gradle seems far too high (guessing several more days of effort) and far too risky (substantial behavioral skew in dependency upgrades before an imminent release). I don't want to outright remove Gradle to unblock the release, but we will need to merge #5604 and the only way to do that will be to disable Gradle. I'll be sending out another PR to do that, and cite this PR as the primary reason.

I do want to admit one specific thing: it may be possible that an earlier version of AGP + Gradle that doesn't require a Kotlin upgrade may work for the SDK 34 fix. I can't be sure of this since the main problem we ran into with the SDK upgrade sounds like it requires a fairly new version of AGP to fix (https://stackoverflow.com/a/76562982/3689782). This may actually be a viable alternative, but I'm not sure that I'm willing to bolt more hacks onto an already very fragile Gradle configuration only to have it fail on us again for a future upgrade.

@BenHenning BenHenning closed this Dec 28, 2024
@BenHenning BenHenning deleted the upgrade-gradle-and-agp branch December 28, 2024 22:46
BenHenning added a commit that referenced this pull request Dec 29, 2024
## 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
- [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".
- [ ] 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 is an infrastructure only change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant