Skip to content

fix: explicitly set jvm target compatibility #103

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 4, 2021
Merged

fix: explicitly set jvm target compatibility #103

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion services/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
plugins {
kotlin("jvm")
`maven`
maven
`maven-publish`
id("org.jetbrains.dokka")
}
Expand Down Expand Up @@ -43,6 +43,18 @@ subprojects {
}
}


// this is the default but it's better to be explicit (e.g. it may change in Kotlin 1.5)
tasks.compileKotlin {
kotlinOptions.jvmTarget = "1.6"
Copy link
Contributor

Choose a reason for hiding this comment

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

question

Why not set this to 1.8 if that's what the outgoing variant thing will be of? Or, what does keeping at 1.6 get us?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1.6 is the default the kotlin compiler already was using.

It gets us 100% Android compatibility and honestly there's no reason to change it since we don't miss out on anything in Kotlin with it turned on right now (that may change in the future). As far as I can tell the kotlin compiler options are taking precedence here w.r.t bytecode compatibility. The java settings are affecting the gradle metadata we are publishing though.

I wasn't able to set the java settings to 1.6 as the dependency selection failed (other libraries we are using have a 1.8 outgoing variant). FWIW amplify-android has these set to 1.8 and use the R8 desugaring capabilities. I'm not 100% sure what the right answer is on this one yet and because of that I'm actually happy to punt this down the line a bit further if we want.

The jvm 11 setting we were seeing is because when we compiled and published the artifact gradle was running with JVM 11 and defaulted the metadata to that. That's not the right setting of course though since clearly we support JVM versions < 11.

KMP projects don't seem to suffer the same default behavior for some reason (which is both good and bad) so this is currently only affecting the generated services because they are JVM only still. When we generate them as actual multiplatform projects we will have to revisit this.

@kiiadi do you have any gradle/java wisdom to share here?

Copy link
Contributor

Choose a reason for hiding this comment

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

are we able to have different jvmTargets per "target" ? I'd imagine the android package we want to target 1.6 for compatibility there - but for all others we probably want to min-ver on 1.8 or (even better) 11.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Android and JVM share the same implementation, there is no separate Android source set. They can't be shared in KMP. The stdlib, coroutines, ktor, okhttp, etc all just have a single JVM target that just works on Android. We don't want this headache anyway, using a single target makes the most sense for maintenance and sanity reasons.

I disagree with 11. We should support as far back as reasonable and comfortable and there is nothing we need in 11 (or 8 for that matter). The issue here, and where I'm not an expert, is how all this relates to gradle telling you what does and doesn't work together. I had to set this to 1.8 for jvmTarget because some of the libraries report compatibility as 1.8+.

This whole issue stems from (1) the outgoing variant metadata was being inferred from the JVM version gradle was invoked with. That is clearly not the right default. (2) we had some initial customer feedback that hit issues when trying to build with JVM 1.8 (note I'm actually fairly positive it would work fine but gradle reports an issue that it wouldn't work because our metadata was saying it wouldn't. The actual compiled bytecode was 1.6 so this is all just a build system issue currently...or at least that is my understanding).

}
tasks.compileTestKotlin {
kotlinOptions.jvmTarget = "1.6"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI you could avoid a repeated block with this:

tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile> {
    kotlinOptions.jvmTarget = "1.6"
}


// FIXME - we can remove this when we implement generated services as multiplatform.
setOutgoingVariantMetadata()

val sourcesJar by tasks.creating(Jar::class) {
group = "publishing"
description = "Assembles Kotlin sources jar"
Expand All @@ -65,3 +77,13 @@ subprojects {

apply(from = rootProject.file("gradle/publish.gradle"))
}


// fixes outgoing variant metadata: https://github.com/awslabs/smithy-kotlin/issues/258
fun Project.setOutgoingVariantMetadata() {
tasks.withType<JavaCompile>() {
val javaVersion = JavaVersion.VERSION_1_8.toString()
sourceCompatibility = javaVersion
targetCompatibility = javaVersion
}
}