Rust support #226

damienmg · 2018-06-01T20:26:08Z

Add support for the rust language.

This also adds the support for output_to_libsubdir so we can create a crates directly. This start to have a lot of output_to_xxx in the proto_language and I wonder if that would not make sense to simply make that a enum like.

I will update this PR as soon as bazelbuild/rules_rust#90 is merged to point to upstream

damienmg · 2018-06-01T20:29:02Z

/cc @pcj @acmcarther @mhlopko

This is a preliminary work for Rust support. This the result of running cargo raze on the grpc-rust crate at the 0.4.0 version with protobuf crates removed (since we import them directly).

Now that bazelbuild/rules_rust#90 has been merged, we can point directly to the upstream repository.

damienmg · 2018-06-01T21:52:08Z

And now with tests

…vention here

damienmg · 2018-06-01T21:58:07Z

For some reason Travis fails in some python tests...

pcj · 2018-06-02T23:15:49Z

@damienmg This is great work.

Can you provide more context on why the generated cargo-raze files need to be checked in?
Can we document how to regenerate those files (did I miss it)?

Looking into the travis failure, not seeing why it would be related to this change. Otherwise should be able to get this merged soon.

With this craze, and cargo raze, we can simply use raze generated repositories to add the necessary dependencies.

damienmg · 2018-06-03T14:26:27Z

@pcj:

I think that would be great to simply have cargo_crarte rules that would generate those file but cargo raze use cargo cache to generate those file so I am not sure how easy it would be. /cc @acmcarther
Sorry there was none, I changed the format with some CL to make it easier to regenerate. Now all the deps are generated using cargo raze and the small shell wrapper simply rewrite the build file labels.

WDYT?

damienmg · 2018-06-03T14:27:12Z

(Also: if you wish I can try to rewrite the history of that branch to skip all the BUILD file CL and only have one)

pcj · 2018-06-03T18:39:09Z

No need, I'll just squash it when merging.

acmcarther

Thanks for this CL! Its a major step forward I think for the Rust-Bazel story.

Its going to take me a bit longer for a real rigorous review as I'm traveling this week. A couple of initial questions here, plus a more general statement:

I'm not convinced that cargo-raze is ready to play this kind of role in a library yet particularly because it doesn't have a good story w.r.t. platform management. In the current implementation, you've provided BUILD files for x86_64-unknown-linux-gnu. This will probably end poorly if someone tries to use these rules for another platform.

Your comments above gave me a pretty good idea of how to work toward a solution to this problem though and I'm going to be thinking about it and documenting that thinking in the cargo-raze repo this week.

acmcarther · 2018-06-01T22:01:18Z

README.md

