Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rust bazel rules #27

Closed
SirVer opened this issue Feb 9, 2018 · 36 comments · Fixed by google/cargo-raze#13
Closed

Rust bazel rules #27

SirVer opened this issue Feb 9, 2018 · 36 comments · Fixed by google/cargo-raze#13

Comments

@SirVer
Copy link

SirVer commented Feb 9, 2018

@acmcarther Sorry, this is not really an issue, but I did not know how to reach you otherwise.

My company wants to explore rust, but one of our requirements is that we build everything in bazel. We have existing rust code, but had little success building it in bazel, mostly due to the many third_party dependencies that rust requires.

Can you give me a bit of a status on bazel + rust? What is working? what is not working? who is investing?

Also, who is maintaining the https://github.com/bazelbuild/rules_rust repo? It seems pretty dead at this point in time.

@SirVer
Copy link
Author

SirVer commented Feb 9, 2018

/cc @thomasschiwietz @pdubroy

@acmcarther
Copy link
Owner

Feel free to reach me directly at my handle at gmail.com.

Rules Rust

https://github.com/bazelbuild/rules_rust was historically maintained entirely by David Chen (dzc) with minor contributions from the Bazel team. Since then, it has picked up a couple of new maintainers (including me) though I don't have strong communications yet with the other maintainers. Its currently entirely a 20% project.

I have several patches that I haven't yet upstreamed that are necessary to effectively build third party crates (which I'm really motivated to get upstreamed if there is external interest in this tool).

Cargo Raze

This project is a separate item that I maintain in a personal capacity (though it will be moving into github.com/google quite soon). I'm highly motivated to add features for production use cases though, and willing to break down the design decisions item-by-item for folks that want to go a different way without falling into the same pits I fell into.

It currently is geared toward a workflow where your organization decides together what dependencies to bring in, and all teams use the same versions of things. Upgrades are intended to be performed across your org. In other words, this is a bit of a one way street. If you intend to open source something, you'll need to port it and its dependencies back across the boundary and publish it as a normal crate.

I'd estimate that the distribution of work you need to do for individual crates works out something like the following:

Percentage Crates Overhead Notes
80% No work at all Builds out of the box, or with just a gen_buildrs directive
10% Minor tweaks or flags User needs to inspect build.rs and manually synthesize flags for the crate
10% Finding and replacing deps, locating flags User needs to provide (vendored/system) deps to replace build.rs generated ones, identify flags

Some things that I really want that aren't working:

  • Dynamic selection of target platform: This is currently set in the Cargo.toml, so your generated BUILD files for your deps will be specific to a single platform. Bazel is getting features that should allow more flexibility in this area and I'm looking into it.
  • Building tests for external deps: I really want to be able to test the things I bring in, but I can't because test dependencies aren't resolved by Cargo's dependency resolver.

Some things that are working that you might be surprised about

  • Build scripts are supported in a limited fashion. We can't interpret stdout printed stuff, such as flag directives or linker instructions, but we can repackage generated source files and make them available to the parent library.
  • Crate features are supported as usual. They work identically to Cargo in that they're additive, and enable dependencies.
  • Vendoring is optional. You can pull crate sources into your org third_party directory, or you can generate new_http_repository rules and only generate the BUILD files.

Here is a real example of my personal project monorepo's Cargo.toml. This file configures external crate resolution, and the cargo-raze tool.

There are a bunch of example repos linked at the top of the README.md in this repo as well.

@SirVer
Copy link
Author

SirVer commented Feb 9, 2018

Feel free to reach me directly at ...

Let's keep this in the open for the wider audience.

I have several patches that I haven't yet upstreamed that are necessary to effectively build third party crates (which I'm really motivated to get upstreamed if there is external interest in this tool).

Please do! Having something that autogenerates BUILD files for bazel would be very nice. Also nice would be something like glaze, but for rust. One day I guess :)

This project is a separate item that I maintain in a personal capacity (though it will be moving into github.com/google quite soon).

If you narrow the scope to only supporting bazel initially you might be able to move this into https://github.com/bazelbuild. I think that is where it belongs instead of being burrowed under all
the other googleorg open source projects. @damienmg @ulfjack Bazelteam, what do you think of having a rust BUILD file generator for external dependencies maintained by a googler under the bazelbuild org?

