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

Explicitly use JDK 17 through gradle toolchains #2572

Merged
merged 1 commit into from
Apr 13, 2023

Conversation

DavidFair
Copy link
Contributor

Changes
Pins the implicit JDK version to 17 (based on our CI settings) to avoid gradle sync errors with 8.x
This is because Gradle has bumped the DSL to 1.8 with version 8.x, so it highlighted the implicit mismatch going on

Issues
Fixes #2495

@nielsvanvelzen
Copy link
Member

Gradle 8 not working is actually because of a bug in AGP that I believe is fixed in the next version so I decided to wait for the next AGP version instead. We don't really need Gradle 8 features right now.

@DavidFair
Copy link
Contributor Author

Do you want me to drop the gradle upgrade?
I think the JDK pinning is still worth it, since the CI runs against 17, and this makes it expicit what we target

@nielsvanvelzen
Copy link
Member

I've read a bit about Gradle toolchains and I now agree that we should indeed start using them so everyone is always using the same JDK version. I'm not a fan of the duplicate configurations so my proposal is to either use the buildSrc folder or a allprojects/subprojects config block in our root build.gradle.kts file.

@nielsvanvelzen nielsvanvelzen added dependencies Pull requests that update a dependency file gradle enhancement New feature or request labels Mar 10, 2023
@DavidFair DavidFair force-pushed the Upgrade_to_gradle_8 branch from 1f8dc41 to 1398620 Compare March 15, 2023 03:08
@nielsvanvelzen
Copy link
Member

Sorry for the long wait. I didn't like the way this PR made it's changes, which is not your fault but just how Gradle works.

I have a much simpler way to set the Java versions for the Kotlin&Android plugins from the root build file which I've pushed to the Renovate PR (#2495). I've also kept the target at Java 1.8 because older Android versions cannot read newer Java bytecode. Compiling with a newer Java version is fine though.

If you'd like you can rebase this PR on top of master so we can still merge the changes for the shared Android SDK numbers (those would go in the main build file now) and possibly the toolchains too. My apologies for taking a few weeks and redoing the primary part of your PR.

@DavidFair DavidFair force-pushed the Upgrade_to_gradle_8 branch from 1398620 to 22a4c10 Compare April 5, 2023 14:49
@DavidFair DavidFair marked this pull request as draft April 5, 2023 14:50
@DavidFair
Copy link
Contributor Author

DavidFair commented Apr 5, 2023

@nielsvanvelzen no problem, I understand gradle making things interesting

The diff now boils down to:

  • Moving our jdk (pinned to 1.8) into the versions catalogue
  • Explicitly specifying our toolchain is 17 to match the CI
  • Using the Plugin mechanism to remove the duplicated SDK levels for Android targets I've dropped this as I've seen the simpler solution you ran with 👍

So I just want to check if you want all of these commits, or if you want some dropped?

@DavidFair DavidFair force-pushed the Upgrade_to_gradle_8 branch from 22a4c10 to 41a80cc Compare April 5, 2023 16:04
@DavidFair DavidFair changed the title Upgrade to gradle 8.0.2 Explicitly use JDK 17 through gradle toolchains Apr 5, 2023
@DavidFair DavidFair marked this pull request as ready for review April 5, 2023 16:05
@nielsvanvelzen
Copy link
Member

Looks good, my only suggestion would be to not use the version catalog for those versions because they won't be updated by Renovate and they also need to be changed in the GH workflows when we update them. Putting something in the catalog creates the assumption that it's the "source of truth" (which it is for all other stuff right now).

@DavidFair
Copy link
Contributor Author

Do you want me to drop the commit moving them into the catalog, or put them somewhere else?

@nielsvanvelzen
Copy link
Member

Just leave both versions hardcoded in the build files

@DavidFair DavidFair force-pushed the Upgrade_to_gradle_8 branch from 41a80cc to 0a67d3d Compare April 12, 2023 19:59
Explicitly mark that we want version 17 of the JDK as the compiler. Previously
we implicitly relied on Gradle getting this right. This means devs and
CI will all use the same JDK explicitly rather than implicitly.

Pin the target JDK version in build.gradle.kts, this should apply to all
targets (including Kotlin) to version 1.8 to maintain compatibility with
older devices
@DavidFair DavidFair force-pushed the Upgrade_to_gradle_8 branch from 0a67d3d to ecfb2c7 Compare April 12, 2023 20:02
Copy link
Member

@nielsvanvelzen nielsvanvelzen left a comment

Choose a reason for hiding this comment

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

Thanks!

@nielsvanvelzen nielsvanvelzen merged commit 4bb2158 into jellyfin:master Apr 13, 2023
@nielsvanvelzen nielsvanvelzen added this to the v0.16.0 milestone Apr 13, 2023
@DavidFair DavidFair deleted the Upgrade_to_gradle_8 branch April 18, 2023 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request gradle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants