Skip to content

Conversation

@jyane
Copy link
Member

@jyane jyane commented Jan 10, 2018

add a bazel build definition to grpclb.

I think src/generated was created for resolving this issue #357 then I did not add generated files. I just use java_grpc_library.

But Bazel requires a full path from WORKSPACE in a proto file (https://github.com/cgrushko/proto_library/blob/04369f0d2ade8c8566727e0b6f3a53f1ba8925c0/src/person.proto#L5) for importing other proto files.
In this PR, it does not relate, but services will require a full path (master...jyane:add-services-bazel#diff-afc0c0ce1aad649c36404301917e883cR21)

"Do not add generated files" or "Use generated files in Bazel" which is better for now?

@ejona86
Copy link
Member

ejona86 commented Jan 11, 2018

I think src/generated was created for resolving this issue #357 then I did not add generated files. I just use java_grpc_library.

+1. That's what I was hoping to do.

But Bazel requires a full path from WORKSPACE in a proto file for importing other proto files.

Yeah. That's going to be a pain. Just before the holidays @carl-mastrangelo and I were figuring out how we wanted to handle that, because we have that problem internally as well. I think the plan was to generally rely on https://github.com/grpc/grpc-proto for any protos shared between repositories. Those protos could have the proper imports and we'd have copies in the repo for only gradle to use.

We'd still have to figure out something for testing and java-specific protos, since they wouldn't live in grpc-proto. We could maybe just use genrule()+sed to rewrite them.


java_proto_library(
name = "load_balancer_java_proto",
deps = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit for in the future: I've been preferring to put the srcs and deps for *_{grpc,proto}_library on one line, since they will normally only have one element and there can be a ton of those rules, for the various languages.

This formatting is still acceptable, though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will fix it, please wait for the merging 🙇

@ejona86 ejona86 requested a review from zhangkun83 January 11, 2018 17:45
@ejona86
Copy link
Member

ejona86 commented Jan 11, 2018

@jyane, I'm going to try to merge #3933 before this PR, to use the shiny new CI.

@jyane
Copy link
Member Author

jyane commented Jan 11, 2018

fixed, PTAL.

Just before the holidays c.arl-mastrangelo and I were figuring out how we wanted to handle that, because we have that problem internally as well.

We'd still have to figure out something for testing and java-specific protos, since they wouldn't live in grpc-proto. We could maybe just use genrule()+sed to rewrite them.

It is very interesting for me :) I have tried to add import_prefix to proto_library but I gave up(jyane/bazel@fc96cc6).

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Jan 11, 2018
@kokoro-team kokoro-team removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Jan 11, 2018
@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Jan 11, 2018
@ejona86
Copy link
Member

ejona86 commented Jan 17, 2018

@zhangkun83, could you also review this?

@ejona86 ejona86 merged commit 4b54df3 into grpc:master Jan 17, 2018
@jyane jyane deleted the add-bazel-grpclb branch January 18, 2018 03:49
@jyane jyane mentioned this pull request Feb 13, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants