Usage of Worker/Processor and protoc-bridge/protoc-jar in proto rules

[simuons]

Hi,

what is the reason/rationale for proto rules being implemented with Worker/Processor and protoc-bridge/protoc-jar

From my understanding current implementation has few "drawbacks"

1. @com_google_protobuf//:protoc is ignored. It's used by proto_library but scalapb_proto_library uses one of the https://github.com/os72/protoc-jar/tree/master/bin a. it requires extra work/care to keep both protoc at same version
2. it requires extra hops to generate scala code a. bazel invokes scala via Worker/Processor b. protoc_bridge is invoked which i. extracts own protoc to temp directory https://github.com/os72/protoc-jar/blob/master/src/main/java/com/github/os72/protocjar/Protoc.java#L197 ii. starts new protoc process https://github.com/os72/protoc-jar/blob/master/src/main/java/com/github/os72/protocjar/Protoc.java#L125 c. protoc calls scala plugin to generate code
3. to use custom plugin we had to copy Worker and Processor to supply it

As I know there is an aim to use persistent worker. But I guess because new process is spawned benefits of persistent worker cannot be used (my knowledge is limited on this topic).

I have a POC https://github.com/simuons/rules_scala/blob/scala-proto-gen/scala_proto/scala_proto.bzl#L595 which

1. uses @com_google_protobuf//:protoc from bazel (the same as proto_library uses)
2. calls protoc directly with supplied plugin and is based on descriptor sets to avoid --proto_path construction (mentioned here https://blog.bazel.build/2017/02/27/protocol-buffers.html # Descriptor Sets)
3. needs less code to supply own plugin https://github.com/simuons/rules_scala/blob/scala-proto-gen/src/scala/scripts/ScalaPBPlugin.scala to a rule https://github.com/simuons/rules_scala/blob/scala-proto-gen/test/proto/BUILD#L124

What do you think? If all I wrote makes any sense I'd like to proceed with that.

[johnynek]

This all makes sense to me.

@azymnis contributed the original version and I think he just wanted to get unblocked. Ideally, I'd love to have only 1 version of the rules, and not break previous builds. So, if we could improve without breakage, that would be great, e.g. not breaking any of the current tests.

Also note the issue here if you are improving matters: #548 -- I think using aspects to transform the proto dependency graph into a scala_proto dependency graph would make things work more similar to the java proto rules.

[johnynek]

One note: the persistent worker is still used for the scala/jvm code, and without that worker support you have to spin up a new JVM each time you build a target. This will likely be a considerable slowdown.

Have you benchmarked your version? I wonder if we can keep the worker support, but use the same protoc as bazel and sidestep keeping anything in sync.

[azymnis]

@simuons there really wasn't a reason. I was just hacking things together till they worked. If your version works with the built in @com_google_protobuf//:protoc we should totally go with that instead.

@johnynek would the current set of tests passing be enough to make sure that we don't break existing users? We have moved away from protobuf in scala at KH so our code would not be a good candidate for beta testing. Maybe we can try looking for any existing users?

[johnynek]

We can try a build at Stripe. We have mostly thrift, but moving towards scala.

[simuons]

@johnynek @azymnis thanks for your prompt responses.

I totally agree on having one set of rules. However I'm not sure how to handle https://github.com/bazelbuild/rules_scala/blob/master/scala_proto/scala_proto.bzl#L415 (as it is tightly coupled to scalapb). Anyway I'll try to converge both rules to single one.

I intentionally didn't mention aspects here. It's definitely in my plans but I see this change in this issue as a smaller increment towards end goal.

I'll dig more into workers and do some benchmarks.

[ittaiz]

I’ll just add that Simonas works at Wix and we’ll of course try it out on our internal code base first On Fri, 15 Feb 2019 at 9:45 Simonas Pinevičius notifications@github.com wrote:

@johnynek https://github.com/johnynek @azymnis https://github.com/azymnis thanks for your prompt responses.

I totally agree on having one set of rules. However I'm not sure how to handle https://github.com/bazelbuild/rules_scala/blob/master/scala_proto/scala_proto.bzl#L415 (as it is tightly coupled to scalapb). Anyway I'll try to converge both rules to single one.

I intentionally didn't mention aspects here. It's definitely in my plans but I see this change in this issue as a smaller increment towards end goal.

I'll dig more into workers and do some benchmarks.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bazelbuild/rules_scala/issues/690#issuecomment-463940289, or mute the thread https://github.com/notifications/unsubscribe-auth/ABUIF2Fy1swfyMk8fXte1DoZE_aeWhZHks5vNmWjgaJpZM4a59n4 .

[johnynek]

BTW: you can look at the scrooge example:

https://github.com/bazelbuild/rules_scala/blob/18889f641ed62dd09aeb71d1505f3834ccc33be3/twitter_scrooge/twitter_scrooge.bzl#L340

for seeing one using aspects. We have been using that rule at Stripe pretty heavily. We have a tool that parses the thrift and generates fine grained BUILD files so you can depend on individual thrifts without any pain on the user.

We have a proto parser as well, I guess we could make a tool that generates build files for protos too.

[ianoc]

@simuons I think you fixed this?

[simuons]

This issue is not relevant anymore.