Replace rust rules with new prost and tonic rules

[titanous]

Closes #143.

[titanous]

I've updated this to use crates_universe, which works better than cargo-raze. Let me know if you like the approach or have any other suggestions and I can finish off the documentation.

[aaliddell]

Awesome work.

Since this is a breaking change in the rust rules, I expect this will need a major version bump, since I don’t think the current rust rules are marked experimental. This isn’t a problem, but it won’t land in the 4.2.0 release currently “in progress”.

I’ve not heard of that crates_universe before, but I’ll take a look. As long as it doesn’t make things more complex for end users, I don’t mind which we use.

Let me know when that blocking issue is resolved and we’ll get this into dev branch where CI is less broken.

[bpalermo]

Chiming in, should we support crates_repository instead as suggested here?
https://bazelbuild.github.io/rules_rust/crate_universe.html#direct-packages

[titanous]

I haven’t written the docs up yet, but the basic flow is going to be that the consumer of these rules will usually define the cargo dependencies in their workspace (just like with the JavaScript rules). I need to do a bit of testing, I think we can make it work with any configuration of cargo-raze or variant of crates_universe.

[UebelAndre]

Chiming in, should we support crates_repository instead as suggested here? https://bazelbuild.github.io/rules_rust/crate_universe.html#direct-packages

As of right now there's not really any functionality for consuming dependencies from crates_repository externally. I think the use crates_vendor here is going to be familiar to more folks so think this approach is fine for now.

I opened bazelbuild/rules_rust#1423 to maybe make the crates_vendor experience a bit better. Though it's just a quick though I had based on your comment.

[ncs-pl]

Sorry to write this here but issues are not available on your fork...
Bazel cannot build packages that use rust_tonic_grpc_library or rust_prost_proto_library and raises the following error:

ERROR: no such package '@crate_index//': The repository '@crate_index' could not be resolved: Repository '@crate_index' is not defined and referenced by '<target>'

From my understanding, this error is raised because you are directly creating a rust_library with deps from @crate_index (which is an internal repository)

[titanous]

Yeah, I need to finish up the docs, you need to define the dependencies in your workspace so that either a @crate_index repository is created with crates_universe or override the dependencies using the relevant attributes for the rules.

[UebelAndre]

You might want to set repository_name To something unique on crates_vendor So there aren’t conflicts for users who use the default name

[titanous]

@UebelAndre will that work if consumers use tonic/prost from another repo within their own workspace in binaries that depend on proto libraries generated with these rules? I don't know how Rust handles including the same crates (with potentially differing versions) from multiple sources.

[titanous]

This is now blocked on neoeinstein/protoc-gen-prost#17 getting released.

[UebelAndre]

I haven’t looked to closely, but the general pattern used in rules_rust is there’s a toolchain which provides all the necessary info for rules. The repo provides some toolchains by default that users can instantiate off they want. Otherwise, they can create their own toolchain to use their own dependencies. So if that’s how this is setup then the renamed set of dependencies should be just fine. It’s also recommended so the rules here don’t accidentally block the use of other dependencies needed in a workspace but not defined here.

[titanous]

Thanks, can you point me to an example of such a toolchain definition in rules_rust? (specifically for a rule that depends on specific crates, ideally)

[UebelAndre]

Maybe take a look at @rules_rust//wasm_bindgen.

[UebelAndre]

This is now blocked on neoeinstein/protoc-gen-prost#17 getting released.

I opened neoeinstein/protoc-gen-prost#18 but think you'll just have to depend on that project via a git commit and not a published crate.

[realtimetodie]

Thank you @titanous for your work. The introduction of Prost as the default backend works for simple use cases.

There are some Protocol Buffer features that do not work. Looking at the way how Prost works, I'm not sure, whether it will be possible to implement these features without some major overhaul.

• Protobuf package specifiers are not supported

  foo.proto

  syntax = "proto3";
  
  package foo.bar.baz;
  
  ...

  This will lead to an Bazel build error. Bazel expects a new file with the name foo.rs, but a new file with the name foo.bar.baz.rs is created instead.

• Imports are not supported

  rust_prost_proto_library(
      name = "foo_proto_library",
      protos = [":foo_proto"],
      deps = ["//proto:bar_proto_library"],
  )

[BnMcG]

Hello - is there anything blocking being able to merge this?

[aaliddell]

I've not tested it yet, since the PR is still marked as draft and the comment in the first post. Are there any outstanding items that need completing?

[bpalermo]

@titanous any updates?

[titanous]

I have not had time to address the comment from @realtimetodie or to make the dependency management work sustainably. It works fine for simple use cases though.

[realtimetodie]

I'll publish an example repository over the weekend. I just need to get my ass up.

[realtimetodie]

it took me five beers and here it is. originally, I posted #202 (comment) and pointed out some limitations. my thing has nothing to do with rules_proto_grpc or @titanous's PR. it is straightforward.

https://github.com/realtimetodie/bazel-rust-grpc-aperture

write me an email if u have questions.

[titanous]

