search

bazelbuild / rules_kotlin

Bazel rules for Kotlin
Apache License 2.0
331 stars 206 forks source link

rules_kotlin should link the embeddable Kotlin compiler #624

Open mauriciogg opened 2 years ago

mauriciogg commented 2 years ago

There are two versions of the Kotlin compiler jar:

kotlin-compiler.jar: this is the vanilla version and is the one distributed in the Kotlin Github repo. kotlin-compiler-embeddable .jar: this is a shaded version of kotlin-compiler.jar, which among other classes, shades the intellij classes. It can be used to to link safely against the compiler in an environment where there might be class collisions. The rules_kotlin toolchain links against kotlin-compiler.jar: KotlinToolchain.kt The Gradle Kotlin plugin links against the embeddable compiler: kotlin-gradle-plugin. NOTE: I'm not very familiar with Gradle code so this might not be 100% true.

Under most circumstances linking against either versions should not make any difference as long as everything is compiled against the same version. The problem arises when third_party dependencies that expect the embeddable compiler are brought into the mix. This is true for compiler plugins built using Gradle. In order to work around this issue the kt_compiler_plugin rule has an attr target_embedded_compiler that when true simply deshades the plugin jars so that they can be used against the vanilla compiler. However this does not work as intended (e.g Anvil). This approach only deshades the main plugin jar, but none of its dependencies. A plugin might declare multiple jar artifacts as dependencies and some of those might need to be deshaded as well. An easy fix would be to deshade all jars in the transitive dependencies of the plugin, but this is suboptimal and error prone since we might deshade jars that dont need deshading in the fist place. Its not entirely clear how to find out which need to be deshaded and which don't.

Instead of applying the deshading in the plugin rule, rules_kotlin should depend on the embeddable compiler and load that artifact in KotlinToolchain.kt. The only issue with that approach is that ATM this artifact is not distributed in the release archive (and seems like its they are not planning on enabling it https://youtrack.jetbrains.com/issue/KT-49723). One solution might be just shading the artifacts after downloading it in compiler.bzl.

I've started working on a PR to do so, just need to work some quirks with the initial bootstrap compilation. Does this sounds like a sound approach?

restingbull commented 2 years ago

Can you attach a repro case for un-deshaded classes? Need to understand the transitive closure that is missing.

mauriciogg commented 2 years ago

PR https://github.com/bazelbuild/rules_kotlin/pull/628 contains a simple repo.

bazel build //repro/src/main/java/com/repro/lib:lib will throw the following compiler exception

ERROR: /Users/mgalindo/Snapchat/Dev/rules_kotlin_repo/rules_kotlin/examples/anvil/repo/src/main/java/com/repo/lib/BUILD.bazel:4:15: KotlinCompile //repo/src/main/java/com/repo/lib:lib { kt: 1, java: 0, srcjars: 0 } for darwin failed: (Exit 1): build failed: error executing command bazel-out/host/bin/external/io_bazel_rules_kotlin/src/main/kotlin/build '--flagfile=bazel-out/darwin-fastbuild/bin/repo/src/main/java/com/repo/lib/lib-kt-java.jar-0.params'
exception: java.lang.NoSuchMethodError: 'org.jetbrains.kotlin.com.intellij.psi.PsiElement[] org.jetbrains.kotlin.psi.KtClassOrObject.getChildren()'
    at com.squareup.anvil.compiler.internal.PsiUtilsKt.classBodies(PsiUtils.kt:440)
    at com.squareup.anvil.compiler.internal.PsiUtilsKt.functions(PsiUtils.kt:432)
    at com.squareup.anvil.compiler.codegen.dagger.ProvidesMethodFactoryGenerator.generateCodePrivate(ProvidesMethodFactoryGenerator.kt:71)
    at com.squareup.anvil.compiler.codegen.PrivateCodeGenerator.generateCode(PrivateCodeGenerator.kt:21)
    at com.squareup.anvil.compiler.codegen.CodeGenerationExtension.analysisCompleted$generateCode(CodeGenerationExtension.kt:92)
    at com.squareup.anvil.compiler.codegen.CodeGenerationExtension.analysisCompleted(CodeGenerationExtension.kt:113)
    at org.jetbrains.kotlin.cli.jvm.compiler.TopDownAnalyzerFacadeForJVM.analyzeFilesWithJavaIntegration$invokeExtensionsOnAnalysisComplete(TopDownAnalyzerFacadeForJVM.kt:111)
    at org.jetbrains.kotlin.cli.jvm.compiler.TopDownAnalyzerFacadeForJVM.analyzeFilesWithJavaIntegration(TopDownAnalyzerFacadeForJVM.kt:121)
    at org.jetbrains.kotlin.cli.jvm.compiler.TopDownAnalyzerFacadeForJVM.analyzeFilesWithJavaIntegration$default(TopDownAnalyzerFacadeForJVM.kt:85)
    at org.jetbrains.kotlin.cli.jvm.compiler.KotlinToJVMBytecodeCompiler$analyze$1.invoke(KotlinToJVMBytecodeCompiler.kt:514)
    at org.jetbrains.kotlin.cli.jvm.compiler.KotlinToJVMBytecodeCompiler$analyze$1.invoke(KotlinToJVMBytecodeCompiler.kt:505)
    at org.jetbrains.kotlin.cli.common.messages.AnalyzerWithCompilerReport.analyzeAndReport(AnalyzerWithCompilerReport.kt:112)
    at org.jetbrains.kotlin.cli.jvm.compiler.KotlinToJVMBytecodeCompiler.analyze(KotlinToJVMBytecodeCompiler.kt:505)
    at org.jetbrains.kotlin.cli.jvm.compiler.KotlinToJVMBytecodeCompiler.compileModules$cli(KotlinToJVMBytecodeCompiler.kt:189)
    at org.jetbrains.kotlin.cli.jvm.compiler.KotlinToJVMBytecodeCompiler.compileModules$cli$default(KotlinToJVMBytecodeCompiler.kt:155)
    at org.jetbrains.kotlin.cli.jvm.K2JVMCompiler.doExecute(K2JVMCompiler.kt:169)
    at org.jetbrains.kotlin.cli.jvm.K2JVMCompiler.doExecute(K2JVMCompiler.kt:52)
    at org.jetbrains.kotlin.cli.common.CLICompiler.execImpl(CLICompiler.kt:88)
    at org.jetbrains.kotlin.cli.common.CLICompiler.execImpl(CLICompiler.kt:44)
    at org.jetbrains.kotlin.cli.common.CLITool.exec(CLITool.kt:98)
    at io.bazel.kotlin.compiler.BazelK2JVMCompiler.exec(BazelK2JVMCompiler.kt:30)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:566)
    at io.bazel.kotlin.builder.toolchain.KotlinToolchain$KotlinCliToolInvoker.compile(KotlinToolchain.kt:186)
    at io.bazel.kotlin.builder.tasks.jvm.Compilation_taskKt$runPlugins$1$1$1.invoke(compilation_task.kt:206)
    at io.bazel.kotlin.builder.tasks.jvm.Compilation_taskKt$runPlugins$1$1$1.invoke(compilation_task.kt:206)
    at io.bazel.kotlin.builder.toolchain.CompilationTaskContext.executeCompilerTask(CompilationTaskContext.kt:122)
    at io.bazel.kotlin.builder.toolchain.CompilationTaskContext.executeCompilerTask$default(CompilationTaskContext.kt:114)
    at io.bazel.kotlin.builder.tasks.jvm.Compilation_taskKt$runPlugins$1.invoke(compilation_task.kt:204)
    at io.bazel.kotlin.builder.tasks.jvm.Compilation_taskKt$runPlugins$1.invoke(compilation_task.kt:188)
    at io.bazel.kotlin.builder.toolchain.CompilationTaskContext.execute(CompilationTaskContext.kt:149)
    at io.bazel.kotlin.builder.toolchain.CompilationTaskContext.execute(CompilationTaskContext.kt:141)
    at io.bazel.kotlin.builder.tasks.jvm.Compilation_taskKt.runPlugins(compilation_task.kt:188)
    at io.bazel.kotlin.builder.tasks.jvm.KotlinJvmTaskExecutor.execute(KotlinJvmTaskExecutor.kt:54)
    at io.bazel.kotlin.builder.tasks.KotlinBuilder.executeJvmTask(KotlinBuilder.kt:233)
    at io.bazel.kotlin.builder.tasks.KotlinBuilder.build(KotlinBuilder.kt:130)
    at io.bazel.kotlin.builder.tasks.CompileKotlin.invoke(CompileKotlin.kt:27)
    at io.bazel.worker.PersistentWorker$compileWork$2$result$1.invoke(PersistentWorker.kt:96)
    at io.bazel.worker.PersistentWorker$compileWork$2$result$1.invoke(PersistentWorker.kt:94)
    at io.bazel.worker.WorkerContext$TaskContext.resultOf(WorkerContext.kt:128)
    at io.bazel.worker.WorkerContext.doTask(WorkerContext.kt:156)
    at io.bazel.worker.PersistentWorker$compileWork$2.invokeSuspend(PersistentWorker.kt:94)
    at io.bazel.worker.PersistentWorker$compileWork$2.invoke(PersistentWorker.kt)
    at io.bazel.worker.PersistentWorker$compileWork$2.invoke(PersistentWorker.kt)
    at kotlinx.coroutines.intrinsics.UndispatchedKt.startUndispatchedOrReturn(Undispatched.kt:89)
    at kotlinx.coroutines.BuildersKt__Builders_commonKt.withContext(Builders.common.kt:165)
    at kotlinx.coroutines.BuildersKt.withContext(Unknown Source)
    at io.bazel.worker.PersistentWorker.compileWork(PersistentWorker.kt:93)
    at io.bazel.worker.PersistentWorker.access$compileWork(PersistentWorker.kt:45)
    at io.bazel.worker.PersistentWorker$start$1$1$1$1$2$1.invokeSuspend(PersistentWorker.kt:71)
    at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
    at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
    at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:571)
    at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:750)
    at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:678)
    at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:665)
