Add Prost/Tonic rules #479
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is still very WIP in terms of its integration with rules_rust, however, I wanted to put this up as a reference for those who may be interested in pushing it forward more. Ideally, rules_rust would be updated to expose an interface similar to rules_go: https://github.com/bazelbuild/rules_go#protobuf-and-grpc That allows plugins appropriate access into build internals so that the generate code can be shifted by the user. This quite necessary for rust, as there is no canonical rust api for protobuf/grpc; Prost/Tonic is just one of them. The current rust_proto_library and rust_grpc_library are tied to grpc_rust and rust_protobuf crates currently, and this cannot be shifted with the rust_protobuf_toolchain. This PR contains a couple parts: 1. Bazel rules and macros that define an external prost_library and tonic_library that can generate code from a proto_library target 2. A prostgen executable that does the heavy lifting of running the code generation, and making sure that different prost_library targets can be used in different targets 3. The cargo file with the requisite crates for prostgen 4. A set of test cases that we had run into when using prost_library and tonic_library. The //proto/prostgen/raze folder doesn't quite interoperate with the //proto/raze folder, and I haven't had a chance to nail that down yet. There are also some targets missing from WORKSPACE needed for the tests. For googlers, the high-level approach is described in go/rust-protobuf.
GregBowyer
reviewed
Nov 9, 2020
| transitive = [transitive_sources, extern_descriptors], | ||
| ), | ||
| env = { | ||
| # We've patched rustfmt to expect that it can resolve the path to |
There was a problem hiding this comment.
Super minor nit: s/rustfmt/prost/ ?
There was a problem hiding this comment.
ahh, yeah. That should be prost-build, not rustfmt.
wchargin
reviewed
Nov 12, 2020
| let config = tonic_build::configure() | ||
| .build_client(self.grpc) | ||
| .build_server(self.grpc) | ||
| .format(true) |
There was a problem hiding this comment.
I’ve only skimmed the build, but I didn’t see a data dep anywhere; is
this hermetic? (If so, I’d love to steal your incantation!)
cf. #388 (comment)
There was a problem hiding this comment.
Ah! I see now that the _prost_generator rule is properly wired up to
the toolchain. I was hoping that there was a way that didn’t require
having to do that :-) but that should definitely work.
|
Something in here makes (at least my remote build setup) fail catastrophically |
dfreese
commented
Nov 16, 2020
| ), | ||
| "_generator": attr.label( | ||
| executable = True, | ||
| cfg = "host", |
| "_protoc": attr.label( | ||
| doc = "The location of the `protoc` binary. It should be an executable target.", | ||
| executable = True, | ||
| cfg = "host", |
I hadn't been super careful about host v. exec, so one thing that could cause a problem is that the prostgen binary should be marked for "exec" not "host". The other thing that might be causing issues is the handling of the rustfmt and protoc binaries. |
|
I didnt try "exec" although I can confirm its not the binaries those seem fine.
I will try exec shortly and let you know
…On Mon, Nov 16, 2020, at 9:21 AM, David Freese wrote:
> Something in here makes (at least my remote build setup) fail catastrophically
I hadn't been super careful about host v. exec, so one thing that could cause a problem is that the prostgen binary should be marked for "exec" not "host".
The other thing that might be causing issues is the handling of the rustfmt and protoc binaries.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#479 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA6X3ZJXR2FVTTVVUF47XLSQFNQPANCNFSM4TLXIUOQ>.
|
|
No dice with "exec".
Right now my current workaround is in bazelrc to define
```
build:remote --strategy=ProstSourceGeneration=local
```
…On Mon, Nov 16, 2020, at 10:38 AM, Greg Bowyer wrote:
I didnt try "exec" although I can confirm its not the binaries those seem fine.
I will try exec shortly and let you know
On Mon, Nov 16, 2020, at 9:21 AM, David Freese wrote:
>
>> Something in here makes (at least my remote build setup) fail catastrophically
> I hadn't been super careful about host v. exec, so one thing that could cause a problem is that the prostgen binary should be marked for "exec" not "host".
> The other thing that might be causing issues is the handling of the rustfmt and protoc binaries.
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub <#479 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA6X3ZJXR2FVTTVVUF47XLSQFNQPANCNFSM4TLXIUOQ>.
>
|
dfreese
commented
Feb 26, 2021
Comment on lines
+98
to
+113
| # Since tonic_build has logic to run rustfmt, and does so by default | ||
| # for that matter, inject rustfmt into PATH, so that | ||
| # std::process::Command can find it. This seemed to be more clear | ||
| # than setting up a separate bazel run action, and removes another | ||
| # generated file. A run action can't have the same input and | ||
| # output file. This is also compatible with prostgen currently | ||
| # creating a lib.rs in the output directory, rather than taking in | ||
| # an output directory and an output file. The idea with only taking | ||
| # in an output directory was to try and leave open the possibility | ||
| # of using include! macros pointing towards the generated files. | ||
| # This might make things a little easier to read, rather than being | ||
| # extremely nested by package name. | ||
| # | ||
| # May not be perfect either, as the files are formatted before they | ||
| # get indented to match their module nesting. | ||
| "PATH": rustfmt.dirname, |
There was a problem hiding this comment.
relevant section for tonic-build PR
There was a problem hiding this comment.
How does hyperium/tonic#571 solve for this? What's the code change here?
There was a problem hiding this comment.
Pinging this question again, can you explain what would change should that PR get merged?
There was a problem hiding this comment.
Could you move to rustfmt as a library to solve this issue?
There was a problem hiding this comment.
There was a change merged that allows you to specify RUSTFMT as a path to a rustfmt binary. So this could become: "RUSTFMT": rustfmt.path
There was a problem hiding this comment.
Once 4.0.1 or higher comes out
|
I'm closing this out since no one is working on it and (despite it's age of the PR), the draft appears to be leading some folks to think the work is in progress. I'd be happy to see it reopened and pushed to completion since this would be an awesome feature! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is still very WIP in terms of its integration with rules_rust,
however, I wanted to put this up as a reference for those who may be
interested in pushing it forward more. Ideally, rules_rust would be
updated to expose an interface similar to rules_go:
https://github.com/bazelbuild/rules_go#protobuf-and-grpc
That allows plugins appropriate access into build internals so that the
generate code can be shifted by the user. This quite necessary for
rust, as there is no canonical rust api for protobuf/grpc; Prost/Tonic
is just one of them.
The current rust_proto_library and rust_grpc_library are tied to
grpc_rust and rust_protobuf crates currently, and this cannot be shifted
with the rust_protobuf_toolchain.
This PR contains a couple parts:
tonic_library that can generate code from a proto_library target
code generation, and making sure that different prost_library
targets can be used in different targets
and tonic_library.
The //proto/prostgen/raze folder doesn't quite interoperate with the
//proto/raze folder, and I haven't had a chance to nail that down yet.
There are also some targets missing from WORKSPACE needed for the tests.
For googlers, the high-level approach is described in go/rust-protobuf.