This is ready for initial review. It now supports building a crate from multiple named proto packages which resolves the issues mentioned in #202 (comment). There are probably still a few rough edges, and a bit of documentation is needed about how to make the necessary prost/tonic dependency targets.

The expectation is that the rule will be used to build a single rust_library for an entire hierarchy of protobuf packages (that may import each other) instead of the standard Bazel approach of building small targets for each proto file. This is due to a few issues:

1. protoc-gen-prost/protoc-gen-tonic are designed to emit one file each per protobuf package (not per .proto file), and adding support for a per-file mode would be a pretty invasive change, especially given that this mode works fine for non-Bazel use cases.
2. rules_rust does not provide a way to build a crate composed of multiple rust_librarys and the crate namespace is flat, so using the smaller per-proto approach results in polluting the top-level crate namespace where every .proto file ends up as a crate in the top namespace and there's no way to build an ergonomic module hierarchy that matches the protobuf packages piecemeal.
3. There is no equivalent to the java_package/go_package/etc file options, so automatically mapping imports to crate names requires a bunch of implementation work to add a rust_crate file options extension and even then we would still have the limitations discussed above.

[echochamber]

@titanous thanks for all your time and energy writing this!
Whats the current status on this PR?
Is it just lacking review or are there other outstanding blockers?
If there are any additional outstanding blockers and you are currently lacking either the time or desire resolve them, would you be ok with me picking it up from here and seeing if I can get it over the finish line?

[titanous]

Please go ahead and take over!

The main thing outstanding is implementing a good solution for the dependencies and documenting it.

I also received no feedback from the maintainers about the approach and ended up iterating a bit more in my monorepo (I can post an export of that if you'd like). I do not currently have the bandwidth to continue work on this PR.

[echochamber]

An export would be wonderful!

As far as dependencies go, I have a macro solution I implemented for my own workspace that I was working on turning into a ruleset when I stumbled upon your PR. I am thinking about applying my dependency solution to your PR, but would appreciate your feedback the approach if you don't mind (or anybody else who also has feedback 😄 )

Declaring a prost_library and tonic_library in my current solution looks like so:

• Full example here.
• My macros

proto_library(
    name = "some_proto",
    srcs = ["some.proto"],
    deps = ["@go_googleapis//google/api:annotations_proto"],
)

prost_library(
    name = "some_prost_rs",
    externs = {
        ".google.api": "::googleapi_rs::google::api",
    },
    protos = [
        ":some_proto",
    ],
    deps = [
        "//rust/proto/common:googleapi_rs",
    ],
)

tonic_library(
    name = "some_tonic_rs",
    prost_proto = ":some_prost_rs",
    proto_package = "path.to.my.proto.v1",
    proto = ":some_proto",
)

It allows you to compile each proto_library target separately, which is nice. Then your prost_library depends upon each prost library that wraps the respective proto_library your proto_library depends upon. The downside is this approach also requires us to manually specify the externs_path mapping for each proto package provided in any prost_library target in our deps=[...] (direct deps only).

I think that a small ergonomics improvement could be made via making prost_library a rule instead of a macro, removing the need to provide the externs mapping but would require each prost_library to declare the proto packages of the protos they wrap like so: declare_proto_packages=["adep.package"] (only the protos from proto_library.srcs would need to be declared, not deps/transitive deps). This would work like so:

• Create a new ProstProtoProvider type that adds the declare_proto_packages attribute.
• In the prost_library rule, use the provider to get the declare_proto_packages and name/crate_name to generate the [externs_path=.adep.declared.package=::adep_crate_name::adep::declared.package, extern_path=...] (only for direct deps, not needed for transitive deps).

[titanous]

Here's the export: https://github.com/titanous/rules_proto_grpc_rust

I think your approach is interesting, and might be reasonably ergonomic with a gazelle plugin to the annoying bits automatically...

[echochamber]

@titanous Sounds good and thank you

@aaliddell This will be my first time writing custom bazel rules. Is it ok for me to have a custom rule_impl function that wraps calling proto_compile_impl or proto_compile, so instead of having my rule do implementation = proto_compile_impl, it would be implementation = prost_proto_compile_impl,. I only ask because I wasn't able to find any examples of this being done elsewhere in the repo. See #202 (comment) for why I want to do this.

[aaliddell]

I've been really lacking in my tracking of this work, sorry @titanous & @echochamber

Am I right in saying that conceptually the core of this PR is good (compiles & links etc) but the remaining aspect is how to make it pleasant to work with? I'm going to have to do a bit of re-reading on Rust crates structuring, since the last time I looked at them was with the current rust rules... 🙄

@aaliddell This will be my first time writing custom bazel rules. Is it ok for me to have a custom rule_impl function that wraps calling proto_compile_impl or proto_compile, so instead of having my rule do implementation = proto_compile_impl, it would be implementation = prost_proto_compile_impl,. I only ask because I wasn't able to find any examples of this being done elsewhere in the repo. See #202 (comment) for why I want to do this.

Yep, the proto_compile function is setup to allow you to do this, as doc_template_compile uses this.

[aaliddell]

Closing since #265 merged. Thanks for all the work and once again sorry for the long response times