go_proto/go_grpc compilers generate code with wrong WKT imports (github.com/golang/protobuf/ptypes)

[andremarianiello]

What version of rules_go are you using?

v0.28.0

What version of gazelle are you using?

v0.23.0

What version of protoc are you using?

v3.17.3

What version of Bazel are you using?

4.1.0

Does this issue reproduce with the latest releases of all the above?

Yes, AFAIK these are the latest.

What operating system and processor architecture are you using?

linux/amd64

Any other potentially useful information about your toolchain?

No

What did you do?

I have a .proto file in package a which also contains code that depends on grpc-go. I have a .proto file in package b which imports a.proto and the timestamp WKT. I constructed a WORKSPACE and used gazelle to generate build files for a and b. I tried to build with bazel build //b. https://github.com/andremarianiello/bazel-ptypes-issue contains a minimal reproduction.

What did you expect to see?

I expected a successful build. Eliminating any one of the dependencies (a -> grpc-go, b -> a, b -> timestamp) results in a successful build.

What did you see instead?

Running bazel build //b with this setup produces

compilepkg: missing strict dependencies:
        /root/.cache/bazel/_bazel_root/783533fb48c8e7116244c958ce4acbf8/sandbox/linux-sandbox/400/execroot/__main__/bazel-out/k8-fastbuild/bin/b/b_go_proto_/example.com/example/b/b.pb.go: import of "github.com/golang/protobuf/ptypes/timestamp"
No dependencies were provided.
Check that imports in Go sources match importpath attributes in deps.
Target //b:b failed to build

Additional Information

b.pb.go is produced with an import of github.com/golang/protobuf/ptypes/timestamp. However, well-known types (AFAIK) should be producing imports using the google.golang.org WKTs in recent versions of protoc. This import is being generated because of the following protoc invocation

bazel-out/k8-opt-exec-2B5CBBC6/bin/external/io_bazel_rules_go/go/tools/builders/go-protoc/go-protoc-bin -protoc bazel-out/k8-opt-exec-2B5CBBC6/bin/external/com_google_protobuf/protoc -importpath example.com/example/b -out_path bazel-out/k8-fastbuild/bin/b/b_go_proto_/ -plugin bazel-out/k8-opt-exec-2B5CBBC6/bin/external/io_bazel_rules_go/proto/go_grpc_reset_plugin_/protoc-gen-go -option 'plugins=grpc' -descriptor_set bazel-out/k8-fastbuild/bin/a/a_proto-descriptor-set.proto.bin -descriptor_set bazel-out/k8-fastbuild/bin/external/com_google_protobuf/timestamp_proto-descriptor-set.proto.bin -descriptor_set bazel-out/k8-fastbuild/bin/b/b_proto-descriptor-set.proto.bin -expected bazel-out/k8-fastbuild/bin/b/b_go_proto_/example.com/example/b/b.pb.go -import 'google/protobuf/any.proto=github.com/golang/protobuf/ptypes/any' -import 'google/protobuf/duration.proto=github.com/golang/protobuf/ptypes/duration' -import 'google/protobuf/timestamp.proto=github.com/golang/protobuf/ptypes/timestamp' -import 'google/rpc/status.proto=google.golang.org/genproto/googleapis/rpc/status' -import 'a/a.proto=example.com/example/a' -import 'b/b.proto=example.com/example/b' b/b.proto

which contains numerous imports for WKTs overridden with flags, including -import 'google/protobuf/timestamp.proto=github.com/golang/protobuf/ptypes/timestamp'. This import is causing the bad import to be generated. Running the protoc command without that timestamp import produces "google.golang.org/protobuf/types/known/timestamppb" in b.pb.go as we would like. This import appears in the protoc invocation because b depends on a, a depends on grpc-go, and grpc-go depends on the proto well-known types, including timestamp. Since the timestamp WKT is a transitive dependency of b, the 'google/protobuf/timestamp.proto=github.com/golang/protobuf/ptypes/timestamp' import is propagated up to the build for b . This is good for most imports, but I'm not sure this is the right behavior for WKTs. I tried building with a modified version of rules_go that prevents WKTs from being add to imports, via

