Migrate `drake-ros` to Bzlmod by hidmic · Pull Request #396 · RobotLocomotion/drake-ros

hidmic · 2025-07-30T21:46:05Z

Precisely what the title says. WORKSPACE files are going away in Bazel 9 (later this year). drake-ros has to transition to Bzlmod to stay viable. That is what this patch does, plus some long overdue cleanup.

This change is

hidmic · 2025-07-30T21:47:52Z

This builds but it is a WIP. launch support is broken, quite a few tests are failing, and CI is missing the --enable_bzlmod flag for bazel builds. Coming up next.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2025-08-06T19:44:46Z

Rebased, transitioned to Bazel 8, and restored tests for all Bazel packages. The story for transitive dependencies could be better, particularly around toolchains. Now CI and the colcon builds need an update, but since I don't expect any more structural changes, I think this is ready for a first review.

Note for reviewers: don't be deterred by the large diff. MODULE.bazel.lock files explain most of it.

hidmic · 2025-08-06T19:45:54Z

@@ -1,17 +0,0 @@
-workspace(name = "lint")


I'd rather do proper linting than turning this into a module of sorts.

What is the action item for this discussion?

Linting properly. f6ed39d thru c13a05f do so.

hidmic · 2025-08-06T19:46:37Z

+# -*- python -*-
+package(default_visibility = ["//visibility:public"])
+
+alias(


Simplest way I found to forward dependencies for generated macros to use downstream.

BTW this is probably better a comment explaining the intent for downstream consumption, here or in a readme or both.

hidmic · 2025-08-06T19:48:37Z

-#include "ros2/tools/dload_shim.h"
-#include "network_isolation.h"
+#include "lib/dynamic_load/dload_shim.h"
+#include "lib/network_isolation/network_isolation.h"


This is a peculiar gotcha for bzlmod: the module name isn't prepended to C++ headers nor Python modules. It seems to be a known ~~issue~~ quirk, e.g. see bazel-contrib/rules_python#1679.

We can probably do better, playing with imports and include prefixes, but sticking to the normal way for now.

BTW: This is probably best resolved as a TODO comment; suggestion below

Code snippet:

// TODO(rules_python#1679) the bare `lib` namespacing here is because // bzlmod doesn't prepended the module name to C++ headers nor Python modules.

Sounds good. Linked bazelbuild/bazel#18128 which is the actual upstream issue.

FYI the new behavior of rules_python is a feature, not a bug. I am perfectly fine with a TODO and not changing anything right now. I just want us all to be on the same page about what the endgame is. The top-level package name that we import from should be unique in the world of all python software, and exist an actual directory in git. The current name lib meets the latter criterion, but not the former.

Makes sense. TODOs added.

FYI For my money, the TODO should say something like "Using lib here is a poor choice of name because is unlikely to be globally unique; we should rename this to something more unique."

Fair. Added the extra qualification in 95f9736

hidmic · 2025-08-06T19:50:46Z

+        jobs = jobs,
+    )
+
+local_ros2 = module_extension(


This precludes the multi-distro user story for now. We were not relying on it anyways. It can be brought in if we introduce some notion of named distribution to this local_ros2 extension though.

BTW: This is probably best resolved as a TODO comment, along with an issue if it needs more explanation than fits in a comment.

Does it make sense to? Do we need / want the feature? I want us to do some housekeeping after we are done with this and I'd rather not add issues that will never be addressed.

I think that's fine, I just would prefer to put some sort of breadcrumb in the code to note this fact. e.g.

Code snippet:

# NOTE: This assumes that there is exactly one local ROS2 distro installed; # have multiple installed distros at once is not supported.

Note added.

jwnimmer-tri

BTW we will definitely use Reviewable for this, not GitHub review comments.

I an happy take a high-level skim for any big-picture points, but I'll defer the full review to others.

Reviewed all commit messages.
Reviewable status: 0 of 148 files reviewed, 4 unresolved discussions (waiting on @calderpg-tri)

hidmic · 2025-08-06T19:51:59Z

@@ -1 +0,0 @@
-from .rmw_isolation import isolate_rmw_by_path


Dropping the rmw_isolation feature entirely. It was brittle, we have network_isolation now, and it was making the bzlmod migration harder.

hidmic · 2025-08-06T19:53:15Z

+)
+
+DRAKE_COMMIT = "v1.43.0"
+DRAKE_CHECKSUM = "635c125187c6758b1bfc7698b59f6f9bfc64a902575e191cccce74be8c6f47ee"


Need Drake versioning info here, as MODULE.bazel may not load *.bzl files.

