Protobuf support #92
Protobuf support #92
Conversation
|
/cc @acmcarther @pcj: so now there is the choice. I am fine with pushing both or dropping whichever PR you prefer. |
|
Ok seems like I will need to test it on mac tomorrow. I should probably do so on rules_protobuf's PR too |
|
Apologies for the delay in looking at this. My opinion on this PR is much the same as for pubref/rules_protobuf#226 (review) . That is to say, I'm not happy to see something "official" using cargo-raze because that tool doesn't generate composable libraries. My aversion is primarily because I view the submission of either of these PRs as a commitment that I'd rather we not yet make. If I'm the only one that sees things that way, then I'm fine with either item being submitted. Truth be told, I'd really love for more folks to start thinking deeply about the dependency management problem though. |
|
No worries, I am happy to hold the PR until we can find a nice thing for the dependency issue. It however miss some work for macOS and I also start using it for my own project so I rather not start to use one solution whereas we will drop it and the other will be the recommended one. So my main concern about this PR and pubref/rules_protobuf#226 is which one I should standardize on? |
|
Blocked on bazelbuild/bazel#5650. Or on using a different approach (e.g. the rules_go approach) where we collect all the proto and compile them in the final rules. |
|
So TODO list for this PR:
|
|
Ok so I need to rebase again :(. I tested it on mac it works once the crate a regenerated. |
damienmg
force-pushed
the
protobuf
branch
2 times, most recently
from
66dccce
to
c846077
Compare
|
Ok we don't need multi-platform support apparently, just getting rid of the target flag is enough here. So I am just missing documentation about getting protobuf deps and it should be good for review |
This allow to runs the tests in examples as a remote repository and as a local repository.
With this craze, and cargo raze, we can simply use raze generated repositories to add the necessary dependencies.
Those library add protobuf / gRPC support to Rust language in Bazel using the native proto_library. It generates one crate per proto_library which is efficient for incrementality but can lead to naming collisions. We expose a bit of rust_library internal to re-use it in rust_proto_library and create a new provider "rust_libs" that will declare a group of rust libraries.
And a test with it.
This a workaround a bug in Bazel that prevent the toolchain from being loaded with the target transition (see bazelbuild/bazel#5650). This is not ideal because it recompiles several time the same proto if you depends on it from several dependency path and can lead to seeing two compilation of the same proto as different type. This is also terrible for incremental build and contrary to the (protobuf blog entry)[https://blog.bazel.build/2017/02/27/protocol-buffers.html].
The lazy_static crates now set a configuration from the build.rs, ideally we would be able to handle that but that is not the case with the current cargo raze method.
This new dependency is quite unfortunate, it make using rules_rust more complex since it needs to import a bzl file from skylib to be able to use rules_rust, that adds 10 lines in the WORKSPACE without much improvement. I guess this cannot be better until recursive workspace are a thing.
Also to keep the same lockfile, check in the Cargo.lock and have a lever to not re-run cargo generate-lockfile.
- Documents how to create your custom toolchain when you need to import
your own crates (or to use a different protobuf/grpc stubs compiler).
- Add extra_aliased_targets to simply the cargo-raze generation
- Use skylark http_archive to import protobuf.
- Regenerate crates with google/cargo-raze#77 so that we don't need to
have custom rewriting policy. Hence, deleted the wrapper around
cargo raze, an update to the build file should simply be
`cargo genrate-lockfiles && cargo raze` now.
|
@mfarrugi @acmcarther: this PR should be finally good for final review. It's quite long but you can ignore all files under proto/raze/remote as they are generated by |
|
@damienmg I am taking a look. |
There was a problem hiding this comment.
If there are .proto's that overlap, will they be compiled into two separate (identical?) crates? Will this cause problems?
Are we happy to happy with including raze files here? It adds a bit of noise.
I don't really mind it since there's not really an alternative, and it serves as a template for others' raze configurations.
(lmk if the suggested changes feature is more cumbersome than the equivalent 's|xxx|yyy|g' comments)
| load("@io_bazel_rules_rust//rust:repositories.bzl", "rust_repositories") | ||
| rust_repositories() | ||
|
|
||
| new_git_repository( |
There was a problem hiding this comment.
It's a little confusing that these are duplicated to compensate for the lack of recursive workspaces.. should we pull out an examples_dependencies.bzl?
There was a problem hiding this comment.
The duplicate is bazel_skylib below. Putting those in a separate bzl file won't work due to the way the workspace file is parsed (the workspace file is splitted into separate chunk starting with load statement and every repository called in each chunk needs to have been declared before).
We could do bunch of things to simply things here:
- Have rust_proto_respositories() load rust_repositories if they were not defined: this could lead to complex case where you want to use your own rust_toolchain but the default proto dependencies. I think this is not worth doin (that would remove 2 lines here).
- Have 2 bzl files for the examples: one that defines bazel_skylib and rules rust and a second one that does the rest, the workspace file would looks like:
load("//:examples-deps1.bzl", "deps1")
deps1()
load("//:examples-deps2.bzl", "deps2")
deps2()That would reduce code duplication but make the examples/WORKSPACE no longer a good example since you generally don't want to do so.
- Remove the deps on bazel_skylib, for example by vendoring it, so there is no need to load it in the WORKSPACE
I would be in favor of that last option but maybe to keep for a different PR?
There was a problem hiding this comment.
Definitely for a different PR.
I agree the examples should look as straight forward as possible, and this is pretty complicated no matter what.
proto/optional_output_wrapper.sh
Outdated
| # limitations under the License. | ||
|
|
||
| # A simple wrapper around a binary to ensure we always create some outputs | ||
| # Optional outputs are not available in Skylark :( |
There was a problem hiding this comment.
Is there an issue for this? Does this imply they are available in java rules?
There was a problem hiding this comment.
I don't think there is an issue for that but indeed in the native rules you can define "optional" output that might not be created, not in skylark.
This was a leftover from compiling the crates in the proto aspect.
Also move the optional_output_wrapper from the toolchain to the library itself. For some reason the runfiles where not shipped to the actions with the old way which wasn't important for Bash that did not had any runfiles but it is important for python.
There was a problem hiding this comment.
first pass, I think I might have left a couple of comments unaddressed (resolving comment is nice but Gthub still needs to work on the UI presentation).
If there are .proto's that overlap, will they be compiled into two separate (identical?) crates? Will this cause problems?
Indeed they will. The original design did not had this flaw but it is blocked by a bug in the toolchain, see bazelbuild/bazel#5650. Fixing it is pretty straightforward but I couldn't upstream the change, I gave up.
Are we happy to happy with including raze files here? It adds a bit of noise.
I don't really mind it since there's not really an alternative, and it serves as a template for others' raze configurations.
This is a separate issue, I tried to address it with #100 by creating a special crate_import rule but I got blocked by the lack of feature_flag in Skylark. I was told it might come at some point in the future but it needs to have configuration trimming.
(lmk if the suggested changes feature is more cumbersome than the equivalent 's|xxx|yyy|g' comments)
That's fine. It is nice to read. I end-up not selecting it because I don't want more trouble with CLA bot but applying it offline is fine too.
| load("@io_bazel_rules_rust//rust:repositories.bzl", "rust_repositories") | ||
| rust_repositories() | ||
|
|
||
| new_git_repository( |
There was a problem hiding this comment.
The duplicate is bazel_skylib below. Putting those in a separate bzl file won't work due to the way the workspace file is parsed (the workspace file is splitted into separate chunk starting with load statement and every repository called in each chunk needs to have been declared before).
We could do bunch of things to simply things here:
- Have rust_proto_respositories() load rust_repositories if they were not defined: this could lead to complex case where you want to use your own rust_toolchain but the default proto dependencies. I think this is not worth doin (that would remove 2 lines here).
- Have 2 bzl files for the examples: one that defines bazel_skylib and rules rust and a second one that does the rest, the workspace file would looks like:
load("//:examples-deps1.bzl", "deps1")
deps1()
load("//:examples-deps2.bzl", "deps2")
deps2()That would reduce code duplication but make the examples/WORKSPACE no longer a good example since you generally don't want to do so.
- Remove the deps on bazel_skylib, for example by vendoring it, so there is no need to load it in the WORKSPACE
I would be in favor of that last option but maybe to keep for a different PR?
proto/optional_output_wrapper.sh
Outdated
| # limitations under the License. | ||
|
|
||
| # A simple wrapper around a binary to ensure we always create some outputs | ||
| # Optional outputs are not available in Skylark :( |
There was a problem hiding this comment.
I don't think there is an issue for that but indeed in the native rules you can define "optional" output that might not be created, not in skylark.
There was a problem hiding this comment.
lgtm.
maybe give @acmcarther a chance to comment on raze / whatever else
There was a problem hiding this comment.
Thanks for your patience in giving me a chance to review.
I'm quite amazed with the approach taken here. It looks to me that it should be easy enough to override the prepackaged dependencies with local dependencies, and it seems you gave a lot of thought to documenting this process for users.
I'm probably missing something nit-worthy that will be more obvious when everything is merged in, but I can follow up with those in small PRs if I see them.
|
Thanks for taking the time to review it :) Thrilled to see this merged :) Since I still don't have write access perhaps someone can merge this one and my other PR? |
|
I think I see a documentation error. The examples have multiple references to |
@damienmg How does it blow up in this case? Presumably if your library transitively depends on two identical versions of a library, something blows up at compile time? |
|
Let say you have A -> B (b_rust) and A -> D (d_rust) and then you try to
use both in a single crate, then you would need to import b_rust::A to
provide A proto definition to B proto, and d_rust::A to provide A proto
definition to D proto.
This will results in type errors (different path). You can convert them
with serialization/deserialization but that's a pretty big overhead.
I don't think it will result in incomprehensible errors though.
…On Wed, Nov 14, 2018 at 5:25 PM Marco Farrugia ***@***.***> wrote:
If there are .proto's that overlap, will they be compiled into two
separate (identical?) crates? Will this cause problems?
Indeed they will. The original design did not had this flaw but it is
blocked by a bug in the toolchain, see bazelbuild/bazel#5650
<bazelbuild/bazel#5650>. Fixing it is pretty
straightforward but I couldn't upstream the change, I gave up.
@damienmg <https://github.com/damienmg> How does it blow up in this case?
Presumably if your library transitively depends on two identical versions
of a library, something blows up at compile time?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#92 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADjHfzPkd9tbLW4307Q8LIBlhpdzkbHNks5uvEQJgaJpZM4UZ1_h>
.
|
This PR adds rust_(proto|grpc)_library to compile protobuf stubs.
This one generates one crates per proto_library which is nice for incrementality but might leads to crate names collision.
It is a bit concurrent with pubref/rules_protobuf#226 and I am fine with dropping either PR (I just need one).