+| [Objective-C](objc)                              | [objc_proto_compile](objc#objc_proto_compile)                                                                                                       | [objc_proto_library](objc#objc_proto_library) <sup>4</sup>                                                                        | [v1.6.1](https://github.com/grpc/grpc/commit/f5600e99be0fdcada4b3039c0f656a305264884a)                             |
+| [Python](python)                                 | [py_proto_compile](python#py_proto_compile)                                                                                                         | [py_proto_library](python#py_proto_library)                                                                                       | [v1.6.1](https://github.com/grpc/grpc/commit/f5600e99be0fdcada4b3039c0f656a305264884a)                             |
+| [Ruby](ruby)                                     | [ruby_proto_compile](ruby#ruby_proto_compile)                                                                                                       |                                                                                                                                   | [v1.6.1](https://github.com/grpc/grpc/commit/f5600e99be0fdcada4b3039c0f656a305264884a)                             |
+| [Rust](rust)                                     | [rust_proto_compile](rust#rust_proto_compile)                                                                                                       |                                                                                                                                   | [v1.6.1](https://github.com/grpc/grpc/commit/f5600e99be0fdcada4b3039c0f656a305264884a)                             |


Add rust_proto_library here as well?

acmcarther · 2018-06-04T01:36:09Z

rust/rules.bzl

+    "@org_pubref_rules_protobuf//rust/raze:protobuf",
+]
+
+GRPC_COMPILE_DEPS = PB_COMPILE_DEPS + [


I think these need to be a (default) parameter of the rust_proto_library rule. My thinking is that folks will want to centrally control the whole set of third party crates that they bring in, and maintaining compatibility with the dependencies that rust_proto_library uses internally might be important.

In the event that they don't care (yet), the default value will give them something functional.

EDIT: Added "internally" after "rust_proto_library uses"

Add a compile_deps argument, defaulted to None so people can overwrite it.

acmcarther · 2018-06-04T01:39:22Z

rust/rules.bzl

+    content.append("extern crate tls_api;")
+  for dep in ctx.attr.deps:
+    content.append("extern crate %s;" % dep.label.name)
+    content.append("use %s::*;" % dep.label.name)


Have you tested this with the protobuf file generator you're using? I was historically using stepancheg's rust proto library, and it uses crate-wide "allow" directives. These directives must appear before any use statement. To solve that, I just put the use statements after the content.

This may not be an issue for your particular proto library.

I haven't seen any issue in the examples I am testing on, but I moved the use statement at the end.

acmcarther · 2018-06-04T01:40:49Z

rust/rules.bzl

+      content.append("pub use %s_grpc::*;" % _basename(f))
+  ctx.actions.write(
+    ctx.outputs.out,
+    "\n".join(content),


As an extension of the above content, I had to do some postprocessing to the generated protobuf content -- specifically replacing super:: stuff. Is this not applicable for you?

Actually, as I look at this, I'm not seeing where the complied source even goes...

The generated source goes into the same folder as the lib.rs. I don't need to replace supper:: stuff because I maintain the same lib structure. I don't know though how it will behave with some multi component package in proto.

damienmg

I am fine with waiting for a better cargo-raze story to polish that CL. I would be especially happy to drop all those build file and replace them with only a bzl file that load cargo_crate rules :)

As a side-note, I have a prototype approach of a different implementation for rust Proto/gRPC support. There are native proto_library rules that expose a provider and you can use aspect to generate 1 crate per proto_library. This generate a lot of crates so I am not sure it is better but that increase incrementality around protobuf. It also integrate with native protobuf but the code is 3 times the size of this PR (Skylark equivalent of rules.bzl) mostly because @pcj made an awesome work at making it easy to add new language on rules_protobuf.

This new approach would live in the rules_rust repository in the same way that rules_go provide a go_proto_library and provide I believe a bit nicer experience for the end-user (use native proto_library) but it adds a lot of complexity.

I am up to whatever people prefer (check in both contrib, only rules_protobuf or only rules_rust), I just need one rust gRPC support for myself and incrementality is not a problem at the moment. Anyway I'll try to clean-up the prototype and share it as a work in progress so you can have a look at it.

damienmg · 2018-06-04T19:56:50Z

README.md

+| [Objective-C](objc)                              | [objc_proto_compile](objc#objc_proto_compile)                                                                                                       | [objc_proto_library](objc#objc_proto_library) <sup>4</sup>                                                                        | [v1.6.1](https://github.com/grpc/grpc/commit/f5600e99be0fdcada4b3039c0f656a305264884a)                             |
+| [Python](python)                                 | [py_proto_compile](python#py_proto_compile)                                                                                                         | [py_proto_library](python#py_proto_library)                                                                                       | [v1.6.1](https://github.com/grpc/grpc/commit/f5600e99be0fdcada4b3039c0f656a305264884a)                             |
+| [Ruby](ruby)                                     | [ruby_proto_compile](ruby#ruby_proto_compile)                                                                                                       |                                                                                                                                   | [v1.6.1](https://github.com/grpc/grpc/commit/f5600e99be0fdcada4b3039c0f656a305264884a)                             |
+| [Rust](rust)                                     | [rust_proto_compile](rust#rust_proto_compile)                                                                                                       |                                                                                                                                   | [v1.6.1](https://github.com/grpc/grpc/commit/f5600e99be0fdcada4b3039c0f656a305264884a)                             |


damienmg · 2018-06-04T20:08:10Z

rust/rules.bzl

+    content.append("extern crate tls_api;")
+  for dep in ctx.attr.deps:
+    content.append("extern crate %s;" % dep.label.name)
+    content.append("use %s::*;" % dep.label.name)


I haven't seen any issue in the examples I am testing on, but I moved the use statement at the end.

damienmg · 2018-06-04T20:13:33Z

rust/rules.bzl

+    "@org_pubref_rules_protobuf//rust/raze:protobuf",
+]
+
+GRPC_COMPILE_DEPS = PB_COMPILE_DEPS + [


Add a compile_deps argument, defaulted to None so people can overwrite it.

damienmg · 2018-06-04T20:15:31Z

rust/rules.bzl

+      content.append("pub use %s_grpc::*;" % _basename(f))
+  ctx.actions.write(
+    ctx.outputs.out,
+    "\n".join(content),


The generated source goes into the same folder as the lib.rs. I don't need to replace supper:: stuff because I maintain the same lib structure. I don't know though how it will behave with some multi component package in proto.

pcj · 2018-06-05T14:33:01Z

Obviously I'm a little biased towards rules_protobuf but am happy with either approach. My observations from rules_go was that after the initial commit of a go_proto_library rule in that repository, people celebrated, but it took a lot of further effort to then hash out all the corner cases and redesigns that we'd already had to work out here. So while I think the end product was good, it was a significant amount of duplicate effort.

damienmg · 2018-06-05T14:54:59Z

Yes I had to duplicate a lot of effort and used the rules_protobuf as an exemple. I think the end-user experience is better with the native proto_library approach but the developer experience is way better with rules_protobuf. Maybe that would be a way to look at it: transform rules_protobuf into something that works well with proto_library and offer the same kind of simple interface? Then rules author depends on it and just use rules_protobuf as a skylark library?

I wonder what are the plan for Bazel protobuf support now, maybe @lberki knows.

This give SHA-256 in crates.bzl improving caching

damienmg · 2018-06-11T18:43:07Z

/cc @mfarrugi since he expressed interest in the other PR.

I would love to get rules_rust's maintainer's opinion on which one we should pursue

mfarrugi · 2018-06-11T19:10:05Z

I don't have much to add for the developer perspective. From a user perspective, I slightly prefer having something that is wired up to the native rules because it is one less dependency, but is essentially the same ux otherwise.

damienmg · 2018-06-12T09:14:36Z

@mfarrugi there is a slight UX diff in the native approach you declare your proto_library and just depends on a leaf rust_{grpc,proto}_library that specialize all the depdenncies for rust. This is the same approach for native java rules and go rules.

In the rules_protobuf way, you declare a rust_proto_{library, compile} per proto library. This creates more library in the end if you are multi language but it is simpler if you are mono language.

mfarrugi · 2018-06-13T00:48:01Z

If I understand correctly it is not a large difference, though I expect the majority of bazel users are not going to be mono language.

damienmg · 2018-10-26T13:50:04Z

I'll close this one in favor of integrating inside the rules_rust repository

pcj · 2018-10-26T17:27:18Z

OK, I have also rewritten rules_proto in the meantime with a rust implementation based on yours at https://github.com/stackb/rules_proto/tree/master/rust. I'm OK maintaining those for the time being.

damienmg added 2 commits June 1, 2018 22:30

Add cargo-raze build files for grpc-rust crate

a2c0e84

This is a preliminary work for Rust support. This the result of running cargo raze on the grpc-rust crate at the 0.4.0 version with protobuf crates removed (since we import them directly).

Add Rust support

20d33f0

damienmg force-pushed the rust_support branch from ba34ca0 to 20d33f0 Compare June 1, 2018 20:30

damienmg added 2 commits June 1, 2018 23:46

Add an integration test for the Rust example

62e32fd

Point rules_rust to the upstream repository

aad76a2

Now that bazelbuild/rules_rust#90 has been merged, we can point directly to the upstream repository.

Move the test to examples/helloworld/rust as this seems to be the con…

421d7f6

…vention here

damienmg added 4 commits June 3, 2018 16:19

Add a dummy Cargo craze to use cargo raze on it.

21e7fbf

With this craze, and cargo raze, we can simply use raze generated repositories to add the necessary dependencies.

Add generate files generated with the new raze

8017e7c

Use the new generated files from cargo raze

6fad944

Remove generated files from raze in the old format

083230a

acmcarther reviewed Jun 4, 2018

View reviewed changes

Address acmcarther@ suggestions

f01c1fe

damienmg commented Jun 4, 2018

View reviewed changes

damienmg mentioned this pull request Jun 4, 2018

Protobuf support bazelbuild/rules_rust#92

Merged

Regenerate files with cargo-raze HEAD

61f52c2

This give SHA-256 in crates.bzl improving caching

acmcarther mentioned this pull request Jun 11, 2018

Produce a proposal for composable workspaces google/cargo-raze#52

Open

damienmg closed this Oct 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust support #226

Rust support #226

damienmg commented Jun 1, 2018

damienmg commented Jun 1, 2018

damienmg commented Jun 1, 2018

damienmg commented Jun 1, 2018

pcj commented Jun 2, 2018

damienmg commented Jun 3, 2018

damienmg commented Jun 3, 2018

pcj commented Jun 3, 2018

acmcarther left a comment

acmcarther Jun 1, 2018

damienmg Jun 4, 2018

acmcarther Jun 4, 2018 •

edited

Loading

damienmg Jun 4, 2018

acmcarther Jun 4, 2018

damienmg Jun 4, 2018

acmcarther Jun 4, 2018

damienmg Jun 4, 2018

damienmg left a comment

damienmg Jun 4, 2018

damienmg Jun 4, 2018

damienmg Jun 4, 2018

damienmg Jun 4, 2018

pcj commented Jun 5, 2018

damienmg commented Jun 5, 2018

damienmg commented Jun 11, 2018

mfarrugi commented Jun 11, 2018

damienmg commented Jun 12, 2018

mfarrugi commented Jun 13, 2018 •

edited

Loading

damienmg commented Oct 26, 2018

pcj commented Oct 26, 2018

Rust support #226

Rust support #226

Conversation

damienmg commented Jun 1, 2018

damienmg commented Jun 1, 2018

damienmg commented Jun 1, 2018

damienmg commented Jun 1, 2018

pcj commented Jun 2, 2018

damienmg commented Jun 3, 2018

damienmg commented Jun 3, 2018

pcj commented Jun 3, 2018

acmcarther left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acmcarther Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damienmg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcj commented Jun 5, 2018

damienmg commented Jun 5, 2018

damienmg commented Jun 11, 2018

mfarrugi commented Jun 11, 2018

damienmg commented Jun 12, 2018

mfarrugi commented Jun 13, 2018 • edited Loading

damienmg commented Oct 26, 2018

pcj commented Oct 26, 2018

acmcarther Jun 4, 2018 •

edited

Loading

mfarrugi commented Jun 13, 2018 •

edited

Loading