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

Update gradle to 8.8 and AGP to 8.5.0 #2575

Merged
merged 25 commits into from
Jul 11, 2024
Merged

Conversation

jingtang10
Copy link
Collaborator

@jingtang10 jingtang10 commented Jun 18, 2024

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Description
Update gradle to 8.8 and AGP to 8.5.0

Alternative(s) considered
NA

Type
Builds

Screenshots (if applicable)

Checklist

  • I have read and acknowledged the Code of conduct.
  • I have read the Contributing page.
  • I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA.
  • I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach.
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project.
  • I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally.
  • I have built and run the demo app(s) to verify my change fixes the issue and/or does not break the demo app(s).

Copy link
Collaborator

@aditya-07 aditya-07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check to see if additional changes are required.
Also, is there a reason if we are updating to these specific versions?

buildSrc/build.gradle.kts Outdated Show resolved Hide resolved
gradle/wrapper/gradle-wrapper.properties Outdated Show resolved Hide resolved
@jingtang10 jingtang10 changed the title Update gradle to 8.6 and AGP to 8.4.1 Update gradle to 8.8 and AGP to 8.5 Jun 18, 2024
@jingtang10 jingtang10 changed the title Update gradle to 8.8 and AGP to 8.5 Update gradle to 8.8 and AGP to 8.5.0 Jun 18, 2024
@jingtang10
Copy link
Collaborator Author

addressed comments
thanks aditya for reviewing

Copy link
Collaborator

@MJ1998 MJ1998 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you verified compatibility of other dependencies with this agp?
I guess our test suite should be enough ?

Update: Some tests failed which is a good sign!

You might wanna take a quick look at this PR to see what all things were updated: #2064

@jingtang10
Copy link
Collaborator Author

After taking a look at the failure:

Lint found 2 errors, 38 warnings. First failure:
[1185](https://github.com/google/android-fhir/actions/runs/9566698928/job/26372611822?pr=2575#step:5:1186)
[1186](https://github.com/google/android-fhir/actions/runs/9566698928/job/26372611822?pr=2575#step:5:1187)/home/runner/work/android-fhir/android-fhir/datacapture/src/main/java/com/google/android/fhir/datacapture/extensions/MoreLocalDates.kt:128: Error: Implicit cast from IsoChronology to Chronology requires API level 26 (current min is 24) [NewApi]
[1187](https://github.com/google/android-fhir/actions/runs/9566698928/job/26372611822?pr=2575#step:5:1188)    IsoChronology.INSTANCE,
[1188](https://github.com/google/android-fhir/actions/runs/9566698928/job/26372611822?pr=2575#step:5:1189)    ~~~~~~~~~~~~~~~~~~~~~~

and trying to fix it without updating min sdk version, I realised we do need the ISOChronology class in order to get the localized date time pattern... without using this class, it's possible to generate the date time formatter object that is localized, but we won't be able to get the actually localized pattern e.g. yyyy-mm-dd (by calling the api getLocalizedDateTimePattern)... but we do need this pattern itself as the hint text...

now an alternative is to not use that as hint text, but instead use a formatted date e.g. 2019-09-12... which might be ok but it will just diverge a bit from the behaviour we have when the entry format string is provided by the questionnaire.

so i think the easiest thing to do is actually update the minsdk.. which i think wasn't an issue only because of the build tool wasn't picking it up since the api was indeed introduced in api level 26... so this way we're only making what was already the case explicit... (device on 24 would have had problem with this already).

@MJ1998
Copy link
Collaborator

MJ1998 commented Jun 24, 2024

We dropped the minSdk (for all libraries) to 24 due to this issue #2077.

@pld
Copy link
Collaborator

pld commented Jun 26, 2024

Just to confirm, we're fine raising the min level to 26

@jingtang10 jingtang10 enabled auto-merge (squash) July 11, 2024 10:21
@jingtang10
Copy link
Collaborator Author

discussed today at the developers call and no objection so far to this api level change.

if we don't hear anything else we'll include this change in the next release

Copy link
Collaborator

@aditya-07 aditya-07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@jingtang10 jingtang10 merged commit a36e7a6 into google:master Jul 11, 2024
8 checks passed
@jingtang10 jingtang10 deleted the gradle branch July 11, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

4 participants