-
Notifications
You must be signed in to change notification settings - Fork 645
Migrate from kotlinOptions to compilerOptions #2746
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
Conversation
4ed7ff5
to
80f0da4
Compare
@sandwwraith PTAL while Sergey is on vacation |
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.
PR title says '..in benchmarks module', but you actually migrated to compilerOptions everywhere, right?
} | ||
} | ||
jvmToolchain(jdkToolchainVersion) |
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 wasn't here before, why is it needed?
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.
In master (2.0.20), KGP requires toolchain and and jvmTarget to be consistent. Otherwise, things like org.gradle.api.InvalidUserCodeException: Inconsistent JVM-target compatibility detected for tasks 'compileTestJava' (1.8) and 'compileTestKotlin' (11).
pop up
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.
Hm, given that we have both options.release = 8
in JavaCompile
and freeCompilerArgs.add("-Xjdk-release=1.8")
in KotlinCompile
, do we actually need toolchains?
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.
Since you've added jdkToolchainVersion
as a property, this block on line 21:
java {
toolchain {
languageVersion.set(JavaLanguageVersion.of(11))
}
}
probably should use it as well instead of JavaLanguageVersion.of(11)
Co-authored-by: Zongle Wang <[email protected]>
Co-authored-by: Zongle Wang <[email protected]>
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.
see my replies under existing comment
No description provided.