-
Notifications
You must be signed in to change notification settings - Fork 208
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
add kt_plugin rule #308
add kt_plugin rule #308
Conversation
@@ -42,6 +42,11 @@ kt_jvm_import( | |||
] | |||
] | |||
|
|||
java_import( |
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.
kt_jvm_import
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.
done!
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.
A few changes, the largest being to use providers over aspect.
@restingbull Thank you for taking a look. Could you please also have a look at my questions in the issue? |
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.
LGTM.
One nit, mostly to future proof -- otherwise, it's good to go.
examples/plugin/src/BUILD
Outdated
kt_plugin( | ||
name = "open_for_testing_plugin", | ||
id = "org.jetbrains.kotlin.allopen", | ||
options = ["annotation=plugin.OpenForTesting"], |
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.
I'm being pedantic... and defensive about the long term maintenance.
Could you make the options in an attr.string_dict?
e.g.
options = {
"annotation":"plugin.OpenForTesting"
}
and assemble thiem with an equals in the impl.
It's safer to maintain and validate (I know, it sounds weird, but it can be an issue. One doesn't normally consider sanitizing input in build tooling... but, well, it's necessary) if the arguments are split into key/value pairs.
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.
I changed the interface, and added a check for equal signs being in the key part of the dict.
taking my own look, but first-cut, this is awesome, and thank you. |
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.
This looks pretty great. Can you please add a blurb in the README?
Also, we need to add some way to add annotation processor options, and handle disabling/altering kapt, but that totally can be a follow-up. This keeps kapt as a special case, and I think that's still the best approach.
@@ -22,7 +22,7 @@ import java.io.ObjectOutputStream | |||
import java.util.Base64 | |||
|
|||
// TODO(hs) move the kapt specific stuff to the JVM package. | |||
class KotlinCompilerPluginArgsEncoder( | |||
class KotlinKaptCompilerPluginArgsEncoder( |
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.
Nit: Probably doesn't need Kotlin as a prefix here, as it's implied in Kapt.
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.
Changed it
"id": attr.string( | ||
doc = """The ID of the plugin""", | ||
), | ||
"options": attr.string_dict( |
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.
So these are passed to the plugin globally, right? I take it in this PR we're not considering per-target options?
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.
What do you mean with per-target options? In the current implementation, the user can specify a different plugin for each target if he wants to, with different options.
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.
Oh wait - I think I just realized something - this change doesn't create an exported_plugins equivalent. That's fine, but we'll need to file an issue to add that. I was thinking about this (my comment above) in the context of something like an exported_plugins feature, but it's irrelevant if that feature isn't even present.
examples/plugin/src/BUILD
Outdated
@@ -0,0 +1,34 @@ | |||
load("//kotlin:kotlin.bzl", "kt_compiler_plugin", "kt_jvm_library") | |||
|
|||
kt_compiler_plugin( |
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.
Nice!
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.
Meant to specify request changes. (Just the readme. The nits are optional)
@cgruber I added a better docstring for the rule, and mentioned the existence of an example in the Readme. I can also add the usage doc of When do the docs at https://bazelbuild.github.io/rules_kotlin/kotlin/kotlin.html get updated? Does that happen with each release? |
Yup --- it's auto generated by skydoc. However, that is just the api documentation. In the readme (and we should probably break it out, but that is beyond the pr scope, we have been putting guidance on how to use features. If you could put a small section on how to use compiler plugins, and link to the examples, I'll take on the larger effort of creating docs that off configuration guidance. Thanks again for doing this! |
I see, I will add it to the Readme. Also, I discovered a problem with intellij plugin sync
I'm going to look into that. I also added examples of the sam-with-receiver and noarg plugins. |
@@ -42,6 +42,21 @@ kt_jvm_import( | |||
] | |||
] | |||
|
|||
kt_jvm_import( | |||
name = "allopen-compiler-plugin", | |||
jars = ["lib/allopen-compiler-plugin.jar"], |
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.
Change this to "jar" not source jars.
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.
That worked! Sync works now.
The error is probably related to bazelbuild/intellij#1616. If you change to jar, it should work properly. I'll clean up the jars handling. |
This all looks great to me. Thank you so much for this work. |
* upstream/master: Fix non-reproducible archives (bazelbuild#304) Adds a kt_plugin rule (bazelbuild#308) Ensure that KotlionBuilder workers use a clean directory for each compilation. (bazelbuild#298) Apply autoformatting to all files. (bazelbuild#302) Optional outputs (bazelbuild#291) Change plugins to use depsets, as opposed to lists. (bazelbuild#292) Add Corbin to the codeowners. (bazelbuild#293) Update Protobuf to 3.11.3 (bazelbuild#286) Remove tree artifacts (bazelbuild#287) Cleanup src tree (bazelbuild#288) Update README.md (bazelbuild#285) Filter non-kotlin code out of generated sources (bazelbuild#263) Update readme so the dev instructions highlight using a local clone (bazelbuild#283) Remove third_party checked in jars, and properly pull maven dependencies. (bazelbuild#279) Only propagate srcjar if it isn't the default empty jar added in ae70089 to fix bazelbuild/intellij#1616 (bazelbuild#276) Allow resources to be in a kotlin directory (bazelbuild#268)
Creates a `kt_compiler_plugin` rule to allow kotlin compiler plugins to be specified, and then included with a `plugins=` attribute on kt_jvm_* jobs. This should support arbitrary kotlin compiler plugins, such as the android extensions, open-for-testing, and others. This does _not_ add an exported_plugins infrastructure (such as you get with java_plugins), and it does not change the special-case handling of kapt. It also doesn't do anything to resolve plugin inter-0compatibilities (e.g. the ABI plugin possibly needing information from the parcelizable, or other such interactions) The latter are issues yet to be figured out int he kotlinc plugin infrastructure, and we'll adapt this rule as appropriate. Also adds docs and examples.
Proposal for implementing a kt_plugin rule
#239