diff --git a/proto/def.bzl b/proto/def.bzl
index bd904516..d059fec1 100644
--- a/proto/def.bzl
+++ b/proto/def.bzl
@@ -47,7 +47,8 @@ def get_imports(attr):
     direct = dict()
     for dep in proto_deps:
         for src in dep[ProtoInfo].check_deps_sources.to_list():
-            direct["{}={}".format(proto_path(src, dep[ProtoInfo]), attr.importpath)] = True
+            if not proto_path(src, dep[ProtoInfo]).startswith("google/protobuf/"):
+                direct["{}={}".format(proto_path(src, dep[ProtoInfo]), attr.importpath)] = True

     deps = getattr(attr, "deps", []) + getattr(attr, "embed", [])
     transitive = [

and I successfully built with that change. However, I don't know if this is the right fix, which is why I am filing an issue instead of a PR. Any help with this would be much appreciated, thanks!

[USB-05]

I tried the patch and it is not working for me.

local_repository( name = "io_bazel_rules_go", path = "/Users/usb05/Downloads/rules_go", )

[FastNav]

Update here. It seems like the master branch build of rules_go is broken by the proposed patch because //proto/wkt:api_go_proto, etc. relies on the following import resolutions that are pulled in from transitive deps:

google/protobuf/source_context.proto -> google.golang.org/genproto/protobuf/source_context google/protobuf/type.proto -> google.golang.org/genproto/protobuf/ptype google/protobuf/api.proto -> google.golang.org/genproto/protobuf/api

Changing the patch to leave these resolutions untouched unbreaks rules_go (at least for whatever tests I can run locally without errors). e.g.

diff --git a/proto/def.bzl b/proto/def.bzl
index a20459d6..dc49dc6b 100644
--- a/proto/def.bzl
+++ b/proto/def.bzl
@@ -47,7 +47,9 @@ def get_imports(attr):
     direct = dict()
     for dep in proto_deps:
         for src in dep[ProtoInfo].check_deps_sources.to_list():
-            direct["{}={}".format(proto_path(src, dep[ProtoInfo]), attr.importpath)] = True
+            if not (proto_path(src, dep[ProtoInfo]).startswith("google/protobuf/") and
+                    not attr.importpath.startswith("google.golang.org/")):
+                direct["{}={}".format(proto_path(src, dep[ProtoInfo]), attr.importpath)] = True

     deps = getattr(attr, "deps", []) + getattr(attr, "embed", [])
     transitive = [

It also unbreaks my own broken Bazel project. I'm also not sure what the right solution to this problem is though. It seems like a potentially recurring problem when dependencies disagree about the Go importmap of a single Proto import path.

[mgenov]

I'm encountering similar issue on my side after upgrade of rules_go and at the same time updated the google cloud storage API dependency. It would be good if this issue could be resolved either in the google cloud libraries or in rules_go.

[m9rco]

I have the same problem

[jordan-bonecutter]

I fixed this by upgrading the protobuf version from 3.13 to 3.14. Not sure if this is a universal solution though

[bweston92]

@jordan-bonecutter - I fixed "some" issues with that, however some targets are working and some aren't with identical import and use of some WKTs, it is bizarre.

If you/anyone knows why this might be happening I'd love to know :+1:

[dev344]

Following @jordan-bonecutter's advice I upgraded from 3.13 to 3.14 and that fixed it for me.

Before

http_archive(
    name = "com_google_protobuf",
    sha256 = "1c744a6a1f2c901e68c5521bc275e22bdc66256eeb605c2781923365b7087e5f",
    strip_prefix = "protobuf-3.13.0",
    urls = ["https://github.com/protocolbuffers/protobuf/archive/v3.13.0.zip"],
)

After

http_archive(
    name = "com_google_protobuf",
    sha256 = "d0f5f605d0d656007ce6c8b5a82df3037e1d8fe8b121ed42e536f569dec16113",
    strip_prefix = "protobuf-3.14.0",
    urls = [
        "https://mirror.bazel.build/github.com/protocolbuffers/protobuf/archive/v3.14.0.tar.gz",
        "https://github.com/protocolbuffers/protobuf/archive/v3.14.0.tar.gz",
    ],
)