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

Expose bindgen toolchain through bzlmod #2740

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jwnrt
Copy link

@jwnrt jwnrt commented Jul 12, 2024

With this change I was able to use the default bindgen toolchain with bzlmod by adding this to my MODULE.bazel:

bindgen = use_extension("@rules_rust//bindgen:extension.bzl", "bindgen")
use_repo(bindgen, "bindgen_toolchains")

register_toolchains("@bindgen_toolchains//:default_bindgen_toolchain")

Had to backport a change from recent versions of LLVM which use this WORKSPACE_ROOT environment variable to fix paths to generated files. Updating the LLVM archive would also fix that problem.

Happy to add tests and documentation if this matches what you had in mind for getting this working.

@jwnrt jwnrt force-pushed the bzlmod-bindgen branch 3 times, most recently from cd2f5f9 to 237339a Compare July 14, 2024 13:36
Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Just had some questions and nits 😄

bindgen/transitive_repositories.bzl Show resolved Hide resolved
bindgen/extension.bzl Show resolved Hide resolved
bindgen/extension.bzl Outdated Show resolved Hide resolved
@lalten
Copy link

lalten commented Aug 29, 2024

@jwnrt any chance you could pick this up again? Seems like this would fix #2787

@jwnrt jwnrt force-pushed the bzlmod-bindgen branch 2 times, most recently from 3eb82d9 to b8a817a Compare August 30, 2024 09:45
@jwnrt
Copy link
Author

jwnrt commented Aug 30, 2024

@jwnrt any chance you could pick this up again? Seems like this would fix #2787

Sorry, I took my eye off this when I realised I wouldn't need it for OpenTitan.

I'm happy to finish it off but I won't have time for at least a week. Happy for anyone else to take it over if they get there before me.

Aside, it turns out the local_repository thing I switched to using isn't available in Bazel 6, so that might be blocked on #2812. I'll switch it back to the manual re-implementation for now.

I still have no resolution for the repo_mapping thing. It's not supported in Bzlmod, but may break non-bzlmod users who depend on a different version of zlib if removed. The best solution would be for Bzlmod to stop erroring on that parameter and just ignore it. That would be an upstream change.

@AlexOrozco1256
Copy link

@jwnrt Hello James, is there any plan to fix this issue? Has a bug been filed with bazelbuild for the repo_mapping issue? Thanks!

@jwnrt jwnrt force-pushed the bzlmod-bindgen branch 3 times, most recently from caa3139 to a39df01 Compare September 13, 2024 17:58
With Bzlmod enabled, `repo_mapping` isn't that useful since repositories
are namespaced under their parents. Unfortunately, Bazel errors out if
you try to use it.

This commit conditionally provides the parameter if needed, but allows
it to be disabled.
@jwnrt
Copy link
Author

jwnrt commented Sep 13, 2024

Thanks for the reminder, I've updated the PR to resolve those issues.

Based on bazelbuild/bazel#21846 it seems to me that Bazel are moving away from supporting repo_mapping entirely. Even if upstream Bazel stops erroring on repo_mapping for Bzlmod, rules_rust would have to greatly increase its minimum supported Bazel version to benefit.

I've worked around it by making repo_mapping optional (but used by default). I've documented it as being unstable / subject to change.

@ahans
Copy link

ahans commented Sep 27, 2024

@jwnrt Thanks for your effort, sounds like a good solution to me. @UebelAndre Could you take another look, so we can merge this PR? The issue blocks the transition of rules_ros2 to bzlmod.

@AlexOrozco1256
Copy link

@jwnrt Thank you!! Sorry it took me a while to get back but I appreciate your help!

@hlopko
Copy link
Member

hlopko commented Oct 8, 2024

(just a drive by that this is an exciting change and it would be great to have it merged - thanks @jwnrt and @UebelAndre)

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 this pull request may close these issues.

6 participants