(BTW I don't understand what this message means.)

Prior to this patch, Drake's version specification lived in a version.bzl file that was available to all WORKSPACE files, here at drake_ros and downstream. That is gone. It is no longer possible to distribute information like that across MODULE files. We don't have a good user story yet to handle the "package depends on drake and drake_ros, drake_ros depends on drake" scenario, where drake deps must be the same version or at least API compatible. There are solutions to this depending on which workflow we want.

I see what you're getting at.

However, since drake is a bazel_dep (with an override), I don't think we're ever at risk of having two different copies? All we've lost is the default version. If the user supplies their own drake in their project's module file, I believe that drake_ros will automatically use it.

True. So long as you are careful with versions downstream all is fine.

hidmic · 2025-08-06T19:54:45Z

@@ -1,38 +1,44 @@
 #!/bin/bash


All infra automation (prereqs scripts, containers, GHA workflows) needs a ton of love. I'll improve some of it here, and do the rest in follow-up PRs.

BTW: This is best resolved with a TODO comment, possibly connected to an issue if it needs a longer checklist of specifics.

Sure. Let me see how far do I need to go to fix CI first.

calderpg-tri · 2025-08-06T20:13:22Z

@jwnimmer-tri @ggould-tri I will go through this and raise any bazel questions that need more attention from you. @ggould-tri and I will test this in anzu before it lands.

jwnimmer-tri

Sounds good. I'll post my checkpoint comments for now.

Reviewed 15 of 148 files at r1.
Reviewable status: 15 of 148 files reviewed, 23 unresolved discussions (waiting on @calderpg-tri and @ggould-tri)

bazel_ros2_rules/BUILD.bazel line 1 at r1 (raw file):

# Empty build file to mark package root.

nit Don't remove the newline at end of file. All text files should always end with a newline character.

default.bazelrc line 1 at r1 (raw file):

# Use C++20.

BTW Drake has switched to C++23 now (https://drake.mit.edu/installation.html). It wouldn't hurt to switch Drake-ROS now, too. Plausibly that is a separate / issue PR, though.

bazel_ros2_rules/WORKSPACE line 1 at r1 (raw file):

# Empty workspace file to mark its root.

nit Missing newline at end of file.

drake_ros/WORKSPACE line 1 at r1 (raw file):

# Empty workspace file to mark its root.

nit Missing newline at end of file.

bazel_ros2_rules/.bazeliskrc line 3 at r1 (raw file):

# When bazelisk in use, this dotfile specifies which version of Bazel should be
# used to build and test.
USE_BAZEL_VERSION=8.3.1

BTW Drake has switched from .bazeliskrc to .bazelversion, for simplicity. At some point, Drake-ROS should probably also follow suit.

Ditto throughout this PR (or for a future PR).

drake_ros/MODULE.bazel line 25 at r1 (raw file):

use_repo(drake_dep_repositories, "eigen", "gtest", "python")

bazel_dep(name = "bazel_ros2_rules", version = "1.0.0")

nit This MVS requirement seems meaningless?

drake_ros/MODULE.bazel line 51 at r1 (raw file):

local_ros2.distribution(
    include_packages = DRAKE_ROS_REQUIRED_PACKAGES + DRAKE_ROS_TEST_DEPENDENCIES,
    #overlays = ["/home/mhidalgo/Workspaces/jazzy_ws/install"],

nit Remove commented-out code.

drake_ros/MODULE.bazel line 53 at r1 (raw file):

    #overlays = ["/home/mhidalgo/Workspaces/jazzy_ws/install"],
)
use_repo(local_ros2, **{"ros2": "local_ros2"})

nit The ** syntax here seems arcane. Why not just write ros="local_ros2 directly?

bazel_ros2_rules/MODULE.bazel.lock line 1 at r1 (raw file):

Why are we checking in the lock files to git? In other projects (Drake, Anzu, Drake-Blender, Drake-models, etc.) we don't do that.

Ditto for all MODULE.bazel.lock files in this PR.

bazel_ros2_rules/MODULE.bazel line 3 at r1 (raw file):

module (
    name = "bazel_ros2_rules",
    version = "1.0.0",

Is this version meaningful? Without a release / tag / bcr publish process in place, I would say not. It's safe to not specify a version. (Drake doesn't.)

Ditto for the drake_ros module, etc. -- all MODULE files in this git repo.

bazel_ros2_rules/MODULE.bazel line 10 at r1 (raw file):

local_python_repository = use_repo_rule(
    "//tools/workspace:python.bzl",

nit Spurious whitespace at end-of-line. Be sure to run buildifier on all of the bazel/bzl files.

bazel_ros2_rules/MODULE.bazel line 36 at r1 (raw file):

)

register_toolchains("@local_toolchains//:all", dev_dependency = True)

nit Missing newline at end of file.

.gitignore line 1 at r1 (raw file):

.vscode

nit Generally, we prefer that personal ignores go into the user's personal ignore file, not the project ignore file. The project should only ignore files that it creates automatically. Developers are responsible for setting up the IDE and IDE-specific ignores on their developer computer, once and for all, instead of copy-pasting the details into hundreds of projects one by one.

drake_ros_examples/MODULE.bazel line 35 at r1 (raw file):

    module_name = "bazel_ros2_rules",
    path = "../bazel_ros2_rules",
)

nit The MVS requirements seem spurious, given that it's not part of any registry?

Suggestion:

bazel_dep(name = "drake_ros")
local_path_override(
    module_name = "drake_ros",
    path = "../drake_ros",
)

bazel_dep(name = "bazel_ros2_rules")
local_path_override(
    module_name = "bazel_ros2_rules",
    path = "../bazel_ros2_rules",
)

drake_ros_examples/MODULE.bazel line 51 at r1 (raw file):

local_ros2.distribution(
    include_packages = ROS_REQUIRED_PACKAGES,
    overlays = ["/home/mhidalgo/Workspaces/jazzy_ws/install"],

nit This line seems inappropriate for landing on master.

drake_ros_examples/MODULE.bazel line 53 at r1 (raw file):

    overlays = ["/home/mhidalgo/Workspaces/jazzy_ws/install"],
)
use_repo(local_ros2, **{"ros2": "local_ros2"})

nit Ditto on "why **"?

ggould-tri

Checkpointing review in progress.

Reviewed 16 of 148 files at r1, all commit messages.
Reviewable status: 24 of 148 files reviewed, 26 unresolved discussions (waiting on @calderpg-tri and @hidmic)

a discussion (no related file):
Working: I need to test this against current Anzu.

a discussion (no related file):
This PR seems like it completely removes buildifier linting from the repository. Is that intentional? It seems like a odd thing to slip into a PR without even mentioning it. And per Jeremy's comment on bazel_ros2_rules/MODULE.bazel it's already dropping defects.

.gitignore line 4 at r1 (raw file):

bazel-*
**/__pycache__/

minor: This should also include **/MODULE.bazel.lock

ggould-tri

Reviewed 6 of 148 files at r1.
Reviewable status: 29 of 148 files reviewed, 30 unresolved discussions (waiting on @calderpg-tri and @hidmic)

bazel_ros2_rules/deps/python/BUILD.bazel line 12 at r1 (raw file):

    name = "headers",
    actual = "@python//:headers"
)

nit: Missing terminal newline. Also a red flag that you have no linter.

bazel_ros2_rules/WORKSPACE line 1 at r1 (raw file):

# Empty workspace file to mark its root.

BTW: I see that there is advice to keep around an empty WORKSPACE file to satisfy legacy tools, and this file is consistent with that advice. But this file should at least name which tools are still depending on it so we can remove this file when those tools are bzlmod-aware.

(same for the other WORKSPACE files)

bazel_ros2_rules/deps/python/version.bzl line 3 at r1 (raw file):

load("@python//:version.bzl", _PYTHON_EXTENSION_SUFFIX = "PYTHON_EXTENSION_SUFFIX")

PYTHON_EXTENSION_SUFFIX = _PYTHON_EXTENSION_SUFFIX

nit: Missing terminal newline. Also a red flag that you have no linter.

bazel_ros2_rules/deps/python/numpy/BUILD.bazel line 12 at r1 (raw file):

    name = "libs",
    actual = "@python//numpy:libs"
)

nit: Missing terminal newline. Also a red flag that you have no linter.

... I'm going to just stop marking these from here out. It applies to lots of files, and buildifier should be able to flag and fix once it's working again.

ggould-tri

Reviewed 11 of 148 files at r1.
Reviewable status: 40 of 148 files reviewed, 31 unresolved discussions (waiting on @calderpg-tri and @hidmic)

bazel_ros2_rules/lib/repos.bzl line 55 at r1 (raw file):

        doc = "The expected SHA-256 of the file downloaded.",
    ),
    "sha256_url": attr.string(

BTW: I'm not sure what the purpose of this is. The principal reason to check a SHA is that you don't trust the integrity of the download. But if you don't trust the integrity of the download, why would you trust a downloaded SHA any more than that? Is the concern that download_and_extract might give you partial content?

jwnimmer-tri

Reviewable status: 40 of 148 files reviewed, 31 unresolved discussions (waiting on @calderpg-tri, @ggould-tri, and @hidmic)

jwnimmer-tri · 2025-08-11T14:33:50Z

+)
+
+DRAKE_COMMIT = "v1.43.0"
+DRAKE_CHECKSUM = "635c125187c6758b1bfc7698b59f6f9bfc64a902575e191cccce74be8c6f47ee"


(BTW I don't understand what this message means.)

hidmic

Thank you for taking the time!

Reviewable status: 40 of 148 files reviewed, 31 unresolved discussions (waiting on @calderpg-tri, @ggould-tri, and @jwnimmer-tri)

a discussion (no related file):

Previously, ggould-tri wrote…

This PR seems like it completely removes buildifier linting from the repository. Is that intentional? It seems like a odd thing to slip into a PR without even mentioning it. And per Jeremy's comment on bazel_ros2_rules/MODULE.bazel it's already dropping defects.

As I mentioned somewhere above, this is still a work in progress. I put it up early so that you guys could take a look and maybe spot structural defects. Linting I was less concerned about and support was already quite frail. I dropped the hacky, partial linter we had to get the build going. I'll be adding proper linting support (for everything, not just Starlark code) next.

.gitignore line 1 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Generally, we prefer that personal ignores go into the user's personal ignore file, not the project ignore file. The project should only ignore files that it creates automatically. Developers are responsible for setting up the IDE and IDE-specific ignores on their developer computer, once and for all, instead of copy-pasting the details into hundreds of projects one by one.

That's perfectly reasonable. I'll rollback this line.

.gitignore line 4 at r1 (raw file):

Previously, ggould-tri wrote…

minor: This should also include **/MODULE.bazel.lock

I'm not entirely convinced we should be excluding lockfiles. See answer below.

default.bazelrc line 1 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Drake has switched to C++23 now (https://drake.mit.edu/installation.html). It wouldn't hurt to switch Drake-ROS now, too. Plausibly that is a separate / issue PR, though.

We can give C++23 a shot in a follow-up PR. Drake might have switched, but ROS 2 remains tied to C++17, at leas in public facing APIs. ISO C++ should ensure backwards compatibility but I don't know how well do different compilers fare in that sense.

bazel_ros2_rules/.bazeliskrc line 3 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Drake has switched from .bazeliskrc to .bazelversion, for simplicity. At some point, Drake-ROS should probably also follow suit.

Ditto throughout this PR (or for a future PR).

I'm fine with switching now. I'll do that.

bazel_ros2_rules/MODULE.bazel line 3 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Is this version meaningful? Without a release / tag / bcr publish process in place, I would say not. It's safe to not specify a version. (Drake doesn't.)

Ditto for the drake_ros module, etc. -- all MODULE files in this git repo.

That's fair. We can remove it until we have a process (if we ever do).

bazel_ros2_rules/MODULE.bazel.lock line 1 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Why are we checking in the lock files to git? In other projects (Drake, Anzu, Drake-Blender, Drake-models, etc.) we don't do that.

Ditto for all MODULE.bazel.lock files in this PR.

If everybody has a different lockfile it somewhat lowers the value of the lockfile. See https://bazel.build/external/lockfile#best-practices. One could argue that bazel doesn't allow version specifications like pip or cargo do, butbazel_dep does have this concept of compatibility_level that would seem to justify a lockfile.

bazel_ros2_rules/WORKSPACE line 1 at r1 (raw file):

Previously, ggould-tri wrote…

BTW: I see that there is advice to keep around an empty WORKSPACE file to satisfy legacy tools, and this file is consistent with that advice. But this file should at least name which tools are still depending on it so we can remove this file when those tools are bzlmod-aware.

(same for the other WORKSPACE files)

Once this patch lands, there will be no tool within this repository that requires WORKSPACE files. I'm not listing anything about downstream packages that might. I'm keeping them to maintain compatibility with Bazel versions earlier than 8. We can decide we won't support earlier versions, that's up for debate.

bazel_ros2_rules/lib/repos.bzl line 55 at r1 (raw file):

Previously, ggould-tri wrote…

BTW: I'm not sure what the purpose of this is. The principal reason to check a SHA is that you don't trust the integrity of the download. But if you don't trust the integrity of the download, why would you trust a downloaded SHA any more than that? Is the concern that download_and_extract might give you partial content?

That is a great question I don't have an answer for. I don't know what the rationale was when this feature was added. I agree with you it's a bit unusual.

drake_ros/MODULE.bazel line 53 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit The ** syntax here seems arcane. Why not just write ros="local_ros2 directly?

I just reproduced a sample snippet that now I cannot find. No reason other than inertia. Keyword arguments look better.

drake_ros_examples/MODULE.bazel line 35 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit The MVS requirements seem spurious, given that it's not part of any registry?

Sure, that's reasonable.

drake_ros_examples/MODULE.bazel line 51 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit This line seems inappropriate for landing on master.

Argh, absolutely. Thought I had removed them all.

hidmic · 2025-08-11T18:16:18Z

+)
+
+DRAKE_COMMIT = "v1.43.0"
+DRAKE_CHECKSUM = "635c125187c6758b1bfc7698b59f6f9bfc64a902575e191cccce74be8c6f47ee"


