cmd/protoc-gen-go-grpc: add code generator #3453

neild · 2020-03-12T23:44:26Z

Add a code generator for gRPC services.

No tests included here. A followup PR updates code generation to use
this generator, which acts as a test of the generator as well.

johanbrandhorst · 2020-03-13T19:23:31Z

cmd/protoc-gen-go-grpc/internal_gengogrpc/grpc.go

+
+	g.P("// ", clientName, " is the client API for ", service.GoName, " service.")
+	g.P("//")
+	g.P("// For semantics around ctx use and closing/ending streaming RPCs, please refer to https://godoc.org/google.golang.org/grpc#ClientConn.NewStream.")


Should we make this link to go.dev?

johanbrandhorst · 2020-03-13T20:56:59Z

cmd/protoc-gen-go-grpc/main.go

+// protoc-gen-go is a plugin for the Google protocol buffer compiler to generate
+// Go code. Install it by building this program and making it accessible within
+// your PATH with the name:
+//	protoc-gen-go
+//
+// The 'go' suffix becomes part of the argument for the protocol compiler,
+// such that it can be invoked as:
+//	protoc --go_out=paths=source_relative:. path/to/file.proto
+//
+// This generates Go bindings for the protocol buffer defined by file.proto.
+// With that input, the output will be written to:
+//	path/to/file.pb.go
+//
+// See the README and documentation for protocol buffers to learn more:
+//	https://developers.google.com/protocol-buffers/
+package main


This looks wrong

Oops, forgot to update the doc comment when copying this file. Fixed.

jhump

Is this something that can be merged soon?

I decided to start a v2 of my protoreflect library, to update it to use the latest APIv2 for the protobuf runtime. But I quickly wound up stuck because the new protoc-gen-go can't generate gRPC stuff. I am keen to start kicking the tires on these things.

I guess in the meantime, I'll just check out this pull request locally and try out the protoc-gen-go-grpc command that way.

jhump · 2020-03-17T00:26:35Z

cmd/protoc-gen-go-grpc/go.mod

@@ -0,0 +1,8 @@
+module google.golang.org/grpc/cmd/protoc-gen-go-grpc
+
+go 1.14


I know Go 1.11 is not technically supported by the Go team anymore. But if AppEngine is still a supported target, it still has a Go 1.11 runtime. And Go 1.11 does not gracefully handle these go version declarations.

1.14 was inadvertent. Changed to 1.9, which is the oldest Go version supported by the protobuf module(s).

It's unfortunate that 1.9 is different than the version supported by the top-level gRPC module, but I think it's better (and possibly necessary) for this to remain in sync with the protobuf module which will import it.

meling · 2020-03-17T22:51:26Z

Seems vet is failing this.

cmd/protoc-gen-go-grpc/internal_gengogrpc/grpc.go:23:1: don't use an underscore in package name

Begs the question, why use underscore in the package name, going against stated package naming guidelines? I was initially wondering if this worked similar to the internal package, but it doesn't, so the package .../internal_gengogrpc is publicly accessible. It is unclear what the intention is.

This is also annoying when opening the project in VSCode, which in the default setup has vet running and calls these out as warnings...

neild · 2020-03-17T23:26:39Z

Begs the question, why use underscore in the package name, going against stated package naming guidelines? I was initially wondering if this worked similar to the internal package, but it doesn't, so the package .../internal_gengogrpc is publicly accessible. It is unclear what the intention is.

The intent was to have a clear visual indication to users that this isn't a package intended for general use. Running "internalgengogrpc" together makes the "internal" name fade into the background, for me at least.

Changed to just "gengogrpc", since this package is less internal than it was when it lived in the google.golang.org/protobuf repo; it's now a point of contact between two separate projects, rather than two revisions of the same project.

pjediny · 2020-03-18T21:03:16Z

@neild if you did not want to have it public, why didn't you just put it in subfolder like internal/gengogrpc?

neild · 2020-03-18T21:59:02Z

@neild if you did not want to have it public, why didn't you just put it in subfolder like internal/gengogrpc?

