bazelbuild
[Api Discussion via docs] Adding a more manual/advanced scalapb rule
In the change i made to make these rules aspect based previously it has blocked/made difficult some of scalapb's options that don't translate well into aspects (and aren't supported in the java protobuf so don't come up there). This proposes adding another rule which would give full flexibility in invoking the scalapb to cover these use cases.
Using both rules in a repo does pose the risk of classpath conflicts of multiple implementations that are different of the same class name. Though I don't see a way around this issue in general. We could have the direct rule check to ensure none of its dependencies also contain the same class name it is emitting, to ensure it doesn't depend on an aspect for the same protobuf?
Thoughts @ittaiz @simuons @johnynek ?
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
:memo: Please visit https://cla.developers.google.com/ to sign.
Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.
What to do if you already signed the CLA
Individual signers
- It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
Corporate signers
- Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
- The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
- The email used to register you as an authorized contributor must also be attached to your GitHub account.
ℹ️ Googlers: Go here for more info.
@ianoc thanks for taking the time! We are interested in the aspect approach for performance and since the current approach generates multiple copies of dependent protos (causes issues in IJ as well). We are planning towards end of next week to prototype a suggested API on how to support multiple custom generators using toolchains and aspects. We might fail but we want to give it a try :)
In general I'm not against this if you have concrete use-cases internally you're aware of. I think that the conflict check can be valuable depending on cost (both build time and dev time)
We don't have them internally, the use cases that were coming up are:
https://github.com/bazelbuild/rules_scala/issues/765 https://github.com/bazelbuild/rules_scala/issues/753 https://github.com/bazelbuild/rules_scala/issues/751
so if your work managed to cover these and we didn't need it at all that would be better again
Very interesting. #751 and #753 indeed sound very very similar to our needs and concerns. This is what we'll try to solve (don't know if we'll be able to though). Can you or @johnynek refresh my memory about why aspects are better? My idea before the aspect approach was to separate the "to-be-generated" protos from the "classpath" protos (which I'm fairly certain is an option in protoc). This should solve the multiple copies of a proto appearing in the scala classpath later on and will also greatly improve performance (less generation and less compilation in the next step).
Am I missing something? Are these indeed the benefits but you think I'm mistaken about protoc support for this?
@ittaiz You can with protoc for sure separate out the proto's from having multiple copies. (using the internal function of src to jar thats in there now would do this). The aspects make the whole chain fully automatic which is the big win(to add the aspects to our largest scala repo i had to add the toolchain then strip the per-target options -- so a pretty simple PR, it immediately dropped 6-8 minutes off average our CI times for the repo which was super nice). Aspects make a few things better here though:
- You don't need to define the scalapb targets at every step of the chain. In our case this is huge since we have cross language bazel dependencies/generated proto's and similar, so you can define the scalapb in the repo you need it/where you need it and all of the intermediate targets are automatically supplied
- You don't have folks globbing protobuf files/proto_library targets out of laziness into one scalapb (well they can, but internally via the aspect thats not what will happen).
- With the former two issues you can wind up with 2 scalapb's referring to the same proto_library without a bunch of tooling/convention to stop it. So the aspects ensure you don't get duplicated classes from this.
- Not the aspects so much, but the fact the options are configured on the toolchain while a pain for the extra options in scalapb makes the whole repo consistent in how the scala is generated which is a plus. (Mostly here the
flat_packageoption has caused us problems in the past)
I think mostly what I was suggesting here is pretty close to what you were thinking about doing pre-aspects here, just having a rule that wouldn't produce all the classes all the time.
No I'm afraid there didn't seem to be a consensus on design to drive it. So I've long since moved onto other things.