[Monorepo explanation]

I just recently left Google (for epitaph, my ldap was hrapp), so I am well aware of the design of google3/third_party. I was also working out of the Munich office, so I know quite a few of the bazel team.

Lyft also has a mono repo going and right now we only build the c++ stuff with bazel - and we want to do exactly what you describe: one version for all code. Right now, the rust stuff is still build with cargo, but we want to make building consistent. I am interested in pushing on the rust stuff - but I do not even know where to start. The rust_rules are very bare bone and pulling in all our dependencies and writing bazel files for them is frightening. I really like the idea of generating new_http_repository easily for them

Dynamic selection of target platform:

No priority for us right now - linux x86_64 all the way!

Building tests for external deps: I really want to be able to test the things I bring in, but I can't because test dependencies aren't resolved by Cargo's dependency resolver.

Again, not a priority for us.

Some things that are working that you might be surprised about
Build scripts are supported in a limited fashion. We can't interpret stdout printed stuff, such as flag directives or linker instructions, but we can repackage generated source files and make them available to the parent library.

I am surprised by that. That sounds awesome! Does this mean proto support is not terribly difficult to add? We are also using grpc + proto3 in rust - I saw in your Cargo.toml that you seem to have that covered already. How does that work?

Crate features are supported as usual. They work identically to Cargo in that they're additive, and enable dependencies.

That is also surprising and nice!

Vendoring is optional. You can pull crate sources into your org third_party directory, or you can generate new_http_repository rules and only generate the BUILD files.

We probably mostly want to go the new_http_repository route, vendoring is a last resort.

We are currently sprinting till the end of q1, after that I want to revisit this, but I would be more than happy to run a few experiments next week already. For context, my main dependency is https://github.com/googlecartographer/point_cloud_viewer. That repo contains crates with few and easy dependencies (quadree, octree) and ones with many and crazy ones (web_viewer pulls in iron which pulls in openssl and other fun stuff). The crates are also depending on each other via path dependencies. How do you deal with that?

Making progress on getting some of this build would be awesome for me and any guidance on how to start would be appreciated. If you would need to get this into your monorepo to build, how would you approach it?

@acmcarther
Copy link
Owner

acmcarther commented Feb 9, 2018

If you narrow the scope to only supporting bazel initially you might be able to move this into https://github.com/bazelbuild.

I'm quite literally a single button push away from the google org repo. Let me look into changing the destination repo... I'll probably need to get new approvals 😛

I am surprised by that. That sounds awesome! Does this mean proto support is not terribly difficult to add? We are also using grpc + proto3 in rust - I saw in your Cargo.toml that you seem to have that covered already. How does that work?

Yes, I have proto support already. It was the original motivation behind working on this project for me. Support requires a patch to the protobuf library to fix the generated output directory rust-protobuf issue here, and a slightly elaborate genrule dance to convert singular rust source files into linkable files. Sample build file. The grpc stuff (especially tokio) is very new to me but it appears to be working. It requires an identical patch to the rust-protobuf library and similar packaging shenanegans. If you're already familiar with grpcio, the module structure looks like this

// Proto
use interconnect::interconnect::FindServerRequest;
use interconnect::interconnect::FindServerResponse_RejectedConnection;
// Grpc
use interconnect::interconnect_grpc::GatewayServiceClient;

(although the module structure looks redundant, it actually facilitates having multiple proto files in a single packaged crate)

FWIW, I also support (and heavily use) custom bindgen deps as well:

For context, my main dependency is https://github.com/googlecartographer/point_cloud_viewer.

The primary challenge of supporting this particular dependency isn't actually its transitive deps (as I've already figured out how to handle the most challenging one -- openssl), its that it isn't published to crates io and thus cargo-raze would need to know how to subdivide the crates by their dependencies on its own. Can it be published to crates io?

I can elaborate on a brute force method if publishing is off the table, but it won't be pleasant. Think:

  1. new_git_repository
  2. a hand written BUILD file with rust_library for each subcrate
  3. Integrating its crate deps into your workspace's deps so that you can access them in the hand written build file.

Longer term, I will need to support unpublished deps like this one, but I need to think about it a little more. May not be as complicated as it first appears as this would just be a git dependency which Cargo already knows how to resolve.

@SirVer
Copy link
Author

SirVer commented Feb 13, 2018

@acmcarther I took some time to play around with raze and I am seriously impressed. It did not work out completely, but I can see how the workflow will work and it seems like it is nearly working - great work! I filed a few bugs against this repo for more help.

My first goal is to make simple rust stuff compile, next integrate support for protobuf, then grpc-rs. So still lots of ways to go, since even the first step did not quite work out. I would appreciate any guidance.

Answers inlined:

Can it be published to crates io?

It could, but it is not too much use for others right now. It is mirrored into our mono repo since we need to patch the protobufs already. I am maintaining a set of BUILD rules already in our repo for this, so it is fine to assume that this is the repo I want to build bazel rules for, i.e. this is my source, not my dependency.

@SirVer
Copy link
Author

SirVer commented Feb 13, 2018

@acmcarther Could you make the code of this file load("//tools/bazel-ext/proto:rust.bzl", "rust_proto_library") available anywhere? Ideally in the example repo you are maintaining (which is excellent, btw :)).

@acmcarther
Copy link
Owner

#27 (comment)

Doing this now, will update when done.

As a primer, GRPC support currently requires a bunch more overhead than it should, as the grpcio doesn't yet support the latest GRPC. This is primarily because the GRPC headers stopped being separated by C and C++, which made bindgen slightly more annoying (tikv/grpc-rs#110). The latest version of GRPC supplies a dep fetching function, but we're stuck with a version prior to that.

@acmcarther
Copy link
Owner

Alright. I have a branched version of the cargo-raze-examples repo that contains a GRPC + protobuf using example:

@SirVer: https://github.com/acmcarther/cargo-raze-examples/tree/protobuf-example/bazel/using_protobuf

Its really really unpolished but might be useful for you to get off the ground. I'll polish it up and get it merged with master after I get some of the outstanding bugs fixed up.

Please file issues if anything doesnt work right. I was able to compile the proto and rust_library that depended on it locally.

@SirVer
Copy link
Author

SirVer commented Feb 14, 2018

🎉 @acmcarther Awesome, thank you. I will dig into this and try to make heads and tails of it and report back.

I really appreciate the hard work you are putting in here!

Update: @acmcarther Is //tools/bazel-ext/proto:rust.bzl checked-in in that example? bazel/tools seems remarkably empty.

@acmcarther
Copy link
Owner

Got bit by the gitignore. Fixed in latest commit acmcarther/cargo-raze-examples@2bd5f18

Happy to help -- the more people there are looking at this, the better the tools get for everyone.

@mfarrugi
Copy link
Contributor

Out of curiosity, why use rules_proto over the native rules? (I am not familiar with either)

@acmcarther
Copy link
Owner

No reason. I implemented this before the native rules were available.