We want the github.com/golang/protobuf/protoc-gen-go binary to continue to support gRPC code generation, to avoid breaking users who depend on it doing so. (In contrast, the brand-new google.golang.org/protobuf/cmp/protoc-gen-go binary does not support gRPC and is intended to be used in conjunction with protoc-gen-go-grpc.)

To allow github.com/golang/protobuf/protoc-gen-go to generate gRPC service definitions, it needs to be able to import the gRPC code generator. Therefore, the gengogrpc package can't be internal to the gRPC module.

achew22 · 2020-03-21T04:10:12Z

cmd/protoc-gen-go-grpc/gengogrpc/grpc.go

+		g.P(deprecationComment)
+	}
+	serviceDescVar := "_" + service.GoName + "_serviceDesc"
+	g.P("func Register", service.GoName, "Server(s *", grpcPackage.Ident("Server"), ", srv ", serverType, ") {")


Would it be possible to have Register${service.GoName}Server take an interface with RegisterService on it instead of the concrete type? This would be useful in testing to allow registration in other objects.

I attempted this PR in the other protobuf repo, but it seems the princess might be in this castle instead golang/protobuf#1060

This PR shouldn't introduce any changes from the current implementation; it moves where the generator is located, but not any of the semantics. Any changes to semantics should happen as a followup PR once this is merged.

That's very reasonable. Do you know what the timeline on that is? I'm not looking for a date, O(days), O(weeks), or O(months)? I'm trying to scope of I can postpone my project, or if I should temporarily apply patches to the compiler in my repo.

Also, I'm so excited for the V2 API. It looks like you've done a wonderful job. Thanks for all your hard work!

baolitoumu · 2020-04-23T08:44:25Z

Is anyone here. Sorry, because many people are waiting, it is very painful

dfawley · 2020-04-23T16:06:03Z

This is on my plate. Sorry for the delay - I'm behind on many things right now, not just this. I hope to get to it soon.

tmc · 2020-04-27T20:37:37Z

Hopefully this can get in this week!

odsod · 2020-04-29T04:09:01Z

Is this really a blocker? We bumped our github.com/golang/protobuf/protoc-gen-go plugin to v1.4.0 and are generating v2 proto and gRPC stubs.

It should not be necessary to block on this PR to start using the v2 APIs in gRPC setups. Just bump github.com/golang/protobuf/protoc-gen-go to 1.4.0 and run with plugins=grpc until the new protoc plugin lands on the master branch over here.

dfawley

Sorry for the excessive delay in reviewing.

dfawley · 2020-04-30T17:24:18Z

cmd/protoc-gen-go-grpc/gengogrpc/grpc.go

+	// TODO: Remove this. We don't need to include these references any more.
+	g.P("// Reference imports to suppress errors if they are not otherwise used.")
+	g.P("var _ ", contextPackage.Ident("Context"))
+	g.P("var _ ", grpcPackage.Ident("ClientConnInterface"))
+	g.P()


Why is this a TODO, then?

dfawley · 2020-04-30T17:28:23Z

cmd/protoc-gen-go-grpc/gengogrpc/grpc.go

+	g.P("// Unimplemented", serverType, " can be embedded to have forward compatible implementations.")
+	g.P("type Unimplemented", serverType, " struct {")


Is this an acceptable time to change behavior and require this to be embedded for all services? Or is it required that no changes are needed in your code when switching from v1 to v2 proto?

Up to you and whatever your compatibility guarantees are. This would, of course, be a breaking change which would require that all users change their code before using the new generator.

If we were to leave the generator in github.com/golang/protobuf unchanged and independent of the one here, then this might be a reasonable time to make a breaking change--since this is a brand new generator, it has no existing users. On the other hand, doing so will serve as an additional barrier for users migrating from the old one.

Either way, my preference would be to submit this CL with the existing API to provide a baseline identical to the current version and for you to make whatever changes you want in a followup change.

This would be a pretty good opportunity to do this, but it's probably not reasonable to require code changes in order to run the generator from a new location. But if we check this code in without that requirement, then I don't think we'll ever be able to do it.

dfawley · 2020-04-30T17:37:00Z

cmd/protoc-gen-go-grpc/gengogrpc/grpc.go

+// This package is exported to permit github.com/golang/protobuf/protoc-gen-go
+// to depend on it. No other packages should depend on it.


