Create scala_proto_library rule using scalaPB #281

azymnis · 2017-09-26T18:47:46Z

This addresses issue #144

@johnynek @ittaiz I put this up to get your feedback. This currently "works" but I need to test it more rigorously, add all available options that can be passed to scalapb, etc.

Note that I'm taking a different approach to the scrooge rules: there's only one parameter that you can pass into the rule and that is deps, which has to be of type proto_library. Since proto_library already does the work of figuring out all transitive proto file dependencies, there is a lot less work that this rule has to do. This would not allow for remote proto jars, but I would argue that using these is a really bad idea in the first place.

Anyway, let me know what you think.

bazel-io · 2017-09-26T18:47:47Z

Can one of the admins verify this patch?

ittaiz · 2017-09-26T18:51:09Z

test this please (ci bot)

azymnis · 2017-09-26T21:24:31Z

@ittaiz the build seems to have failed. Is this a macos issue?

johnynek · 2017-09-26T23:45:58Z

yeah, passes on linux. The bazel ci is broken (needs to accept xcode license) cc @damienmg

The travis macos timed out. Maybe a transient travis issue.

Will review!

Thanks man, this is great!

johnynek

just a couple of minor comments.

This is great! Thanks for jumping on it.

johnynek · 2017-09-26T23:51:56Z

scala_proto/scala_proto.bzl

+    )
+
+    native.maven_jar(
+        name = "scala_proto_rules_slalapbc",


looks like a typo here: slala

Whoops let me fix

johnynek · 2017-09-26T23:57:21Z

src/scala/scripts/ScalaPBGenerator.scala

+    val jarOutput = args.get(0)
+    val protoFiles = args.get(1).split(':').toList
+
+    val tmp = Paths.get(Option(System.getenv("TMPDIR")).getOrElse("/tmp"))


I think we actually want:

System.getProperty("java.io.tmpdir")

Which I think bazel sets up correctly.

we got burned on this somewhere else in some context. Did you copy this from another rule?

Yeah I copied this from the scrooge rules. Want me to fix that one too?

johnynek · 2017-09-27T00:01:02Z

test/BUILD

+scala_proto_library(
+    name = "test_proto",
+    deps = ["//test/proto:test2", "//test/proto:test3"]
+)


can you add a test that creates these things in some scala code?

Sure, let me do that... I'll also add a mock grpc service to see how that works too

johnynek · 2017-09-27T05:23:54Z

Yeah, if you could fix the Scrooge rules it would be great.

On Tue, Sep 26, 2017 at 18:31 Argyris Zymnis ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/BUILD <#281 (comment)> : > @@ -432,3 +435,8 @@ scala_binary( main_class = "scala.test.BinaryDependentOnJava", deps = [":JavaOnlySources"], ) + +scala_proto_library( + name = "test_proto", + deps = ["//test/proto:test2", "//test/proto:test3"] +) Sure, let me do that... I'll also add a mock grpc service to see how that works too — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#281 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEJdoy4nhL1FxQ3A6Ku_VA0hMNQq5hBks5smc-fgaJpZM4PktRQ> .

-- P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin

azymnis · 2017-09-27T06:54:17Z

@johnynek so I went ahead and added support for grpc service definitions but this opens up a whole can of worms: grpc depends on a ton of dependencies that I had to pull in. Added a flag to turn grpc service stub generation on and off for a target (this is supported by scalapb compiler).

I also added a test for a dumb grpc server and client. The code compiles and runs on my machine, but not sure what the easiest way would be to actually run this in CI.

Anyway, let me know what you think. Not happy that we need to pull in the universe to get grpc bindings to work, but not sure that there's really anything that we can do about it.

damienmg · 2017-09-27T09:17:07Z

Jenkins: retest this please

(license should be fixed on all the mac slave now)

azymnis · 2017-09-27T22:46:28Z

@damienmg there was an error in one of my BUILD files, which I just fixed

@johnynek @ittaiz what do you think of calling the rule scalapb_proto_library?

johnynek · 2017-09-27T22:53:30Z

test this please

johnynek · 2017-09-27T23:01:21Z

scala_proto/scala_proto.bzl

+    "//external:io_bazel_rules_scala/dependency/proto/scalapb_fastparse",
+]
+
+GRPC_DEPS = [


deep dependency stack. :/

Yeah it's kind of nuts actually

johnynek · 2017-09-27T23:02:02Z

This looks good to me.

👍

azymnis · 2017-09-28T18:48:36Z

I just added all other available scalapbc flags to this (java_conversions, flat_package, single_line_to_string).

BTW I noticed that some of the travis builds time out. Is that a regression that I introduced or has it been going on for a while? This build is really really slow. Should we maybe break it up into separate jobs? Is calling bazel clean the cause of slowness?

johnynek · 2017-09-28T19:13:18Z

test this please

johnynek · 2017-09-28T19:15:48Z

scala_proto/scala_proto.bzl

+        executable = ctx.executable._pluck_scalapb_scala,
+        inputs = list(acc_imports) + [argfile],
+        outputs = [ctx.outputs.srcjar],
+        mnemonic="ProtoScalaRule",


should this be ProtoScalaPbRule or something?

Yeah let me rename it

azymnis · 2017-09-29T17:13:15Z

Any ideas about what to do with build timeouts?

johnynek · 2017-09-28T19:17:51Z

scala_proto/scala_proto.bzl

+
+    external_deps = list(SCALAPB_DEPS + GRPC_DEPS if (with_grpc) else SCALAPB_DEPS)
+
+    if with_java_conversions:


this doesn't seem like a java_conversion. Could we just call this with_java since you are creating a whole new target, no?

Sure. Maybe I should just remove all the with_ prefixes?

sounds good.

Actually, sorry misread your comment. Sure let me just change that

johnynek · 2017-09-28T19:18:46Z

scala_proto/scala_proto.bzl

+    if ctx.attr.with_grpc:
+        flags.append("grpc")
+    if ctx.attr.with_java_conversions:
+        flags.append("java_conversions")


how does this flat change with scalapb? It seems like you are generating the java below with a native call?

johnynek · 2017-09-28T19:20:04Z

scala_proto/scala_proto.bzl

+            mandatory=True,
+            allow_rules=["proto_library"]
+        ),
+        "with_grpc": attr.bool(default=False),