Prior to this patch, Drake's version specification lived in a version.bzl file that was available to all WORKSPACE files, here at drake_ros and downstream. That is gone. It is no longer possible to distribute information like that across MODULE files. We don't have a good user story yet to handle the "package depends on drake and drake_ros, drake_ros depends on drake" scenario, where drake deps must be the same version or at least API compatible. There are solutions to this depending on which workflow we want.

jwnimmer-tri

Reviewable status: 40 of 148 files reviewed, 28 unresolved discussions (waiting on @calderpg-tri, @ggould-tri, and @hidmic)

bazel_ros2_rules/MODULE.bazel.lock line 1 at r1 (raw file):

Previously, hidmic (Michel Hidalgo) wrote…

If everybody has a different lockfile it somewhat lowers the value of the lockfile. See https://bazel.build/external/lockfile#best-practices. One could argue that bazel doesn't allow version specifications like pip or cargo do, butbazel_dep does have this concept of compatibility_level that would seem to justify a lockfile.

I'm familiar with that document, but I don't know why anything there is acutely relevant to this project.

The lockfile computation is a deterministic function of our MODULE.bazel. If new versions get published to the registry, they do NOT automatically get used, whether the lockfile is checked in or not. Unlike e.g. python, bazel uses minimal version selection.

Checking in the lockfile means that we have churn in git (and associated merge conflicts) without any upside that I'm aware of. Only more burden -- we need to check (currently manually, though maybe eventually could be automated) that the lockfile is actually up-to-date in any commit.

Other projects like https://github.com/bazel-contrib/rules_python or https://github.com/bazelbuild/rules_cc don't check-in the lockfile.

What's the upside?

jwnimmer-tri · 2025-08-11T19:50:52Z

+)
+
+DRAKE_COMMIT = "v1.43.0"
+DRAKE_CHECKSUM = "635c125187c6758b1bfc7698b59f6f9bfc64a902575e191cccce74be8c6f47ee"


I see what you're getting at.

However, since drake is a bazel_dep (with an override), I don't think we're ever at risk of having two different copies? All we've lost is the default version. If the user supplies their own drake in their project's module file, I believe that drake_ros will automatically use it.

jwnimmer-tri · 2025-09-10T13:58:50Z

I suspect this is due to the status checks only existing in the PR branch and not main itself.

That seems plausible.

Would the preferred course of action be to turn off the current bazel_build_and_test, merge this PR, and then turn on the three new status checks on for future PRs into main?

FWIW that sounds OK to me.

IanTheEngineer

Ok. I've removed the the status checks stopping PR's from being merged to main for now. I left in place the review requirements. Once this PR merges, I'll add the status checks back in for the three new CI test setups.

Reviewable status: 120 of 243 files reviewed, 9 unresolved discussions (waiting on @calderpg-tri, @ggould-tri, @hidmic, and @jwnimmer-tri)

calderpg-tri

@calderpg-tri reviewed 36 of 118 files at r2, 1 of 5 files at r3, 11 of 51 files at r5, 18 of 24 files at r7, 107 of 107 files at r8, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @ggould-tri and @hidmic)

ros2_example_bazel_installed/ros2_example_apps/inquirer.cc line 7 at r8 (raw file):

#include "ros2_example_common_msgs/action/do.hpp"
#include "ros2_example_common_msgs/srv/query.hpp"
#include <rclcpp/rclcpp.hpp>

btw this include ordering is opposite what we have elsewhere (e.g. Drake or Anzu)

drake_ros/core/drake_ros.cc line 1 at r8 (raw file):

#include "drake_ros/core/drake_ros.h"

Is there a reason these include paths are changing?

drake_ros/core/subscription.h line 55 at r8 (raw file):

  void return_dynamic_message(
      rclcpp::dynamic_typesupport::DynamicMessage::SharedPtr&
          message)  // NOLINT

minor Presumably you either want this all on one line (the NOLINT case), or you want it autoformatted (i.e. no need for NOLINT)?

