feat: allow passing KSP options via kt_ksp_plugin#1478
feat: allow passing KSP options via kt_ksp_plugin#1478restingbull merged 4 commits intobazel-contrib:masterfrom
Conversation
baf7ec3 to
9cf6e33
Compare
9cf6e33 to
fe71e4b
Compare
|
Could you add an integration test - write the simplest possible KSP plugin that prints the options passed to it and make asserts on that? |
| options = {} | ||
| for t in targets: | ||
| if _KspPluginInfo in t: | ||
| options.update(t[_KspPluginInfo].options) |
There was a problem hiding this comment.
This will silently overwrite options that conflict with other plugins.
There was a problem hiding this comment.
That's a good point. Tbh, probably it makes more sense to have those options at the plugin consumer targets rather than kt_ksp_plugin. Otherwise we'll have to either pick the behavior for the end users (i.e. either merge the options or throw an error on conflict), or provide a config option for the merge strategy. If we go for the latter, probably such option would have to live at plugin consumer targets (kt_jvm_library, kt_jvm_test). And at that point I think it makes more sense to simply move the options there.
There was a problem hiding this comment.
On the other hand, kinda weird to have plugin-specific options on kt_jvm_library 🤔 If that doesn't work, then I'd prefer merging the options, as it was, just with explicit mention of that behavior in the docs.
There was a problem hiding this comment.
Yeah, it looks kinda weird. Would there be any practical use case for it?
There was a problem hiding this comment.
Yeah, in my case I need to pass a list of service names to be used in codegen. The same service names are passed to some other bazel rules, so I'd like to have a single place where I declare those.
There was a problem hiding this comment.
Let's go with fail for now. Principal of least surprise and all.
There was a problem hiding this comment.
| "Should have at least one KotlinKsp2 action when plugin has options", | ||
| ) | ||
|
|
||
| if ksp2_actions: |
There was a problem hiding this comment.
Is this conditional actually needed since the assertion above this already checks for the existence of a KSP action?
There was a problem hiding this comment.
Good catch, yeah, that was unnecessary
restingbull
left a comment
There was a problem hiding this comment.
Generally LGTM -- except for the silent overwrite on option conflict.
What would be a good merge strategy?
1008236 to
c90f0fb
Compare
c90f0fb to
b7772e1
Compare
|
Hey folks, is there any ETA for when this will get released? |
Today kotlin 2.3.20 was released, so I guess it's a good time to cut a release of rules_kotlin :) |
|
Any chance for a release this week? |
|
Seems like BCR PR to bump the version is missing even though a new rules version got released |
New version appears to be released correctly, doesn't it? |
|
Yeah, I've created a PR to get it to BCR 🙂 |
|
Ah, thanks then :) |
Adds
optionsarg tokt_ksp_plugin, so that end users can pass options to KSP processors from the build files and use those in codegen.