gRPC: Changing go_grpc_compiler causes link time failure due protobuf messages not being generated

[zecke]

Build/link regression after upgrading gazelle and re-running //:gazelle. This appears due #2071. With this change we are seeing link time failures when there we define messages and service in two different files of the same go_proto_library.

The https://pkg.go.dev/google.golang.org/grpc/cmd/protoc-gen-go-grpc tool (as used by the grpc_v2 compiler) does not generate the necessary struct definitions. This appears to be the task of https://github.com/protocolbuffers/protobuf-go/tree/master/cmd/protoc-gen-go which is not yet used in rules_go.

I created https://github.com/zecke/gazelle-grpc as a simple reproducer. There are two protobuf files in the root. One defines a Message and the other a gRPC service that uses the message. With the latest gazelle it fails to link due undefined references.

bazel build //:gazelle-grpc                                
INFO: Analyzed target //:gazelle-grpc (1 packages loaded, 5 targets configured).
ERROR: /Users/ich/source/sre/github.com/zecke/gazelle-grpc/BUILD.bazel:26:11: GoCompilePkg gazelle-grpc.a failed: (Exit 1): builder failed: error executing GoCompilePkg command (from target //:gazelle-grpc) bazel-out/darwin_arm64-opt-exec-ST-d57f47055a04/bin/external/rules_go++go_sdk+main___download_0/builder_reset/builder compilepkg -sdk external/rules_go++go_sdk+main___download_0 -goroot ... (remaining 71 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
bazel-out/darwin_arm64-fastbuild/bin/gazelle_grpc_go_proto_/github.com/zecke/gazelle-grpc/service_grpc.pb.go:29:36: undefined: Foo
bazel-out/darwin_arm64-fastbuild/bin/gazelle_grpc_go_proto_/github.com/zecke/gazelle-grpc/service_grpc.pb.go:40:62: undefined: Foo
bazel-out/darwin_arm64-fastbuild/bin/gazelle_grpc_go_proto_/github.com/zecke/gazelle-grpc/service_grpc.pb.go:42:13: undefined: Foo
bazel-out/darwin_arm64-fastbuild/bin/gazelle_grpc_go_proto_/github.com/zecke/gazelle-grpc/service_grpc.pb.go:54:29: undefined: Foo
bazel-out/darwin_arm64-fastbuild/bin/gazelle_grpc_go_proto_/github.com/zecke/gazelle-grpc/service_grpc.pb.go:64:65: undefined: Foo
bazel-out/darwin_arm64-fastbuild/bin/gazelle_grpc_go_proto_/github.com/zecke/gazelle-grpc/service_grpc.pb.go:88:12: undefined: Foo
bazel-out/darwin_arm64-fastbuild/bin/gazelle_grpc_go_proto_/github.com/zecke/gazelle-grpc/service_grpc.pb.go:100:53: undefined: Foo
compilepkg: error running subcommand external/rules_go++go_sdk+main___download_0/pkg/tool/darwin_arm64/compile: exit status 2
Target //:gazelle-grpc failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 0.341s, Critical Path: 0.09s
INFO: 2 processes: 2 action cache hit, 2 internal.
ERROR: Build did NOT complete successfully

ls -la bazel-out/darwin_arm64-fastbuild/bin/gazelle_grpc_go_proto_/github.com/zecke/gazelle-grpc/
total 16
drwxr-xr-x  4 k  wheel   128 Aug 15 17:21 .
drwxr-xr-x  3 k  wheel    96 Aug 15 17:21 ..
-r-xr-xr-x  1 k  wheel    32 Aug 15 17:21 message_grpc.pb.go
-r-xr-xr-x  1 k  wheel  4091 Aug 15 17:21 service_grpc.pb.go

Changing compilers in the go_proto_library rule back to the previous compiler is fixing the build issue. My proposal would be to revert #2071 until rules_go allows to fully move away from "github.com/golang/protobuf/protoc-gen-go

(there is another issue is that the compilers will use @io_bazel_rules_go regardless of having/not having a repo_name set in MODULE.bazel)

[fmeum]

@sluongng

[sluongng]

The correct combination of compilers here should be

    compilers = [
        "@io_bazel_rules_go//proto:go_proto",
        "@io_bazel_rules_go//proto:go_grpc_v2",
    ],

One is for message protofiles and one for "grpc" protofiles (ones with service definition)

So the problem isn't go_grpc_v2 being the default, but with Gazelle's protobuf language plugin failing to detect that the first compiler is needed.

It has been a while, but I think my PR #2085 aimed to solve this problem. I probably won't have spare capacity to pick up the PR until mid-October 2025 though. So, any contributors/maintainers would be welcome to take over the PR 🙏

[fmeum]

bazel-gazelle/config/config.go

Line 126 in 3ca2882

ModuleToApparentName func(string) string

can be used to get the correct repo names.

@zecke Would you be interested in taking up Son's PR and/or using what I linked above to fix the repo name issue?

[zecke]

The correct combination of compilers here should be

    compilers = [
        "@io_bazel_rules_go//proto:go_proto",
        "@io_bazel_rules_go//proto:go_grpc_v2",
    ],

On paper the above mixes github.com/golang/protobuf (deprecated) and google.golang.org/protobuf (the new thing).

In practice the old code is calling the new code (https://github.com/golang/protobuf/blob/v1.5.4/protoc-gen-go/main.go). Which then asks the question, what desired change does #2071 ?

One is for message protofiles and one for "grpc" protofiles (ones with service definition)

So the problem isn't go_grpc_v2 being the default, but with Gazelle's protobuf language plugin failing to detect that the first compiler is needed.

It has been a while, but I think my PR #2085 aimed to solve this problem. I probably won't have spare capacity to pick up the PR until mid-October 2025 though. So, any contributors/maintainers would be welcome to take over the PR 🙏

From a user point of view it looks like we pre-maturely switched from github.com/golang/protobuf/protoc-gen-go that can generate code for gRPC and PB, to a solution that only generates gRPC.

go_grpc_v2 is not documented in rules_go and TODO around the deprecation is deprecated itself (https://github.com/bazel-contrib/rules_go/blame/4d0a821ecd186c9d73b6a0b7735cb90fafa9701b/proto/BUILD.bazel#L33-L34)

I think we agree that a proper solution takes a bit more time and I can help to move things forward. As a consumer it is easier to have automation bazel_dep than having to go to each repo by hand and insert the compiler and a # keep.

My two cents is to rollback and we should introduce a go_proto_v2 in rules_go to solely rely on golang.google.org/protobuf (over that google still updates a deprecated repo).

[zecke]

ModuleToApparentName

Let me give it a try. It looks reasonable simple.

[sluongng]

On paper the above mixes github.com/golang/protobuf (deprecated) and google.golang.org/protobuf (the new thing).

Here are the 2 compilers mentioned

go_proto_compiler(
    name = "go_proto",
    plugin = "@org_golang_google_protobuf//cmd/protoc-gen-go",
    visibility = ["//visibility:public"],
    deps = PROTO_RUNTIME_DEPS + WELL_KNOWN_TYPES_APIV2,
)

go_proto_compiler(
    name = "go_grpc_v2",
    options = ["require_unimplemented_servers=false"],
    plugin = "@org_golang_google_grpc_cmd_protoc_gen_go_grpc//:protoc-gen-go-grpc",
    suffix = "_grpc.pb.go",
    visibility = ["//visibility:public"],
    deps = PROTO_RUNTIME_DEPS + WELL_KNOWN_TYPES_APIV2 + [
        "@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",
    ],
)

So github.com/golang/protobuf should not be used unless I'm missing something obvious?

[sluongng]

go_grpc_v2 is not documented in rules_go and TODO around the deprecation is deprecated itself (https://github.com/bazel-contrib/rules_go/blame/4d0a821ecd186c9d73b6a0b7735cb90fafa9701b/proto/BUILD.bazel#L33-L34)

I think we agree that a proper solution takes a bit more time and I can help to move things forward. As a consumer it is easier to have automation bazel_dep than having to go to each repo by hand and insert the compiler and a # keep.

My two cents is to rollback and we should introduce a go_proto_v2 in rules_go to solely rely on golang.google.org/protobuf (over that google still updates a deprecated repo).

I get the sentiment.

Unlike a product with the proper backing of a company, rules_go and gazelle are open-source projects relying solely on open-source volunteers to drive changes.

That means the chance of finishing a "migration" like this is pretty slim: folks send a one-shot PR to fix their problem, then they move on to do something else.

The "deprecation" was not introduced by me. It was introduced by another contributor to solve the deprecation issue upstream. They solved their problem and moved on (not casting blame on them, it's very normal for all OSS projects). My PR to switch the default was another brick laid on top of this existing foundation. I was making some changes to how compilers are managed, and switching the default would help reduce the code I have to manage on my end.

So I fear that reverting the progress we have made so far will simply set back the whole thing forever. It also creates a precedent where only a fully polished change would get accepted, raising the barrier for future contributions.

I will let the project maintainers make the call to revert the change or not. But I do suspect that finishing my PR would be much faster 😅

[zecke]

So github.com/golang/protobuf should not be used unless I'm missing something obvious?

Should might be too strong. If we would start from scratch today, we wouldn't be using github.com/golang/protobuf. I am not aware of any guarantees by google that they continue updating the old code.

From https://github.com/golang/protobuf:

It has been superseded by the google.golang.org/protobuf module, which contains an updated and simplified API, support for protobuf reflection, and many other improvements. We recommend that new code use the google.golang.org/protobuf module.

From https://pkg.go.dev/github.com/golang/protobuf:

Deprecated: Use the "google.golang.org/protobuf" module instead.

[zecke]

So I fear that reverting the progress we have made so far will simply set back the whole thing forever. It also creates a precedent where only a fully polished change would get accepted, raising the barrier for future contributions.

Genuine question: What is the goal here? Given that grpc_v2 is not documented in rules_go and that the old stuff is a wrapper around the new bits. Is there a bigger goal in separating the pb and grpc generation?

[zecke]

I had another look and need to correct myself. go_proto and go_grpc_v2 already use the new thing. Only go_grpc is using the deprecated (which wraps the new thing) golang proto compilers.

I think the easiest (and correct) way is to do what go_grpc_library has been doing all along and just generate both the .pb.go and the _grpc.pb.go.

[aran]

I'm not sure if this is the same issue, but the latest gazelle is forcefully making a couple unhelpful changes if I don't have # keep to protect my rule:

• removing other compilers (we use protoc-gen-go-json in a few places)
• switching the go repo to @io_bazel_rules_go (ours is called @rules_go)

[itmecho]

I ran into this whilst trying to upgrade our work monorepo to go 1.25. It might be a horrendous thing to do but I've applied this patch and it's got everything working for the moment

diff --git a/language/go/generate.go b/language/go/generate.go
index cecf0b1..759a974 100644
--- a/language/go/generate.go
+++ b/language/go/generate.go
@@ -602,10 +602,12 @@ func (g *generator) generateProto(mode proto.Mode, targets []protoTarget, import
 			break
 		}
 	}
+	compilers := gc.goProtoCompilers
 	if atLeastOneTargetHasServices {
-		goProtoLibrary.SetAttr("compilers", gc.goGrpcCompilers)
-	} else if gc.goProtoCompilersSet {
-		goProtoLibrary.SetAttr("compilers", gc.goProtoCompilers)
+		compilers = append(compilers, gc.goGrpcCompilers...)
+	}
+	if len(compilers) > 0 {
+		goProtoLibrary.SetAttr("compilers", compilers)
 	}
 	if g.shouldSetVisibility {
 		goProtoLibrary.SetAttr("visibility", visibility)