You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "thomasbruggink (via GitHub)" <gi...@apache.org> on 2024/09/26 14:31:43 UTC
[PR] THRIFT-5775 Kotlin build failed for broken toolchain in docker [thrift]
thomasbruggink opened a new pull request, #3043:
URL: https://github.com/apache/thrift/pull/3043
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5775 Kotlin build failed for broken toolchain in docker [thrift]
Posted by "jimexist (via GitHub)" <gi...@apache.org>.
jimexist merged PR #3043:
URL: https://github.com/apache/thrift/pull/3043
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5775 Kotlin build failed for broken toolchain in docker [thrift]
Posted by "thomasbruggink (via GitHub)" <gi...@apache.org>.
thomasbruggink commented on PR #3043:
URL: https://github.com/apache/thrift/pull/3043#issuecomment-2380448000
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5775 Kotlin build failed for broken toolchain in docker [thrift]
Posted by "jimexist (via GitHub)" <gi...@apache.org>.
jimexist commented on PR #3043:
URL: https://github.com/apache/thrift/pull/3043#issuecomment-2380719799
is there a way to test that the change you wanted to see is actually happening in the CI?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5775 Kotlin build failed for broken toolchain in docker [thrift]
Posted by "thomasbruggink (via GitHub)" <gi...@apache.org>.
thomasbruggink commented on PR #3043:
URL: https://github.com/apache/thrift/pull/3043#issuecomment-2380374599
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5775 Kotlin build failed for broken toolchain in docker [thrift]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3043:
URL: https://github.com/apache/thrift/pull/3043#discussion_r1828512723
##########
lib/kotlin/cross-test-server/build.gradle.kts:
##########
@@ -48,12 +48,6 @@ dependencies {
testImplementation("org.jetbrains.kotlin:kotlin-test-junit")
}
-kotlin {
- jvmToolchain {
- (this as JavaToolchainSpec).languageVersion.set(JavaLanguageVersion.of(8))
- }
-}
-
Review Comment:
The change is fine with me. I think it's good to explicit whenever doing compilation, whether compiling test code or not. But, I don't think it makes any substantive difference here. So, if omitting it here doesn't cause any problems, it is fine with me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5775 Kotlin build failed for broken toolchain in docker [thrift]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3043:
URL: https://github.com/apache/thrift/pull/3043#issuecomment-2380418986
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5775 Kotlin build failed for broken toolchain in docker [thrift]
Posted by "thomasbruggink (via GitHub)" <gi...@apache.org>.
thomasbruggink commented on PR #3043:
URL: https://github.com/apache/thrift/pull/3043#issuecomment-2452850853
@ctubbsii @jimexist is there anything still pending in this PR that I forgot to attend?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org