rules_ros icon indicating copy to clipboard operation
rules_ros copied to clipboard

feat: port to bzlmod

Open finn-ball opened this issue 10 months ago • 1 comments

Port project to bzlmod: https://github.com/mvukov/rules_ros/issues/13

Currently relies on a branch from this PR, hence in draft: https://github.com/nelhage/rules_boost/pull/553

finn-ball avatar Mar 26 '24 11:03 finn-ball

Do we need to specify gflags or shall we remove this as a dependency?

finn-ball avatar Mar 26 '24 11:03 finn-ball

Since my changes for rules_boost have been merged, this is now ready for review.

finn-ball avatar May 27 '24 08:05 finn-ball

Do we know the exact version of boost that is added here? ROS is very picky about the exact version of boost. For noetic it must be 1.71

hofbi avatar May 27 '24 09:05 hofbi

Do we know the exact version of boost that is added here? ROS is very picky about the exact version of boost. For noetic it must be 1.71

rules_boost doesn't currently allow you to pick a version easily. rules_ros brings in whatever version of boost rules_boost defines: https://github.com/mvukov/rules_ros/blob/06e8ede950248825ac6da43b027e1342ca18db18/repositories/deps.bzl#L13 Which is pinned in rules_boost here: https://github.com/nelhage/rules_boost/blob/83ce910e5a266a08bd5634e8ab480d6c2e32a571/boost/boost.bzl#L118 I assume one could override this manually in their project. Though my changes currently are in line with what this ruleset provides.

Someone had made a PR in rules_boost in which attempted to allow the user to choose a boost version: https://github.com/nelhage/rules_boost/pull/557

This would be, in my opinion, a welcome change if it worked.

finn-ball avatar May 27 '24 09:05 finn-ball

Picking the correct verison, 1.71 in this case, should be required to make it work correctly. I don't think ROS will work with 1.84

hofbi avatar May 27 '24 09:05 hofbi

I'd suggest that be a separate issue/PR as this PR copies exactly the same mechanism rules_ros currently uses, though instead of bringing it in via the deps.bzl as noted above:

load("@com_github_nelhage_rules_boost//:boost/boost.bzl", "boost_deps")

def ros_deps():
    """ Sets up ROS deps.
    """
    boost_deps()

It defines it in the MODULE file instead:

non_module_boost_repositories = use_extension("@rules_boost//:boost/repositories.bzl", "non_module_dependencies")
use_repo(
    non_module_boost_repositories,
    "boost",
)

finn-ball avatar May 27 '24 09:05 finn-ball

Makes sense.

Last small comment. How about paths such as in https://github.com/mvukov/rules_ros/blob/main/third_party/ros/roslaunch/deps.py.tpl#L1? Do they have to change with bzlmod?

hofbi avatar May 27 '24 12:05 hofbi

Makes sense.

Last small comment. How about paths such as in https://github.com/mvukov/rules_ros/blob/main/third_party/ros/roslaunch/deps.py.tpl#L1? Do they have to change with bzlmod?

done

finn-ball avatar May 27 '24 13:05 finn-ball

Thanks for the work, folks. Once this is done, I can add rules_ros to Bazel Central Registry (https://registry.bazel.build)

Filed an issue there to track this.

udaya2899 avatar Jun 10 '24 15:06 udaya2899

Why is nobzlmod deleted? Maybe there are some folks around who would still like to use this in nobzlmode.

mvukov avatar Jun 16 '24 13:06 mvukov

Why is nobzlmod deleted? Maybe there are some folks around who would still like to use this in nobzlmode.

The deletion of --noenable_bzlmod only directly affects building and testing inside this repository. There's nothing to stop a non-bzlmod project depending on this one. The change just means the project will actually be using the changes I introduced.

Regardless of the above, bzlmod is currently the default for bazel and WORKSPACE is being depreciated in the next major release of bazel.

finn-ball avatar Jun 17 '24 08:06 finn-ball

Friendly nudge on this PR. @mvukov - Would be great to have this merged.

udaya2899 avatar Jun 21 '24 10:06 udaya2899

I don't think there is anything left for me to resolve in this PR. As far as my testing is concerned, it works and is ready to merge.

finn-ball avatar Jun 27 '24 10:06 finn-ball

Regardless of the above, bzlmod is currently the default for bazel and WORKSPACE is being depreciated in the next major release of bazel.

In my eyes, this is not a good enough reason to ditch workspace workflow, in particular if there are users of the workspace workflow who are depending on this feature. I asked @hofbi to weigh in.

I don't use this repo personally TBH (working on and using rules_ros2), so users have to weigh in.

mvukov avatar Jun 30 '24 19:06 mvukov

Also relevant comment https://github.com/mvukov/rules_ros2/pull/344#issuecomment-2198651329

mvukov avatar Jun 30 '24 19:06 mvukov

@hofbi To my knowledge you're so far the only one using actively this repo. Are you OK removing non-bzlmod support in this PR?

@mvukov thanks for asking. We are already on bzlmod, so dropping the legacy workspace is totally fine with me.

hofbi avatar Jun 30 '24 19:06 hofbi

In my eyes, this is not a good enough reason to ditch workspace workflow, in particular if there are users of the workspace workflow who are depending on this feature. I asked @hofbi to weigh in.

As I said before, WORKSPACE users can still depend on this repository, even with that flag removed. That flag just means that when executing the tests inside this repository, it uses the default bzlmod. Regardless, bazel 8 users won't be able to use the WORKSPACE workflow anyway and I'd expect that flag to be removed in the next major release.

finn-ball avatar Jul 01 '24 08:07 finn-ball

@hofbi To my knowledge you're so far the only one using actively this repo. Are you OK removing non-bzlmod support in this PR?

We recently started using this repo as well and are also on Bzlmod already. So looking very much forward to see this merged (and added to BCR afterwards).

mering avatar Jul 01 '24 09:07 mering

As I said before, WORKSPACE users can still depend on this repository, even with that flag removed. That flag just means that when executing the tests inside this repository, it uses the default bzlmod. Regardless, bazel 8 users won't be able to use the WORKSPACE workflow anyway and I'd expect that flag to be removed in the next major release.

https://bazel.build/about/roadmap#bzlmod:-external: in 8 workspace is still going to work, in 9 is going to be removed.

mvukov avatar Jul 02 '24 06:07 mvukov

Thanks for the hard work. I'll test this ASAP and merge the PR if all green.

mvukov avatar Jul 02 '24 06:07 mvukov

Would it still make sense to setup a CI for this project to simplify the verification for the other and potential future PRs?

hofbi avatar Jul 03 '24 17:07 hofbi