THRIFT-5775 Kotlin build failed for broken toolchain in docker #3043

thomasbruggink · 2024-09-26T14:31:40Z

This PR adds JDK 8 to both docker containers to support the kotlin build.
Kotlin requires toolchain 8 and cant build this without the JDK for that language level being present.

Also correct readme since docker desktop on Mac also fixes the permissions with volume sharing automatically.

This PR adds JDK 8 to both docker containers to support the kotlin build. Kotlin requires toolchain 8 and cant build this without the JDK for that language level being present. Also correct readme since docker desktop on Mac also fixes the permissions with volume sharing automatically.

ctubbsii

While this workaround probably fixes the issue with running the tests in Docker, it really shouldn't be necessary to install JDK 8 in order to build. JDK 17 should be sufficient.

A better change would be to modify the kotlin files build.gradle.kts (in a few places) to remove the configuration that forces the use of the JDK 8 toolchain, and change it to allow the use of JDK 17, the same as for the Java build, but with the proper configuration options set to allow it to cross compile to Java 8.

For more information see https://jakewharton.com/kotlins-jdk-release-compatibility-flag/ and the linked Kotlin issues. I don't know enough about Kotlin to immediately know which specific combination of flags/config will work correctly so that the bytecode is Java 8 compatible, but the toolchain is using 17, like the Java library build. That article also hints that toolchains shouldn't be used at all, but I don't know enough about Gradle to remove their use, and that seems like a separate issue. For now, it should be possible to configure these properly to use the JDK 17 toolchain cross-compiling to Java 8 (not just with the byte code for Java 8, which can be done with the jvmTarget option, but also restricted to using APIs that are available for Java 8, which I think can be done with -Xjdk-release or maybe options.release = 8, like is done in the Java build configuration; both might be needed or some similar config).

I trust @jimexist knows more about these options than I do, so I've requested their review.

thomasbruggink · 2024-09-28T02:48:55Z

@ctubbsii thanks for the review.
Sure we can also change the kotlin build gradle files to build without the toolchain option. Let me submit a commit that does this and let me know which option you prefer.
I'll rebase and remove the commit that we dont end up.

Output of the added commit shows class files are java 8:

> file cross-test-client/build/classes/kotlin/main/org/apache/thrift/test/TestClientKt.class     
cross-test-client/build/classes/kotlin/main/org/apache/thrift/test/TestClientKt.class: compiled Java class data, version 52.0 (Java 1.8)

ctubbsii · 2024-09-28T05:44:41Z

Merely changing the source and target is not enough. You also need API compliance with the cross compilation to Java 8. For javac, that's what the --release flag does, in addition to setting the source and target. We need something similar implemented here for the Kotlin stuff. We don't actually need to remove the toolchain, because that's a separate matter. It's fine to keep the toolchain and set it to 17, like we do for Java. But we need to make sure the Kotlin option is set that is equivalent to the javac release flag. That's why I linked to the additional detailed information about that option. I'm just not sure the right way to use it. However, I can tell that your added commit doesn't do that. It only sets the source and target.

thomasbruggink · 2024-09-28T06:54:27Z

Thanks for the pointers, I missed the nuance of the --release flag.
As the blog post you linked points out, there currently isn't a flag in the Kotlin Gradle plugin that allows setting the -Xjdk-release flag. However it can be done by using the freeCompilerArgs as pointed out in the same blogpost.

Unfortunately this conflicts with manually setting a toolchain since it generates conflicting build options.
It seems the way that worked in the end is to manually define the source/target compatibility for the java plugin, then tell the Kotlin compiler to use target 1.8 and set the release flag.
I think this way we could keep the cross-test-client and cross-test-server running with installed JDK?

Let me know what you think.

…oss-test-client/server

jimexist · 2024-09-28T16:01:46Z

is there a way to test that the change you wanted to see is actually happening in the CI?

thomasbruggink · 2024-09-30T13:31:24Z

is there a way to test that the change you wanted to see is actually happening in the CI?

The issue can be reproduced by only installing Java 11 or newer. It seems that the action runner for Ubuntu 22.04 has Java 8 installed by default https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2204-Readme.md#java
The toolchain option in gradle says that automatically detects toolchains, so I think it can find the JDK 8 installation and will use javac from the JDK 8 installation. This in contrast to the docker build environment where it needs to use the JDK 17 javac and produce something that works on java 8.

Maybe this would be testable by manually removing JDK 8 from the runner. But the fact that we now removed the toolchain option and added -release and it still compiles might also be proof that it works as expected.

To reproduce in the docker image:

$ docker run -v $(pwd):/thrift/src -it thrift /bin/bash
$ ./bootstrap.sh
$ ./configure
$ make all # <- this will brake on the kotlin compile step before this PR

jimexist · 2024-10-01T00:47:01Z

this will brake on the kotlin compile step before this PR

i wonder why the GitHub action didn't catch this? or maybe we should add that step in the GitHub action to ensure it works?

thomasbruggink · 2024-10-01T01:14:52Z

I believe its because of the use of toolchain which searches for the correct JDK version and all github action runners come pre-installed with JDK 8, 11, 17 and 21.
This in contrast to the container where we only have JDK 17 installed.

This is why my original commit was to just install JDK 8 into the container ^^

ctubbsii · 2024-10-01T07:59:30Z