I think it should be reasonable to switch (docs here: https://docs.bazel.build/versions/master/be/protocol-buffer.html#proto_library). I'm not sure how to integrate GRPC with it though.

@mfarrugi
Copy link
Contributor

mfarrugi commented Feb 15, 2018 via email

@SirVer
Copy link
Author

SirVer commented Feb 21, 2018

@mfarrugi At least at lyft we also use the pubref rules at the moment. They seem to be the agreed upon standard atm.

@acmcarther I have partial success to report and a bug report! I could replicate building a proto file and using it in a real world example. I did not try gRPC yet. I encountered a bug during that:

rust_proto_library(
    name = "proto_proto",
    protos = ["src/proto.proto"],
    with_grpc = False,
)

This does not work - bazel is reporting that the output proto was never created. The build file needs to sit right next to the .proto file (i.e. in the src/ directory), then it works nicely.

@SirVer
Copy link
Author

SirVer commented Feb 21, 2018

@acmcarther Another issue that I cannot figure out is sdl2 - everything builds fine, but my rust binary never links all dependencies. This is how I define the sdl2 dependency (on OS X):

cc_library(
    name = "sdl2",
    srcs = glob(["lib/libSDL2.dylib"]),
    hdrs = glob(["include/SDL2/*.h"]),
    linkopts = [
        "-lm -liconv -Wl,-framework,CoreAudio -Wl,-framework,AudioToolbox -Wl,-framework,ForceFeedback -lobjc -Wl,-framework,CoreVideo -Wl,-framework,Cocoa -Wl,-framework,Carbon -Wl,-framework,IOKit"
    ],
    defines = ["_THREAD_SAFE"],
    includes = ["include/SDL2"],
    visibility = ["//visibility:public"],
)

Building my rust sdl_library results in these linking errors. While building a c++ binary that uses SDL2 works fine. My hunch is that linkopts is not passed through or taken into account by the rust rules?

@acmcarther
Copy link
Owner

I'll need to take a more serious look later at what you have to make heads or tails of it. That said, I'm successfully compiling SDL2 on my own project... though I'm doing it statically (against the hopes and dream of everyone apparently). If you can stomach that, here is my build file:

https://gist.github.com/acmcarther/c3bcb8655485ebbe33cdc22a7bfb79bd

and SDL_config_linux.h, which I had to generate manually for some reason https://gist.github.com/acmcarther/2dd1df48aa377d8ea5140e26b41acc02

@acmcarther
Copy link
Owner

Its possible that directly linking to dylibs doesn't work on OSX as well. bazelbuild/rules_rust#61 appears to fix it for linux.

@mfarrugi
Copy link
Contributor

mfarrugi commented Feb 21, 2018 via email

@SirVer
Copy link
Author

SirVer commented Feb 22, 2018

What is the holdup on these two merge requests?

@acmcarther
Copy link
Owner

acmcarther commented Feb 22, 2018

bazelbuild/rules_rust#38 is blocked on an actual problem, detailed at bazelbuild/rules_rust#38 (comment). Basically, I don't currently know a mechanism to have the toolchain help define the output file extension (rustc unilaterally picks extension based on the target). Could use some eyes on if you have the Bazel chops. Else I can throw a couple of hours at it tomorrow and see if I get something that at least passes the tests.

I believe bazelbuild/rules_rust#61 is just blocked on me taking a good look at it, and on the prior PR getting merged.

@SirVer
Copy link
Author

SirVer commented Feb 23, 2018

@acmcarther I am back on making the bazel build work here at Lyft. Now I run into a problem that I wonder if you had too: the error chain quickstart example is failing to build with bazel. The problem is libbacktrace.

I tried building it separately with blaze by downloading it from https://github.com/alexcrichton/backtrace-rs/tree/master/backtrace-sys/src/libbacktrace, crafting rules around it and linking it in:

[raze.crates.backtrace-sys.'0.1.16']
additional_deps = [
  "@//third_party/libbacktrace",
]

Unfortunately, my linking errors did not go away with this - I figured out that that libbacktrace-sys actually redefines the symbols here: https://github.com/alexcrichton/backtrace-rs/blob/master/backtrace-sys/build.rs#L135

Have you managed to make this work?

@acmcarther
Copy link
Owner

You should be able to compiile the backtrace and backtrace-sys crates without the system library at all -- this is what I'm doing locally. Backtrace-sys isn't actually expected to work on OSX anyway.
https://github.com/alexcrichton/backtrace-rs/blob/master/Cargo.toml#L73. I'm sure the functionality will be diminished in its absence though.

I'm seriously at a loss w.r.t. the symbol stuff in that build script.

@SirVer
Copy link
Author

SirVer commented Feb 26, 2018

I figured it out - basically the build script changes the symbol names of the backtrace library so that it does not clash with the one that is in the stdlibrary. So I just search & replaced the symbol names in the source code before building it with blaze and that seems to have done the trick.

@SirVer
Copy link
Author

SirVer commented Mar 2, 2018

@acmcarther sorta weekly update: I made a ton of progress with building our rust stuff. I still punted on grpc for now and https://github.com/rusoto/rusoto gives me a lot of trouble since they expect env!() to work for a few things...

Otherwise my main problem right now is that I have to manually tweak four of the raze generated build files - and since they always get overwritten I do a lot of git checkout -f. A diff between HEAD and after running cargo raze in our repo is in this gist. I expect this to get worse as I wrangle with rusoto.

  1. 2 of the 4 have additional data dependencies that need to be kept up to date.
  2. sdl2-sys expects the HOST variable to be set and since we do not cross compile, it is easy enough to provide.
  3. image seems like a cargo raze bug. It generates crate_root = "./src/lib.rs" which is rejected on build, since it starts with ./.

Can you advise on how these deviations can be handled a bit more ergonomically?

Edit: Use numbers instead of bullets.

@acmcarther
Copy link
Owner

acmcarther commented Mar 3, 2018

2 of the 4 have additional data dependencies that need to be kept up to date.

mfarrugi and I have been discussing this here: google/cargo-raze#6. I think a [raze] crate data attribute should answer this use case. I didn't notice it earlier as I was accidentally building un-sandboxed 😢. We discussed something more automated, but its hard to guess what crates will need as data, and something overly aggressive will bloat runfiles severely.

sdl2-sys expects the HOST variable to be set and since we do not cross compile, it is easy enough to provide.

Perhaps this should be built in to the generated genrule?

image seems like a cargo raze bug. It generates crate_root = "./src/lib.rs" which is rejected on build, since it starts with ./.

Definitely a bug. crate_root is directly based on Cargo.toml's [lib.path], and it sounds like cargo is more flexible than bazel. Should be easy enough to trim any ./ for now.

I'll make a serious effort to get these three fixes in over the weekend -- they're all straightforward and easy to reproduce.

@SirVer
Copy link
Author

SirVer commented Mar 3, 2018

We discussed something more automated, but its hard to guess what crates will need as data, and something overly aggressive will bloat runfiles severely.

We could curate something like a Crates.io repository (or homebrew), but for raze. I envision a set of recipes for various crates and versions. Raze could clone that on run and then would know what sort of data files are required for the different repositories. Other configurations like build_genrs or fancy add ons to the BUILD could be stated there too.

Perhaps this should be built in to the generated genrule?

That would be beneficial to me. How about we add a HOST option to the raze options in Cargo.toml. Then cross compilation would still be viable.

I'll make a serious effort to get these three fixes in over the weekend -- they're all straightforward and easy to reproduce.

Thank you - much appreciated! I will be traveling next week, so I will not make more progress on the build though.

@acmcarther
Copy link
Owner

Status report:

I implemented both the crate root trimming feature, and the data feature (data here: google/cargo-raze#10), but I was struck by the complete lack of testability.

Since you're traveling next week anyway, I've decided to delay the features, and finish the rewrite to use cargo metadata: google/cargo-raze#7. This will unblock actually testing features as they're added, which is nice. Unfortunately, this necessitates a very small change to cargo metadata and the Cargo release cycle is pretty slow so it might be a while before the change can be merged cargo-raze side. It also has the added cost of requiring users to use the very latest version of Cargo.

With that in mind, I might still end up submitting the changes you requested into the current version of the BUILD planner, but it'll be a bit janky.

@SirVer
Copy link
Author

SirVer commented Mar 15, 2018

The issues 1. and 3. have been fixed with recent cargo raze updates. Thanks for that! I am now working on making grpc work for us. Some updates here:

This file contains an overridden version of the grpcio-sys crate. I can't remember why I overrode it. Try not overriding it and see what happens!

You needed to do that because they use let version = env!("CARGO_PKG_VERSION"); in channel.rs. That is the same problem that I see with rusoto as described in #27 (comment). Can we not set environment variables inside the bazel build rules and export this variable?

@mfarrugi
Copy link
Contributor

mfarrugi commented Mar 15, 2018 via email

@SirVer
Copy link
Author

SirVer commented Mar 15, 2018

We can but there are many, the most immediate work around is asking the
environment variable to your command line or bazel rc

That does not work for CARGO_PKG_VERSION, because it is different for each crate. Cargo raze has this information though, it could autogenerate them into the BUILD files it writes and pass it through the build rules into the environment, given the hooks in the build rules are being made.

I suggest adding a build_env flag to rust_* rules that set environment variables for the rustc call to enable usage of the env! macro.

@acmcarther
Copy link
Owner

FWIW: CARGO_PKG_VERSION is rarely load-bearing. Have you seen a crate that behaved differently based on that value (e.g. 0.5.0 enabled some feature that 0.4.0 disables)? I'm seriously curious -- I'll file an issue against their repo if such a thing exists. I'm on a bit of a crusade against the "nothing but Cargo exists" undercurrent.

This is a gap in my knowledge, but IIUC environment variable control is part of the hermeticity story in Bazel. I'm not particularly opposed to an escape hatch, but I worry that allowing arbitrary environment variables to be declared in rust_library might be too powerful. For lack of an alternative, I'd accept it -- but I'd want to confer with someone well informed about best practices before submitting code to rules_rust to enable it.

That said, there are real consequences for the HOST envvar, and i suspect others. I was hoping the list was bounded though...


This is definitely a real problem, and I don't intent to stonewall or anything. I just want to make sure I understand the problem space.

@SirVer
Copy link
Author

SirVer commented Mar 16, 2018

@acmcarther Working around with the bazel.rc is working for me for now and is a satisfying hack for the moment.

I continued with grpc building and got it working. I copied your vendored gprc-io-sys packet, because I got linking errors without it. I did not have time to dig deeper what they actually were.

Now it works 🎆 ! The only wart I have is that I have to add

 rustc_flags = [
      "-Clink-args=-lstdc++",
    ],

to every rust crate or binary that transitively depends on grpcio, which is not ideal of course. If I leave this out, I get linking errors:

  = note: bazel-out/k8-fastbuild/bin/third_party/point_cloud_viewer/point_viewer_grpc/octree_server.deps/libssl.a(d1_both.pic.o):(.data.DW.ref.__gxx_per
sonality_v0[DW.ref.__gxx_personality_v0]+0x0): undefined reference to `__gxx_personality_v0'
          bazel-out/k8-fastbuild/bin/third_party/point_cloud_viewer/point_viewer_grpc/octree_server.deps/libssl.a(ssl_key_share.pic.o):(.data.rel.ro+0x8
0): undefined reference to `vtable for __cxxabiv1::__si_class_type_info'
          bazel-out/k8-fastbuild/bin/third_party/point_cloud_viewer/point_viewer_grpc/octree_server.deps/libssl.a(ssl_key_share.pic.o):(.data.rel.ro+0x9
8): undefined reference to `vtable for __cxxabiv1::__si_class_type_info'
          bazel-out/k8-fastbuild/bin/third_party/point_cloud_viewer/point_viewer_grpc/octree_server.deps/libssl.a(ssl_key_share.pic.o):(.data.rel.ro._ZT
IN4bssl11SSLKeyShareE[_ZTIN4bssl11SSLKeyShareE]+0x0): undefined reference to `vtable for __cxxabiv1::__class_type_info'
          collect2: error: ld returned 1 exit status

For a c++ lib, I would set alwayslink = 1 for this. Did you encounter this or do you have a suggestion how to work around it?

@acmcarther
Copy link
Owner

Hmm..... That rustc_flags thing is really unpleasant. The underlying cause is that grpc has a cc_library dependency that needs it? I recall that grpc recently stopped distinguishing between C sources and cpp sources... I assume this issue came out of that.

Sounds like -Clink-args might need to be "first class" in rules_rust somehow and carried through the dependency tree. A minimal contrived example would be nice in order to have something to include as a rules_rust test case and to help understand the problem.

I remember fighting with the more recent version of grpc and forcibly moving to an older version to escape this. Thanks for seeing it through to a state that compiles!

@SirVer
Copy link
Author

SirVer commented Mar 23, 2018

@acmcarther I put a simple repro case into this report bazelbuild/rules_rust#78

@mfarrugi
Copy link
Contributor

Discussion looks to have moved over there, I think we can close this issue (and then I don't have to remember there are two raze projects :))

@SirVer
Copy link
Author

SirVer commented Apr 1, 2018

Right, let's close this issue and I will open future discussion points in the google repo. Thanks @mfarrugi and @acmcarther for your help with all of this!

@SirVer SirVer closed this as completed Apr 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants