Fix #5535: Upgrade builds to target SDK 34 #5604
Merged
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
|
Hi @theMr17, 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
BenHenning
requested changes
Dec 27, 2024
There was a problem hiding this comment.
Thanks @theMr17! Just one comment--I think this can go into full review. Just waiting on a second CI run to see what issues exist there. PTAL.
app/src/main/java/org/oppia/android/app/utility/FontScaleConfigurationUtil.kt
Show resolved
Hide resolved
oppiabot
bot
removed
the
stale
|
FYI @theMr17 this will also need to be updated: |
|
It looks like there are some incompatibilities with Gradle and AAPT2: Digging a bit to see what's going on. Non-app tests seem to be passing fine on Gradle, and everything on Bazel seems okay so far (other than the one failing script test, though still waiting on the runs to finish). |
|
Ah, found an error in the app-specific tests that's much more specific: The past part seems particularly relevant: |
|
https://stackoverflow.com/a/77271439 immediately comes to mind. It may be the case that compiling with SDK 34 activates some AAPT2 upgrade that's not compatible with our version of AGP. If that's true, this is going to get rather interesting since we have to finish the next release before Dec 31. :) |
|
Ah, AGP shouldn't matter here. However, what likely does is build tools. I see on Bazel builds that we're using 32.0.0 but on Gradle we're using 30.0.2. Without digging into past PRs that might explain why there's a discrepancy. @theMr17 can you instead please try updating the 30.0.2 versions in each .gradle file with 32.0.0, instead? If you have time, please try building the app locally with Gradle (if you don't, let's see what CI says). I think upgrading the build tools for Gradle and fixing the failing scripts test should cover most of the problems here (assuming the build tools upgrade doesn't introduce a different Gradle compatibility problem). |
|
Hey @BenHenning, thanks for the review! Seems like updating the build tools version to 30.0.0 does not fix the aapt2 issue. Do you have any other suggestions which might be the cause of this issue? /cc @adhiamboperes. |
|
Addressed the review comment, PTAL @BenHenning. |
Coverage ReportResultsNumber of files assessed: 2 Passing coverageFiles with passing code coverage
Exempted coverageFiles exempted from coverage
|
Ah, it should be changed to 32.0.0 per my earlier message. That being said, I tried locally and it failed due to 31.x+ build tools removing dx in favor of d8 (which is actually why we upgraded to Bazel 5.x+ at the same time as changing to 32.0.0 for Bazel builds since Bazel removed support for DX). Since we're using a very old AGP, we have a fundamental incompatibility that may be especially challenging to work around. I think we're actually at an important shift here--we may not be able to upgrade to a newer version of AGP since the last time that we tried it broke our proto (model) module. I'm going to try and upgrade AGP and see how painful this will be and circle back. |
|
#5628 (comment) outlines the journey to try and get this PR working on Gradle. I don't think it's feasible. I'm going to instead send out a PR to disable Gradle, and move this PR to be based on that one (so that CI effectively passes here). |
6 tasks
BenHenning
changed the title
BenHenning
reviewed
Dec 28, 2024
There was a problem hiding this comment.
Thanks @theMr17! I think the PR LGTM with Gradle being disabled, so I'll wait for the base PR to go in before merging this (and for CI to pass before approving) but I think this does what we need in order to unblock the release.
app/src/main/java/org/oppia/android/app/utility/FontScaleConfigurationUtil.kt
Show resolved
Hide resolved
|
Requesting the build stats workflow to run on this branch since it can't actually pass on develop (due to the differences in required SDK version). Edit: Ah, it won't work since the workflow now automatically runs against develop regardless of the base branch requested. |
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.
BenHenning
approved these changes
Dec 29, 2024
BenHenning
changed the title
|
Unassigning @BenHenning since they have already approved the PR. |
|
Assigning @adhiamboperes for code owner reviews. Thanks! |
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 #5535
This PR updates all build and target SDKs for both Gradle and Bazel builds to SDK 34 to comply with the latest Play Store requirements. The key considerations for this upgrade include:
scaledDensityDeprecation: The scaledDensity property in the DisplayMetrics class is now deprecated [Feature Request]: Migrate away from scaledDensity #5625.Details about SDK 34 changes, potential issues, and mitigations can be found in the #5535 issue thread.
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: