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

Bazel expose protoc-gen-grpc-gateway #668

Merged

Conversation

emcfarlane
Copy link
Contributor

Exposes go_binary() rule for protoc-gen-grpc-gateway so dependencies can be vendored for the go_proto_compiler() rule when using //vendor dependencies.

@emcfarlane
Copy link
Contributor Author

Closing, recommendation is to remove all BUILD files for vendor/ dependencies.

@emcfarlane emcfarlane closed this Jun 11, 2018
@emcfarlane
Copy link
Contributor Author

Reopening, we are defining go_proto_compilier(...) internally to reference vendor/ deps and require this to be exposed.

@emcfarlane emcfarlane reopened this Jun 14, 2018
@achew22
Copy link
Collaborator

achew22 commented Jun 14, 2018

Can you rebase this and get CI passing?

Out of curiosity, what are you doing that requires a different go_proto_compiler?

@emcfarlane emcfarlane force-pushed the public-protoc-gen-grpc-gateway branch from 0085861 to ed56eaf Compare June 15, 2018 09:49
@emcfarlane
Copy link
Contributor Author

So some of our deps for https://github.com/grpc-ecosystem/grpc-gateway/blob/master/protoc-gen-grpc-gateway/BUILD.bazel#L31 are registered in bazel under //vendor/.... This leads to compile errors as the package types are incompatible. Not sure if there is a nicer way to do this? The current solution is to point the go library deps at the vendored packages and use the external repositories just for binaries.

@emcfarlane
Copy link
Contributor Author

@achew22 any idea why this is breaking?

@achew22
Copy link
Collaborator

achew22 commented Jun 15, 2018

Looks like protoc-go changed their output again. Can you run make examples and upload?

@emcfarlane
Copy link
Contributor Author

I'm unable to get the correct versions without playing with it more, seems to edit all the examples/*.go files.

@achew22
Copy link
Collaborator

achew22 commented Jun 15, 2018

I would expect that. Since the upstream protoc changed, probably all the .pb.go files will need to be updated.

@emcfarlane emcfarlane force-pushed the public-protoc-gen-grpc-gateway branch from ed56eaf to baa8b89 Compare June 29, 2018 12:17
@codecov-io
Copy link

Codecov Report

Merging #668 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #668   +/-   ##
=======================================
  Coverage   56.47%   56.47%           
=======================================
  Files          30       30           
  Lines        3005     3005           
=======================================
  Hits         1697     1697           
  Misses       1145     1145           
  Partials      163      163

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee3ef70...baa8b89. Read the comment docs.

@achew22
Copy link
Collaborator

achew22 commented Jun 29, 2018

Out of curiosity, what would you think about adding a new go_proto_compiler entry in there that has the option you care about enabled as well. WDYT? I'm happy to merge if you think it's not the right way to go.

@emcfarlane
Copy link
Contributor Author

@achew22 our go_proto_compiler rule looks like:

go_proto_compiler(
    name = "go_gateway",
    options = ["logtostderr=true"],
    plugin = "@grpc_ecosystem_grpc_gateway//protoc-gen-grpc-gateway",
    suffix = ".pb.gw.go",
    visibility = ["//visibility:public"],
    deps = [
        "//vendor/github.com/golang/protobuf/proto:go_default_library",
        "//vendor/github.com/grpc-ecosystem/grpc-gateway/runtime:go_default_library",
        "//vendor/github.com/grpc-ecosystem/grpc-gateway/utilities:go_default_library",
        "//vendor/golang.org/x/net/context:go_default_library",
        "//vendor/google.golang.org/grpc:go_default_library",
        "//vendor/google.golang.org/grpc/codes:go_default_library",
        "//vendor/google.golang.org/grpc/grpclog:go_default_library",
        "//vendor/google.golang.org/grpc/status:go_default_library",
    ],
)

I think there will be nicer ways to do this in the future with the rules_go. Maybe something in bazel-contrib/rules_go#1548 will fix it.

@achew22
Copy link
Collaborator

achew22 commented Jun 29, 2018

Oh, so is the major factor that you're vendoring in groc-gateway? If you weren't vendoring would the existing target work for you?

@emcfarlane
Copy link
Contributor Author

Yes and other grpc libraries that can cause version conflicts.

@achew22 achew22 merged commit fa90cfb into grpc-ecosystem:master Jun 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants