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.

Sign up for GitHub

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

Jump to bottom

Added Bazel support #599

Merged
merged 3 commits into from
Apr 16, 2018
Merged

Added Bazel support #599

merged 3 commits into from
Apr 16, 2018

Conversation

f0rmiga
Copy link
Contributor

@f0rmiga f0rmiga commented Apr 14, 2018 •
edited
Loading

Sorry for the size of the PR. I would be up for a peer review if wanted.

Almost every target was generated by Gazelle.
Gazelle tried to index //google/api/... because of the third party protos being used. To solve that I excluded the third_party directory from Gazelle and created the repositories.bzl that at this point:
- Downloads the googleapis repository.
- Adds a BUILD.bazel file under //google/api/ with targets for the protos.

Gazelle still tries to remove "@com_github_googleapis_googleapis//google/api:api_proto" and "@com_github_googleapis_googleapis//google/api:go_default_library" in favour of //google/api/..., which is wrong.

To check what Gazelle reports in check mode, run bazel run //:gazelle_diff.

The third_party directory is not being used by any Bazel target at all. If the grpc-gateway team decides to move completely to Bazel, this directory can be removed.

Every .pb.go and .pb.gw.go can also be removed if this repo moves completely to Bazel. In order to have the swagger generator to work with Bazel, I would need to write a specific rule for that.

To compile everything:

$ bazel build //...

To test everything:

$ bazel test //...

@f0rmiga
Copy link
Contributor Author

f0rmiga commented Apr 14, 2018

@achew22

@achew22
Copy link
Collaborator

achew22 commented Apr 14, 2018

Would you be willing to edit .travis.yaml to include bazel in the CI process. You can review https://github.com/bazelbuild/rules_go#how-do-i-run-bazel-on-travis-ci for inspiration

@f0rmiga
Copy link
Contributor Author

f0rmiga commented Apr 14, 2018

Sure. I have Bazel setup for a couple projects of mine in Travis CI.

@achew22
Copy link
Collaborator

achew22 commented Apr 14, 2018

To be a bit more explicit, could you add to the matrix something like USE_BAZEL=true and then put an early termination if statement in?

yugui
yugui reviewed Apr 15, 2018
View reviewed changes
runtime/internal/BUILD.bazel Outdated
load("@io_bazel_rules_go//go:def.bzl", "go_library")
load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library")

package(default_visibility = ["//visibility:public"])
Copy link
Member

Choose a reason for hiding this comment

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

Could you make the visibility of the package consistent with the one by go compiler?
i.e. it looks to be better to limit the default visibility to //runtime:__subpackages__.

BTW, as the original author of Gazelle, I'm sorry -- it should have been done by gazelle. ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are excused hehe. Thanks for the Gazelle, it saves tons of time daily for me.
I also made changes to other package visibilities as I saw fit.

f0rmiga
f0rmiga commented Apr 15, 2018
View reviewed changes
examples/examplepb/BUILD.bazel
"//examples/sub:sub_proto",
"//examples/sub2:sub2_proto",
"//protoc-gen-swagger/options:options_proto",
"@com_github_googleapis_googleapis//google/api:api_proto",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yugui Any idea on how to have Gazelle to respect that?

Copy link
Member

@yugui yugui Apr 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I have no idea. Let me merge this PR as it is once #599 (comment) gets ready.

@f0rmiga f0rmiga force-pushed the add-bazel-support branch 6 times, most recently from ac35b66 to 3683241 Compare April 16, 2018 00:59
@f0rmiga
Copy link
Contributor Author

f0rmiga commented Apr 16, 2018 •
edited
Loading

@achew22 What do you think of the CI config? We don't need to test using Bazel 3 times. Not sure how to solve that... any ideas?

@f0rmiga f0rmiga force-pushed the add-bazel-support branch 5 times, most recently from cc3ac6d to 7024322 Compare April 16, 2018 02:57
@achew22
Copy link
Collaborator

achew22 commented Apr 16, 2018

I think you can use exclusions to achieve that

https://docs.travis-ci.com/user/customizing-the-build#Excluding-Jobs

@yugui
Copy link
Member

yugui commented Apr 16, 2018

It still makes sense to build with different versions of go in Bazel. But either is OK for me.

LGTM except for the redundant 3 builds.

@f0rmiga
Copy link
Contributor Author

f0rmiga commented Apr 16, 2018

@yugui We can do that in another PR. What if we merge the way it is right now and I focus on improving the CI build?

@f0rmiga
Copy link
Contributor Author

f0rmiga commented Apr 16, 2018

I actually need to rebase and add changes merged to master.

@achew22
Copy link
Collaborator

achew22 commented Apr 16, 2018

I think @yugui is saying that she would be happy with 3 builds if they built master, 1.10, and 1.9 in Bazel, please correct me if I am wrong. That is totally achievable, but it would require having a custom WORKSPACE file using a local go sdk. I think that would be an interesting path forward. WDYT?

@f0rmiga
Copy link
Contributor Author

f0rmiga commented Apr 16, 2018 •
edited
Loading

@yugui Yes, I think @yugui meant it. I got that! My suggestion is to focus on improving the CI in another PR since things are getting merged to master frequently and those changes should be added to Bazel. What do you think?

@f0rmiga f0rmiga force-pushed the add-bazel-support branch 2 times, most recently from ad99237 to 7581b7b Compare April 16, 2018 15:54
@f0rmiga
Updated Travis CI
7dbea4a 
Update Travis CI 2
@f0rmiga f0rmiga force-pushed the add-bazel-support branch from 7581b7b to 7dbea4a Compare April 16, 2018 15:56
@f0rmiga
Copy link
Contributor Author

f0rmiga commented Apr 16, 2018

@yugui @achew22 CI done.

@achew22 achew22 merged commit 289beb3 into grpc-ecosystem:master Apr 16, 2018
@achew22
Copy link
Collaborator

achew22 commented Apr 16, 2018

@f0rmiga, thanks so much for your contribution! I look forward to adding more support for Bazel going forward 🎆

@f0rmiga
Copy link
Contributor Author

f0rmiga commented Apr 16, 2018

@achew22 You are very welcome. I look forward to contributing to more stuff! :)

@achew22
Copy link
Collaborator

achew22 commented Apr 16, 2018

@f0rmiga Out of curiosity, do you have a theory on how to add swagger generation in Bazel? I'm a little hesitant to use the rules_go tooling around it. WDYT?

@f0rmiga
Copy link
Contributor Author

f0rmiga commented Apr 16, 2018

@achew22 Honestly, I tried to integrate it. I think we would need to create a rule similar to go_proto_library. The problem of trying to use it for swagger is that this rule expects .go files to come out of the compiler so it can compile and output a single .a file. I can give it a shot later this week.

@f0rmiga
Copy link
Contributor Author

f0rmiga commented Apr 16, 2018

@achew22 For reference: https://github.com/bazelbuild/rules_go/blob/c29b08c0b81a1bfdc358c6bd3a59c068165bfefa/proto/def.bzl#L105

@achew22
Copy link
Collaborator

achew22 commented Apr 17, 2018

Fortunately there are other projects who have implemented it in a less go specific way. For example, https://github.com/bazelbuild/rules_closure/blob/dbb96841cc0a5fb2664c37822803b06dab20c7d1/closure/protobuf/closure_proto_library.bzl

@f0rmiga
Copy link
Contributor Author

f0rmiga commented Apr 17, 2018

Nice! Good stuff.

adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
@f0rmiga @achew22
Added Bazel support (grpc-ecosystem#599)
56fee81 
Fixes: grpc-ecosystem#66
@f0rmiga f0rmiga deleted the add-bazel-support branch November 11, 2020 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yugui yugui yugui left review comments

@achew22 achew22 achew22 approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@f0rmiga @achew22 @yugui @googlebot