generate and compile Go from Protobuf #14714

tdyas · 2022-03-07T02:24:58Z

Complete Go protobuf support by actually wiring up compilation of the generated Go sources.

[ci skip-rust]

[ci skip-build-wheels]

tdyas · 2022-03-07T02:33:58Z

~~Open questions:~~

1. Dependencies between protobuf files are not handled by the PR yet. This should probably walk the dependencies of the protobuf_source target and then use BuildGoPackageTargetRequest to obtain the applicable BuildGoPackageRequest. Protobuf dependency inference will automatically add those dependencies.
2. This does not yet handle merging the compilation of multiple proto files with the same import path. We should probably look for sibling proto files with the same import path (from go_package) and merge their builds into one build.
3. Bug in GoCodegenBuildRequest rules?: Using a protobuf_sources as the dependency for a go_package target is not being expanded to the underlying protobuf_source targets. Target generation has actually run since I could use the file-level target instead in the test.
4. The current method of using ImportPathToPackages to resolve import paths to package addresses won't work with multiple go.mod's. There will eventually need to be a way for to know what go.mod is in effect for this target in order to know where to scan for third-party packages for dependencies.

tdyas · 2022-03-07T02:34:14Z

cc @Eric-Arellano

Eric-Arellano · 2022-03-08T23:38:50Z

Bug in GoCodegenBuildRequest rules?

Yep. Fixed. Thanks for the report 🙌

There will eventually need to be a way for to know what go.mod is in effect for this target in order to know where to scan for third-party packages for dependencies.

Yes. This will look like #14693, but Golang-ier. protobuf_source targets will declare which go.mod they belong to via a new plugin field. You use parametrize if you want it to work with multiple go.mods.

We still need to figure out how you refer to that go.mod, if you have a logical "name" like Python and JVM vs the path to it vs something else: #13114

Dependencies between protobuf files are not handled by the PR yet. This should probably walk the dependencies of the protobuf_source target and then use BuildGoPackageTargetRequest to obtain the applicable BuildGoPackageRequest. Protobuf dependency inference will automatically add those dependencies.

I think so, and indeed Pants dep inference already handles inter-Protobuf dependencies.

But I also think this highly relates to your question 2. It seems possible that inter-Protobuf dependencies might end up belonging to the same package? If so, they need to be built together — not via a direct dependency.

We should probably look for sibling proto files with the same import path (from `go_package) and merge their builds into one build.

We should, but also I think "sibling" isn't relevant here, right? It is possible for a protobuf_source file implemented in dir1/ to set the same import path as dir2/, even though not siblings.

I think we will need to create a module mapping of each import path to set of Protobuf files. That set needs to behave as an atomic unit.

It's convenient that this module mapping is the same way we will implement dependency inference from Go -> Protobuf! If Go imports a generated package, we must infer a dep on all protobuf_source targets that are part of the package: the whole set.

Question @tdyas, do we expect that users will need to set the go_package in their .proto file for the package? https://developers.google.com/protocol-buffers/docs/reference/go-generated mentions how you can set the value via CLI, and explicitly calls out Bazel using this. Got me wondering if this should be exposed as a field in the BUILD file?

Generally, it's valuable for Pants to try to stay closer to the standard way of doing things: that you can still use normal Protoc if you want. This is important for incremental adoption. Which would suggest it's better for people to set the option in their .proto file rather than relying on Pants magically setting up argv the right way.

We could also start with requiring you to add it, and if users find that too restrictive we can consider loosening this. Easier to make less strict than vice versa.

tdyas · 2022-03-09T05:29:50Z

We still need to figure out how you refer to that go.mod, if you have a logical "name" like Python and JVM vs the path to it vs something else: #13114

Via the import path for the module?

Eric-Arellano · 2022-03-09T05:33:03Z

Via the import path for the module?

Possibly! That would not fit well with the generic generate-lockfiles and resolves, but we already expect Go to be different than the other languages because Go has such an opinion already on the topic.

(It's probably better to discuss multiple go.mod at #13114. It shouldn't block this PR, so long as #14693 is viable.)

[ci skip-rust] [ci skip-build-wheels]

tdyas · 2022-03-11T17:31:03Z

This is ready for review.

The rules infer dependencies from generated Go code to existing Go packages (including the protobuf runtime) and dependencies on other Go Protobuf packages.

tdyas · 2022-03-11T17:31:29Z

cc @chrisjrn in case there anything relevant here for JVM rules

tdyas · 2022-03-14T16:40:58Z

ping

stuhood

@Eric-Arellano should probably still review, but here's my stamp.

stuhood · 2022-03-14T19:10:49Z

src/python/pants/backend/codegen/protobuf/go/rules.py

 @rule
 async def setup_build_go_package_request_for_protobuf(
-    _: GoCodegenBuildProtobufRequest,
+    request: GoCodegenBuildProtobufRequest,


@Eric-Arellano: Relative to #14751, I'm still not quite sure why Go needs a union separate from the GenerateSourcesRequest union, which already indicates that code can be generated for Go.

A lot of work went in here, and I won't block. But it would be good to be sure that it is necessary relative to using an existing BuildGoPackageRequest with a target which "can be converted to" Go, rather than one that already has matching field(sets) (i.e.: this code becoming codegen aware rather than using direct field access).

The inference @rules (on the other hand) do seem necessarily custom, since they map from protobuf sources to a Go path.

stuhood · 2022-03-14T19:15:06Z

src/python/pants/backend/codegen/protobuf/go/rules.py

+    package_mapping: ImportPathToPackages,
+    go_protobuf_mapping: GoProtobufImportPathMapping,
+    analyzer: PackageAnalyzerSetup,
+) -> BuildGoPackageRequest:


Should this return a FallibleBuildGoPackageRequest to fail slow here?

Yeah I realized this was a bad call on my part when adding the plugin API - we want ./pants check :: to fail gracefully. I think it's fine to fix this is a followup, given that it's changing the generic plugin hook

Will do in follow-up.

tdyas mentioned this pull request Mar 7, 2022

Go: support Protobuf #12820

Closed

Tom Dyas added 5 commits March 11, 2022 00:22

go: compile generated protoc output

a33e262

[ci skip-rust] [ci skip-build-wheels]

checkpoint

c2018a8

[ci skip-rust] [ci skip-build-wheels]

switch to multiple files compiles

d7b0362

[ci skip-rust] [ci skip-build-wheels]

fix bug + test debug

5762038

[ci skip-rust] [ci skip-build-wheels]

test multi file

d737211

[ci skip-rust] [ci skip-build-wheels]

tdyas force-pushed the golang_finish_protobuf branch from e627c3b to d737211 Compare March 11, 2022 14:26

Tom Dyas added 2 commits March 11, 2022 10:37

multi-file packages and proto->proto go dep inf

79618a3

[ci skip-rust] [ci skip-build-wheels]

cleanup for review

8305e1a

[ci skip-rust] [ci skip-build-wheels]

tdyas changed the title ~~[WIIP] go: compile generated protoc output~~ generate and compile Go from Protobuf Mar 11, 2022

tdyas requested a review from Eric-Arellano March 11, 2022 17:30

tdyas marked this pull request as ready for review March 11, 2022 17:30

tdyas requested a review from stuhood March 11, 2022 17:31

stuhood approved these changes Mar 14, 2022

View reviewed changes

tdyas merged commit d2881ba into pantsbuild:main Mar 14, 2022

tdyas deleted the golang_finish_protobuf branch March 14, 2022 22:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

generate and compile Go from Protobuf #14714

generate and compile Go from Protobuf #14714

tdyas commented Mar 7, 2022 •

edited

Loading

tdyas commented Mar 7, 2022 •

edited

Loading

tdyas commented Mar 7, 2022

Eric-Arellano commented Mar 8, 2022

tdyas commented Mar 9, 2022

Eric-Arellano commented Mar 9, 2022

tdyas commented Mar 11, 2022

tdyas commented Mar 11, 2022

tdyas commented Mar 14, 2022

stuhood left a comment

stuhood Mar 14, 2022 •

edited

Loading

stuhood Mar 14, 2022

Eric-Arellano Mar 14, 2022

tdyas Mar 14, 2022

generate and compile Go from Protobuf #14714

generate and compile Go from Protobuf #14714

Conversation

tdyas commented Mar 7, 2022 • edited Loading

tdyas commented Mar 7, 2022 • edited Loading

tdyas commented Mar 7, 2022

Eric-Arellano commented Mar 8, 2022

tdyas commented Mar 9, 2022

Eric-Arellano commented Mar 9, 2022

tdyas commented Mar 11, 2022

tdyas commented Mar 11, 2022

tdyas commented Mar 14, 2022

stuhood left a comment

Choose a reason for hiding this comment

stuhood Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

stuhood Mar 14, 2022

Choose a reason for hiding this comment

Eric-Arellano Mar 14, 2022

Choose a reason for hiding this comment

tdyas Mar 14, 2022

Choose a reason for hiding this comment

tdyas commented Mar 7, 2022 •

edited

Loading

tdyas commented Mar 7, 2022 •

edited

Loading

stuhood Mar 14, 2022 •

edited

Loading