WIP: Generate scala code from protobuf using @com_google_protobuf//:protoc

[simuons]

Resolves #690

Introduces new rule scala_proto_gen which

1. generates scala code using implicit @com_google_protobuf//:protoc. This means single version of protoc for proto_library and scalapb_proto_library (instead of one which comes from protoc-jar)
2. accepts plugin as an executable which implements plugin.proto contract. This means less code for us to supply own scala plugins

scala_proto_srcjar was left intact so it wouldn't break existing users. My suggestion is to deprecate it and remove with subsequent pr.

scala_proto_gen generates code in same way as scala_proto_srcjar does ie all direct and transitive deps are generated into single srcjar (no aspects in this pr)

scalapb_proto_library is implemented on top of scala_proto_gen. One thing I don't like though is that I had to create "private" rule to support JavaInfo in scalapb_proto_library deps (maybe there is a better way to achieve that). I believe scalapb_proto_library deps should accept only proto_library as java_proto_library does.

--proto_path were replaced with --descriptor_set_in this removes burden of constructing -I aliases.

I haven't done benchmarks other than bazel clean followed by bazel test //test/proto/.... I got same times in this pr and master. I don't think this pr has impact on performance. In current implementation protoc process is launched from worker which in turn calls executable plugin and I think in actual code generation phase workers are not used (I might be wrong here).

[simuons]

BTW I'm going to test this on our codebase. I should have done it before opening PR but better later than never I guess.

[johnynek]

Looks like we have two racing proto rewrites. Can you and @ianoc-stripe look at each other’s changes and comment on what aspects from each should be used?

[johnynek]

See #700

[johnynek]

I’m starting to wonder if we should have two separate files for two different implementations if there are trade offs here.

Like unless I am wrong this is trading performance for ease of integration with protoc. For us, and I think many, we don’t want to trade performance for ease of configuration, we’d probably just want to do the extra config work.

[johnynek]

This is going to prevent using workers right? So you have to spin up a new JVM for each file, right?

[simuons]

Yes it would prevent using workers. I thought this is what is happening now as well. Now I think my assumptions might be wrong as protoc-bridge is doing some tricks to reuse jvm. I'll dig more and update.

[ittaiz]

@johnynek I think @simuons needs to upload a diagram of our (rules_scala) current process map when using proto.
He explained it to me and AFAIU this will be a net performance improvement.
I do however agree that we need to see how we have both rewrites.
@simuons I think you should consider yielding to @ianoc since he was first unless you think yours greatly simplifies his (which I don't see but might be wrong).

[simuons]

@ittaiz @johnynek I didn't mean to race/interfere with aspects PR. I'm trying to solve different issue here (ie use single @com_google_protobuf//:protoc) which is orthogonal to aspects. I'd say aspects PR should move forward and ignore this one. I still need to investigate performance impact.

PS thanks for your time and sorry for delayed response.

[ittaiz]

@lberki @philwo May I ask if java rules reuse workers for java generation from proto or does this just work natively without opening a jvm?

…

On Sun, 3 Mar 2019 at 16:22 Simonas Pinevičius ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In scala_proto/scala_proto.bzl <#705 (comment)> : > + return file.short_path + +def _scala_proto_gen_impl(ctx): + protos = [p for p in ctx.attr.deps if hasattr(p, "proto")] # because scalapb_proto_library passes JavaInfo as well + descriptors = depset([f for dep in protos for f in dep.proto.transitive_descriptor_sets]).to_list() + sources = depset([f for dep in protos for f in dep.proto.transitive_sources]).to_list() + roots = depset([f for dep in protos for f in dep.proto.transitive_proto_path]).to_list() + inputs = depset([_strip_root(f, roots) for f in _retained_protos(sources, ctx.attr.blacklisted_protos)]).to_list() + + srcdotjar = ctx.actions.declare_file("_" + ctx.label.name + "_src.jar") + + ctx.actions.run( + inputs = [ctx.executable._protoc, ctx.executable.plugin] + descriptors, + outputs = [srcdotjar], + arguments = [ + "--plugin=protoc-gen-scala=" + ctx.executable.plugin.path, Yes it would prevent using workers. I thought this is what is happening now as well. Now I think my assumptions might be wrong as protoc-bridge is doing some tricks to reuse jvm. I'll dig more and update. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#705 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF3ehQk6UW9uT-VoB-pGNQQy1Am6Iks5vS9qYgaJpZM4bYh17> .

[simuons]

Apparently my assumptions about worker usage when generating scala code were wrong. Code generation indeed happens in worker by the trick of protoc-bridge. protoc is supplied with a plugin that pipes it's inputs/outputs to files https://github.com/scalapb/protoc-bridge/blob/master/src/main/scala/protocbridge/frontend/PosixPluginFrontend.scala#L50 and worker reads/writes to them.

Closing this in favour of less aggressive attempt to reuse @com_google_protobuf//:protoc #708

[lberki]

@ittaiz : iff something goes through java_common.compile, it should use workers. AFAIU the Scala plugin is written in Scala, so I suppose then no workers will be used? (modulo, of course, protoc-bridge, which I don't really understand...)