Fix missing plugin inputs #318
Conversation
justhecuke-zz
requested review from
cgruber,
djwhang,
jin,
restingbull and
timpeut
as code owners
kotlin/internal/jvm/compile.bzl
Outdated
| inputs = depset(ctx.files.srcs, transitive = [compile_deps.compile_jars]), | ||
| inputs = depset( | ||
| ctx.files.srcs, | ||
| transitive = [compile_deps.compile_jars] + [ap.classpath for ap in annotation_processors.to_list()]), |
There was a problem hiding this comment.
I'm not sure if the to_list() is strictly required, but I'm not sure how this would handle a depset of structs rather than a depset of files.
There was a problem hiding this comment.
We want to avoid to_list like the plague, unfortunately. It would be better to export the processor classpath from https://github.com/bazelbuild/rules_kotlin/blob/master/kotlin/internal/jvm/plugins.bzl#L36 to limit the number of times we call to_list. (probably should even happen there, tbh.
There was a problem hiding this comment.
I think I've made the change you requested. I couldn't do it within the exact method you pointed to, not that I could figure anyways, but I've made changes similar to the existing pattern.
|
PTAL @cgruber |
|
PTAL @restingbull |
kotlin/internal/jvm/compile.bzl
Outdated
| inputs = depset(ctx.files.srcs, transitive = [compile_deps.compile_jars]), | ||
| inputs = depset( | ||
| ctx.files.srcs, | ||
| transitive = [compile_deps.compile_jars] + [ap.classpath for ap in annotation_processors.to_list()]), |
There was a problem hiding this comment.
We want to avoid to_list like the plague, unfortunately. It would be better to export the processor classpath from https://github.com/bazelbuild/rules_kotlin/blob/master/kotlin/internal/jvm/plugins.bzl#L36 to limit the number of times we call to_list. (probably should even happen there, tbh.
Bug is improper/non execution of plugins and sometimes missing classes when running the plugin. Errors during the build from dagger and such not generating classes. Jars on the processorpath didn't exist and were pointing to the target config rather than host. Especially bad when building for android since it'd try to compile for arm, possibly meaning that valid java/kotlin wouldn't be able to build due to android sdk not using java/kotlin toolchain although I didn't witness this. End result is flaky failures during build that generally required a |
There was a problem hiding this comment.
Oh wow - we're hitting this exact same thing, as of dagger 2.27. I guess it acted as a forcing function. The only comment I would have had would be to avoid to_list, but @restingbull already got there.
This looks fine to me, but I'll wait to merge until @restingbull gives it his final nod, since he has outstanding updates to review.
Of note, I'll make sure this gets in and that I get a release out sooner than later. (been rather distracted personally and professionally - this is being prioritized now)
java_plugin runtime jars were not being added to the KotlinCompile action's input list which meant that bazel was not properly ensuring that they were available.
This PR just adds the plugin jars we need to the action input and lets bazel handle the rest.
Before this, kotlin's plugin implementation was fundamentally broken and only worked by chance.