-
Notifications
You must be signed in to change notification settings - Fork 767
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] Introduce grpc_web_toolchain #872
Conversation
This change adds a new rule `grpc_web_toolchain` which we will use to provide the generator/runtime to `closure_grpc_web_library`. Updates grpc#507 RELNOTES: [bazel] `closure_grpc_web_library` now uses toolchains to resolve the generator and runtime. To migrate, add the following snipped to your `WORKSPACE`: ```starlark load("@com_github_grpc_grpc_web//bazel:repositories.bzl", "grpc_web_toolchains") grpc_web_toolchains() ```
This is not ready for merging because the required change in Bazel has not been released yet. Requires Bazel@HEAD (> 3.3.x). |
This is ready for review now. PTAL, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Yannic Thanks for bringing this to the finish line! I know this is a long time coming.
I reviewed this to the best of my ability, although I must admit I don't fully understand some of the concepts like what exactly a toolchain is. I trust that these code will be exercised by https://github.com/grpc/grpc-web/blob/master/scripts/docker-run-tests.sh#L22, https://github.com/grpc/grpc-web/blob/master/net/grpc/gateway/docker/prereqs/Dockerfile#L54, and etc.
LGTM. Thanks again!
name = "closure_toolchain_impl", | ||
generator = "//javascript/net/grpc/web:protoc-gen-grpc-web", | ||
runtime_library = "//javascript/net/grpc/web:closure_grpcweb_runtime", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess my question here is that, do the attribute generator
and runtime_library
have special meaning here? Or it can be any string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both attributes are Labels that have to point to targets (just like, e.g, cc_library
's deps
). generator
also must be an executable target (e.g. {cc,go,java}_binary
) and runtime_library
must point to a closure_js_library
(compatible) target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
Yes, we have tests to verify this actually works.
Toolchains are complicated, especially if you haven't used them before. If you want to read up on them, here's an introduction.
name = "closure_toolchain_impl", | ||
generator = "//javascript/net/grpc/web:protoc-gen-grpc-web", | ||
runtime_library = "//javascript/net/grpc/web:closure_grpcweb_runtime", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both attributes are Labels that have to point to targets (just like, e.g, cc_library
's deps
). generator
also must be an executable target (e.g. {cc,go,java}_binary
) and runtime_library
must point to a closure_js_library
(compatible) target.
This change adds a new rule
grpc_web_toolchain
which we will use toprovide the generator/runtime to
closure_grpc_web_library
.Updates #507
RELNOTES: [bazel]
closure_grpc_web_library
now uses toolchains toresolve the generator and runtime. To migrate, add the following snipped
to your
WORKSPACE
:RELNOTES: [bazel] gRPC-Web now follows best practices for exporting
rules and exports all public symbols in
@com_github_grpc_grpc_web//bazel:defs.bzl
.We intend to remove the existing
closure_grpc_web_library.bzl
in afuture release.