rules_kotlin
rules_kotlin copied to clipboard
bazelbuild
Fix missing assets in kotlin inner sandwich lib + handle merging mani…
@Bencodes @oliviernotteghem this does not fully address the regression introduced in 1.9 for us. We have kt_android_library rules that use other attributes, specifically proguard_specs, that are not getting passed in here.
On a more fundamental level, I still do not understand the value of whitelisting specific attributes to pass down, rather than popping exports out and passing down the rest. If the goal is to not send exports, then we should not send exports, but not make firm opinions about what android_library attributes are and are not allowed. Doing it this way means that, for instance, if a new attribute is added to android_library, work is required here to support it. I don't see the value in doing this. What am I missing?
@pswaminathan : are you sure proguard_specs are needed by the inner sandwich android lib? These should be set on the outer lib, no?
re : whitelisting attributes. While optimizing build time, we wanted to keep input to a minimal list. This way target is less likely to be invalidated. Also, one can argue **kwargs here does not contain here all attributes anyway (a lot are 'popped out' already implicitely : https://github.com/bazelbuild/rules_kotlin/blob/ca5b2167724ec0f1873b46e4e98f60aac52f50f7/kotlin/internal/jvm/android.bzl#L29-L41)
A good test would be to pop out only the exports, and run full build time regression test to see if there is no regression. If so, your approach should be safe. But decision is contigent on the proguard_specs issue. Can you look into this specific one?
@oliviernotteghem thanks for responding!
are you sure proguard_specs are needed by the inner sandwich android lib? These should be set on the outer lib, no?
I'm not sure. I don't think it matters so much because the inner lib gets exported by the outer lib anyway, so they'll get collected downstream either way. But I don't see proguard_specs on the outer lib either. So I think my ask is less "shouldn't proguard_specs be on the inner lib" and more "shouldn't it be somewhere". Adding it to the outer lib feels like a reasonable alternative.
While optimizing build time, we wanted to keep input to a minimal list. This way target is less likely to be invalidated.
I hear that. But I don't think the default should be "some android_library attributes don't count". We can disagree on this, I suppose, and if I need to add a different tag to certain targets, I can do that. It just feels like the default should be that things work, and not that attributes are silently dropped.
Also, one can argue **kwargs here does not contain here all attributes anyway (a lot are 'popped out' already implicitely
Yes, but they are popped out because they get used. If we popped out every attribute used by android_library and passed in each one explicitly, that would still implement the base functionality. I don't care about **kwargs per se; I care that functionality is being dropped.
A good test would be to pop out only the exports, and run full build time regression test to see if there is no regression. If so, your approach should be safe. But decision is contigent on the proguard_specs issue. Can you look into this specific one?
When you say this, do you mean to run the full rules_kotlin test suite? That is being done and passes with this approach in #1110. Though in that PR, I just modified that branch. Your PR here differs in that you removed one of the branches entirely. If you'd rather, I would be happy to incorporate that approach (removing one of the branches) into my PR.
@oliviernotteghem thinking about this a little more:
Looking at the list of android_library attributes, the only ones that aren't handled at the moment by either the outer or inner library are proguard_specs and the IDL arguments. The former can probably go on the outer library, while the latter probably need to be set on the inner library. I don't actually have any experience using the AIDL features, so I have no way of confirming/checking.
If you agree with adding this and want to do that in this PR, I'm on board. Otherwise I can submit another PR with that if we want to merge this in first.
re : "run full build time regression test". Sorry, I am referring to our internal build time benchmark. I would have to do this.
I can't really help with aidl either (we're not using it in our codebase), but I like the idea of amending PR with setting proguard_specs (and aidl if we figure out how to test it) : this way it will not only contain are fixes (which addressed case like when kt android library are resource-less) but also the bug you found with proguard_specs.
Going to close this PR since we rolled back to the previous behavior https://github.com/bazelbuild/rules_kotlin/commit/0f4c0af37ea4451fa01e001cf704a8fc6c897d22