Bzlmod

[lalten]

Incomplete, work in progress 🚧

[lalten]

Current state is that things are complaining about boost. I think rather than trying to use the WORKSPACE-only rules_boost it would be best to switch over to bazelbuild/bazel-central-registry#1280 (comment) (once that's ready).

[mvukov]

Boost is used at a single place

rules_ros2/repositories/resource_retriever.BUILD.bazel

Line 13 in 68a2b81

"@boost//:shared_ptr",

(IMO, that package is outdated but still needed by the foxglove bridge). The receiver class is fairly small and contained last thing I remember. So, I'd suggest to patch it and use std::shared_ptr; then we can remove the boost dep. I can try to do this over the weekend or you can try to do it if you have some spare time. I'd target this change against the main branch to keep this PR as small as possible.

[lalten]

Here's the issues/todos as of now:

• asio should probably depend on openssl like it does in WORKSPACE, but it doesn't in Bzlmod.
• pybind_bazel is harder than necessary to integrate and causes issues about Python not being found in CI (works locally), might revert that and stick to the http_archive.
• I don't understand why but Python3.10 causes issues. 3.11 works mostly, but py_loader complains about a compilation mismatch then.
• Need to update bzlmod situation in examples. I have the feeling that users need to add a bunch of bazel_deps in their repo that are transitive dependencies of rules_ros2. That's kind of what bzlmod is supposed to avoid. Let's see how it works when we get there.

[mvukov]

High level comments: let's leave version updates and reordering of deps for follow-up PRs to make the review easier.

Great job so far!

[lalten]

Getting closer...
Still getting this from the WORKSPACE version of asio but not in the MODULE.bazel one.

external/asio/include/asio/ssl/detail/openssl_types.hpp:23:10: fatal error: 'openssl/conf.h' file not found
#include <openssl/conf.h>
         ^~~~~~~~~~~~~~~~
1 error generated.

The reason is that Bzlmod asio is configured as header-only: https://github.com/bazelbuild/bazel-central-registry/blob/main/modules/asio/1.28.2/patches/add_build_file.patch

But in WORKSPACE we use the checked-in asio.BUILD that has

rules_ros2/repositories/asio.BUILD.bazel

Lines 13 to 15 in 4b8441a

	defines = [
	"ASIO_SEPARATE_COMPILATION",
	],

to switch from header-only to lib compilation and (because of that) has to add

rules_ros2/repositories/asio.BUILD.bazel

Line 18 in 4b8441a

deps = ["@openssl//:ssl"],

CI is green with this change, but that may just mean that there are no tests that really need ssl :)

Not sure what's the better approach here. But IMO WORKSPACE and MODULE.bazel should both do the same thing.

We still need OpenSSL for libcurl, which is not on BCR yet.

[lalten]

I am unsure what causes the test failures (or how to fix them), could you have a look @mvukov or @ahans ?
Fixed in 435e95a

[mering]

Things seem to work, the only drawback is that load("@rules_python//python:defs.bzl", "py_binary") must become load("@rules_ros2_pythons//3.10:defs.bzl", "py_binary"), so there is a bunch of 3.10 hardcoded everywhere. While not pretty I think that would be acceptable

Why do you think this is necessary? AFAIK rules_python will use the default toolchain when using load("@rules_python//python:defs.bzl", "py_binary"), so load("@rules_ros2_pythons//3.10:defs.bzl", "py_binary") should be equivalent to registering the 3.10 toolchain as default.

[lalten]

Things seem to work, the only drawback is that load("@rules_python//python:defs.bzl", "py_binary") must become load("@rules_ros2_pythons//3.10:defs.bzl", "py_binary"), so there is a bunch of 3.10 hardcoded everywhere. While not pretty I think that would be acceptable

Why do you think this is necessary? AFAIK rules_python will use the default toolchain when using load("@rules_python//python:defs.bzl", "py_binary"), so load("@rules_ros2_pythons//3.10:defs.bzl", "py_binary") should be equivalent to registering the 3.10 toolchain as default.

right, but my understanding is that if the root repo uses a default != 3.10, the rules_ros2_pip_deps are not guaranteed to be compatible

[mering]

Things seem to work, the only drawback is that load("@rules_python//python:defs.bzl", "py_binary") must become load("@rules_ros2_pythons//3.10:defs.bzl", "py_binary"), so there is a bunch of 3.10 hardcoded everywhere. While not pretty I think that would be acceptable

Why do you think this is necessary? AFAIK rules_python will use the default toolchain when using load("@rules_python//python:defs.bzl", "py_binary"), so load("@rules_ros2_pythons//3.10:defs.bzl", "py_binary") should be equivalent to registering the 3.10 toolchain as default.

right, but my understanding is that if the root repo uses a default != 3.10, the rules_ros2_pip_deps are not guaranteed to be compatible

Are you sure that they will chosen in the first place? My understanding is that it will only use pip deps with the correct version. This is why most projects specify the same pip deps for many Python versions. IIUC when it won't find pip deps in the correct version, it will throw an error.

[mering]

Many projects use a pattern like https://github.com/bazelbuild/bazel-central-registry/blob/63e59351d9d3bb520c9db83529c9541a2b05c800/modules/grpc/1.63.1/MODULE.bazel#L47-L76.

[udaya2899]

@lalten were you able to make progress on this PR?

[lalten]

@lalten were you able to make progress on this PR?

I'm working on this on and off when I find time, which I didn't since the last commit 🙈

[udaya2899]

No worries, thank you for your contribution. As far as I understand, you got everything working already, right? Maybe time to merge 👀 ?

[lalten]

Tried to update this branch (still waiting on #344 and #340) but the new rust features require some work to be compatible with Bzlmod.

In particular it's not obvious how to get the bindgen stuff into Bzlmod. Looks like bazelbuild/rules_rust#2787

[ahans]

@lalten @mvukov Anything I can lend a hand with? Being able to move to bzlmod would be big!

[lalten]

My previous comment regarding the rust stuff is still the state of things. I'm thinking if we could add a custom build config flag or so to disable the rust support that might be a way around those issues.

[ahans]

My previous comment regarding the rust stuff is still the state of things. I'm thinking if we could add a custom build config flag or so to disable the rust support that might be a way around those issues.

Yeah, that sounds good. You could tag all Rust-related tests with no_bzlmod and then use --tag_test_filters to exclude them. Or simply exclude them in the CI config by doing something like bazel test -- //... -//ros2/test/rust/.... We'd need some documentation making clear that Rust support is only available in the WORKSPACE version, but that should be about it? The bindgen discussion sounds like it could be a while until this is fully supported with bzlmod... @mvukov What do you think?

[mvukov]

I have no objections merging bzlmod support. But, workspace-based workflow must work, no compromises on that one for the time being. I am not personally using bzlmod (yet) in any projects, so, no Rust support is fine.

Something to think about: https://github.com/mvukov/rules_ros is for instance migrated to bzlmod recently. Since no one is using the workspace workflow, it was decided to remove workspace support. Something I found along the way:

• Python toolchain to be used is the one from the parent project, i.e. rules_ros.
• In this repo we have rules_ros2_pip_deps, in the other rules_ros_pip_deps. Turns out one can not override this repo from his/her own repo. The issue is tracked in Thirdparty pip dependencies override root project's pip dependencies bazelbuild/rules_python#1791. Depending on what parts of and how you use rules_ros2, this might be a (big) problem. In workspace based workflow one can extend requirements.txt with their own deps and use that both with rules_ros2 in their own repo. (Of course, one needs to be careful not to break things.) This is a blocker IMO to remove workspace workflow from rules_ros2.