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 the used JDK in the project to v21 #3582

Merged
merged 10 commits into from
Oct 3, 2024
Merged

Conversation

jmartinesp
Copy link
Member

Content

Changes the Gradle config to use JDK 21, and change it in the CI flows too.

Motivation and context

JDK 21 is the new default in Android Studio Ladybug.

Tests

Let's see how the CI behaves.

Checklist

  • Changes have been tested on an Android device or Android emulator with API 23
  • UI change has been tested on both light and dark themes
  • Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
  • Pull request is based on the develop branch
  • Pull request title will be used in the release note, it clearly define what will change for the user
  • Pull request includes screenshots or videos if containing UI changes
  • Pull request includes a sign off
  • You've made a self review of your PR

@jmartinesp jmartinesp added the PR-Build For changes related to build, tools, CI/CD label Oct 2, 2024
@jmartinesp jmartinesp requested a review from a team as a code owner October 2, 2024 15:19
@jmartinesp jmartinesp requested review from bmarty and removed request for a team October 2, 2024 15:19
Copy link
Contributor

github-actions bot commented Oct 2, 2024

📱 Scan the QR code below to install the build (arm64 only) for this PR.
QR code
If you can't scan the QR code you can install the build via this link: https://i.diawi.com/5ZcSnd

@bmarty
Copy link
Member

bmarty commented Oct 2, 2024

Note: I have created #3585 to unblock develop, and partially cherry-picked 29b61ca.

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.10%. Comparing base (9f7e4d5) to head (d115c2e).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3582      +/-   ##
===========================================
- Coverage    82.38%   81.10%   -1.28%     
===========================================
  Files         1742     1753      +11     
  Lines        41680    42248     +568     
  Branches      5030     5106      +76     
===========================================
- Hits         34337    34267      -70     
- Misses        5576     6213     +637     
- Partials      1767     1768       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jmartinesp
Copy link
Member Author

Note: I have created #3585 to unblock develop, and partially cherry-picked 29b61ca.

What I don't know is how did tests even pass the CI in the previous PRs.

@jmartinesp
Copy link
Member Author

Also, ouch:

image

It seems like we'll lose almost 2% of coverage.

@jmartinesp
Copy link
Member Author

It seems like some code, *Preview ones mostly, is now taken into account for code coverage. That's odd, the Kover filters may need some tweaks.

@jmartinesp
Copy link
Member Author

jmartinesp commented Oct 3, 2024

It seems like 3977455 fixed the Preview code added to coverage issue, which added lots of untested code because their callback functions would never be called. There's still some coverage regression, but it's now -0.29%, not -1.84%.

@jmartinesp jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Oct 3, 2024
@github-actions github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Oct 3, 2024
@bmarty
Copy link
Member

bmarty commented Oct 3, 2024

Note: I have created #3585 to unblock develop, and partially cherry-picked 29b61ca.

What I don't know is how did tests even pass the CI in the previous PRs.

#3565 has been merged, then #3554 but the latest was based on an old develop branch. Result is a not compiling develop branch. We may want to mitigate this by enabling merge queue, but this was slowing down the process, and so I prefer to keep it that way and fix issues when they happen, which is quite not frequent, so acceptable.

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks. FTR I had to install JDK21 on my computer:

brew install openjdk@21
sudo ln -sfn /opt/homebrew/opt/openjdk@21/libexec/openjdk.jdk /Library/Java/JavaVirtualMachines/openjdk-21.jdk

I have added 2 cleanup commits, please review!

@jmartinesp
Copy link
Member Author

Thanks. FTR I had to install JDK21 on my computer

Thanks for the tip! This is indeed needed if you aren't using the JDK embedded in AS Ladybug.

Also, thanks for the cleanup!

@jmartinesp jmartinesp enabled auto-merge (squash) October 3, 2024 09:06
@jmartinesp jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Oct 3, 2024
@github-actions github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Oct 3, 2024
Copy link

sonarcloud bot commented Oct 3, 2024

@jmartinesp jmartinesp merged commit f8fa218 into develop Oct 3, 2024
29 of 30 checks passed
@jmartinesp jmartinesp deleted the misc/upgrade-jdk-to-21 branch October 3, 2024 09:38
flexibility2 added a commit to flexibility2/element-x-android that referenced this pull request Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Build For changes related to build, tools, CI/CD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants