-
Notifications
You must be signed in to change notification settings - Fork 440
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
add a bindgen rule #102
Comments
Surfacing bindgen would require us to do one of three things (as I see it)
Here is my macro for it: https://github.com/acmcarther/void/blob/master/tools/rust/bindgen.bzl . This requires adding the Example Usages:
What about an LLVM label? |
I was thinking #3. At a glance it looks like your setup is loading the
system clang/llvm, though I haven't tested through the build.rs scripts yet.
…On Thu, Jul 5, 2018, 20:52 Alex McArther ***@***.***> wrote:
Surfacing bindgen would require us to do one of three things (as I see it)
1. Couple rules_rust to cargo-raze by exposing a cargo-raze workspace
that contains the bindgen binary
2. Check in a generated set of BUILD files for bindgen and its deps
3. Include the bindgen genrule, but do not provide a default binding
for the bindgen binary itself
Here is my genrule for it:
https://github.com/acmcarther/void/blob/master/tools/rust/bindgen.bzl .
This requires adding the extra_aliased_targets = [ "cargo_bin_bindgen" ]
item to the raze section
Usage:
https://github.com/acmcarther/void/blob/37e3c7c569c963d4b6d6cb9c39f032650a14eb15/third_party/reliable_io.BUILD
What about an LLVM label?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#102 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACLjeKBgCRcs2IdZ6Ek-obNhHFU9VYuwks5uDrTogaJpZM4VEtQc>
.
|
Oh, you're referring to Bindgen's clang-sys dependency? My current configuration is loading libclang at runtime I think (via libloading), but you could directly link it. |
Made some progress here, chasing down all the deps to get it working sandboxed was mighty tedious! Invocation looks like this: rust_bindgen(
name = "bindings",
header = ":wrapper.h",
c_lib = "//path/to/c/package:target",
# Boilerplate
bindgen = "//ext/cargo:cargo_bin_bindgen",
clang = "//ext/clang:clang",
libllvm = "//ext/clang:lib",
libstdcxx = "//ext/gcc:lib",
) Any thoughts on trimming the boilerplate? I haven't seen any way to configure rules in the WORKSPACE for usage elsewhere, though I imagine it must exist. I need to tidy it before sharing. Minor tangent: overriding raze targets is fairly awkward; it might be good to support a 'handwritten_build = path/to/BUILD' for the likes of libloading, libbacktrace and whatever other degenerate crates are compiling their own c portion. |
I'd probably suggest (via documentation) that users write a local alias for the rule that supplies defaults for the four WORKSPACE-specific dependencies. They would then import their own version of the bindgen rule and it would be fully functional. I don't think you can get around having to specify them unless you provide defaults from Filed google/cargo-raze#58 for the cargo-raze thing. |
Rule looks like https://github.com/mfarrugi/rules_rust/blob/master/rust/bindgen.bzl Main difference from your macro is that it tries to pick through a cc_library to put together the clang invocation. I don't think it works with the default cc_toolchain (while sandboxed?) because it cannot find standard library headers, and in general I'm not sure that we're interfacing with the cc api in the right way. This also looks hugely tedious to package (and test) w/ dependencies, and would be cleaner to package with a proper clang cc Also wrote https://github.com/mfarrugi/rules_rust/blob/master/rust/cbindgen.bzl#L25 on a lark, but it is more dependent on cargo and cbindgen is a bit less mature than bindgen.. probably won't go further with that soon. |
We use cbindgen and bindgen extensively. I was assuming that I'd have to more or less vendor the pre-built executable and use genrules to replicate our cmake workflow (calling some shell scripts via add_custom_target). I'd be super pleased if there were a more integrated solution. |
This bindgen impl will work with the vendored exe, but bindgen and cbindgen
are easy enough to build with raze. The impl is not that much more than the
genrule, really.
I wanted it as a build step to protect against upstream c breaking me.
I linked a cbindgen rule impl somewhere, but it depends on cargo metadata
and will not be useful until the rest of the rules generate Cargo.toml and
we expose cargo from the tarball. The cbindgen ule is much simpler overall
since it doesn't depend on llvm or anything else.
…On Tue, Aug 7, 2018, 04:47 Chip Collier ***@***.***> wrote:
We use cbindgen and bindgen extensively. I was assuming that I'd have to
more or less vendor the pre-built executable and use genrules to replicate
our cmake workflow (calling some shell scripts via add_custom_target). I'd
be super pleased if there were a more integrated solution.
Currently for bindgen we do not include the target in 'all' so it's only
built on demand and the results are kept in git since they do not change
that frequently.
On the other hand, with cbindgen we call it frequently so that any changes
to a crate have an immediate effect for downstream targets.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#102 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACLjeO_lX-Acp2lAx97Z_0l26Fd7WsD8ks5uOVQmgaJpZM4VEtQc>
.
|
@photex please give the bindgen rule a try, and let us know if you have any issues. |
I've had to abandon my Bazel conversion due to difficulties particularly when cross-compiling Rust targets. :( |
Any updates on a cbindgen rule? |
@mfarrugi is this issue not resolved? There's definitely a bindgen rule now. |
It would be nice to make bindgen easily available, asking users to provide an llvm label.
@acmcarther did you mentioning having this implemented somewhere already?
The text was updated successfully, but these errors were encountered: