Add kt_jvm_proto_helper as individual rule.

[steineldar]

This allows the BUILD file to have separate java proto and kt proto rules in case there is a mix of java and proto projects in the same repo, and there are stricter requirement to the output (e.g. leaner dependency chains, smaller binaries for pure-java outputs etc.).

This also bypasses an issue in the other rules where the kt_jvm_*_library would compile the whole dependency chain to kotlin, and if the bazel repo is large with many proto dependencies, an can easily end up caching all of these when containing the same classes. If only some of these are refreshed / synced on changes you can easily end up with an IDE claiming your new fields still dont' exist, or removed classes are still there etc.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: steineldar / name: Stein Eldar Johnsen (8343c18, f3aadf5, 5c6c35c, 656f83b, 0752d7e)
• ✅ login: jamesward / name: James Ward (bc7e4ed, c0997e0, 8bccc2c)
• ✅ login: aragos (0c1a59c, 165b622)

[aragos]

I agree with the basic premise of this change - it should be possible to pass a proto's java_proto_library target to the kt_jvm_proto_library for the same proto (rather than having the latter generate the former). Ideally kt_jvm_proto_library would have done so from the start. :)

That said, I'm not sure I agree with the approach in this pull request. The helper suffix is rather generic and doesn't tell the BUILD-file reader what the rule does. Given the above (the original rule should have behaved differently) and the fact that there are likely many packages depending on the current behavior (making it difficult to rename the existing rule to java_and_kt_jvm_proto_library or some such; plus changing the behavior of kt_jvm_proto_library as desired) I wonder if a different approach may not be better:

Instead of creating a new rule the existing rule could be amended to gain this additional functionality. For example, a new attribute java_protos could be added, with the constraint that if set it must contain exactly the java_proto_*library targets required to build the Kotlin protos. If not set the rule would generate those targets as it does today.

Let me know what you think!

[steineldar]

I decided to not update the existing rule for the reason you mention there, that it's tricky to update the rule without breaking anything. It should have supported this from the start, though overloading the rule to behave differently based on arguments sounds also fishy to me.

Options:

1. Keep as-is, but as you mention the name is iffy.
2. Keep the basic change as is but rename the rule to something better... Maybe adding _library to the end of the name, as it'l be the library of the helper code. -> kt_jvm_proto_helper_library?
3. Update the existing rules to take in the java_protos dependencies. I doubt I can do much proper validation that it matches the output, but it should fail building the helper lib if things are missing? No way to check if unneeded deps are added AFAIK (unless we just count...). I'm always hesitant to change behavior like that, but if that is preferred, I can make an update to see if it does not get too big of a change.

Personally I would prefer no.2 (less work for me) :-p .

[steineldar]

Also without the java_proto_library build step, I believe there is no difference between _proto_ and _grpc_ rules left...

Different code generation and argument handling. Ignore comment...

[steineldar]

For updating the kt_jvm_proto_library rule directly: #397 -- not sure if I like the outcome...

[steineldar]

I renamed the rule to add _library to it, and I added kt_jvm_grpc-*.

I'm starting to be pretty certain not to change the existing rules:

• The two rules (kt_jvm_grpc_library and kt_jvm_proto_library) has very different arguments and expected behavior with it. E.g. kt_jvm_grpc_library already has srcs and deps and requires it to be 1 each (courtesy of the java_grpc_library rule), while kt_jvm_proto_library only has deps (used for same value as srcs in kt_grpc_*).
• kt_grpc_* rule is currently made in such a way it requires the grpc_library to only contain the grpc service, as it will not build the default kt helpers... But it will build the java_grpc_library associated with it, so ... How am I expected to modify this rule to be as consistent as the others?

PS: Note that I'm just going on vacation, so response times may be varying, though I will check in once in a while with the progress of this PR.

[steineldar]

NOTE: If I don't get feedback on this before I'm back from christmas vacation (early/mid January), I will close this PR assuming it's not wanted. No point in keeping it open forever.

[aragos]

Apologies for the huge delay here, this fell off my radar.

I'm still not a fan of the helper terminology in public APIs. And while I agree with you that for kt_jvm_grpc_ibrary the API would be too complex if we expanded the old rule, I think for kt_jvm_proto_library this can be done. I've added a commit with a sketch of what that could look like, what do you think?

[jamesward]

@steineldar WDYT?

[steineldar]

LGTM: 👍🏻 I'm fine with going for this. Not super happy with the naming, but I'll go with this if you don't like my "helper" naming.

AFAICS the parameter naming between kt_jvm_proto_library and kt_jvm_grpc_no_java_library seems consistent enough, which is the important part here IMO.

[jamesward]

So are we good with this as-is or does it need more before we merge?