ros2_example_bazel_installed/ros2_example_apps/roslaunch_eg_nodes/eg_listener.cpp line 27 at r8 (raw file):

  rclcpp::Subscription<std_msgs::msg::String>::SharedPtr subscription_;

  size_t max_count_;

These need to be initialized or const members

jwnimmer-tri

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @calderpg-tri, @ggould-tri, and @hidmic)

drake_ros/core/subscription.h line 55 at r8 (raw file):

Previously, calderpg-tri wrote…

minor Presumably you either want this all on one line (the NOLINT case), or you want it autoformatted (i.e. no need for NOLINT)?

I think it's probably an overly-abbreviated NOLINT(runtime/referneces), not a formatting marker. (I'd probably try NOLINTNEXTLINE(runtime/referneces) to make it easier.)

jwnimmer-tri

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @calderpg-tri, @ggould-tri, and @hidmic)

drake_ros/core/subscription.h line 55 at r8 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think it's probably an overly-abbreviated NOLINT(runtime/referneces), not a formatting marker. (I'd probably try NOLINTNEXTLINE(runtime/referneces) to make it easier.)

(runtime/references, that is.)

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic

Reviewable status: 131 of 243 files reviewed, 7 unresolved discussions (waiting on @calderpg-tri, @ggould-tri, and @jwnimmer-tri)

bazel_ros2_rules/MODULE.bazel line 46 at r7 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW I think drake is only used for linting? In that case, I imagine it should be a dev_dependency (along with pycodestyle and styleguide). Although see the other thread -- Drake v1.45.0 removes the ability for downstream modules to call its linter binaries entirely, so maybe this is not worth fixing now.

I did make it a dev dependency at first, but full workspace bazel test downstream would fail. Likely exposing more than I should from each module. I'll fix that and linting altogether in a follow-up.

bazel_ros2_rules/tools/lint/BUILD.bazel line 1 at r7 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Don't we want to call add_lint_tests() here? It's sibling directories do that.

Perks of using Drake macros. I'll drop these in a follow-up.

drake_ros/MODULE.bazel line 29 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

For reference, see RobotLocomotion/drake#22689 and RobotLocomotion/drake#23393. Downstream projects will no longer be able to call Drake's cpplint as of the next Drake release v1.45.0 next week. This repo will be unable to upgrade to the latest Drake until the linting gets rewritten. I'm fine if you don't want to attack that in this PR.

Understood. I'll fix this in the immediately following PR.

drake_ros/core/drake_ros.cc line 1 at r8 (raw file):

Previously, calderpg-tri wrote…

Is there a reason these include paths are changing?

The extra include prefix is unnecessary.

drake_ros/core/subscription.h line 55 at r8 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

(runtime/references, that is.)

All correct. Rewrote it in 95f9736

ros2_example_bazel_installed/ros2_example_apps/inquirer.cc line 7 at r8 (raw file):

Previously, calderpg-tri wrote…

btw this include ordering is opposite what we have elsewhere (e.g. Drake or Anzu)

Ordering changed in 95f9736

ros2_example_bazel_installed/ros2_example_apps/roslaunch_eg_nodes/eg_listener.cpp line 27 at r8 (raw file):

Previously, calderpg-tri wrote…

These need to be initialized or const members

Done in 95f9736

ros2_example_bazel_installed/tools/ros2.py line 15 at r7 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW The demotion to a one-letter variable name seems like a readability regression.

Agreed. Using better naming in 95f9736

hidmic · 2025-09-19T14:57:24Z

-#include "ros2/tools/dload_shim.h"
-#include "network_isolation.h"
+#include "lib/dynamic_load/dload_shim.h"
+#include "lib/network_isolation/network_isolation.h"


Fair. Added the extra qualification in 95f9736

jwnimmer-tri

@jwnimmer-tri reviewed 1 of 112 files at r9.
Reviewable status: 132 of 243 files reviewed, 3 unresolved discussions (waiting on @calderpg-tri and @ggould-tri)

ros2_example_bazel_installed/tools/ros2.py line 15 at r7 (raw file):

Previously, hidmic (Michel Hidalgo) wrote…

Agreed. Using better naming in 95f9736

FYI I don't know why this file merited any changes in the first place. The names in the old code were perfectly fine.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic

Reviewable status: 131 of 243 files reviewed, 3 unresolved discussions (waiting on @calderpg-tri, @ggould-tri, and @jwnimmer-tri)

ros2_example_bazel_installed/tools/ros2.py line 15 at r7 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI I don't know why this file merited any changes in the first place. The names in the old code were perfectly fine.

Consistency. We have 3-5 places with roughly the same code pattern. I normalized naming in this file to match the others. It could have been done in reverse as well. I don't mind which.

jwnimmer-tri

Reviewable status: 131 of 243 files reviewed, 3 unresolved discussions (waiting on @calderpg-tri and @ggould-tri)

ros2_example_bazel_installed/tools/ros2.py line 15 at r7 (raw file):

Previously, hidmic (Michel Hidalgo) wrote…

Consistency. We have 3-5 places with roughly the same code pattern. I normalized naming in this file to match the others. It could have been done in reverse as well. I don't mind which.

Ah, thanks. Somehow I missed the other changes.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

calderpg-tri

@calderpg-tri reviewed 4 of 112 files at r9, 1 of 108 files at r10, 107 of 107 files at r11, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @hidmic)

drake_ros/core/drake_ros.cc line 1 at r8 (raw file):

Previously, hidmic (Michel Hidalgo) wrote…

The extra include prefix is unnecessary.

In Drake and Anzu we always use the more complete include path. It would be odd for drake-ros to be different.

ros2_example_bazel_installed/ros2_example_apps/roslaunch_eg_nodes/eg_listener.cpp line 27 at r8 (raw file):

Previously, hidmic (Michel Hidalgo) wrote…

Done in 95f9736

count_ appears to be missing initialization here, e.g. size_t count_ = 0; or size_t count_{}; (yes, count_ is initialized in the constructor, but style guide specifies initialization here)

drake_ros/core/subscription.h line 53 at r11 (raw file):

  }

  using DynamicMessage = rclcpp::dynamic_typesupport::DynamicMessage;

I think if you're going to have this using, you should use it consistently here

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic

Reviewable status: 129 of 246 files reviewed, 3 unresolved discussions (waiting on @calderpg-tri, @ggould-tri, and @jwnimmer-tri)

drake_ros/core/subscription.h line 53 at r11 (raw file):

Previously, calderpg-tri wrote…

I think if you're going to have this using, you should use it consistently here

Sure thing. See 90eb481.

ros2_example_bazel_installed/ros2_example_apps/roslaunch_eg_nodes/eg_listener.cpp line 27 at r8 (raw file):

Previously, calderpg-tri wrote…

count_ appears to be missing initialization here, e.g. size_t count_ = 0; or size_t count_{}; (yes, count_ is initialized in the constructor, but style guide specifies initialization here)

Done in 90eb481.

drake_ros/core/drake_ros.cc line 1 at r8 (raw file):

Previously, calderpg-tri wrote…

In Drake and Anzu we always use the more complete include path. It would be odd for drake-ros to be different.

Fair enough. Rolled back (and made it consistent throughout) in 90eb481.

calderpg-tri

@calderpg-tri reviewed 117 of 117 files at r12, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hidmic)

drake_ros/core/serializer.h line 6 at r12 (raw file):

#include <drake/common/value.h>
#include <drake_ros/core/serializer_interface.h>

This should be #include "drake_ros/core/serializer_interface.h" and after the other includes

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic

Reviewable status: 138 of 246 files reviewed, 1 unresolved discussion (waiting on @calderpg-tri)

drake_ros/core/serializer.h line 6 at r12 (raw file):

Previously, calderpg-tri wrote…

This should be #include "drake_ros/core/serializer_interface.h" and after the other includes

Quoted includes in a public interface?

calderpg-tri

@calderpg-tri reviewed 8 of 108 files at r13.
Reviewable status: 146 of 246 files reviewed, 1 unresolved discussion (waiting on @hidmic)

drake_ros/core/serializer.h line 6 at r12 (raw file):

Previously, hidmic (Michel Hidalgo) wrote…

Quoted includes in a public interface?

The other headers here use quoted includes, I don't see why this file would need to be different

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic

Reviewable status: 137 of 246 files reviewed, 1 unresolved discussion (waiting on @calderpg-tri)

drake_ros/core/serializer.h line 6 at r12 (raw file):

Previously, calderpg-tri wrote…

The other headers here use quoted includes, I don't see why this file would need to be different

Hmm, it is actually inconsistent. Some use angle brackets, some use quotes. I generally avoid installing headers with quoted includes because they incur an extra search paths that will hardly match for any header that isn't a private header, but it's not like we care about binary release here so I don't mind strongly, really. Changed in 8fbdf71.

calderpg-tri

@calderpg-tri reviewed 1 of 108 files at r13, 108 of 108 files at r14, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @hidmic)

IanTheEngineer · 2025-09-22T19:06:56Z