So the plan is to delete the code there and reference this code instead?

This has pros and cons:

Pro: v1 and v2 codegen do not need to be maintained separately

Con: changes here will impact the v1 codegen, meaning we need to test the v1 codegen (in the other repo) if we want to make changes here.

Might it be better to just have separate implementations and allow them to drift? (Also: clearly marking the v1 codegen as deprecated and no longer receiving new features.)

My plan had been to delete the generator from github.com/golang/protobuf and reference the one here. If you'd rather we deprecate that one and let it drift, I'm fine with that approach. What's your preference?

Not sure that the con is as significant, since github.com/golang/protobuf can specify that they depend on a specific version of the generator herein. Since v1 is not meant to be receiving new features, they should not advance to newer versions of the grpc gen, except for important bug fixes that advance only the minor version number. This should reduce the need for cross-repo testing to those bug fixes that are deemed necessary to backport to v1. That said, I think both strategies are fine.

I'd rather let them drift and mark the old one as deprecated. Then we don't need to export code from here that is internal-only, which I see as a win. Even though anyone else would be crazy to use it, if we were to change it, it would be a breaking change for the code in the proto repo referencing it.

dfawley · 2020-05-27T21:16:31Z

Even with this merged, it is unreleased (v0.0) and subject to change. We do plan to add a requirement that the Unimplemented service implementation be included in all registered services, with an escape hatch in the form of a command-line arg as suggested by @neild.

johanbrandhorst · 2020-05-28T11:31:00Z

So happy to finally see this merged, great work!

chalin · 2020-06-23T17:01:57Z

Even with this merged, it is unreleased (v0.0) and subject to change. We do plan to add ...

@dfawley - is there an issue I can use to track the release?

dfawley · 2020-06-23T17:09:30Z

Let's use #3669 to track this work for now.

jeffwidman · 2020-06-23T17:23:29Z

@dfawley is that the correct issue number? That issue looks unrelated, unless I misunderstand something?

ydnar · 2020-06-23T19:42:37Z

Somewhat related: would you consider moving the code-generation parts of this out of package main into a sub-package that can be imported by another program?

dfawley · 2020-06-26T00:56:23Z

@jeffwidman yes, that's the right one :) That's the only thing blocking a 1.0 release of the tool.

Somewhat related: would you consider moving the code-generation parts of this out of package main into a sub-package that can be imported by another program?

Probably not, but it would be interesting to hear your use case(s). I'd rather not have to maintain a separate library for this, guaranteeing backward compatibility forever, unless it's important to do so.

ydnar · 2020-06-26T03:22:38Z

Somewhat related: would you consider moving the code-generation parts of this out of package main into a sub-package that can be imported by another program?

Probably not, but it would be interesting to hear your use case(s). I'd rather not have to maintain a separate library for this, guaranteeing backward compatibility forever, unless it's important to do so.

As the protogen package allows for protoc plugins to adapt the Protobuf AST prior to Go code generation, I think splitting out the generateFile function into its own package would allow adaptation of the AST prior to serialization without forking the entire repo or command.

e.g. something like a cmd/protoc-gen-go-grpc/grpcgen package, with a single entry point (GenerateFile) with the same method signature as the existing generateFile.

A caller would have two opportunities to adapt the output:

Modify the input in the protogen.Plugin
Modify the output in the response.

dfawley · 2020-06-26T17:32:06Z

Interesting. This is a very high level function. Since it's so high level, it should be possible to accomplish these things as a separate step before/after running the generator as a binary. But on the other hand, it would be possible to support this kind of API with minimal effort. Would you mind filing an issue for this?

neild · 2020-06-26T18:27:46Z

As the protogen package allows for protoc plugins to adapt the Protobuf AST prior to Go code generation

I'm not certain what this means.

The protogen package is a toolkit for writing protoc plugins that generate Go code. It provides a view of the protobuf descriptors with annotations about the canonical Go names for various objects (messages, fields, etc.).

We do not support modifying the code generated by protoc-gen-go, however. The protogen package does not provide access to the generated code, and it does not support modifying the descriptors handled by protoc-gen-go.

