refactor: extract rules_ros2_workspace_deps #254
Conversation
WORKSPACE
Outdated
| load("//repositories:repositories.bzl", "ros2_repositories") | ||
| load("//repositories:repositories.bzl", "ros2_repositories", "rules_ros2_workspace_deps") | ||
|
|
||
| rules_ros2_workspace_deps() |
There was a problem hiding this comment.
Can we call this within ros2_repositories()? ... and skip those calls in the workspace files?
There was a problem hiding this comment.
Well yes that's possible, but it would have to be changed back for Bzlmod -- #238 continues to use ros_repositories() for ROS 2 repos, but all the other repos like googletest, asio, websocketpp, ... should be comoing from bzlmod directly.
I think it makes sense to have them separate.
If we really want to keep full backwards compatibility and not require WORKSPACE users to update which setup method is used, we can keep the ros2_repositories name and split out the ROS 2 repos into another macro that will be called directly by the Bzlmod extension. Not a huge fan of this option though because I think the ros2_repositories name would be a misleading.
|
I'd move console_bridge, foxglove_bridge and probably also mcap to ros2_repositories() as those are ros-related. WDYT? |
e17809b to
4ae7ff8
Compare
02e22d2 to
4b7016b
Compare
repositories/repositories.bzl
Outdated
| """Imports external/third-party repositories. | ||
| """ | ||
| def rules_ros2_workspace_deps(): | ||
| """Import http_archive dependencies for the WORKSPACE version of rules_ros2.""" |
There was a problem hiding this comment.
"""Imports dependent third-party repositories for the non-blzmod (hence, workspace-) version of the repository.
In particular, imports third-party package repositories excluding ROS 2 packages. ROS 2-specific repositories are imported with `ros2_repositories()` macro.
"""
repositories/repositories.bzl
Outdated
| def ros2_repositories(): | ||
| """Imports external/third-party repositories. | ||
| """ | ||
| def rules_ros2_workspace_deps(): |
There was a problem hiding this comment.
Well, let's be consistent. I'd name this e.g. ros2_workspace_repositories(). IMO, this is still a bit ambiguous compared to ros_repositories, but the docs should do the explanation. IMHO, the suffix should be _repositories, not _deps. We use _deps for configuration of repositories.
Another alternative is to keep this as ros2_repositories and rename the other one as ros2_package_repositories() (which could be more specific).
WDYT?
There was a problem hiding this comment.
Don't care too much, the best move would be to migrate to Bzlmod and then just drop Workspace altogether :D
4b7016b to
73f0629
Compare
Extracted from / in preparation for #238
No functional or version changes in this one. Only the WORKSPACE incantation changes.