bindgen: don't register_toolchains in Bzlmod

[lalten]

I'm currently in the process of getting rules_ros2 onto Bzlmod. One challenge is that its rust support requires rules_rust_bindgen, which depends on llvm-project, which takes more than GitHub Action's maximum configurable job timeout (10h) to build. See mvukov/rules_ros2#238 (comment)

This PR adds new "bring your own clang" build flags: --@rules_rust_bindgen//:clang and --@rules_rust_bindgen//:libclang. As the added test shows, you can point these at toolchains_llvm's pre-compiled clang.

The fancy !!set and << merge key yaml magic is documented in https://github.com/bazelbuild/continuous-integration/tree/master?tab=readme-ov-file#sharing-configuration-between-tasks

Let me know what you think about this. I'm also curious what's the rationale for depending on llvm-project instead of toolchains_llvm in the first place. Maybe instead of making this configurable (and thus adding complexity) rules_rust_bindgen could just be switched over to toolchains_llvm completely?

[UebelAndre]

Thanks! Outside of bzlmod the expectation was that consumers could bring their own clang by defining their own toolchains and not registering the default ones via:

rules_rust/extensions/bindgen/repositories.bzl

Lines 91 to 100 in d256c7b

	def rust_bindgen_register_toolchains(register_toolchains = True):
	"""Registers the default toolchains for the `rules_rust` [bindgen][bg] rules.
	[bg]: https://rust-lang.github.io/rust-bindgen/
	Args:
	register_toolchains (bool, optional): Whether or not to register toolchains.
	"""
	if register_toolchains:
	native.register_toolchains(str(Label("//:default_bindgen_toolchain")))

I think that would be a better approach than to have build flags users need to put in their .bazelrc files. I'm still fairly new to bzlmod but does that sound possible?

[lalten]

Outside of bzlmod the expectation was that consumers could bring their own clang by defining their own toolchains

Yes that's what we did as well so far in another repo that doesn't use the Bzlmod bindgen yet.

I'm not a toolchains registering expert but https://bazel.build/external/migration#register-toolchains:~:text=You%20cannot%20call%20native.register_toolchains%20in%20a%20module%20extension. sounds like it is not possible to do this outside of MODULE.bazel

[lalten]

Friendly bump on this @UebelAndre :)

[UebelAndre]

I'm not a toolchains registering expert but https://bazel.build/external/migration#register-toolchains:~:text=You%20cannot%20call%20native.register_toolchains%20in%20a%20module%20extension. sounds like it is not possible to do this outside of MODULE.bazel

I think what you can do is set the toolchain registration as dev_dependency = True and require that consumers of rules_rust_bindgen always explicitly register their own. I might then make a target @rules_rust_bindgen//toolchain for folks to use and document the need to add the following to their MODULE.bazel files.

register_toolchains(
    "@rules_rust_bindgen//toolchain",
)

Or in your case

register_toolchains(
    "//my/bindgen/toolchain",
)

[lalten]

set the toolchain registration as dev_dependency = True and require that consumers of rules_rust_bindgen always explicitly register their own.

Interesting idea! I tried this out and it works.

I might then make a target @rules_rust_bindgen//toolchain for folks to use and document the need

sgtm

[UebelAndre]

set the toolchain registration as dev_dependency = True and require that consumers of rules_rust_bindgen always explicitly register their own.

Interesting idea! I tried this out and it works.

I might then make a target @rules_rust_bindgen//toolchain for folks to use and document the need

sgtm

Do you wanna put up a pull request for this? 😅

[UebelAndre]

Nice, thanks! For the example though you'll wanna make sure it's running in CI by adding it to .bazelci/presubmit.yml. Then I think this is good to go!

[UebelAndre]

I think you wanna actually have a use of the toolchain in this example through toolchain resolution. Why not copy the simple test here instead?

[lalten]

Using bindgen_test_suite here isn't straightforward because its deps are dev_dependencies of @rules_rust_bindgen. I used a symlink to reuse the integration test instead.

[UebelAndre]

What is a bindgen_test_suite?

[lalten]

from https://github.com/bazelbuild/rules_rust/blob/82576d04b31a3123c824375385c3b2d5339da16d/extensions/bindgen/test/bindgen_test.bzl

[lalten]

I pushed my commits but I've run into an interesting problem: Somehow the toolchain depends on itself. Maybe you have an idea?

❯ cd examples/bindgen_toolchain
❯ bazel test //...
ERROR: Misconfigured toolchains: //:my_bindgen_toolchain is declared as a toolchain but has inappropriate dependencies. Declared toolchains should be created with the 'toolchain' rule and should not have dependencies that themselves require toolchains.
.-> //:libclang
|   toolchain types @@bazel_tools//tools/cpp:toolchain_type
|   toolchain type @@bazel_tools//tools/cpp:toolchain_type
|   RegisteredToolchains
|   //:my_bindgen_toolchain
`-- //:libclang
Target //:clang up-to-date:
  bazel-bin/clang.exe
ERROR: Analysis of target '//test/integration/simple:simple' failed; build aborted
INFO: Elapsed time: 0.227s, Critical Path: 0.00s
INFO: 1 process: 5 action cache hit, 1 internal.
ERROR: Build did NOT complete successfully
ERROR: No test targets were found, yet testing was requested

[UebelAndre]

Why the native_binary for this attribute?

[lalten]

@llvm_toolchain_llvm//:bin/clang is a filegroup, not an executable Label, so it needs to be "converted" with a genrule with executable=True, or a native_binary.

[UebelAndre]

I would genuinely just duplicate the code (which is not a lot). If I end up adding more tests I don't want them to appear so an example. We have no bindgen example currently so this would effectively become that and should show general use of the bindgen rules but with a custom toolchain (which is super useful info in an example).

[lalten]

makes sense, done

[UebelAndre]

This is caused because you haven't registered a toolchain target but instead a rust_bindgen_toolchain target. You're missing that intermediate.

[UebelAndre]

This I think is the solution to #3231 (comment)

[lalten]

great point! I missed that one somehow. Now it works 👍

[UebelAndre]

nit: can this be named README.md? That's the pattern we've used all over the repo.

[lalten]

friendly bump @UebelAndre