Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gazelle and inconvenient go_proto_library #317

Closed
ashi009 opened this issue Mar 17, 2017 · 12 comments
Closed

gazelle and inconvenient go_proto_library #317

ashi009 opened this issue Mar 17, 2017 · 12 comments
Labels

Comments

@ashi009
Copy link
Contributor

ashi009 commented Mar 17, 2017

gazelle lists all imports as xxx:go_default_library in generated deps, but go_proto_library doesn't satisfy this convention, unless the proto file is put under xxx_proto directory and being called as name="go_default_library". Which is awkward.

#315 should address this issue as well.

@jayconrod
Copy link
Contributor

This is a pain point for a lot of projects. We've been discussing internally how to improve this, but we don't have a concrete plan. This is definitely on our roadmap though.

@pmbethe09
Copy link
Contributor

If you want your code to build with 'go build', then you do have to put each proto in its own directory to satisfy Go, e.g. https://github.com/golang/protobuf/tree/master/ptypes/timestamp
And then use a
go_default_library(name="go_default_library"...

I agree that it is awkward, but talk to the Go team about that!

We are looking at upgrading gazelle to handle bazel-style naming, e.g. multiple things in a directory (but remember: those won't build with 'go build', and will break any go tooling/IDEs)

@ashi009
Copy link
Contributor Author

ashi009 commented Nov 23, 2017

@pmbethe09 I believe I've seen @jayconrod commented in another issue on make bazel work with native go toolchain in the future. So what's the long term plan on this?

With the help of proto_library, I can now organize code in the following structure to satisfy the native go tool chain:

proto
├── BUILD.bazel
├── message.proto
└── message_proto
    └── BUILD.bazel

The outer BUILD file has:

proto_library(
    name = "message_proto_proto",
    srcs = ["message.proto"],
    visibility = ["//visibility:public"],
    deps = ["@com_google_protobuf//:empty_proto"],
)

And the inner one has:

load("@io_bazel_rules_go//go:def.bzl", "go_library")
load("@io_bazel_rules_go//proto:def.bzl", "go_grpc_library")

go_grpc_library(
    name = "message_proto_go_proto",
    importpath = "minikube/signed_message/proto/message_proto",
    proto = "//signed_message/proto:message_proto_proto",
    visibility = ["//visibility:public"],
    deps = ["@com_github_golang_protobuf//ptypes/empty:go_default_library"],
)

go_library(
    name = "go_default_library",
    embed = [":message_proto_go_proto"],
    importpath = "minikube/signed_message/proto/message_proto",
    visibility = ["//visibility:public"],
)

By doing this, I can have proto files in one place, and have the XXX_proto rules in their own directories. This also creates the possibility to create go library with non-standard protoc plugin (eg. grpc gateway) and avoids having library names collision.

@jayconrod
Copy link
Contributor

I believe I've seen jayconrod commented in another issue on make bazel work with native go toolchain in the future. So what's the long term plan on this?

We support building with the native Go toolchain already. You can just call go_register_toolchains(go_version = "host") in your WORKSPACE file. rules_go doesn't require source files to be in any particular layout (only Gazelle does) whether you're using the toolchain installed on your host or a downloaded SDK.

With the help of proto_library, I can now organize code in the following structure to satisfy the native go tool chain

If I understand correctly, you need the go_library to be in the inner directory so that it has an import path that ends with message_proto. Other Go packages import the library this way, correct?

This does seem a little inconvenient, but in the near future (when #859 is closed), you'll be able to set the importpath attribute (via option go_package in the .proto) and import the library from anywhere in the repo. The directory won't matter. You'll still need something like this if you also want to be able to "go build" though.

@ashi009
Copy link
Contributor Author

ashi009 commented Nov 28, 2017

My understanding for "work with native go toolchain" here is that to organize the go code in a bazel workspace in a way that host go run and go build (even go test) will work without any additional configuration. But I'll take your answer ;-)

If I understand correctly, you need the go_library to be in the inner directory so that it has an import path that ends with message_proto. Other Go packages import the library this way, correct?

That's right. By doing this, it generates all the files in the correct directory, which host go toolchain understands. Other tools like gocode can use the generated go file to provide autocompletion.

Ideally I want be able to generate different go_libararys with different protoc plugins. Which needs importpath takes precedence over go_package. However protoc doesn't respect the importpath setting atm. Will #859 solve this?

@jayconrod
Copy link
Contributor

Ideally I want be able to generate different go_libararys with different protoc plugins.

There's no way to do this in a single build with the proto rules at the moment, since they are designed around toolchains, and only one proto toolchain can be active at a time. We may need to reconsider our design here if compiling with multiple plugins in a common use case.

The toolchain thing is a general Bazel problem: it also prevents us from compiling binaries for multiple platforms in the same build, which is something Kubernetes and other projects want. Toolchains are a new feature, so hopefully this will continue to improve.

However protoc doesn't respect the importpath setting atm.

importpath tells other rules the import path of the library that protoc will produce. It doesn't affect the behavior of protoc. I'm not sure if we could change that without modifying the input proto file.

Just to clarify, if you were able to generate multiple go_libraries from the same proto with different plugins, would you want to use them in the same build? Could you tell us a little more about your situation?

@ashi009
Copy link
Contributor Author

ashi009 commented Nov 30, 2017

@jayconrod Most of these issues are from the need of creating grpc gateway packages.

For our case, we have two separate servers for every exposed services, one grpc server, and one grpc-gateway server. The former don't really need the code generated by the grpc-gateway plugin, while the latter requires it to build. Although the gateway version of go_library is completely compatible with the grpc version, it does introduce many unneeded dependencies to the build graph, which I intend to avoid.

One solution is to allow build two 'go_libraries' from a single proto file. When build different binaries, use different version of generated package.

Also note that, for the grpc-gateway plugin, it depends on the output of grpc plugin -- as it only generates partial output -- an additional proto.gw.go file as a complement to the proto.go. So this use case actually needs two plugins. A workaround about it is to create a second rule embed the first one (see comment in #992).

@jayconrod
Copy link
Contributor

It sounds like you might need to define a new go_proto_toolchain that knows about the gateway plugin and a new rule that uses it, let's call it go_grpc_gateway_library. Then, you would declare a go_grpc_library and a go_grpc_gateway_library in the same package based on the same proto, and different binaries would depend on different targets for that library.

The two libraries would have the same importpath, but that would be fine, since you'd only use one or the other in any given build. Not sure whether it makes sense for the gateway library to embed the other one; that depends on the details of the plugin.

@ianthehat Does that sound about right?

@ashi009
Copy link
Contributor Author

ashi009 commented Dec 1, 2017

Then, you would declare a go_grpc_library and a go_grpc_gateway_library in the same package based on the same proto, and different binaries would depend on different targets for that library.

This is problematic, which means I have to manually maintain those build files, and it's error prone. I'd prefer to generate those targets in different packages so we can distinguish them at the import time -- and make gazelle also happy.

Not sure whether it makes sense for the gateway library to embed the other one; that depends on the details of the plugin.

Yeah, I've made it an optional attribute to allow manually setting it when necessary, which also opens the opportunity to chain up with other protoc generated grpc code (eg. gogo -- not our case though).

@jayconrod
Copy link
Contributor

Ok, we're going to try moving in a bit of a different direction. #1098 is a change to go_proto_library. Instead of using toolchains, you'll specify which plugin to use through an attribute on the rule. You can define multiple go_proto_library rules with different plugins and use whichever one is appropriate.

We still don't have a good way to change the import path. It doesn't seem like there's a way to do this without changing the proto file as it's being compiled or changing the binary proto descriptor.

@ashi009
Copy link
Contributor Author

ashi009 commented Dec 4, 2017

#1098 LGMT. Looking forward to having it checked in.

@jayconrod
Copy link
Contributor

#1098 and some related changes are checked in and released. The one thing that is still wrong is that Gazelle will generate go_grpc_library rules instead of go_proto_library with the grpc compiler. I've opened bazel-contrib/bazel-gazelle#8 to cover that.

I'll close this issue now, but please take a look and reopen if we're missing something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants