mvukov / rules_ros

Build ROS (1) with Bazel
Apache License 2.0
24 stars 12 forks source link

feat: port to bzlmod #14

Closed finn-ball closed 4 months ago

finn-ball commented 8 months ago

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

finn-ball commented 8 months ago

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

finn-ball commented 6 months ago

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

hofbi commented 6 months ago

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

finn-ball commented 6 months ago

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.

hofbi commented 6 months ago

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

finn-ball commented 6 months ago

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",
)
hofbi commented 6 months ago

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?

finn-ball commented 6 months ago

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

udaya2899 commented 5 months ago

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.

mvukov commented 5 months ago

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

finn-ball commented 5 months ago

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.

udaya2899 commented 5 months ago

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

finn-ball commented 5 months ago

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.

mvukov commented 5 months ago

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 commented 5 months ago

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

hofbi commented 5 months ago

@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.

finn-ball commented 5 months ago

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.

mering commented 5 months ago

@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).

mvukov commented 5 months ago

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 commented 5 months ago

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

hofbi commented 4 months ago

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