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

Make use of jvmToolchain for building the project. #583

Merged
merged 1 commit into from
May 16, 2023

Conversation

psteiger
Copy link
Contributor

@psteiger psteiger commented May 14, 2023

This allows for a more repeatable, predictable build between different local configurations.

On Kotlin modules, this is achieved by adding the following to project-level build.gradle:

kotlin {
  jvmToolchain(11)
}

On Java modules,

java {
  toolchain {
    languageVersion.set(JavaLanguageVersion.of(11))
  }
}

For non-Android Kotlin, when using the JVM Toolchain, the following is not needed anymore:

sourceCompatibility = deps.build.javaVersion.toString()
targetCompatibility = deps.build.javaVersion.toString()

For Android Kotlin modules, because of a bug present in AGP 7.4.x up to 8.1.0-alpha08, the following is still needed:

android {
    compileOptions {
        sourceCompatibility = <MAJOR_JDK_VERSION>
        targetCompatibility = <MAJOR_JDK_VERSION>
    }
}

Because AGP is bundled with the Android IntelliJ IDEA plugin, we cannot upgrade it (maximum supported version is 7.4 on 2023.1.1). We should open a new issue for removing above configuration block when a newer stable IntelliJ IDEA version ships with support for AGP 8.1.0.alpha09 or higher

Fixes #584

@psteiger psteiger force-pushed the jvmtoolchain branch 2 times, most recently from 631f374 to dd0f7aa Compare May 14, 2023 18:48
@psteiger psteiger changed the title Make use of jvmToolchain for compiling the project on JDK > 11. Make use of jvmToolchain for compiling the project. May 14, 2023
This allows for a more repeatable, predictable build between different local configurations.
@psteiger psteiger requested review from tyvsmith and jbarr21 May 14, 2023 19:05
@psteiger psteiger added the Android Android related tickets label May 14, 2023
@psteiger psteiger changed the title Make use of jvmToolchain for compiling the project. Make use of jvmToolchain for building the project. May 14, 2023
@@ -18,6 +18,11 @@ apply plugin: 'com.android.library'
apply plugin: 'org.jetbrains.kotlin.android'
apply plugin: "com.vanniktech.maven.publish"

kotlin {
Copy link
Member

Choose a reason for hiding this comment

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

We should probably apply this by default to all build targets (with the couple exceptions until we convert/fix them), so this isn't managed in ~20 spots.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed with the general statement of centralizing Kotlin module configurations. I can try to move those two to the project-level build.gradle.

With that said, the reason I did not do it on our project-level build.gradle is IMO we do config centralization poorly, in a non-Gradle-idiomatic way. We should probably be using a Gradle conventions plugin instead of those hard-to-read (to me) configurations at project-level, either built from buildSrc or includedBuilds, then we could just apply the plugin like

plugins {
 id("ribs-kotlin-convention-plugin")
}

This seems to be the Gradle-way for applying common configurations between different modules.

Perhaps we could open an issue to improve on that.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Let's punt this, and create an issue for the plugin, as well as moving the remaining java modules over and enabling explicitApi on the outlier.

@psteiger psteiger merged commit 5b9b66e into uber:main May 16, 2023
@psteiger psteiger deleted the jvmtoolchain branch May 18, 2023 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Android related tickets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use JVM toolchain for building the project
2 participants