scalapb / ScalaPB

Protocol buffer compiler for Scala.
https://scalapb.github.io/
Apache License 2.0
1.3k stars 278 forks source link

Support and document buf #1425

Open thesamet opened 1 year ago

thesamet commented 1 year ago

In order to support build by bufs, we need the following:

Luka-J9 commented 1 year ago

I haven't explored grpc compilers (outside the netty one that comes by default). I know some are in your purview (ZIO I think), should publishing those be included in the scope of work as well?

It might also be nice to include some instructions on publishing your own? (Maybe include a nice GHA)

Also I think we don't need to use the JVM based build since under the covers the plugins are wrapped up in docker. I think the only requirement is that it's compiled in x86, which the native plugin is already. For reference my tests last week used the native plugin on an M1. Unless there is some other functional reason to use the JVM implementation.

Luka-J9 commented 1 year ago

Another thought. Given the fact that buf will need to be run before normal code compilation in this model should we consider writing an sbt wrapper around buf so it hooks in cleanly?

thesamet commented 1 year ago

Should publishing those be included in the scope of work as well?

Yes, I can take on doing this for zio-grpc and scalapb-validate native plugins to buf after this is done to ScalaPB (so I can follow the pattern established here), and check that it all works together.

Also I think we don't need to use the JVM based build since under the covers the plugins are wrapped up in docker.

Great, let's use the x86 native plugin then! I didn't know that.

Given the fact that buf will need to be run before normal code compilation in this model should we consider writing an sbt wrapper around buf so it hooks in cleanly?

Yes, I think this should be considered, and if so - should it be a mode of operation for sbt-protoc, or a separate plugin with possibly some code re-use. Few important features of sbt-protoc to be considered: unpacking jars with proto dependencies, ensuring scalapb-options.proto file get read, checks whether source changed before running.

thesamet commented 1 year ago

What's the advantage for an sbt user that already chose to use buf outside sbt, to have their sbt build use buf as well and not sbt-protoc with stock protoc? Trying to understand the added value of building the plugin/wrapper mentioned in your message.

Luka-J9 commented 1 year ago

unpacking jars with proto dependencies, ensuring scalapb-options.proto file get read, checks whether source changed before running.

So interestingly with buf, using the plugin, all code, including 3rd party dependencies can be compiled client side, so no need to unpack jars, or even vend jars. Because there are no jars to manage this way you don't actually need to publish scalapb-options as well, since now there can be no mismatch between jar published and the generation options in the proto-file. This solves problems where there is different generation preferences amongst different teams of people, or the use/non use of different plugins. For example this issue where overrides were needed due to validate. I've run into other issues where the who was overriding what was less clear, and with buf this can be defined once on the client to suite their needs/preferences.

If you choose not to also compile imports/"build the world" you would only need the jars that represent the generated code, the protofiles would not be necessary to unpack. However given the fact that you'd need to make sure your buf deps are in line with the jars I'd imagine that this would be viewed as not ideal. This problem (making sure your buf deps and jar deps line up) is also problematic currently when using buf in tandem with sbt-protoc.

What's the advantage for an sbt user that already chose to use buf outside sbt, to have their sbt build use buf as well and not sbt-protoc with stock protoc?

I think buf should be viewed as a replacement for protoc, as it is built as an implementation of protoc with dependency management/backwards compatibility checks in mind. Hence the suggestion of an sbt-buf, as you'd be linking the lifecycle of buf to sbt builds much like how sbt-protoc is linking the lifecycle of protoc with sbt builds.

noirbizarre commented 1 year ago

What's the advantage for an sbt user that already chose to use buf outside sbt, to have their sbt build use buf as well and not sbt-protoc with stock protoc? Trying to understand the added value of building the plugin/wrapper mentioned in your message.

My 2 cents: I think (because this is my use case) the main benefit is for the case where you need to publish synchronized versions for multiple languages.

In our case, having each project consuming the proto files an generating the proper langage files have been a nightmare lately. Having a single repository able to publish the same proto files version and langage specific generated files for Python, Scala, Go... simplified everything: no proto files to copy, no git submodules, no synchronized toolchain to maintain... Just consume the right versionned package

There is 2 aspects on buf support:

onmomo commented 1 year ago

any chance to bump the scalapb buf module to the latest scalapb version?

thesamet commented 1 year ago

@Luka-J9 is this something that can be automated?

Luka-J9 commented 1 year ago

@Luka-J9 is this something that can be automated?

Yep! Will need a new GitHub action I believe - I'm on vacation this week but I can look into it when I get back

Luka-J9 commented 1 year ago

Something to note: Buf is fairly prescriptive about how protobuf should be structured. I'll assume that we just want to publish - so I'll turn those off those warnings for the initial publish.

The main two ones are the file structure being scalapb.v1 rather than scalapb and enums being prefixed by the enums name (e.g. EXACT -> MATCH_TYPE_EXACT)

@thesamet let me know if the above assumption is valid or if you want me to address the lint issues as well

thesamet commented 1 year ago

@Luka-J9 - what is scalapb.v1? Let's not rename enums for making the linter happy at this time.

Luka-J9 commented 1 year ago

@Luka-J9 - what is scalapb.v1? Let's not rename enums for making the linter happy at this time.

It prescribes packaging and writing protos in a versioned directory/package. That way you can make breaking changes in a different package (e.g. scalapb.v2) while maintaining the previous version

Luka-J9 commented 1 year ago

@thesamet - Took a quick look and it seemed easy enough to add instead of waiting until Monday I added a PR that I believe will automatically publish on push to master (draft release) and when a tagged release is cut (official release).

I'm not sure how to test that it works as intended but I linked the reference documentation. The relevant linting rules have been disabled per the previous conversation.

Once this works a follow up PR can be added to ensure no breaking changes can be committed and probably a lint/breaking test on PR rather than just on merge.

I can follow up on any feedback in the PR but my availability is spotty until Monday. Thanks!