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

Add filegroup for options proto files #821

Merged
merged 1 commit into from
Dec 6, 2018

Conversation

kellycampbell
Copy link
Contributor

This allows other projects to generate proto code for other languages, e.g. py_proto_library

@achew22
Copy link
Collaborator

achew22 commented Dec 5, 2018

Perfect, thanks so much for your contribution! If you have any more issues please open an issue and let's work through it.

It looks like there is a formatting issue with your change. Could you run buildifier (bazel run :buildifier) and update the PR? Thanks

PS: I recently learned about the --override_repository flag which can be used like --override_repository "com_github_grpc_ecosystem_grpc_gateway=/path/to/a/local/copy/that/you/want/to/test/with" if you want to test out changes like this in your repo without having to rewrite your WORKSPACE.

This allows other projects to generate proto code for other
languages, e.g. py_proto_library
@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #821   +/-   ##
=======================================
  Coverage   49.32%   49.32%           
=======================================
  Files          39       39           
  Lines        3751     3751           
=======================================
  Hits         1850     1850           
  Misses       1723     1723           
  Partials      178      178

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 d8ad87e...ba17731. Read the comment docs.

@achew22
Copy link
Collaborator

achew22 commented Dec 6, 2018

I was thinking about this, and as a curiosity, what are the circumstances where you need a py_proto_library for this? I expect these protos to really only be consumed by the swagger code generator

@kellycampbell
Copy link
Contributor Author

We have python gRPC backends which implement the services defined with rest annotations and documented using these swagger annotations.

It only needs them because they're imported and referenced by our services .profo file. If I run protoc for python it errors out if it can't find these options protos. Nothing in our python code actually needs the annotations.

Ideally, the py_proto_library rule should be able to use the proto provider from the proto_library to find the files, but currently the one we're using doesn't. (https://github.com/pubref/rules_protobuf/tree/master/python)

@achew22
Copy link
Collaborator

achew22 commented Dec 6, 2018

That's an unfortunate outcome but it makes sense that proto works that way. I meant to merge earlier, but forgot to click the button. Sorry about that

@achew22 achew22 merged commit 7ad920c into grpc-ecosystem:master Dec 6, 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