@calderpg-tri asked if I could merge this PR. Reviewable isn't loading for any of us anymore, but CI is green. If there turns out to be an outstanding reviewer request, we can address it in a follow-up PR.

IanTheEngineer · 2025-09-22T19:15:40Z

Now that this has merged, I re-enabled CI on main branch for the following tests:

bazel_ros2_rules_test: Build and test bazel_ros2_rules modules with bazel
bazel_drake_ros_test: Build and test drake_ros modules with bazel
colcon_drake_ros_test: Build and test drake_ros packages with colcon

Migrate drake-ros to bzlmod

e272183

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic force-pushed the bzlmod-migration branch from 097978e to af150e6 Compare August 6, 2025 11:39

All Bazel packages build

80a53c8

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic force-pushed the bzlmod-migration branch from af150e6 to 80a53c8 Compare August 6, 2025 19:05

All Bazel packages' tests pass

feeef9d

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic marked this pull request as ready for review August 6, 2025 19:44

hidmic requested review from calderpg-tri and jwnimmer-tri August 6, 2025 19:45

hidmic commented Aug 6, 2025

View reviewed changes

jwnimmer-tri self-assigned this Aug 6, 2025

hidmic commented Aug 6, 2025

View reviewed changes

jwnimmer-tri reviewed Aug 6, 2025

View reviewed changes

hidmic commented Aug 6, 2025

View reviewed changes

hidmic requested a review from ggould-tri August 6, 2025 20:10

jwnimmer-tri assigned ggould-tri and calderpg-tri Aug 6, 2025

jwnimmer-tri reviewed Aug 6, 2025

View reviewed changes

ggould-tri requested changes Aug 7, 2025

View reviewed changes

ggould-tri reviewed Aug 7, 2025

View reviewed changes

jwnimmer-tri reviewed Aug 11, 2025

View reviewed changes

hidmic commented Aug 11, 2025

View reviewed changes

jwnimmer-tri reviewed Aug 11, 2025

View reviewed changes

IanTheEngineer reviewed Sep 10, 2025

View reviewed changes

hidmic mentioned this pull request Sep 16, 2025

Add support for Bazel builds Ekumen-OS/beluga#521

Merged

14 tasks

Fix overlapping dependencies in rosidl_interfaces_group macro

0a86b4f

frneer force-pushed the bzlmod-migration branch from 6eddc3b to 0a86b4f Compare September 16, 2025 19:04

frneer mentioned this pull request Sep 18, 2025

Remove rosidl packages patches #398

Closed

calderpg-tri requested changes Sep 18, 2025

View reviewed changes

jwnimmer-tri reviewed Sep 18, 2025

View reviewed changes

Address peer review comments

95f9736

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic commented Sep 19, 2025

View reviewed changes

jwnimmer-tri reviewed Sep 19, 2025

View reviewed changes

Drop extra const

22c576a

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic commented Sep 19, 2025

View reviewed changes

jwnimmer-tri reviewed Sep 19, 2025

View reviewed changes

Fix .clang-format include categories

b38e137

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

calderpg-tri requested changes Sep 22, 2025

View reviewed changes

Address peer review comments

90eb481

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic commented Sep 22, 2025

View reviewed changes

calderpg-tri requested changes Sep 22, 2025

View reviewed changes

Add missing header from mock include dir

1af8786

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic commented Sep 22, 2025

View reviewed changes

calderpg-tri requested changes Sep 22, 2025

View reviewed changes

Roll back to quoted include

8fbdf71

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic commented Sep 22, 2025

View reviewed changes

calderpg-tri approved these changes Sep 22, 2025

View reviewed changes

IanTheEngineer merged commit d4db65a into RobotLocomotion:main Sep 22, 2025
4 checks passed

gergondet-woven mentioned this pull request Nov 25, 2025

Reproducible and faster generation #403

Open

Uh oh!

Conversation

hidmic commented Jul 30, 2025 • edited by EricCousineau-TRI Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hidmic commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hidmic commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwnimmer-tri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calderpg-tri commented Aug 6, 2025

Uh oh!

jwnimmer-tri left a comment

Choose a reason for hiding this comment

Uh oh!

ggould-tri left a comment

Choose a reason for hiding this comment

Uh oh!

ggould-tri left a comment

Choose a reason for hiding this comment

Uh oh!

ggould-tri left a comment

hidmic commented Jul 30, 2025 •

edited by EricCousineau-TRI

Loading

hidmic commented Jul 30, 2025 •

edited

Loading

hidmic commented Aug 6, 2025 •

edited

Loading

hidmic Aug 6, 2025 •

edited

Loading