-
Notifications
You must be signed in to change notification settings - Fork 637
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
Properly fix Java 8 API compatibility (#2218) #2350
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,15 +3,22 @@ | |
*/ | ||
|
||
import org.gradle.api.* | ||
import org.gradle.api.file.* | ||
import org.gradle.api.provider.* | ||
import org.gradle.api.tasks.* | ||
import org.gradle.api.tasks.bundling.* | ||
import org.gradle.api.tasks.compile.* | ||
import org.gradle.jvm.toolchain.* | ||
import org.gradle.kotlin.dsl.* | ||
import org.gradle.language.base.plugins.LifecycleBasePlugin.* | ||
import org.gradle.process.* | ||
import org.jetbrains.kotlin.gradle.dsl.* | ||
import org.jetbrains.kotlin.gradle.plugin.* | ||
import org.jetbrains.kotlin.gradle.plugin.mpp.* | ||
import org.jetbrains.kotlin.gradle.plugin.mpp.pm20.* | ||
import org.jetbrains.kotlin.gradle.plugin.mpp.pm20.util.* | ||
import org.jetbrains.kotlin.gradle.targets.jvm.* | ||
import org.jetbrains.kotlin.gradle.tasks.KotlinCompile | ||
import org.jetbrains.kotlin.gradle.tasks.KotlinJvmCompile | ||
import java.io.* | ||
|
||
object Java9Modularity { | ||
|
@@ -42,70 +49,154 @@ object Java9Modularity { | |
|
||
// derive the names of the source set and compile module task | ||
val sourceSetName = defaultSourceSet.name + "Module" | ||
val compileModuleTaskName = compileKotlinTask.name + "Module" | ||
|
||
kotlin.sourceSets.create(sourceSetName) { | ||
val sourceFile = this.kotlin.find { it.name == "module-info.java" } | ||
val targetFile = compileKotlinTask.destinationDirectory.file("../module-info.class").get().asFile | ||
val targetDirectory = compileKotlinTask.destinationDirectory.map { | ||
it.dir("../${it.asFile.name}Module") | ||
} | ||
|
||
// only configure the compilation if necessary | ||
if (sourceFile != null) { | ||
// the default source set depends on this new source set | ||
defaultSourceSet.dependsOn(this) | ||
// register and wire a task to verify module-info.java content | ||
// | ||
// this will compile the whole sources again with a JPMS-aware target Java version, | ||
// so that the Kotlin compiler can do the necessary verifications | ||
// while compiling with `jdk-release=1.8` those verifications are not done | ||
// | ||
// this task is only going to be executed when running with `check` or explicitly, | ||
// not during normal build operations | ||
val verifyModuleTask = registerVerifyModuleTask( | ||
compileKotlinTask, | ||
sourceFile | ||
) | ||
tasks.named("check") { | ||
dependsOn(verifyModuleTask) | ||
} | ||
|
||
// register a new compile module task | ||
val compileModuleTask = registerCompileModuleTask(compileModuleTaskName, compileKotlinTask, sourceFile, targetFile) | ||
val compileModuleTask = registerCompileModuleTask( | ||
compileKotlinTask, | ||
sourceFile, | ||
targetDirectory | ||
) | ||
|
||
// add the resulting module descriptor to this target's artifact | ||
artifactTask.dependsOn(compileModuleTask) | ||
artifactTask.from(targetFile) { | ||
artifactTask.from(compileModuleTask) { | ||
if (multiRelease) { | ||
into("META-INF/versions/9/") | ||
} | ||
} | ||
} else { | ||
logger.info("No module-info.java file found in ${this.kotlin.srcDirs}, can't configure compilation of module-info!") | ||
// remove the source set to prevent Gradle warnings | ||
kotlin.sourceSets.remove(this) | ||
} | ||
|
||
// remove the source set to prevent Gradle warnings | ||
kotlin.sourceSets.remove(this) | ||
} | ||
} | ||
} | ||
} | ||
|
||
private fun Project.registerCompileModuleTask(taskName: String, compileTask: KotlinCompile, sourceFile: File, targetFile: File) = | ||
tasks.register(taskName, JavaCompile::class) { | ||
// Also add the module-info.java source file to the Kotlin compile task; | ||
// the Kotlin compiler will parse and check module dependencies, | ||
// but it currently won't compile to a module-info.class file. | ||
compileTask.source(sourceFile) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you remove this line, please provide an alternative way. It was here to perform validation of the sources in accordance to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you tell me what exactly that did / checked? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm sorry we lack documentation about this. Actually, Kotlin compiler is able to read There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, I see. If I just add If with the new (with-my-PR) setup I use JVM toolchain 17 (to reproduce the problematic bytecode eventually) and add jdk-release 1.8 / jvm-target 1.8, the correct bytecode is produced but still those errors are not verified as the module infos seem to be ignored. If I additionally add the If I remove the "jdk-release" and only keep the "jvm-target", the verifications work again, but the wrong byte-code is produced. grml There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me try to sum this up: the problem is, when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. However, after giving it a second thought, I'm not so sure we need this verification at all. We have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, it wont actually. The This verification still has some bugs like https://youtrack.jetbrains.com/issue/KT-60583 and https://youtrack.jetbrains.com/issue/KT-60582 for example. I've added the alternative means for the validation that @ALikhachev requested now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Thank you for you careful study. Can you please also file this bug
so a compiler team can look properly at it (cc @udalov ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
/** | ||
* Add a Kotlin compile task that compiles `module-info.java` source file and Kotlin sources together, | ||
* the Kotlin compiler will parse and check module dependencies, | ||
* but it currently won't compile to a module-info.class file. | ||
*/ | ||
private fun Project.registerVerifyModuleTask( | ||
compileTask: KotlinCompile, | ||
sourceFile: File | ||
): TaskProvider<out KotlinJvmCompile> { | ||
apply<KotlinBaseApiPlugin>() | ||
val verifyModuleTaskName = "verify${compileTask.name.removePrefix("compile").capitalize()}Module" | ||
// work-around for https://youtrack.jetbrains.com/issue/KT-60542 | ||
val verifyModuleTask = plugins | ||
.findPlugin(KotlinBaseApiPlugin::class)!! | ||
.registerKotlinJvmCompileTask(verifyModuleTaskName) | ||
verifyModuleTask { | ||
group = VERIFICATION_GROUP | ||
description = "Verify Kotlin sources for JPMS problems" | ||
libraries.from(compileTask.libraries) | ||
source(compileTask.sources) | ||
source(compileTask.javaSources) | ||
// part of work-around for https://youtrack.jetbrains.com/issue/KT-60541 | ||
@Suppress("INVISIBLE_MEMBER") | ||
source(compileTask.scriptSources) | ||
source(sourceFile) | ||
destinationDirectory.set(temporaryDir) | ||
multiPlatformEnabled.set(compileTask.multiPlatformEnabled) | ||
kotlinOptions { | ||
moduleName = compileTask.kotlinOptions.moduleName | ||
jvmTarget = "9" | ||
freeCompilerArgs += "-Xjdk-release=9" | ||
} | ||
// work-around for https://youtrack.jetbrains.com/issue/KT-60583 | ||
inputs.files( | ||
libraries.asFileTree.elements.map { libs -> | ||
libs | ||
.filter { it.asFile.exists() } | ||
.map { | ||
zipTree(it.asFile).filter { it.name == "module-info.class" } | ||
} | ||
} | ||
).withPropertyName("moduleInfosOfLibraries") | ||
this as KotlinCompile | ||
// part of work-around for https://youtrack.jetbrains.com/issue/KT-60541 | ||
@Suppress("DEPRECATION") | ||
ownModuleName.set(compileTask.kotlinOptions.moduleName) | ||
// part of work-around for https://youtrack.jetbrains.com/issue/KT-60541 | ||
@Suppress("INVISIBLE_MEMBER") | ||
commonSourceSet.from(compileTask.commonSourceSet) | ||
// part of work-around for https://youtrack.jetbrains.com/issue/KT-60541 | ||
// and work-around for https://youtrack.jetbrains.com/issue/KT-60582 | ||
incremental = false | ||
} | ||
return verifyModuleTask | ||
} | ||
|
||
private fun Project.registerCompileModuleTask( | ||
compileTask: KotlinCompile, | ||
sourceFile: File, | ||
targetDirectory: Provider<out Directory> | ||
) = tasks.register("${compileTask.name}Module", JavaCompile::class) { | ||
// Configure the module compile task. | ||
source(sourceFile) | ||
classpath = files() | ||
destinationDirectory.set(targetDirectory) | ||
// use a Java 11 toolchain with release 9 option | ||
// because for some OS / architecture combinations | ||
// there are no Java 9 builds available | ||
javaCompiler.set( | ||
[email protected]<JavaToolchainService>().compilerFor { | ||
languageVersion.set(JavaLanguageVersion.of(11)) | ||
} | ||
) | ||
options.release.set(9) | ||
|
||
options.compilerArgumentProviders.add(object : CommandLineArgumentProvider { | ||
@get:CompileClasspath | ||
val compileClasspath = compileTask.libraries | ||
|
||
// Configure the module compile task. | ||
dependsOn(compileTask) | ||
source(sourceFile) | ||
outputs.file(targetFile) | ||
classpath = files() | ||
destinationDirectory.set(compileTask.destinationDirectory) | ||
sourceCompatibility = JavaVersion.VERSION_1_9.toString() | ||
targetCompatibility = JavaVersion.VERSION_1_9.toString() | ||
@get:CompileClasspath | ||
val compiledClasses = compileTask.destinationDirectory | ||
|
||
doFirst { | ||
@get:Input | ||
val moduleName = sourceFile | ||
.readLines() | ||
.single { it.contains("module ") } | ||
.substringAfter("module ") | ||
.substringBefore(' ') | ||
.trim() | ||
|
||
override fun asArguments() = mutableListOf( | ||
// Provide the module path to the compiler instead of using a classpath. | ||
// The module path should be the same as the classpath of the compiler. | ||
options.compilerArgs = listOf( | ||
"--release", "9", | ||
"--module-path", compileTask.libraries.asPath, | ||
"-Xlint:-requires-transitive-automatic" | ||
) | ||
} | ||
|
||
doLast { | ||
// Move the compiled file out of the Kotlin compile task's destination dir, | ||
// so it won't disturb Gradle's caching mechanisms. | ||
val compiledFile = destinationDirectory.file(targetFile.name).get().asFile | ||
targetFile.parentFile.mkdirs() | ||
compiledFile.renameTo(targetFile) | ||
} | ||
} | ||
"--module-path", | ||
compileClasspath.asPath, | ||
"--patch-module", | ||
"$moduleName=${compiledClasses.get()}", | ||
"-Xlint:-requires-transitive-automatic" | ||
) | ||
}) | ||
} | ||
} |
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.
Am I right that this actually will cause the full compilation to happen twice (you've mentioned it somewhere)?
I believe it should be mentioned in the comments then
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.
Yes, when running with
check
this is correct.During pure building it should not happen.
I added that info to the comment now.