lib/kotlin/cross-test-server/build.gradle.kts

@@ -68,7 +62,7 @@ tasks {

    task<Exec>("compileThrift") {
        val thriftBin = if (hasProperty("thrift.compiler")) {
-            file(property("thrift.compiler"))
+            file(property("thrift.compiler")!!)


What does this change do?

it's not really a relevant change, property() returns any? while file expects any resulting in a warning when compiling. Explicitly casting this from any? to any with !! removes the warning.

ctubbsii · 2024-10-01T09:15:11Z

lib/kotlin/cross-test-server/build.gradle.kts

-kotlin {
-    jvmToolchain {
-        (this as JavaToolchainSpec).languageVersion.set(JavaLanguageVersion.of(8))
-    }
-}
-


Are the cross-test-client and cross-test-server build.gradle.kts files inheriting from config from lib/kotlin/build.gradle.kts ? It's not obvious to me why nothing is needed in these files.

It's not, so removing the toolChain here results in using whatever JDK is configured on the host system.
Running the cross-test tools with the default JDK configured on the system is perhaps not a bad thing? Since this results in behavior similar to a real world user of the kotlin lib and they might also use any JDK >= 8.

If you prefer this to also have a -release target of 8 or perhaps make server 8 and client 17 let me know 🙇

ctubbsii · 2024-10-01T09:17:40Z

lib/kotlin/build.gradle.kts

+java {
+    sourceCompatibility = JavaVersion.VERSION_1_8
+    targetCompatibility = JavaVersion.VERSION_1_8


I think we still want to set the toolchain for now, to make sure that an appropriate JDK is available.

Suggested change

java {

sourceCompatibility = JavaVersion.VERSION_1_8

targetCompatibility = JavaVersion.VERSION_1_8

kotlin {

jvmToolchain(17)

ok, seems with the current configuration i can set the toolchain to 17 and all targets to 8 👍

ctubbsii · 2024-10-01T09:18:51Z

lib/kotlin/build.gradle.kts

    compilerOptions {
-        jvmTarget = org.jetbrains.kotlin.gradle.dsl.JvmTarget.JVM_1_8
+        jvmTarget = JvmTarget.JVM_1_8
+        freeCompilerArgs = listOf("-Xjdk-release=1.8")


The docs say that setting -Xjdk-release=version automatically sets the -jvm-target version as well. I don't think it hurts to have both, though, so long as they are the same.

So, I think this part is okay. But the examples I saw appended to the list:

Suggested change

freeCompilerArgs = listOf("-Xjdk-release=1.8")

freeCompilerArgs += listOf("-Xjdk-release=1.8")

Added it for extra clarification, I dont mind removing it either, let me know.

As for the +=, the blog modifies the freeCompilerArgs afterwards, whereas we modify it here during the initial configuration. So += doesn't work here.

ctubbsii reviewed Sep 27, 2024

View reviewed changes

ctubbsii requested a review from jimexist September 27, 2024 20:03

Remove gradle toolchains from kotlin and set source/target to java 8

218fc71

thomasbruggink force-pushed the add-jdk-8-for-kotlin branch from 4c1d1fc to 5687710 Compare September 28, 2024 06:57

Manually set the release target for kotlin, remove overwrites from cr…

b21f431

…oss-test-client/server

thomasbruggink force-pushed the add-jdk-8-for-kotlin branch from 5687710 to b21f431 Compare September 28, 2024 07:18

ctubbsii reviewed Oct 1, 2024

View reviewed changes

Set toolchain to 17 on the kotlin lib module

c4720e5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THRIFT-5775 Kotlin build failed for broken toolchain in docker #3043

THRIFT-5775 Kotlin build failed for broken toolchain in docker #3043

thomasbruggink commented Sep 26, 2024

ctubbsii left a comment •

edited

Loading

thomasbruggink commented Sep 28, 2024 •

edited

Loading

ctubbsii commented Sep 28, 2024

thomasbruggink commented Sep 28, 2024

jimexist commented Sep 28, 2024

thomasbruggink commented Sep 30, 2024

jimexist commented Oct 1, 2024

thomasbruggink commented Oct 1, 2024

ctubbsii Oct 1, 2024

thomasbruggink Oct 5, 2024

ctubbsii Oct 1, 2024

thomasbruggink Oct 5, 2024

ctubbsii Oct 1, 2024

thomasbruggink Oct 5, 2024

ctubbsii Oct 1, 2024

thomasbruggink Oct 5, 2024

	freeCompilerArgs = listOf("-Xjdk-release=1.8")
	freeCompilerArgs += listOf("-Xjdk-release=1.8")

THRIFT-5775 Kotlin build failed for broken toolchain in docker #3043

Are you sure you want to change the base?

THRIFT-5775 Kotlin build failed for broken toolchain in docker #3043

Conversation

thomasbruggink commented Sep 26, 2024

ctubbsii left a comment • edited Loading

Choose a reason for hiding this comment

thomasbruggink commented Sep 28, 2024 • edited Loading

ctubbsii commented Sep 28, 2024

thomasbruggink commented Sep 28, 2024

jimexist commented Sep 28, 2024

thomasbruggink commented Sep 30, 2024

jimexist commented Oct 1, 2024

thomasbruggink commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctubbsii left a comment •

edited

Loading

thomasbruggink commented Sep 28, 2024 •

edited

Loading