ydnar · 2020-06-26T20:34:42Z

As the protogen package allows for protoc plugins to adapt the Protobuf AST prior to Go code generation

I'm not certain what this means.

The protogen package is a toolkit for writing protoc plugins that generate Go code. It provides a view of the protobuf descriptors with annotations about the canonical Go names for various objects (messages, fields, etc.).

We do not support modifying the code generated by protoc-gen-go, however. The protogen package does not provide access to the generated code, and it does not support modifying the descriptors handled by protoc-gen-go.

You can modify the generated Go identifiers in protogen prior to code generation.

dsnet · 2020-06-26T20:42:08Z

As mentioned in another comment, mutation of data structures returned by protogen is not a supported use case and subject to breakage in the future as it will likely violate consistency invariants.

ydnar · 2020-06-26T20:51:25Z

As mentioned in another comment, mutation of data structures returned by protogen is not a supported use case and subject to breakage in the future as it will likely violate consistency invariants.

Then why does the protogen package, and its exported identifiers exist at all?

dsnet · 2020-06-26T21:17:29Z

Because other things need read-only access to them are there is no language feature for immutability.

cee-dub · 2020-06-26T21:27:04Z

Perhaps your team should add some documentation to protogen to this effect since your stance seems to be "nobody should touch this output, if you want different Go code to represent your protos you must fork the generator."

https://github.com/protocolbuffers/protobuf-go/blob/a9513ebdb86068803ccda83ded57e8330a72961e/compiler/protogen/protogen.go#L5-L11

neild · 2020-06-26T21:35:52Z

The protogen package doesn't produce generated code. We don't provide any public packages which produce generated protocol buffer code. It's used by the tools which produce generated code (protoc-gen-go, protoc-gen-go-grpc).

ydnar · 2020-06-26T21:55:28Z

The protogen package doesn't produce generated code. We don't provide any public packages which produce generated protocol buffer code. It's used by the tools which produce generated code (protoc-gen-go, protoc-gen-go-grpc).

We’re well aware. We use protogen to fix the non-idiomatic Go code that protoc-gen-go* generates.

https://github.com/alta/protopatch

Commit `f18fcd9b` updated the following dependencies: - github.com/golang/protobuf: v1.3.2 => v1.4.2 - google.golang.org/genproto - google.golang.org/grpc: v1.26.0 => v1.27.0 However, the protobuf generated files were not re-generated which is what this commit addresses. The newly generated files should be backward compatible. However, the following warning is now emitted when generating them: WARNING: Package "github.com/golang/protobuf/protoc-gen-go/generator" is deprecated. A future release of golang/protobuf will delete this package, which has long been excluded from the compatibility promise. This is because this package was apparently never meant to be public as explained in this comment[0]. The fix for this is to migrate to `google.golang.org/protobuf/compiler/protogen` but this can't be done until a release of `grpc-go` which includes the following patch[1] is out. Update the Makefile to output a note with regard to this when generating the files It appears that with the newly generated files, protobuf now needs to be marked as an explicit dep. Fix it by by running: go mod tidy && go mod vendor && go mod verify Fix: f18fcd9 [0]: golang/protobuf#1104 (comment) [1]: grpc/grpc-go#3453 Signed-off-by: Robin Hahling <[email protected]>

neild force-pushed the gengrpc branch from 698f127 to 8c30214 Compare March 12, 2020 23:45

johanbrandhorst reviewed Mar 13, 2020

View reviewed changes

neild force-pushed the gengrpc branch from 8c30214 to e91eedd Compare March 13, 2020 20:02

johanbrandhorst reviewed Mar 13, 2020

View reviewed changes

neild force-pushed the gengrpc branch from e91eedd to 4ad6146 Compare March 13, 2020 21:45

jhump reviewed Mar 17, 2020

View reviewed changes

menghanl requested a review from dfawley March 17, 2020 01:26

menghanl assigned dfawley Mar 17, 2020

neild force-pushed the gengrpc branch from 4ad6146 to 8930d03 Compare March 17, 2020 18:34

neild force-pushed the gengrpc branch from 8930d03 to b85dca1 Compare March 17, 2020 23:21

achew22 reviewed Mar 21, 2020

