Use google.golang.org/grpc/cmd/protoc-gen-go-grpc #98

ash2k · 2021-01-20T03:28:16Z

Closes #85.

Use google.golang.org/grpc/cmd/protoc-gen-go-grpc to generate gRPC code for Go.

Please let me know if I missed something.

WORKSPACE

ash2k · 2021-01-20T03:30:04Z

go/example/routeguide/server/main.go

@@ -55,6 +55,7 @@ var (
 )

 type routeGuideServer struct {
+	pb.UnimplementedRouteGuideServer


I think this is required now.

ash2k · 2021-01-20T03:31:14Z

go/go_grpc_library.bzl

    "@org_golang_google_grpc//:go_default_library",
    "@org_golang_google_grpc//codes:go_default_library",
    "@org_golang_google_grpc//status:go_default_library",
-    "@org_golang_x_net//context:go_default_library",


This is no longer needed as gRPC now uses the standard library context package, which is the successor to this one.

ash2k · 2021-01-20T03:31:30Z

go/go_grpc_library.bzl

        importpath = kwargs.get("importpath"),
        visibility = kwargs.get("visibility"),
        tags = kwargs.get("tags"),
    )

 GRPC_DEPS = [
-    "@com_github_golang_protobuf//proto:go_default_library",
-    "@org_golang_google_protobuf//reflect/protoreflect:go_default_library",
-    "@org_golang_google_protobuf//runtime/protoimpl:go_default_library",


Replaced with PROTO_DEPS. Is that ok?

IIRC GRPC_DEPS is intended to be the full set of deps for the *_grpc_ rules, but I'd have to check the consistency with other langs. If that's the case, having GRPC_DEPS = [...] + PROTO_DEPS would be suitable though.

go/repositories.bzl

ash2k · 2021-01-20T03:33:08Z

tools/rulegen/go.go

@@ -20,14 +22,13 @@ load("@rules_proto_grpc//{{ .Lang.Dir }}:repositories.bzl", rules_proto_grpc_{{

 rules_proto_grpc_{{ .Lang.Name }}_repos()`)

-
-var goLibraryRuleTemplateString = `load("//{{ .Lang.Dir }}:{{ .Rule.Base}}_{{ .Rule.Kind }}_compile.bzl", "{{ .Rule.Base }}_{{ .Rule.Kind }}_compile")
+var goLibraryRuleTemplateString = `load("//{{ .Lang.Dir }}:{{ .Rule.Base}}_proto_compile.bzl", "{{ .Rule.Base }}_proto_compile")


Both rules now use go_proto_compile rule so they can share this prefix. gRPC rule needs some more loads, see below.

ash2k · 2021-01-20T03:33:59Z

tools/rulegen/grpc_gateway.go

 		DisplayName: "grpc-gateway",
-		Flags: commonLangFlags,
+		Flags:       commonLangFlags,


goimports formatting things, I guess...

ash2k · 2021-01-20T07:04:17Z

Hm, so I'm trying to use this branch to check that it works for my project and it does not. Here is the branch where I'm trying to switch https://gitlab.com/gitlab-org/cluster-integration/gitlab-agent/-/merge_requests/256

I'm getting

make regenerate-proto                                                
bazel run //build:extract_generated_proto
INFO: Analyzed target //build:extract_generated_proto (102 packages loaded, 779 targets configured).
INFO: Found 1 target...
ERROR: /Users/mikhail/src/gitlab-agent/internal/module/modshared/BUILD.bazel:4:18: output 'internal/module/modshared/proto/go_grpc_compile_aspect_verb0/internal/module/modshared/modshared_grpc.pb.go' was not created
ERROR: /Users/mikhail/src/gitlab-agent/internal/module/modshared/BUILD.bazel:4:18: not all outputs were created or valid

Some of my proto files with gRPC services (e.g. internal/module/agent_configuration/rpc/rpc.proto) depend on other proto files without gRPC serviers (e.g. internal/module/modshared/modshared above). But the gRPC aspect is applied to both and... the gRPC plugin does not generate anything for the file without services. I'm not sure how to resolve this. Also, I may be wrong and the problem is caused by something else.

p.s. I also have cases where non-grpc proto depends on non-grpc proto: internal/module/agent_tracker/agent_tracker.proto also depends on modshared.proto. This does work.

ash2k · 2021-01-20T07:29:25Z

I've made some simplifications in the third commit and now both plugins should be invoked in a single protoc call but the problem is still the same. 😕

bazel build //internal/module/agent_tracker/rpc:grpc_compile
INFO: Analyzed target //internal/module/agent_tracker/rpc:grpc_compile (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
ERROR: /Users/mikhail/src/gitlab-agent/internal/module/agent_tracker/BUILD.bazel:4:18: output 'internal/module/agent_tracker/proto/go_grpc_compile_aspect_verb0/internal/module/agent_tracker/agent_tracker_grpc.pb.go' was not created
ERROR: /Users/mikhail/src/gitlab-agent/internal/module/agent_tracker/BUILD.bazel:4:18: not all outputs were created or valid

…r Go

aaliddell · 2021-02-07T23:18:03Z

example/github.com/grpc-ecosystem/grpc-gateway/gateway_openapiv2_compile/BUILD.bazel


-gateway_swagger_compile(
+gateway_openapiv2_compile(


This is going to be a backwards incompatible change. I was likely going to have to do 3.0.0 for updated deps anyway, so should be fine

aaliddell · 2021-02-07T23:41:02Z

Hm, so I'm trying to use this branch to check that it works for my project and it does not. Here is the branch where I'm trying to switch https://gitlab.com/gitlab-org/cluster-integration/gitlab-agent/-/merge_requests/256

I'm getting
make regenerate-proto                                                
bazel run //build:extract_generated_proto
INFO: Analyzed target //build:extract_generated_proto (102 packages loaded, 779 targets configured).
INFO: Found 1 target...
ERROR: /Users/mikhail/src/gitlab-agent/internal/module/modshared/BUILD.bazel:4:18: output 'internal/module/modshared/proto/go_grpc_compile_aspect_verb0/internal/module/modshared/modshared_grpc.pb.go' was not created
ERROR: /Users/mikhail/src/gitlab-agent/internal/module/modshared/BUILD.bazel:4:18: not all outputs were created or valid
Some of my proto files with gRPC services (e.g. internal/module/agent_configuration/rpc/rpc.proto) depend on other proto files without gRPC serviers (e.g. internal/module/modshared/modshared above). But the gRPC aspect is applied to both and... the gRPC plugin does not generate anything for the file without services. I'm not sure how to resolve this. Also, I may be wrong and the problem is caused by something else.

p.s. I also have cases where non-grpc proto depends on non-grpc proto: internal/module/agent_tracker/agent_tracker.proto also depends on modshared.proto. This does work.

This is a frustrating recurring problem with some languages in this repo. In short, some protoc plugins behave deterministically w.r.t the the input files, such that they create an output file for every input file regardless of content. Unfortunately some plugins only output files that contain services, which makes their output non-predictable and therefore they don't play too well with Bazel, which needs to know output files prior to running a tool. I've had to 'fight' this a number of times to make plugins more deterministic.

For some languages, we can get around this by declaring the entire output directory and passing that to the underlying library rule. However, this requires that rules_<lang> is able to handle receiving a directory containing files, rather than the files themselves. I can't remember where rules_go falls with regard to supporting that.

What I presume we are seeing here is that the old go plugin had the deterministic behaviour, but the new plugin has the non-deterministic behaviour.

ash2k · 2021-02-09T00:38:43Z

@aaliddell Thanks!

What I presume we are seeing here is that the old go plugin had the deterministic behaviour, but the new plugin has the non-deterministic behaviour.

I think the old plugin generates proto+grpc into a single file (at least that is my understanding) but the new one only generates gRPC code (and the proto generation is left to the old plugin, which generates proto API v2 code already).

However, I'm not sure that's the issue. I think the issue is that we had a single plugin which could handle both proto and gRPC generation, but now we have two. And because of that we need to run two plugins for the same file. However, when the file does not have gRPC services, the new plugin does not produce anything.

It seems to me that the solution to this would be to run the new gRPC generation plugin only on proto files which are known to have gRPC services. Currently it's run on the whole tree of dependencies, right? I.e. in my case services.proto depends on/imports no-services.proto and the error is because new plugin is ran on the no-services.proto, when it shouldn't.

To rephrase the above - my understanding (which might be wrong!) is that currently a set of plugins is invoked on all nodes of the dependency graph an aspect finds. This does not work because in this case not all plugins should be invoked on each node.

What if we have two aspects - one for go_proto_compile and another one for go_grpc_compile:

go_proto_compile would call the old plugin on the proto file and on all the dependencies
go_grpc_compile would call the old plugin on the proto file and on all the dependencies AND the new plugin too, but only on the the sources, passed to go_grpc_compile.

I have no idea if this is possible or makes sense, just an idea.

aaliddell · 2021-02-09T20:01:08Z

Ok, gotcha.

However, when the file does not have gRPC services, the new plugin does not produce anything.

Yep, this is the issue of determinism I mentioned.

It seems to me that the solution to this would be to run the new gRPC generation plugin only on proto files which are known to have gRPC services. Currently it's run on the whole tree of dependencies, right?

Yep, the trouble is that we cannot determine where in the tree of deps a service might turn up. We certainly could be opinionated and make certain plugins non-traversing, such that we only the direct srcs get the gRPC plugins applied. I would like to explore that as an option if rules_go does not support directory inputs, since this'd be a reasonable solution for other languages in the same predicament.

I will retarget this PR onto the dev branch, so that I can merge your changes with the dependency updates I've done. Then we can experiment there with solving the gRPC issue 👍

aaliddell · 2021-02-09T20:46:45Z

tools/rulegen/grpc_gateway.go

+				Name:              "gateway_openapiv2_compile",
+				Kind:              "grpc",
+				Implementation:    aspectRuleTemplate,
+				Plugins:           []string{"//github.com/grpc-ecosystem/grpc-gateway:openapiv2_plugin", "//go:go_plugin"},


Why was go_plugin added for the JSON generation?

I only vaguely remember - I think it was because the new openapiv2_plugin didn't generate the Go sources when used only by itself. It may be a wrong way to fix it, I'm not sure.

ash2k · 2021-02-09T23:01:49Z

Yep, the trouble is that we cannot determine where in the tree of deps a service might turn up.

It might be good to add this as a testcase - currently the build is green for this PR even though it doesn't actually work for such a common (?) case.

aaliddell · 2021-02-09T23:12:07Z

Yep, the trouble is that we cannot determine where in the tree of deps a service might turn up.

It might be good to add this as a testcase - currently the build is green for this PR even though it doesn't actually work for such a common (?) case.

This should have been caught by the routeguide tests, so I'm somewhat puzzled that passed 🤔
I just so happened to push a commit today (fd1ec1d) that adds another variant of testing this and has flagged up at least Objective-C as having a problem.

On further diving into the aspects, I'm remembering why we weren't able to use the opinionated approach of only using the gRPC plugin on direct deps: basically, the aspect has no concept of the 'depth' of a dependency tree, since a target could simultaneously be a direct dep and a transitive dep and the aspect will only run once for both cases. To resolve this, we'd have to add a different attribute to the grpc rules to disambiguate this, which would be a very breaking change.

aaliddell · 2021-02-09T23:18:51Z

On second thought, the routeguide tests won't have caught this, as routeguide.proto is based on the gRPC repo's example and does not have an external dependency on any well-known protos etc. So the commit above is the first time that service tree failures would have been explicitly tested.

ash2k commented Jan 20, 2021

View reviewed changes

ash2k force-pushed the ash2k/grpc-go branch 9 times, most recently from acd57e2 to 5b4d952 Compare January 20, 2021 06:10

ash2k added 3 commits January 20, 2021 23:00

Use google.golang.org/grpc/cmd/protoc-gen-go-grpc to generate gRPC fo…

c27171b

…r Go

Fix grpc gateway code generation

99fe92e

Simplify plugin invocation

fd2e9a2

ash2k force-pushed the ash2k/grpc-go branch from 228a16d to fd2e9a2 Compare January 20, 2021 12:02

aaliddell reviewed Feb 7, 2021

View reviewed changes

ash2k mentioned this pull request Feb 9, 2021

proto: add go_proto_apiv2 and go_grpc_apiv2 compilers bazel-contrib/rules_go#2522

Open

This was referenced Feb 9, 2021

OpenAPI v2 annotations #93

Closed

3.0.0 release #106

Merged

aaliddell reviewed Feb 9, 2021

View reviewed changes

aaliddell merged commit 9cfd8ad into rules-proto-grpc:master Feb 21, 2021

ash2k deleted the ash2k/grpc-go branch February 21, 2021 23:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use google.golang.org/grpc/cmd/protoc-gen-go-grpc #98

Use google.golang.org/grpc/cmd/protoc-gen-go-grpc #98

ash2k commented Jan 20, 2021 •

edited

Loading

ash2k Jan 20, 2021

ash2k Jan 20, 2021 •

edited

Loading

ash2k Jan 20, 2021

aaliddell Feb 7, 2021

ash2k Jan 20, 2021 •

edited

Loading

ash2k Jan 20, 2021

ash2k commented Jan 20, 2021 •

edited

Loading

ash2k commented Jan 20, 2021 •

edited

Loading

aaliddell Feb 7, 2021

aaliddell commented Feb 7, 2021

ash2k commented Feb 9, 2021 •

edited

Loading

aaliddell commented Feb 9, 2021

aaliddell Feb 9, 2021

ash2k Feb 9, 2021

ash2k commented Feb 9, 2021

aaliddell commented Feb 9, 2021

aaliddell commented Feb 9, 2021


		gateway_swagger_compile(
		gateway_openapiv2_compile(

Use google.golang.org/grpc/cmd/protoc-gen-go-grpc #98

Use google.golang.org/grpc/cmd/protoc-gen-go-grpc #98

Conversation

ash2k commented Jan 20, 2021 • edited Loading

ash2k Jan 20, 2021

Choose a reason for hiding this comment

ash2k Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

ash2k Jan 20, 2021

Choose a reason for hiding this comment

aaliddell Feb 7, 2021

Choose a reason for hiding this comment

ash2k Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

ash2k Jan 20, 2021

Choose a reason for hiding this comment

ash2k commented Jan 20, 2021 • edited Loading

ash2k commented Jan 20, 2021 • edited Loading

aaliddell Feb 7, 2021

Choose a reason for hiding this comment

aaliddell commented Feb 7, 2021

ash2k commented Feb 9, 2021 • edited Loading

aaliddell commented Feb 9, 2021

aaliddell Feb 9, 2021

Choose a reason for hiding this comment

ash2k Feb 9, 2021

Choose a reason for hiding this comment

ash2k commented Feb 9, 2021

aaliddell commented Feb 9, 2021

aaliddell commented Feb 9, 2021

ash2k commented Jan 20, 2021 •

edited

Loading

ash2k Jan 20, 2021 •

edited

Loading

ash2k Jan 20, 2021 •

edited

Loading

ash2k commented Jan 20, 2021 •

edited

Loading

ash2k commented Jan 20, 2021 •

edited

Loading

ash2k commented Feb 9, 2021 •

edited

Loading