restingbull commented 2 years ago

I looked into just using the embeddable compiler (available from Maven)

It's doable, but comes with some awkward api changes:

Given that most compiler plugins will be written against the embedded compiler, (thanks, Gradle) it might make sense to converge on using the embedded compiler in the long term...

What are your thoughts, @mauriciogg @Bencodes @nkoroste @cgruber ?

mauriciogg commented 2 years ago

I looked into just using the embeddable compiler (available from Maven)

It's doable, but comes with some awkward api changes:

  • com_github_jetbrains_kotlin would be removed -- Not against this, as it should be a private implementation detail
  • have to start shading non-embedded compiler plugins

Given that most compiler plugins will be written against the embedded compiler, (thanks, Gradle) it might make sense to converge on using the embedded compiler in the long term...

What are your thoughts, @mauriciogg @Bencodes @nkoroste @cgruber ?

Yeah, I have a POC https://github.com/bazelbuild/rules_kotlin/pull/636 that makes the shaded artifacts available as part of the com_github_jetbrains_kotlin repo. The main thing I dont like about this approach is that shaded libs all need to be placed next to each other under lib/ (since this is what the java classloader expects) which seems a little brittle and ATM im sure I'm shading more jars than the ones that need to be shaded just to get all the libs under the same directory. So probably getting rid of the com_github_jetbrains_kotlin and just loading the maven jars directly might end up being a cleaner solution

mauriciogg commented 2 years ago

I looked into just using the embeddable compiler (available from Maven) It's doable, but comes with some awkward api changes:

  • com_github_jetbrains_kotlin would be removed -- Not against this, as it should be a private implementation detail
  • have to start shading non-embedded compiler plugins

Given that most compiler plugins will be written against the embedded compiler, (thanks, Gradle) it might make sense to converge on using the embedded compiler in the long term... What are your thoughts, @mauriciogg @Bencodes @nkoroste @cgruber ?

Yeah, I have a POC #636 that makes the shaded artifacts available as part of the com_github_jetbrains_kotlin repo. The main thing I dont like about this approach is that shaded libs all need to be placed next to each other under lib/ (since this is what the java classloader expects) which seems a little brittle and ATM im sure I'm shading more jars than the ones that need to be shaded just to get all the libs under the same directory. So probably getting rid of the com_github_jetbrains_kotlin and just loading the maven jars directly might end up being a cleaner solution

Also, shading kotlin-compiler from the github release is not as straightforward since there are other stuff that needs to be shaded besides class files. In particular interface to implementations mappings for intellij plugins are declared in xml files which are ignored by jarjar and its hard to say which strings needs to be shaded and which don't. Trying to compile without those files shaded results in an error like:

exception: java.lang.IllegalArgumentException: Missing extension point: org.jetbrains.kotlin.com.intellij.psi.classFileDecompiler in container org.jetbrains.kotlin.com.intellij.core.CoreApplicationEnvironment$1@614da410

Which can be traced to META-INF/extensions/core.xml declaring the following extension point (which needs to be shaded):


<extensionPoint qualifiedName="com.intellij.psi.classFileDecompiler"
                        interface="com.intellij.psi.compiled.ClassFileDecompilers$Decompiler"
                        dynamic="true"/>```

So in addition to jarjar patching class files it would need to also patch resource files which seems a bit brittle.
restingbull commented 2 years ago

Reopening as a feature request.