-
Notifications
You must be signed in to change notification settings - Fork 4k
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
THRIFT-5775 Kotlin build failed for broken toolchain in docker #3043
base: master
Are you sure you want to change the base?
Conversation
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@ctubbsii thanks for the review. Output of the added commit shows class files are java 8:
|
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 |
Thanks for the pointers, I missed the nuance of the Unfortunately this conflicts with manually setting a toolchain since it generates conflicting build options. Let me know what you think. |
4c1d1fc
to
5687710
Compare
…oss-test-client/server
5687710
to
b21f431
Compare
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 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 To reproduce in the docker image:
|
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? |
I believe its because of the use of This is why my original commit was to just install JDK 8 into the container ^^ |
@@ -68,7 +62,7 @@ tasks { | |||
|
|||
task<Exec>("compileThrift") { | |||
val thriftBin = if (hasProperty("thrift.compiler")) { | |||
file(property("thrift.compiler")) | |||
file(property("thrift.compiler")!!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this change do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
kotlin { | ||
jvmToolchain { | ||
(this as JavaToolchainSpec).languageVersion.set(JavaLanguageVersion.of(8)) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙇
java { | ||
sourceCompatibility = JavaVersion.VERSION_1_8 | ||
targetCompatibility = JavaVersion.VERSION_1_8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still want to set the toolchain for now, to make sure that an appropriate JDK is available.
java { | |
sourceCompatibility = JavaVersion.VERSION_1_8 | |
targetCompatibility = JavaVersion.VERSION_1_8 | |
kotlin { | |
jvmToolchain(17) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, seems with the current configuration i can set the toolchain to 17 and all targets to 8 👍
compilerOptions { | ||
jvmTarget = org.jetbrains.kotlin.gradle.dsl.JvmTarget.JVM_1_8 | ||
jvmTarget = JvmTarget.JVM_1_8 | ||
freeCompilerArgs = listOf("-Xjdk-release=1.8") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
freeCompilerArgs = listOf("-Xjdk-release=1.8") | |
freeCompilerArgs += listOf("-Xjdk-release=1.8") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.