Support multiple kt_android_library targets with same package name.
#790
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
arunkumar9t2
requested review from
cgruber,
jin,
timpeut and
restingbull
as code owners
arunkumar9t2
changed the title
kt_android_library targets with same package name. Fixes #787kt_android_library targets with same package name.
|
Fixes #787 |
|
Did you try inverting the base_deps and base_name in https://github.com/bazelbuild/rules_kotlin/blob/master/kotlin/internal/jvm/android.bzl#L56? It would be much less invasive that changing the order inside the rules. |
kotlin/internal/jvm/compile.bzl
Outdated
| @@ -51,6 +51,11 @@ load( | |||
| def _java_info(target): | |||
| return target[JavaInfo] if JavaInfo in target else None | |||
|
|
|||
| def _is_android_sandwich(current_label_name, target): | |||
| """Checks if the given target is <target>_base target from android sandwich macro""" | |||
| is_base_target = current_label_name.removesuffix("_kt") == target.label.name.removesuffix("_base") | |||
There was a problem hiding this comment.
Doesn't work on Bazel versions older than 5.1.0, bazelbuild/bazel#14899
There was a problem hiding this comment.
@Nikolas-LFDesigns thanks, I removed this impl and just switched the order in deps attribute. That also worked.
arunkumar9t2
force-pushed
the
arun/support-merged-resources-jar-in-android-sandwich
branch
2 times, most recently
from
1b08d68 to
62c5d7f
Compare
…ixes bazelbuild#787 When two `kt_android_library` or `android_library` has the same package name either provided via `custom_package` or due to same package name in `AndroidManifest.xml`, R class fields from current module's resources were not compilable due to order of jars appearing in `--direct_dependencies`. This CL ensure the merged resources jar always comes first.
arunkumar9t2
force-pushed
the
arun/support-merged-resources-jar-in-android-sandwich
branch
from
62c5d7f to
c69a774
Compare
|
Thanks @corbinrsmith, that worked as well. Updated PR. |
corbinrsmith
approved these changes
Jun 29, 2022
kotlin/internal/jvm/compile.bzl
Outdated
| @@ -89,7 +94,7 @@ def _compiler_toolchains(ctx): | |||
| java_runtime = find_java_runtime_toolchain(ctx, ctx.attr._host_javabase), | |||
| ) | |||
|
|
|||
| def _jvm_deps(toolchains, associated_targets, deps, runtime_deps = []): | |||
| def _jvm_deps(current_label_name, toolchains, associated_targets, deps, runtime_deps = []): | |||
There was a problem hiding this comment.
Don't infer off label name -- too brittle.
restingbull
approved these changes
Jun 29, 2022
arunkumar9t2
deleted the
arun/support-merged-resources-jar-in-android-sandwich
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When two
kt_android_libraryorandroid_libraryhas the same package name either provided viacustom_packageor due to same package name inAndroidManifest.xml,Rclass fields from current module's resources were not compilable due to order of jars appearing in--direct_dependencies.The current module's resources jar would contain merged
Rclass from all dependencies but because it appears later in the list, the dependencies'Rclass is used which does not contain merged R class. The problem only appears when two targets contains the same package name.The fix is similar to one applied in Bazel
This PR ensures the merged resources jar always comes ensuring order in deps attribute