can you add comments so users understand how to use each of the options, that would be good.

johnynek · 2017-09-28T19:20:14Z

scala_proto/scala_proto.bzl

+        "with_grpc": attr.bool(default=False),
+        "with_java_conversions": attr.bool(default=False),
+        "with_flat_package": attr.bool(default=False),
+        "with_single_line_to_string": attr.bool(default=False),


I have no idea what this might do.

This makes scalapb generate a toString method for generated classes that prints a single line of text. Not sure why you would want that but it is available as a compiler option.

johnynek · 2017-09-28T19:20:21Z

scala_proto/scala_proto.bzl

+        ),
+        "with_grpc": attr.bool(default=False),
+        "with_java_conversions": attr.bool(default=False),
+        "with_flat_package": attr.bool(default=False),


what is this?

Here's where all the scalapb compiler settings are explained https://scalapb.github.io/sbt-settings.html

I'm happy to add comments for all of these

yeah, adding a link to these settings in the comment would be good.

johnynek · 2017-09-29T18:03:12Z

test this please

…/scalapb

googlebot · 2017-09-29T21:26:37Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

azymnis · 2017-09-29T21:28:21Z

@johnynek I merged your branch and this seems to have made the @googlebot unhappy

johnynek · 2017-09-29T22:32:48Z

it's okay, that bot's not the boss of me.

johnynek · 2017-09-30T00:12:24Z

this is no longer WIP, right?

Ready to merge @azymnis ?

azymnis · 2017-09-30T00:13:42Z

Yup ready!

…

On Fri, Sep 29, 2017 at 5:12 PM P. Oscar Boykin ***@***.***> wrote: this is no longer WIP, right? Ready to merge @azymnis <https://github.com/azymnis> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#281 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMpF-Yy86gNlQByl3mIwbSS7H8HSvViks5snYdqgaJpZM4PktRQ> .

johnynek · 2017-09-30T00:16:04Z

👍

I think I'll let @ittaiz merge #285 first (not squash/rebase I hope) then I will merge this after that.

azymnis · 2017-09-30T01:24:53Z

@johnynek I should prob update the rule docs too in the readme. Hold off before merging till I do that

johnynek · 2017-09-30T23:59:27Z

@azymnis thanks so much for doing this!

I'll tweet about this on Monday (to maximize exposure). I think the fact that the bazel rules now support proto and grpc is a big selling point. I hope this gets us more users, bringing more testing and more contributors!

Thanks again!

azymnis · 2017-10-01T05:30:27Z

@johnynek no problem, happy to work on this and learn more about bazel

ittaiz · 2017-10-01T05:54:12Z

Indeed thank you for this valuable addition.
I was out of the loop due to holidays and backlog but as we (Wix) are moving a lot of our RPC to gRpc this will be heavily used.

ittaiz · 2017-10-01T05:58:45Z

src/scala/scripts/BUILD

+        "//src/java/io/bazel/rulesscala/io_utils",
+        "//src/java/io/bazel/rulesscala/jar",
+        "//src/java/io/bazel/rulesscala/worker",
+        "//src/scala/io/bazel/rules_scala/scrooge_support:compiler",


Why does protobuf generator need to depend on scrooge?

Fix is here: https://github.com/bazelbuild/rules_scala/pull/290/files

ittaiz · 2017-10-01T06:09:11Z

src/scala/scripts/ScalaPBGenerator.scala

+    val scalaPBArgs = s"--scala_out=${flagPrefix}${scalaPBOutput}" :: protoFiles
+    val config = ScalaPBC.processArgs(scalaPBArgs.toArray)
+    val code = ProtocBridge.runWithGenerators(
+      protoc = a => com.github.os72.protocjar.Protoc.runProtoc(config.version +: a.toArray),


This library jar brings with it the compilers for the relevant platforms right? It doesn't download anything during build time according to the running platform, right?

Yes that's right. My understanding while reading the ScalaPBC doc is that it depends on protoc-bridge and protobuf-java which I have added as explicit dependencies.

azymnis · 2017-10-01T14:46:43Z

Oh good point, it shouldn't depend on Scrooge. I'll send a fix

…

On Sat, Sep 30, 2017 at 10:58 PM Ittai Zeidman ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/scala/scripts/BUILD <#281 (comment)> : > @@ -21,3 +21,29 @@ scala_binary( ":generator_lib", ], ) + +scala_library( + name = "scalapb_generator_lib", + srcs = ["ScalaPBGenerator.scala"], + visibility = ["//visibility:public"], + deps = [ + "//external:io_bazel_rules_scala/dependency/proto/protoc", + "//external:io_bazel_rules_scala/dependency/proto/scalapb_plugin", + "//external:io_bazel_rules_scala/dependency/proto/protoc_bridge", + "//external:io_bazel_rules_scala/dependency/proto/scalapbc", + "//external:io_bazel_rules_scala/dependency/com_google_protobuf/protobuf_java", + "//src/java/io/bazel/rulesscala/io_utils", + "//src/java/io/bazel/rulesscala/jar", + "//src/java/io/bazel/rulesscala/worker", + "//src/scala/io/bazel/rules_scala/scrooge_support:compiler", Why does protobuf generator need to depend on scrooge? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#281 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMpFzdDzHUSKj49JXBhBLAa8ieyNgpcks5snyoWgaJpZM4PktRQ> .

Argyris Zymnis added 2 commits September 26, 2017 11:11

First version that compiles a srcjar

6c944d8

Scala libraries now work and compile

a2a299f

googlebot added the cla: yes label Sep 26, 2017

johnynek reviewed Sep 27, 2017

View reviewed changes

Added grpc support and a server/client code test

24ed557

Fix broken build, add test, rename to scalapb_proto_library

b3a1ece

johnynek reviewed Sep 27, 2017

View reviewed changes

Add all other available scalapbc flags

9cdc4c3

Set deps and sources correctly

8123615

johnynek reviewed Sep 28, 2017

View reviewed changes

Argyris Zymnis and others added 2 commits September 28, 2017 13:08

Rename mnemonic

8dd5ea4

use latest 0.6.0 in travis

1d9ea92

johnynek reviewed Sep 29, 2017

View reviewed changes

johnynek and others added 3 commits September 29, 2017 08:16

use workers in travis

3b2f6d8

Merge remote-tracking branch 'upstream/oscar/test-0_6_0' into azymnis…

402a2b2

…/scalapb

Add docstring and rename with_java

2a1ef0a

googlebot added cla: no and removed cla: yes labels Sep 29, 2017

johnynek mentioned this pull request Sep 29, 2017

add 0.6.0 to travis #285

Merged

johnynek added cla: yes and removed cla: no labels Sep 29, 2017

johnynek changed the title ~~WIP: Create scala_proto_library rule using scalaPB~~ Create scala_proto_library rule using scalaPB Sep 30, 2017

Add documentation

053d899

johnynek merged commit 1a856c2 into bazel-contrib:master Sep 30, 2017

ittaiz reviewed Oct 1, 2017

View reviewed changes

rynbrd mentioned this pull request Oct 18, 2017

scalapb_proto_library does not support well known protobufs #312

Closed

azymnis deleted the azymnis/scalapb branch December 10, 2018 17:50


		external_deps = list(SCALAPB_DEPS + GRPC_DEPS if (with_grpc) else SCALAPB_DEPS)

		if with_java_conversions:

Uh oh!

Create scala_proto_library rule using scalaPB #281

Create scala_proto_library rule using scalaPB #281

Uh oh!

Conversation

azymnis commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bazel-io commented Sep 26, 2017

Uh oh!

ittaiz commented Sep 26, 2017

Uh oh!

azymnis commented Sep 26, 2017

Uh oh!

johnynek commented Sep 26, 2017

Uh oh!

johnynek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnynek commented Sep 27, 2017 via email

Uh oh!

azymnis commented Sep 27, 2017

Uh oh!

damienmg commented Sep 27, 2017

Uh oh!

azymnis commented Sep 27, 2017

Uh oh!

johnynek commented Sep 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnynek commented Sep 27, 2017

Uh oh!

azymnis commented Sep 28, 2017

Uh oh!

johnynek commented Sep 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azymnis commented Sep 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

azymnis commented Sep 26, 2017 •

edited

Loading