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.

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

Jump to bottom

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f505b0a
THRIFT-5775 Kotlin build failed for broken toolchain in docker
thomasbruggink Sep 26, 2024
218fc71
Remove gradle toolchains from kotlin and set source/target to java 8
thomasbruggink Sep 28, 2024
b21f431
Manually set the release target for kotlin, remove overwrites from cr…
thomasbruggink Sep 28, 2024
c4720e5
Set toolchain to 17 on the kotlin lib module
thomasbruggink Oct 5, 2024
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
4 changes: 2 additions & 2 deletions build/docker/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,11 +135,11 @@ If you do not want to use the same scripts Travis CI does, you can do it manuall

Build the image:

Linux/Mac:
Linux:

thrift$ docker build --build-arg uid=$(id -u) --build-arg gid=$(id -g) -t thrift build/docker/ubuntu-jammy

Windows:
Windows/Mac:

thrift$ docker build -t thrift build/docker/ubuntu-jammy

Expand Down
14 changes: 8 additions & 6 deletions lib/kotlin/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
* specific language governing permissions and limitations
* under the License.
*/
import org.jetbrains.kotlin.gradle.dsl.JvmTarget
import org.jetbrains.kotlin.gradle.tasks.KotlinCompile

plugins {
kotlin("jvm")
Expand All @@ -34,15 +36,15 @@ dependencies {
testImplementation(kotlin("test"))
}

kotlin {
jvmToolchain {
(this as JavaToolchainSpec).languageVersion.set(JavaLanguageVersion.of(8))
}
java {
sourceCompatibility = JavaVersion.VERSION_1_8
targetCompatibility = JavaVersion.VERSION_1_8
Comment on lines +45 to +47
Copy link
Member

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.

Suggested change
java {
sourceCompatibility = JavaVersion.VERSION_1_8
targetCompatibility = JavaVersion.VERSION_1_8
kotlin {
jvmToolchain(17)

Copy link
Contributor Author

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 👍

}

tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile> {
tasks.withType<KotlinCompile> {
compilerOptions {
jvmTarget = org.jetbrains.kotlin.gradle.dsl.JvmTarget.JVM_1_8
jvmTarget = JvmTarget.JVM_1_8
freeCompilerArgs = listOf("-Xjdk-release=1.8")
Copy link
Member

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:

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

Copy link
Contributor Author

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.

}
}

Expand Down
6 changes: 0 additions & 6 deletions lib/kotlin/cross-test-client/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,6 @@ dependencies {
testImplementation("org.jetbrains.kotlin:kotlin-test-junit")
}

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

tasks {
application {
applicationName = "TestClient"
Expand Down
8 changes: 1 addition & 7 deletions lib/kotlin/cross-test-server/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,6 @@ dependencies {
testImplementation("org.jetbrains.kotlin:kotlin-test-junit")
}

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

Comment on lines -51 to -56
Copy link
Member

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.

Copy link
Contributor Author

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 🙇

tasks {
application {
applicationName = "TestServer"
Expand All @@ -68,7 +62,7 @@ tasks {

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

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?

Copy link
Contributor Author

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.

} else {
project.rootDir.resolve("../../compiler/cpp/thrift")
}
Expand Down
Loading