Tonic/Prost Rules - Take 2

[purkhusid]

This is an continuation of #479.

The plan is to create a completely separate ruleset for Tonic/Prost that can be loaded by including a separate repository.

[purkhusid]

I'm going to pick this up again soon but the biggest blocker that I can't really figure out is how to make the prost/tonic dependencies controllable by the user of the rules.

@dfreese @illicitonion @UebelAndre Do you have any ides of a good approach regarding that?

[UebelAndre]

I'm going to pick this up again soon but the biggest blocker that I can't really figure out is how to make the prost/tonic dependencies controllable by the user of the rules.

@dfreese @illicitonion @UebelAndre Do you have any ides of a good approach regarding that?

Toolchains can probably be used to solve for this. I recently did #678 to address a similar usability concern with the existing proto rules. Maybe that helps? But I'd love to see this get in so thanks for pinging this! 😄

[purkhusid]

@UebelAndre The problem with toolchains is that currently there is also an macro that is being used here

rules_rust/proto/prostgen/prostgen.bzl

Line 253 in cc60173

proc_macro_deps = PROST_COMPILE_PROC_MACRO_DEPS,

This macro is dependent on prost/tonic. I could of course change it so that the user has to supply prost/tonic themselves as deps to the macro. The usability suffers a bit though.

Another issue is that the generator rule depends on this binary which depends on prost/tonic as well:

rules_rust/proto/prostgen/BUILD

Line 10 in cc60173

name = "prostgen_lib",

I guess since the binary is only used for generating the .rs files and not really a dependency that the user has to think about we could just have a separate internal version of prost/tonic that is used for the code generation? The usability also suffers a bit since this can lead to users using a versoin of prost/tonic that can't compile the generated sources.

[UebelAndre]

@UebelAndre The problem with toolchains is that currently there is also an macro that is being used here https://github.com/bazelbuild/rules_rust/pull/595/files#diff-267766b8ea5135dae235eaf034cc1df928c87583e2cec74f1567c1770720d02dR253

This doesn't seem link to a file. Could you maybe link something directly from your branch so I know for sure what you're referring to?

I think there's a bad patter where the rules are designed to use a set of dependencies gathered outside the set of dependencies users actually want to build with. This is generally fine for tools (binaries that are run but otherwise don't share dependencies with your targets) but in cases where a rule produces dependencies, we need to probably have the initial expectation be that users provide their own dependencies. I'm not sure who would have the best insight on something like this. I think you pinged an appropriate group though. I'll take some time this week to do a deeper dive into the rules and see what suggestions I can come up with.

[purkhusid]

Also unrelated to the dependencies issue, how do we feel about using crate_universe internally in rules_rust? Would love to get rid of all the generated files since they make the PR hard to review. @UebelAndre @illicitonion

[purkhusid]

@UebelAndre Sorry, I updated the links.

[UebelAndre]

@UebelAndre Sorry, I updated the links.

Ah yeah, I feel like this is a bad pattern and isn't very maintainable (not specific to anything in these rules, a general pattern of handling dependencies). Maybe instead of using a macro we should be using a rule? @dfreese might have more context here (being the original author) on how this came to be.

[dfreese]

The context as to why it was a macro, rather than a rule was I was building outside of rules_rust, so I was trying to avoid depending on internals, such as //rust/private.

A rule would make a lot more sense here, and would probably allow the extern attribute/macro parameter to be removed and consolidated into deps, but that could be a fair bit of work, at least initially.

[titanous]

I'm building an app that has a frontend written in Rust and compiled to wasm, and it has a tonic/prost gRPC client in the wasm frontend.

This poses a challenge, because tonic and tonic-build need to be compiled without the transport feature for the wasm part, but with it for the server code. Additionally, we need a version of prostgen built against each version of tonic.

To solve this I did the following:

1. Copied the prostgen implementation and rules from this PR to my workspace.
2. Added a new cargo_wasm cargo-raze repository that has tonic/tonic-build without the transport feature in addition to the existing cargo-raze repository that has them with the feature enabled.
3. Updated the *_DEPS constants to have a select based on the platform pointing at cargo_wasm for the wasm platform, and the regular cargo deps for all other platforms.
4. Added a prostgen_toolchain and used it instead of _generator in the rules.
5. Added new prostgen_wasm* rust_binary/rust_library rules with deps pointing at cargo_wasm.
6. Added toolchain definitions for prostgen_wasm with target_compatible_with set to the wasm32 constraint.

These changes mean that all rust_wasm_bindgen rules that transitively depend on any targets build with prostgen will now build properly without the transport feature, while targets for other platforms will have the feature.

Hopefully this is helpful while you continue work on this PR, let me know if I can provide more detail that would be helpful.

[purkhusid]

I unfortunately won't have time to finish this in the foreseeable future, if anybody wants to pick this up they feel free to do so.

[rdelfin]

@purkhusid can you give a summary of what things are missing to get this up to speed and working?

[purkhusid]

@rdelfin The main blocker is that the prost/tonic dependencies can not be overriden by the user of the rules. This can lead to hard to debug behaviour in your application if you might for example have an existing dependency that depends on a different version of prost/tonic than these rules use by default.

The rules would have to be migrated to use toolchains in the places where the dependencies can easily be replaced and then the macros would probably have to be replaced with a proper custom rule so that we can use the dependencies that are provided by the toolchains.

[rdelfin]

Talked to a coworker that knows a lot more about Bazel than myself and basically there's a couple options for where this can go:

• Accept a fixed version of the libraries used for the bindgen binary
• Move the prostgen BUILD file into a templated file, and generate it with the specified dependencies using a repository rule
• Move the prostgen binary into a separate library/repo that can be replaced by the user of the library when setting up the repositories

With all these options we would still make the generated rust_library have the dependencies be configurable of course (as in, what is currently PROST_COMPILE_DEPS, PROST_COMPILE_PROC_MACRO_DEPS and TONIC_COMPILE_DEPS). Any strong preferences?

[UebelAndre]

* Move the prostgen binary into a separate library/repo that can be replaced by the user of the library when setting up the repositories

Does it need to be a separate repo/library? I feel like a toolchain would be the right choice here and would basically accomplish this. Plus, this is a pattern already in use for other rules (rust_wasm_bindgen, rust_bindgen, and rust_proto/grpc_library) where dependencies are wired up there and made available to the rule. Though, perhaps that doesn't work here since the core of the functionality is a macro that generates a couple targets. So really all I'm doing here is ranting about how I wish this was a rule 😅.

I think this leads me to favor option 2

* Move the prostgen `BUILD` file into a templated file, and generate it with the specified dependencies using a repository rule

Finally, I would definitely not even consider option 1, updating the version of these crates shouldn't require a PR to the repo. It's unnecessary churn for users and maintainers. But that's just my personal opinion.

[UebelAndre]

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!