add rule for bindgen (#102) #108

mfarrugi · 2018-07-22T21:19:34Z

This is what I got working for me, but I haven't tested it against anything else.

Main issues I can see,

This is not really testable within this repo, given the implicit raze dependency
the cc toolchain finagling seems rough at best, not sure it works w/ vanilla toolchain

I imagine I can setup a small raze environment based on the examples in this repo, but I would like to get comments on the cc_toolchain usage (and anything else) before continuing on this.

acmcarther

I'm not sure why, but I'm really skeptical about this PR.

Unrelated, the variable names here are really spartan (e.g. "unformatted" "rustfmt", "bindgen" instead of "unformatted_out", "rustfmt_bin", "bindgen_bin"). I let some questionable variable names go through earlier, but during my refactoring I really regretted it every time I looked at them. Are you averse to being a bit more specific on these?

rust/bindgen.bzl

rust/rust.bzl

mfarrugi · 2018-07-23T17:30:27Z

I am happy to expand on the names. My thought process was something like bindgen = ctx.executable.bindgen was clear, and the rule was relatively short.

Do you think it is better to reference ctx.executable.bindgen every time or go with bindgen_bin etc?

mfarrugi · 2018-07-23T17:38:15Z

On the skeptical bit, I think the cc provider and toolchain usage is weird, and even the clang usage feels like it should maybe come from a clang cc_toolchain.

mfarrugi · 2018-09-15T17:49:53Z

I haven't come back to this yet because upstreaming this while requiring libstdcxx as a parameter is extremely suspect to me, though I haven't had any issues with the current incarnation.

mfarrugi · 2018-12-11T10:34:16Z

Shipping this requires either writing a clang_toolchain rule, or figuring out how to use the current cc toolchain api with a clang installation appropriately.

@damienmg can you comment on whether the latter is actually a good idea?

damienmg · 2018-12-11T15:14:25Z

I think the latter is wiser, especially since the c++ toolchain can now be written in Starlark. I'll try to talk a bit with @mhlopko a bit latter about it to see if he can help.

mfarrugi · 2019-01-22T04:55:06Z

Updated this to use a toolchain, which makes it a little cleaner. This "Works On My Machine"..

Things to do before pushing:

Re-address comments from before.
Add default toolchain + working example, similar to protobuf rules
Add/update docs

The example should involve a tarball of clang and a raze imported bindgen, which is hopefully straight forward.

genrule and actions don't seem to be able to call 'tar' anymore, on bazel 21 installed with nix, on ubuntu 18.04 ... might need to turn the build script genrules into a rule

bindgen/raze/crates.bzl

rust/private/rustc.bzl

mfarrugi · 2019-01-27T20:43:10Z

The Ubuntu 16.04 (local and rbe) CI failures are due to the builders having an older version of libstdc++ than libclang wants...

thread 'main' panicked at 'Unable to find libclang: "the `libclang` shared library at bazel-out/host/bin/_solib_k8/_U@clang_S_S_Clibclang.so___Uexternal_Sclang_Slib/libclang.so could not be opened: bazel-out/host/bin/_solib_k8/_U@local_Ulinux_S_S_Clibstdc++___Uexternal_Slocal_Ulinux/libstdc++.so.6: version `GLIBCXX_3.4.22\' not found (required by bazel-out/host/bin/_solib_k8/_U@clang_S_S_Clibclang.so___Uexternal_Sclang_Slib/libclang.so)"', libcore/result.rs:1009:5

Ubuntu 14.04 failure is probably caused by using the wrong clang tarball

Inconsistency detected by ld.so: dl-open.c: 689: _dl_open: Assertion `_dl_debug_initialize (0, args.nsid)->r_state == RT_CONSISTENT' failed!

Will look to fix for 16.04...

mfarrugi · 2019-01-27T21:32:52Z

Alright this looks like it works, ptal @damienmg @acmcarther

There is something to be said about how much boilerplate is required to support a ~150 line rule.

damienmg

First pass, I spotted a few bugs but API-wise it looks good to me.

It is still missing documentation though. Especially how to plug other dependencies (see the protobuf change).

damienmg · 2019-01-28T15:12:50Z

.bazelci/presubmit.yml

  macos:
    osx_targets: &osx_targets
-    - "--" # Allows negative patterns; hack for https://github.com/bazelbuild/continuous-integration/pull/245
+    - "--"


Why remove the comment?

I moved the comment up to the first occurence

Can we duplicate the comment rather? or later the first one might goes away but not this one.

bindgen/bindgen.bzl

damienmg · 2019-01-28T15:19:09Z

bindgen/bindgen.bzl

+        tools = [clang_bin],
+    )
+
+    if rustfmt_bin:


Is running rustfmt needed on generated sources? I meant, they are not made for being read anyway.

I've personally found it useful (in my own hacky version of this) when inspecting generated files. In my experience, bindgen can be a bit finnicky and require special params to configure generation. Its much easier to diagnose issues in a formatted file compared to a super long single line unformatted file.

Beyond being useful, this is what bindgen does by default. I think it is best to keep parity with other tools whenever possible.

Sounds good, can you just add a comment about it?

bindgen/bindgen.bzl

damienmg · 2019-01-28T15:21:39Z

bindgen/bindgen.bzl

+            executable = True,
+            cfg = "host",
+        ),
+        "libclang": attr.label(


I am surprised you did not got hit by bazelbuild/bazel#6889. This will get shipped in the host configuration :(

Not sure if @mfarrugi fully follows the issue, but I'm a bit confused.

Isn't host configuration the configuration we want to run bindgen in?

We want to run this at build-time on the host, so I don't follow what "shipped" means. (I also don't understand the issue :))

If we want to run this in host configuration please add cfg = "host". The default value of cfg is "target" but due to bazelbuild/bazel#6889 it somehow becomes the host configuration. This is a bug in Bazel.

bindgen/repositories.bzl

rust/private/rustc.bzl

acmcarther

Thanks for following up on this (even after so long)! I know the code generation experience with cargo is super clunky and this is a good place for Bazel to stand out.

I second @damienmg's request for a bit more documentation but otherwise I'm happy with the implementation.

WORKSPACE

bindgen/bindgen.bzl

bindgen/repositories.bzl

acmcarther · 2019-01-28T20:26:20Z

bindgen/bindgen.bzl

+            executable = True,
+            cfg = "host",
+        ),
+        "libclang": attr.label(


Not sure if @mfarrugi fully follows the issue, but I'm a bit confused.

Isn't host configuration the configuration we want to run bindgen in?

bindgen/raze/crates.bzl

mfarrugi · 2019-01-28T21:18:15Z

I'll look to finish this up soon.

Any ideas on skipping the local_linux setup on osx? Looks like it requires a repository rule.. but maybe one of you knows of something lighter?

…e more user friendly.

mfarrugi · 2019-02-03T19:58:58Z

Now I just need to skip loading local_linux on osx...

…ldn't blow up on osx.

mfarrugi · 2019-02-03T21:04:30Z

This probably is not far from working on osx, but I don't have a mac to test with and I don't want bounce diffs off of CI.

ptal!

damienmg

Only some nits now. Since buildkite is green, I say it is a go for me :)

damienmg · 2019-02-04T12:48:07Z

.bazelci/presubmit.yml

  macos:
    osx_targets: &osx_targets
-    - "--" # Allows negative patterns; hack for https://github.com/bazelbuild/continuous-integration/pull/245
+    - "--"


Can we duplicate the comment rather? or later the first one might goes away but not this one.

bindgen/README.md

damienmg · 2019-02-04T14:14:14Z

bindgen/bindgen.bzl

+        tools = [clang_bin],
+    )
+
+    if rustfmt_bin:


Sounds good, can you just add a comment about it?

bindgen/bindgen.bzl

damienmg · 2019-02-04T14:17:09Z

bindgen/bindgen.bzl

+            executable = True,
+            cfg = "host",
+        ),
+        "libclang": attr.label(


If we want to run this in host configuration please add cfg = "host". The default value of cfg is "target" but due to bazelbuild/bazel#6889 it somehow becomes the host configuration. This is a bug in Bazel.

Co-Authored-By: mfarrugi <[email protected]>

mfarrugi · 2019-02-05T15:03:49Z

Looks like there are some unrelated RBE failures..

ERROR: While resolving toolchains for target @examples//fibonacci:fibonacci_test: invalid registered toolchain '@bazel_toolchains//configs/ubuntu16_04_clang/1.1/bazel_0.22.0/cpp:cc-toolchain-clang-x86_64-default': no such package '@bazel_toolchains//configs/ubuntu16_04_clang/1.1/bazel_0.22.0/cpp': BUILD file not found on package path

other than that I think this looks good.

@acmcarther any other comments?

bindgen/README.md

mfarrugi · 2019-02-05T20:48:23Z

@nlopezgi where's the right place to complain about rbe ci issues? (https://buildkite.com/bazel/rules-rust-rustlang?branch=master failing after a trivial change convinces me this is not our fault)

nlopezgi · 2019-02-22T16:33:15Z

Hi @mfarrugi
Sorry for the super late reply on this.
The error you were getting related to @bazel_toolchains//configs/ubuntu16_04_clang/1.1/bazel_0.22.0/cpp:cc-toolchain-clang-x86_64-default not being found is likely because the WORKSPACE file pointer to bazel-toolchains needs to be updated regularly (https://releases.bazel.build/bazel-toolchains.html). This is the case because with each Bazel version there are changes to the toolchain configurations that need to be used for RBE builds. Apologies for the issue. We are working towards a better mechanism (https://github.com/bazelbuild/bazel-toolchains/blob/master/rules/rbe_repo.bzl) that will make it easier to keep things working with the fast pace of Bazel releases.

googlebot added the cla: yes label Jul 22, 2018

acmcarther reviewed Jul 23, 2018

View reviewed changes

mfarrugi mentioned this pull request Jul 31, 2018

Implement C++ Starlark API bazelbuild/bazel#4570

Closed

mfarrugi mentioned this pull request Aug 28, 2018

Fix warnings in examples. #124

Merged

mfarrugi force-pushed the marco-bindgen branch from 010ed33 to 3d7f9cf Compare January 22, 2019 04:50

mfarrugi added 12 commits January 27, 2019 15:23

Update bindgen.bzl

f096c8c

Get bindgen rule working locally.

566f2fe

Add a test, too.

ebca9c3

Make the test a teensy bit less simple.

a120a99

raze

bf76c50

Use raze (with hacks) for getting the bindgen binary.

40e5d52

genrule and actions don't seem to be able to call 'tar' anymore, on bazel 21 installed with nix, on ubuntu 18.04 ... might need to turn the build script genrules into a rule

Make rustfmt optional and tidy it all up.

7209c28

add note

8054608

buildifier

ecb07c0

Add docs and move stuff around.

c1baedb

tidy more

725501c

var renames

5070b0a

mfarrugi force-pushed the marco-bindgen branch from 3d7f9cf to 5070b0a Compare January 27, 2019 20:24

mfarrugi commented Jan 27, 2019

View reviewed changes

bindgen/raze/crates.bzl Outdated Show resolved Hide resolved

mfarrugi commented Jan 27, 2019

View reviewed changes

rust/private/rustc.bzl Outdated Show resolved Hide resolved

Add ubuntu 18.04 to .bazelci

65c10d4

mfarrugi force-pushed the marco-bindgen branch from 126db22 to 6b7d1e0 Compare January 27, 2019 21:05

Only run bindgen examples on ubuntu 18.04

2534cd5

damienmg reviewed Jan 28, 2019

View reviewed changes

acmcarther reviewed Jan 28, 2019

View reviewed changes

mfarrugi added 11 commits February 3, 2019 14:50

revert tar path change

4bbcb7a

Move override for libloading out of generated crates.bzl

f558d5e

Copyrights to 2019.

651627f

Have macro forward kwargs to rust_library instead of rust_bindgen.

e824e0d

Consolidate 'remove libstdc++ dep' todos.

45592dd

Rename @clang to @bindgen_clang

485becc

Add more broken stubs for stardoc...

25c3ca9

Commit hackily generated rust_bindgen_toolchain doc ...

11616c3

Fix commented out lines.

243aa62

Refer to ourselves as @io_bazel_rules_rust in WORKSPACE to be a littl…

dfe5ded

…e more user friendly.

Add bindgen overview doc, make minor adjustments to proto docs to match.

20fd058

mfarrugi added 2 commits February 3, 2019 15:46

Replace 'new_local_repository' for libstdc++ with something that shou…

55f0075

…ldn't blow up on osx.

take 2

6c0b35b

acmcarther mentioned this pull request Feb 4, 2019

Add build_file_path [raze.crate.version] override that supports providing a custom build file google/cargo-raze#58

Closed

damienmg approved these changes Feb 4, 2019

View reviewed changes

damienmg and others added 2 commits February 5, 2019 09:51

Apply suggestions from code review

0e7c86e

Co-Authored-By: mfarrugi <[email protected]>

Address last review comments.

f1f91e3

acmcarther approved these changes Feb 5, 2019

View reviewed changes

bindgen/README.md Outdated Show resolved Hide resolved

fix typo

22ab34a

mfarrugi merged commit 1ced2c2 into bazelbuild:master Feb 6, 2019

mfarrugi deleted the marco-bindgen branch February 6, 2019 01:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add rule for bindgen (#102) #108

add rule for bindgen (#102) #108

mfarrugi commented Jul 22, 2018 •

edited

Loading

acmcarther left a comment

mfarrugi commented Jul 23, 2018

mfarrugi commented Jul 23, 2018

mfarrugi commented Sep 15, 2018

mfarrugi commented Dec 11, 2018

damienmg commented Dec 11, 2018

mfarrugi commented Jan 22, 2019 •

edited

Loading

mfarrugi commented Jan 27, 2019

mfarrugi commented Jan 27, 2019

damienmg left a comment

damienmg Jan 28, 2019

mfarrugi Jan 28, 2019

damienmg Feb 4, 2019

damienmg Jan 28, 2019

acmcarther Jan 28, 2019 •

edited

Loading

mfarrugi Jan 28, 2019

damienmg Feb 4, 2019

damienmg Jan 28, 2019

acmcarther Jan 28, 2019

mfarrugi Jan 28, 2019

damienmg Feb 4, 2019

acmcarther left a comment

acmcarther Jan 28, 2019

mfarrugi commented Jan 28, 2019

mfarrugi commented Feb 3, 2019

mfarrugi commented Feb 3, 2019

damienmg left a comment

damienmg Feb 4, 2019

damienmg Feb 4, 2019

damienmg Feb 4, 2019

mfarrugi commented Feb 5, 2019

mfarrugi commented Feb 5, 2019

nlopezgi commented Feb 22, 2019

add rule for bindgen (#102) #108

add rule for bindgen (#102) #108

Conversation

mfarrugi commented Jul 22, 2018 • edited Loading

acmcarther left a comment

Choose a reason for hiding this comment

mfarrugi commented Jul 23, 2018

mfarrugi commented Jul 23, 2018

mfarrugi commented Sep 15, 2018

mfarrugi commented Dec 11, 2018

damienmg commented Dec 11, 2018

mfarrugi commented Jan 22, 2019 • edited Loading

mfarrugi commented Jan 27, 2019

mfarrugi commented Jan 27, 2019

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

acmcarther Jan 28, 2019 • 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

Choose a reason for hiding this comment

acmcarther left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfarrugi commented Jan 28, 2019

mfarrugi commented Feb 3, 2019

mfarrugi commented Feb 3, 2019

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

mfarrugi commented Feb 5, 2019

mfarrugi commented Feb 5, 2019

nlopezgi commented Feb 22, 2019

mfarrugi commented Jul 22, 2018 •

edited

Loading

mfarrugi commented Jan 22, 2019 •

edited

Loading

acmcarther Jan 28, 2019 •

edited

Loading