Modernize protobuf rule #263
Conversation
|
@Yannic I just started some work in tensorboard and needed this exact thing to exist, so thanks! @jart Patching in this PR, then updating tensorboard's 3 collect_js calls to match the now-changed API, I was able to build the example protos in tensorflow using closure_proto_library and have that as a dep in my tensorboard tf_web_library component and successfully see it work in my tensorflow_html_binary demo server for that new component. |
|
verified test this please (I hope this is the right incantation for Bazel CI these days) |
| excludes=() | ||
| if [[ "${TRAVIS_OS_NAME}" == "osx" ]]; then | ||
| # https://github.com/bazelbuild/rules_closure/issues/247 | ||
| excludes+=('//closure/testing/test:noto_fonts_render_as_expected') |
There was a problem hiding this comment.
I would welcome a PR commenting out that test rule, saying, "hey this sometimes works, and is an example of what you could do, depending on your environment." If you do this, please also revert the complexity that rule added to this file.
| if ctx.files.externs: | ||
| print("closure_js_library 'externs' is deprecated; just use 'srcs'") | ||
| srcs = ctx.files.externs + srcs | ||
| def closure_js_library_impl( |
There was a problem hiding this comment.
There's a lot of surface area change here. I feel comfortable with this because you took the time to write more tests. Thank you. I'll do further integration testing with dependent projects afterwards, at some point.
There was a problem hiding this comment.
I tried to make sure there are no changes to the behavior of closure_js_library.
Let me know if you encounter one.
| srcs, deps, testonly, suppress, | ||
| closure_library_base, closure_library_deps, _ClosureWorker, | ||
|
|
||
| includes=[], |
There was a problem hiding this comment.
Please note that Skylark may have the same problem as Python. If you say includes=[] instead of includes=() then code might accidentally mutate the default value. That doesn't seem to be happening here.
There was a problem hiding this comment.
Thanks, I didn't think about that.
| "CLOSURE_LIBRARY_DEPS_ATTR", | ||
| "unfurl") | ||
|
|
||
| # This was taken from https://github.com/bazelbuild/rules_go/blob/67f44035d84a352cffb9465159e199066ecb814c/proto/compiler.bzl#L72 |
There was a problem hiding this comment.
We don't take things. We're just borrowing code with the same copyright / license :)
|
Thanks for the contribution. I'm merging now to get folks unblocked. LGTM |
| excludes=() | ||
| if [[ "${TRAVIS_OS_NAME}" == "osx" ]]; then | ||
| # https://github.com/bazelbuild/rules_closure/issues/247 | ||
| excludes+=('//closure/testing/test:noto_fonts_render_as_expected') |
| if ctx.files.externs: | ||
| print("closure_js_library 'externs' is deprecated; just use 'srcs'") | ||
| srcs = ctx.files.externs + srcs | ||
| def closure_js_library_impl( |
There was a problem hiding this comment.
I tried to make sure there are no changes to the behavior of closure_js_library.
Let me know if you encounter one.
| srcs, deps, testonly, suppress, | ||
| closure_library_base, closure_library_deps, _ClosureWorker, | ||
|
|
||
| includes=[], |
There was a problem hiding this comment.
Thanks, I didn't think about that.
| "CLOSURE_LIBRARY_DEPS_ATTR", | ||
| "unfurl") | ||
|
|
||
| # This was taken from https://github.com/bazelbuild/rules_go/blob/67f44035d84a352cffb9465159e199066ecb814c/proto/compiler.bzl#L72 |
This CL adds a new proto rule which works with Bazel's native |proto_library| rules.
We also upgrade travis to a newer version of Bazel.
Closes #190
cc: @hochhaus