-
Notifications
You must be signed in to change notification settings - Fork 215
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
Remove hard coding "io_bazel_rules_kotlin" to allow alternative names. #433
Conversation
…h alternative names. Use deploy jar for jarjar to make sure runtime deps are present. Rename development workspace to `dev_io_bazel_rules_kotlin` to dissuade hard coding the release repository name
2964f00
to
563d503
Compare
…pace_name to the builder. (non-ideal, it'll do until the files are passed in as args) Add the (mostly undocumented) testing.TestEnvironment provider to the unit tests -- this adds the same env variables as seen in java_test.
… skipped. Starlark formatting. Add repository name to the under-privileged js rules.
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.
Minor consistency nits, and a few questions out of curiosity, but nothing blocking. It's generally quite straightforward. LGTM
@@ -230,6 +233,8 @@ def kt_jvm_junit_test_impl(ctx): | |||
transitive = [runtime_jars], | |||
direct = ctx.files._java_runtime, | |||
), | |||
# adds common test variables, including TEST_WORKSPACE. | |||
testing.TestEnvironment({}), |
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.
Holy crap, we didn't have this? Urgh.... :(
kotlin/internal/jvm/jvm.bzl
Outdated
@@ -513,13 +513,13 @@ kt_compiler_plugin = rule( | |||
default = False, | |||
), | |||
"_jetbrains_deshade_rules": attr.label( | |||
default = "@io_bazel_rules_kotlin//kotlin/internal/jvm:jetbrains-deshade.jarjar", | |||
default = "//kotlin/internal/jvm:jetbrains-deshade.jarjar", |
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.
Sometimes here you're using a plain string "//third_party/foo"
, sometimes a Label("//third_party/foo")
. Should we be consistent, or is this a place where that's actually advisable or necessary?
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.
We don't use compiler plugins in base, so I missed it.
Label provides the fully qualified target, where as string can be resolved relatively against the main repo in macros.
So, in the name of consistency, we ought to use label when referring to repository specific resources.
"BUILD.bazel", | ||
attr._template, | ||
substitutions = { | ||
"{{.KotlinRules}}": attr.kotlin_rules, |
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 is one of the grossest things I've seen, but I get it. Haven't used repository_ctx.template() much myself. (gross in convention, not in concept - it's normal templating in concept)
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 honestly not sure how else to do it.
], | ||
src_map = { | ||
"jarjar_runner_deploy.jar": "jarjar.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.
Just to clarify - you're yoinking the deploy jar and releasing that?
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. No fuss, no muss.
Surprisingly complicated, but there were quite a few instances where the repository name had to be "io_bazel_rules_kotlin"
Aside from being a better behaved rule set, this opens the door to build rules_kotlin using rules_kotlin.