Add Prost and Tonic rules. #2033
Conversation
freeformstu
force-pushed
the
dev/prost-and-tonic
branch
2 times, most recently
from
faf828f to
aa14aa4
Compare
freeformstu
force-pushed
the
dev/prost-and-tonic
branch
2 times, most recently
from
c799f09 to
61d1586
Compare
freeformstu
force-pushed
the
dev/prost-and-tonic
branch
from
61d1586 to
2c6d472
Compare
|
@UebelAndre, could you review? |
There was a problem hiding this comment.
Could you also make sure the new rules show up in the rust_proto docs? I think all you'd need to do is update the following
https://github.com/bazelbuild/rules_rust/blob/0.24.1/docs/BUILD.bazel#L115-L125
|
As for failing CI, I think it would be fine to bump the min tested Rust and Bazel version for this. As for the failing windows builds, I don't think there's enough info in the error to say what's going on |
freeformstu
force-pushed
the
dev/prost-and-tonic
branch
from
c4f97ce to
a58749c
Compare
There was a problem hiding this comment.
This is super exciting, thanks for putting it together!!
I tried to use it in some of my internal repos, and ran into some issues - curious if you have thoughts on how to solve them. The major issue is around protos which live in separate packages - I just created freeformstu#2 with an example - there are two issues which come up, one is around detecting multiple outputs with the same name, and if you work around that, there are issues where you have multiple lib.rs files which need to live in the same crate in order to be able to import symbols from each other (or which would need to know the crate name of the other crates things depend on to be able to include that in the use statements)...
Ideally we'd have a strategy for how we're going to address these issues before merging (though we don't need to solve them up-front); right now each of these proto_library structures work out of the box exactly as-is for other languages (e.g. Go), and it wouldn't be ideal for people to need to change the strucutre of their proto_library targets in order to add Rust support.
But as I say, super excited to see this progress, I'm looking forward to using this and getting to delete a bunch of hacky cargo_build_script targets!
|
Alright, the latest changes fixed all of @illicitonion 's additional test cases. Everything is passing except for the windows builds which are only failing on a subset of the tests. I am seeing the following error: I thought this was an issue with file path length, which it still could be, but I think it might be an issue with any @UebelAndre & @illicitonion - any ideas about how to fix this last issue? If it is just a file length issue, I could just make those test cases incompatible with WIndows so we can get this merged in. |
There was a problem hiding this comment.
This is looking great, thanks so much for putting it together!
I've tested out switching a complex application to use use these rules instead of existing cargo_build_scripts, and things looked a lot tidier - I've left a few comments to discuss, but generally this is looking really solid to me.
proto/prost/BUILD.bazel
Outdated
| name = "default_prost_toolchain_impl", | ||
| prost_plugin = "//proto/prost/private/3rdparty/crates:protoc-gen-prost__protoc-gen-prost", | ||
| prost_plugin_flag = "--plugin=protoc-gen-prost=%s", | ||
| prost_runtime = ":prost_runtime", |
There was a problem hiding this comment.
I'm not sure about providing a default toolchain with a rules_rust-private runtime... When trying this out in some of my private repos, I spent quite a while chasing "X doesn't implement trait Y" issues because of my copy of tonic and prost, despite being the same version numbers, being distinct libraries from the bundled ones, before eventually realising I needed to make and register my own toolchain.
I suspect most of our users will need to specify their own toolchains for this reason, and given how inscrutible the errors are, I'd lean towards documenting this as a required step, and not supplying a default at all...
WDYT?
There was a problem hiding this comment.
I agree, users should just define their own toolchain to avoid any of this confusion. I'll get this deleted.
There was a problem hiding this comment.
I moved the toolchain into private/BUILD.bazel and stopped automatically registering it in the public proto dependencies macros.
| all_rs_files.insert(path); | ||
| } | ||
| } else if let Some(name) = path.file_name() { | ||
| if name == "_" { |
There was a problem hiding this comment.
When does this happen? Can you add a comment?
There was a problem hiding this comment.
There was a problem hiding this comment.
That's correct, and I double checked that this is absolutely necessary. It shows up specifically when the package name is empty. I have added a comment.
| all_rs_files.insert(path); | ||
| } | ||
| } else if let Some(name) = path.file_name() { | ||
| if name == "_" { |
There was a problem hiding this comment.
|
I removed the bazel remote apis tests because they're causing infra issues. If I keep them added I get windows path length issues. If I depend on the remote execution protos via @bazel_remote_apis, tonic doesn't generate anything. I am definitely open to reverting my most recent commit to add them back. Open to suggestions! |
|
@illicitonion @freeformstu I believe there is still some issue remaining that you mentioned regarding googleapis and remoteapis. The failed CI runs for 74e5b08 seem to also indicate that this is not just a windows issue. Curiously, I can get rust_prost_library(
name = "annotations",
proto = "@googleapis//google/api:annotations_proto",
)ERROR: /home/aaron/.cache/bazel/_bazel_aaron/36db8dcbcbc6b1f82b5ba98ed0247b3e/external/googleapis/google/api/BUILD.bazel:9:14: TonicGenProto @googleapis//google/api:annotatio
ns_proto failed: (Exit 101): protoc_wrapper failed: error executing command (from target @googleapis//google/api:annotations_proto)
(cd /home/aaron/.cache/bazel/_bazel_aaron/36db8dcbcbc6b1f82b5ba98ed0247b3e/sandbox/linux-sandbox/612/execroot/turbo-cache && \
exec env - \
PATH=/home/aaron/.cache/bazelisk/downloads/bazelbuild/bazel-6.2.1-linux-x86_64/bin:/home/aaron/.bun/bin:/home/aaron/.bun/bin:/home/aaron/.bun/bin:/home/aaron/.nix-profile
/bin:/nix/var/nix/profiles/default/bin:/usr/local/sbin:/usr/local/bin:/usr/bin:/opt/bin:/usr/lib/llvm/16/bin:/home/aaron/Downloads/node/node-v18.16.0-linux-x64/bin:/home/aaro
n/.cargo/bin:/home/aaron/.local/bin:/home/aaron/Downloads/node/node-v18.16.0-linux-x64/bin:/home/aaron/.cargo/bin:/home/aaron/.local/bin:/home/aaron/Downloads/node/node-v18.1
6.0-linux-x64/bin:/home/aaron/.cargo/bin:/home/aaron/.local/bin \
bazel-out/k8-opt-exec-2B5CBBC6/bin/external/rules_rust/proto/prost/private/protoc_wrapper '--protoc=bazel-out/k8-opt-exec-2B5CBBC6/bin/external/com_google_protobuf/protoc'
'--label=@googleapis//google/api:annotations_proto' '--out_librs=bazel-out/k8-fastbuild/bin/external/googleapis/google/api/annotations_proto.lib.tonic.rs' '--package_info_out
put=annotations_proto=bazel-out/k8-fastbuild/bin/external/googleapis/google/api/annotations_proto.tonic_package_info' '--deps_info=bazel-out/k8-fastbuild/bin/external/googlea
pis/google/api/annotations_proto.tonic_deps_info' '--prost_opt=compile_well_known_types' '--descriptor_set=bazel-out/k8-fastbuild/bin/external/googleapis/google/api/annotatio
ns_proto-descriptor-set.proto.bin' '--plugin=protoc-gen-tonic=bazel-out/k8-opt-exec-2B5CBBC6/bin/external/crate_index__protoc-gen-tonic-0.2.2/protoc-gen-tonic__bin' '--tonic_
opt=no_include' '--tonic_opt=compile_well_known_types' --is_tonic '--rustfmt=external/rustfmt_nightly-2023-06-01__x86_64-unknown-linux-gnu_tools/bin/rustfmt' '--prost_out=baz
el-out/k8-fastbuild/bin' '--plugin=protoc-gen-prost=bazel-out/k8-opt-exec-2B5CBBC6/bin/external/crate_index__protoc-gen-prost-0.2.2/protoc-gen-prost__bin' '--proto_path=exter
nal/googleapis' '--proto_path=bazel-out/k8-fastbuild/bin/external/com_google_protobuf/_virtual_imports/descriptor_proto' '-Igoogle/api/annotations.proto=external/googleapis/g
oogle/api/annotations.proto' '-Igoogle/api/http.proto=external/googleapis/google/api/http.proto' '-Igoogle/protobuf/descriptor.proto=bazel-out/k8-fastbuild/bin/external/com_g
oogle_protobuf/_virtual_imports/descriptor_proto/google/protobuf/descriptor.proto' external/googleapis/google/api/annotations.proto)
# Configuration: ca63adb307a1bd0f693440015ddae19ec8302707b6d51da41eab328714b1af2a
# Execution platform: @local_config_platform//:host
Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
thread 'main' panicked at 'No .rs files were generated by prost.', external/rules_rust/proto/prost/private/protoc_wrapper.rs:724:9 |
Thanks for finding this! I think it should be tracked in a separate issue though. I opened #2046 |
* Setup Prost and Tonic rules. * Regenerate documentation * Add more tests. * Add more tests and address feedback. * Regenerate documentation * Add to proto docs page. * Bump min supported bazel version. * buildifier * Always enable backtracing. * Add more info to failing rename. * Set min rust version to 1.62.0 * Handle rust keywords as package names. * exclude windows from prost toolchain support. * buildifier * redundant * Use prost-types to parse the file descriptor set. * Cleanup and more tests. * Move prost-types to toolchain definition. * fix rustfmt * Add example of building protos with complex imports * impl Display * Fix all tests * Add rust checks for the complex import protos. * Address feedback * Fix buildifier * Depend on remote-apis repo. * Remove bazel remote apis due to file length and transitive dependency issues. * Update patch and docs. * Regenerate documentation * Regenerate documentation * Update docs. --------- Co-authored-by: Daniel Wagner-Hall <[email protected]>
* Setup Prost and Tonic rules. * Regenerate documentation * Add more tests. * Add more tests and address feedback. * Regenerate documentation * Add to proto docs page. * Bump min supported bazel version. * buildifier * Always enable backtracing. * Add more info to failing rename. * Set min rust version to 1.62.0 * Handle rust keywords as package names. * exclude windows from prost toolchain support. * buildifier * redundant * Use prost-types to parse the file descriptor set. * Cleanup and more tests. * Move prost-types to toolchain definition. * fix rustfmt * Add example of building protos with complex imports * impl Display * Fix all tests * Add rust checks for the complex import protos. * Address feedback * Fix buildifier * Depend on remote-apis repo. * Remove bazel remote apis due to file length and transitive dependency issues. * Update patch and docs. * Regenerate documentation * Regenerate documentation * Update docs. --------- Co-authored-by: Daniel Wagner-Hall <[email protected]>
Verification Evidence:
Helloworld server/client:
Server:
Client:
Echo server/client:
Server:
Client:
closes #915
closes #478