-
Notifications
You must be signed in to change notification settings - Fork 233
Support Starlark rules_android #1333
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
Conversation
81ef8e9
to
e9e475d
Compare
exec_properties = exec_properties, | ||
nocompress_extensions = nocompress_extensions, | ||
) | ||
kt_android_library = _kt_android_library |
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 can swap this out for a macro
that aliases the _kt
targets for people so that we aren't outright introducing breaking changes here.
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.
Yeah, we ought to do that. Bit of a goof to leave them public.
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 believe they kind of needed to be public to support some specific use cases where you actually needed the Kotlin provider. I lean more towards just doing a breaking change here since it's a relatively easy find-and-replace.
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.
Generally looks good.
Small nits.
kotlin/internal/jvm/android.bzl
Outdated
nocompress_extensions = nocompress_extensions, | ||
) | ||
kt_android_library = _kt_android_library | ||
kt_android_local_test_impl = _kt_android_local_test_impl |
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.
Should this be the kt_andorid_local_test
?
_utils = "utils", | ||
) | ||
|
||
_ATTRS = _utils.add_dicts(_BASE_ATTRS, _attrs.lib_common_attr, { |
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.
Do we expect to have additional attributes here?
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.
None at the moment, left the placeholder here in case we do need them at some point.
@@ -987,3 +1029,11 @@ def export_only_providers(ctx, actions, attr, outputs): | |||
extensions = ["kt", "java"], | |||
), | |||
) | |||
|
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.
👍 Probably overdue to to do this.
exec_properties = exec_properties, | ||
nocompress_extensions = nocompress_extensions, | ||
) | ||
kt_android_library = _kt_android_library |
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.
Yeah, we ought to do that. Bit of a goof to leave them public.
Swapping out the kt_android_ sandwich for a proper Starlark rule implementation that fully leverages the new processing pipeline inside of
rules_android
.This pull request contains a few breaking changes that folks will likely need to handle:
_kt
targets that people may have been referencing in the past no longer exist. If this causes issues we can explore introducing some thin aliases to help ease the migration process.//third_party:android_sdk
target that was left public has been removed and no longer exists. Moving forward you should rely on the one provided byrules_android
.#894