View reviewed changes

dragonsinth mentioned this pull request Mar 30, 2020

jsonpb: Marshaler.marshalValue() incorrectly marshals nil slice elements golang/protobuf#1066

Closed

neild mentioned this pull request Apr 1, 2020

plugins are not supported : grpc golang/protobuf#1070

Closed

HakShak mentioned this pull request Apr 15, 2020

protoc-gen-go-grpc: program not found or is not executable when using APIv2 golang/protobuf#1053

Closed

marcellanz mentioned this pull request Apr 16, 2020

Fixing two compilation errors in the example snippet and a doc update. cloudstateio/cloudstate#246

Merged

johanbrandhorst mentioned this pull request Apr 20, 2020

v2 release planning grpc-ecosystem/grpc-gateway#1223

Closed

13 tasks

marcellanz mentioned this pull request Apr 20, 2020

Migrate to Go protobuf API v2 cloudstateio/go-support#20

Closed

2 tasks

dfawley mentioned this pull request Apr 23, 2020

Simplify the go quickstart grpc/grpc.io#215

Merged

dfawley reviewed Apr 30, 2020

View reviewed changes

dfawley assigned neild and unassigned dfawley Apr 30, 2020

neild force-pushed the gengrpc branch from b85dca1 to 8f9daae Compare April 30, 2020 20:12

dfawley merged commit 6c9e30c into grpc:master May 27, 2020

dsnet mentioned this pull request May 27, 2020

google.golang.org/grpc: move protoc-gen-go-grpc into grpc module golang/protobuf#903

Closed

chalin mentioned this pull request Jun 23, 2020

Update Go quick-start once cmd/protoc-gen-go-grpc is at v1.0 grpc/grpc.io#298

Closed

1 task

ydnar mentioned this pull request Jun 26, 2020

Feature request: put generateFile from protoc-gen-go-grpc into its own package #3717

Closed

darabos mentioned this pull request Jul 3, 2020

Use protoc-gen-go-grpc lynxkite/lynxkite#45

Merged

johanbrandhorst mentioned this pull request Jul 24, 2020

Upgraded proto related deps: grpc and protobuf; removed gogo from core. grpc-ecosystem/go-grpc-middleware#321

Merged

rolinh mentioned this pull request Jul 27, 2020

api: re-generate protobuf code cilium/cilium#12677

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021

		@@ -0,0 +1,8 @@
		module google.golang.org/grpc/cmd/protoc-gen-go-grpc

		go 1.14

		g.P("// Unimplemented", serverType, " can be embedded to have forward compatible implementations.")
		g.P("type Unimplemented", serverType, " struct {")

		// This package is exported to permit github.com/golang/protobuf/protoc-gen-go
		// to depend on it. No other packages should depend on it.

cmd/protoc-gen-go-grpc: add code generator #3453

cmd/protoc-gen-go-grpc: add code generator #3453

Conversation

neild commented Mar 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhump left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meling commented Mar 17, 2020

neild commented Mar 17, 2020

pjediny commented Mar 18, 2020 • edited Loading

neild commented Mar 18, 2020

achew22 Mar 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baolitoumu commented Apr 23, 2020

dfawley commented Apr 23, 2020

tmc commented Apr 27, 2020

odsod commented Apr 29, 2020

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley commented May 27, 2020

johanbrandhorst commented May 28, 2020

chalin commented Jun 23, 2020

dfawley commented Jun 23, 2020

jeffwidman commented Jun 23, 2020

ydnar commented Jun 23, 2020

dfawley commented Jun 26, 2020

ydnar commented Jun 26, 2020

dfawley commented Jun 26, 2020

neild commented Jun 26, 2020

ydnar commented Jun 26, 2020

dsnet commented Jun 26, 2020

ydnar commented Jun 26, 2020

dsnet commented Jun 26, 2020

cee-dub commented Jun 26, 2020 • edited Loading

neild commented Jun 26, 2020

ydnar commented Jun 26, 2020

pjediny commented Mar 18, 2020 •

edited

Loading

achew22 Mar 21, 2020 •

edited

Loading

cee-dub commented Jun 26, 2020 •

edited

Loading