WKT related error in GoogleCloudPlatform/google-cloud-go

[mj-hd]

I found a bug caused by resolving behavior of gazelle's WKT.
googleapis/google-cloud-go#1017

In bazel with gazelle environment, vendored google's sdk that is using grpc fails to run because google's pre-compiled proto files depends on vendored github.com/golang/protobuf/proto/ptypes/*.

Is there any way to disable or exclude WKT resolving in vendored packages?

Here is code to reproduce.
Error Output:

~/.go/src/github.com/mjhd-devlion/bazel-spanner-bug » bazel run //:repro                                                                                                                    
INFO: Analysed target //:repro (96 packages loaded).
INFO: Found 1 target...
INFO: From Compiling external/com_google_protobuf/src/google/protobuf/compiler/js/embed.cc [for host]:
external/com_google_protobuf/src/google/protobuf/compiler/js/embed.cc:37:12: warning: unused variable 'output_file' [-Wunused-const-variable]
const char output_file[] = "well_known_types_embed.cc";
           ^
1 warning generated.
INFO: From Linking external/com_google_protobuf/libprotobuf.a [for host]:
/Library/Developer/CommandLineTools/usr/bin/libtool: file: bazel-out/host/bin/external/com_google_protobuf/_objs/protobuf/external/com_google_protobuf/src/google/protobuf/io/gzip_stream.o has no symbols
/Library/Developer/CommandLineTools/usr/bin/libtool: file: bazel-out/host/bin/external/com_google_protobuf/_objs/protobuf/external/com_google_protobuf/src/google/protobuf/util/internal/error_listener.o has no symbols
INFO: From Linking external/com_google_protobuf/libprotobuf_lite.a [for host]:
/Library/Developer/CommandLineTools/usr/bin/libtool: file: bazel-out/host/bin/external/com_google_protobuf/_objs/protobuf_lite/external/com_google_protobuf/src/google/protobuf/arenastring.o has no symbols
/Library/Developer/CommandLineTools/usr/bin/libtool: file: bazel-out/host/bin/external/com_google_protobuf/_objs/protobuf_lite/external/com_google_protobuf/src/google/protobuf/stubs/atomicops_internals_x86_msvc.o has no symbols
/Library/Developer/CommandLineTools/usr/bin/libtool: file: bazel-out/host/bin/external/com_google_protobuf/_objs/protobuf_lite/external/com_google_protobuf/src/google/protobuf/stubs/io_win32.o has no symbols
Target //:repro up-to-date:
  bazel-bin/darwin_amd64_stripped/repro
INFO: Elapsed time: 122.332s, Critical Path: 31.68s
INFO: 287 processes: 284 darwin-sandbox, 3 local.
INFO: Build completed successfully, 292 total actions

INFO: Running command line: bazel-bin/darwin_amd64_stripped/repro
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x114ed88]

goroutine 1 [running]:
vendor/github.com/golang/protobuf/proto.makeOneOfMarshaler.func1(0xc42014f170, 0x0, 0x8)
	vendor/github.com/golang/protobuf/proto/table_marshal.go:2281 +0x128
vendor/github.com/golang/protobuf/proto.(*marshalInfo).size(0xc42038b570, 0xc42014f170, 0xc4204c48e0)
	vendor/github.com/golang/protobuf/proto/table_marshal.go:182 +0xaa
vendor/github.com/golang/protobuf/proto.makeMessageSliceMarshaler.func1(0xc4200a3c40, 0x1, 0x8)
	vendor/github.com/golang/protobuf/proto/table_marshal.go:2170 +0x74
vendor/github.com/golang/protobuf/proto.(*marshalInfo).size(0xc42038b1f0, 0xc4200a3c40, 0x1552080)
	vendor/github.com/golang/protobuf/proto/table_marshal.go:182 +0xaa
vendor/github.com/golang/protobuf/proto.makeMessageSliceMarshaler.func1(0xc42014cc00, 0x1, 0x8)
	vendor/github.com/golang/protobuf/proto/table_marshal.go:2170 +0x74
vendor/github.com/golang/protobuf/proto.(*marshalInfo).size(0xc42038a700, 0xc42014cc00, 0x8)
	vendor/github.com/golang/protobuf/proto/table_marshal.go:182 +0xaa
vendor/github.com/golang/protobuf/proto.makeMessageMarshaler.func1(0xc4200ac100, 0x1, 0x6)
	vendor/github.com/golang/protobuf/proto/table_marshal.go:2145 +0x44
vendor/github.com/golang/protobuf/proto.(*marshalInfo).size(0xc42038a380, 0xc4200ac0b0, 0x193b520)
	vendor/github.com/golang/protobuf/proto/table_marshal.go:182 +0xaa
vendor/github.com/golang/protobuf/proto.(*InternalMessageInfo).Size(0x193b520, 0x1660340, 0xc4200ac0b0, 0x1fa01e0)
	vendor/github.com/golang/protobuf/proto/table_marshal.go:124 +0x65
vendor/google.golang.org/genproto/googleapis/spanner/v1.(*ReadRequest).XXX_Size(0xc4200ac0b0, 0x1660340)
	vendor/google.golang.org/genproto/googleapis/spanner/v1/spanner.pb.go:971 +0x43
vendor/github.com/golang/protobuf/proto.(*Buffer).Marshal(0xc4201f8248, 0x1660340, 0xc4200ac0b0, 0xc4204aa120, 0x0)
	vendor/github.com/golang/protobuf/proto/table_marshal.go:2645 +0x8a
vendor/google.golang.org/grpc/encoding/proto.marshal(0x15d0780, 0xc4200ac0b0, 0xc4201f8240, 0x0, 0x0, 0x156a100, 0xc4201d2900, 0xc4201d2900)
	vendor/google.golang.org/grpc/encoding/proto/proto.go:59 +0xda
vendor/google.golang.org/grpc/encoding/proto.codec.Marshal(0x15d0780, 0xc4200ac0b0, 0x161b840, 0xc4201c79c8, 0x10, 0xc420000180, 0x161b840)
	vendor/google.golang.org/grpc/encoding/proto/proto.go:74 +0xaf
vendor/google.golang.org/grpc.encode(0x1e0d1f8, 0x19598d0, 0x15d0780, 0xc4200ac0b0, 0x0, 0x0, 0xc420278300, 0x0, 0x0, 0x0, ...)
	vendor/google.golang.org/grpc/rpc_util.go:494 +0x26e
vendor/google.golang.org/grpc.(*csAttempt).sendMsg(0xc4201540d0, 0x15d0780, 0xc4200ac0b0, 0x0, 0x0)
	vendor/google.golang.org/grpc/stream.go:475 +0x111
vendor/google.golang.org/grpc.(*clientStream).SendMsg(0xc4201cc280, 0x15d0780, 0xc4200ac0b0, 0x192efc0, 0x1608c93)
	vendor/google.golang.org/grpc/stream.go:390 +0x43
vendor/google.golang.org/genproto/googleapis/spanner/v1.(*spannerClient).StreamingRead(0xc4200b25b8, 0x16610c0, 0xc4201f8390, 0xc4200ac0b0, 0x0, 0x0, 0x0, 0x0, 0x149f201, 0xc4201d28a0, ...)
	vendor/google.golang.org/genproto/googleapis/spanner/v1/spanner.pb.go:1587 +0x144
vendor/cloud.google.com/go/spanner.(*txReadOnly).ReadWithOptions.func2(0x16610c0, 0xc4201f8390, 0x0, 0x0, 0x0, 0x20, 0x16645e0, 0xc4200b25b8, 0xc4204c45e0)
	vendor/cloud.google.com/go/spanner/transaction.go:112 +0x209
vendor/cloud.google.com/go/spanner.(*resumableStreamDecoder).next(0xc420188180, 0x148dd1a)
	vendor/cloud.google.com/go/spanner/read.go:392 +0x398
vendor/cloud.google.com/go/spanner.(*RowIterator).Next(0xc420278180, 0x161ac70, 0xc420278180, 0x15f435d)
	vendor/cloud.google.com/go/spanner/read.go:86 +0x7d
vendor/cloud.google.com/go/spanner.(*txReadOnly).ReadRow(0xc4200d3680, 0x1661040, 0xc4200a0008, 0x15f435d, 0x4, 0xc4201a6420, 0x1, 0x1, 0xc4201a6430, 0x1, ...)
	vendor/cloud.google.com/go/spanner/transaction.go:141 +0x14a
main.main()
	main.go:21 +0x19d
ERROR: Non-zero return code '2' from command: Process exited with status 2

[jayconrod]

Thanks for reporting this. I wasn't able to reproduce this with the example you gave though. I don't have the GCP SDK set up on my machine, so I just get an error like this when creating the client:

2018/06/04 12:14:41 spanner: code = "Unknown", desc = "dialing fails for channel[0], google: could not find default credentials. See https://developers.google.com/accounts/docs/application-default-credentials for more information."

Without being able to reproduce, I'm not sure exactly what's breaking for you. I suspect that multiple packages with the same importpath are getting linked into the build. Specifically, both the vendored and external versions of github.com/golang/protobuf/proto are included. I would expect this to cause compile- or link-time errors though, not run-time crashes.

As an experiment, what happens if you replace the go_library rules for github.com/golang/protobuf/proto and github.com/golang/protobuf/ptypes with aliases? Do you still see the crash? Here's a patch that does this:

diff --git a/vendor/github.com/golang/protobuf/proto/BUILD.bazel b/vendor/github.com/golang/protobuf/proto/BUILD.bazel
index 98590c1..e478000 100644
--- a/vendor/github.com/golang/protobuf/proto/BUILD.bazel
+++ b/vendor/github.com/golang/protobuf/proto/BUILD.bazel
@@ -1,25 +1,7 @@
 load("@io_bazel_rules_go//go:def.bzl", "go_library")
 
-go_library(
+alias(
     name = "go_default_library",
-    srcs = [
-        "clone.go",
-        "decode.go",
-        "discard.go",
-        "encode.go",
-        "equal.go",
-        "extensions.go",
-        "lib.go",
-        "message_set.go",
-        "pointer_unsafe.go",
-        "properties.go",
-        "table_marshal.go",
-        "table_merge.go",
-        "table_unmarshal.go",
-        "text.go",
-        "text_parser.go",
-    ],
-    importmap = "vendor/github.com/golang/protobuf/proto",
-    importpath = "github.com/golang/protobuf/proto",
+    actual = "@com_github_golang_protobuf//proto:go_default_library",
     visibility = ["//visibility:public"],
 )
diff --git a/vendor/github.com/golang/protobuf/ptypes/BUILD.bazel b/vendor/github.com/golang/protobuf/ptypes/BUILD.bazel
index a1e092a..518f3c7 100644
--- a/vendor/github.com/golang/protobuf/ptypes/BUILD.bazel
+++ b/vendor/github.com/golang/protobuf/ptypes/BUILD.bazel
@@ -1,20 +1,7 @@
 load("@io_bazel_rules_go//go:def.bzl", "go_library")
 
-go_library(
+alias(
     name = "go_default_library",
-    srcs = [
-        "any.go",
-        "doc.go",
-        "duration.go",
-        "timestamp.go",
-    ],
-    importmap = "vendor/github.com/golang/protobuf/ptypes",
-    importpath = "github.com/golang/protobuf/ptypes",
+    actual = "@com_github_golang_protobuf//ptypes:go_default_library",
     visibility = ["//visibility:public"],
-    deps = [
-        "//vendor/github.com/golang/protobuf/proto:go_default_library",
-        "@io_bazel_rules_go//proto/wkt:any_go_proto",
-        "@io_bazel_rules_go//proto/wkt:duration_go_proto",
-        "@io_bazel_rules_go//proto/wkt:timestamp_go_proto",
-    ],
 )

[jayconrod]

Is there any way to disable or exclude WKT resolving in vendored packages?

There's no way to do this, currently. It would only be safe to do this in builds that don't include go_proto_library in any way. go_proto_library adds implicit dependencies on libraries in @com_github_golang_protobuf//.... go_proto_library can't depend on vendored copies of those libraries (the implementation doesn't know where those libraries would be vendored), so to avoid conflict, Gazelle tries its best to avoid dependencies on the vendored copies.

This is causing other problems though. Gazelle only has special cases for the WKTs. Vendoring other libraries in the same repos may also cause problems. I'd like to confirm this is the cause of the crash you're seeing before deciding on a solution though.

[mj-hd]

Thank you!

As an experiment, what happens if you replace the go_library rules for github.com/golang/protobuf/proto and github.com/golang/protobuf/ptypes with aliases? Do you still see the crash?

Your patch works perfectly, there are no errors.

I suspect that multiple packages with the same importpath are getting linked into the build. Specifically, both the vendored and external versions of github.com/golang/protobuf/proto are included. I would expect this to cause compile- or link-time errors though, not run-time crashes.

I agree with you. When I downgrade gazelle to version 0.10.0 that does not support WKT replacing, following errors are displayed:

GoLink: warning: package "github.com/golang/protobuf/proto" is provided by more than one rule:
    @com_github_golang_protobuf//proto:go_default_library
    //vendor/github.com/golang/protobuf/proto:go_default_library
Set "importmap" to different paths in each library.
This will be an error in the future.

I understood this problem can be resolved by editing each vendor's BUILD.bazel with #keep annotation, but I want to resolve this by WORKSPACE file or root BUILD.bazel file.

[jayconrod]

Great! I'm glad that worked.

A similar issue was brought up in #219 as well. I think the solution to this will be:

• When proto rule generation is enabled, Gazelle will resolve imports of implicit proto dependencies to external rules. This extends the current WKT resolution.
• When proto rule generation is disabled, we won't do any special dependency resolution, since it's not needed.
• We should provide a way to give an explicit hint to the dependency resolver so issues like this can be worked around more easily. For example:

# gazelle:resolve go github.com/golang/protobuf/proto @com_github_golang_protobuf//proto:go_default_library

[jayconrod]

Ok, this should be fixed at tip of master. There are basically two ways to get this to build:

Using pre-generated protos

Apply this patch:

diff --git a/BUILD.bazel b/BUILD.bazel
index 9b8d652..4e5d6e9 100644
--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -1,6 +1,8 @@
 load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")
 load("@bazel_gazelle//:def.bzl", "gazelle")
 
+# gazelle:proto disable_global
+
 gazelle(
     name = "gazelle",
     command = "fix",

This will disable all proto-related special cases in Go dependency resolution. You'll build this binary like a normal Go binary without any proto code generation at build time.

Using build-time generated proto code

Apply this patch instead:

diff --git a/BUILD.bazel b/BUILD.bazel
index 9b8d652..1eb31bf 100644
--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -1,11 +1,13 @@
 load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")
 load("@bazel_gazelle//:def.bzl", "gazelle")
 
+# gazelle:prefix github.com/mjhd-devlion/repro
+# gazelle:exclude vendor/google.golang.org/grpc
+# gazelle:exclude vendor/golang.org/x/oauth2
+
 gazelle(
     name = "gazelle",
     command = "fix",
-    external = "vendored",
-    prefix = "github.com/mjhd-devlion/repro",
 )
 
 go_library(
diff --git a/WORKSPACE b/WORKSPACE
index 8303f60..d70deba 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -18,6 +18,18 @@ go_rules_dependencies()
 
 go_register_toolchains()
 
-load("@bazel_gazelle//:deps.bzl", "gazelle_dependencies")
+load("@bazel_gazelle//:deps.bzl", "gazelle_dependencies", "go_repository")
 
 gazelle_dependencies()
+
+go_repository(
+    name = "org_golang_x_oauth2",
+    commit = "ef147856a6ddbb60760db74283d2424e98c87bff",
+    importpath = "golang.org/x/oauth2",
+)
+
+go_repository(
+    name = "com_google_cloud_go",
+    commit = "e14b7b4aa815fab3cebdd0cc0462f94aed282791",
+    importpath = "cloud.google.com/go",
+)

This does several things:

• Declares an external repository for @org_golang_x_oauth2. Something depends on a library in @org_golang_google_grpc that depends on this.
• Declares an external repository for @com_google_cloud_go. golang.org/x/oauth2/google depends on this.
• Excludes vendor/google.golang.org/grpc. We need to use the external version of this, since it's an implicit dependency of any go_proto_library with services.
• Excludes vendor/golang.org/x/oauth2 since the external gRPC depends on the external oauth2, so we want that instead.
• Removes external = "vendored" from the gazelle rule. We want to use the external versions of things if they aren't found in the current repository. Vendored library that are present and not excluded will still be used though.
• Switch prefix to a directive. Not strictly necessary,

[jayconrod]

See bazel-contrib/rules_go#1548 for more info on recent proto dependency changes.