Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: port to bzlmod #14

Merged
merged 3 commits into from
Jul 6, 2024
Merged

feat: port to bzlmod #14

merged 3 commits into from
Jul 6, 2024

Conversation

finn-ball
Copy link
Contributor

@finn-ball finn-ball commented Mar 26, 2024

Port project to bzlmod:
#13

@finn-ball
Copy link
Contributor Author

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

.bazelversion Outdated Show resolved Hide resolved
@finn-ball finn-ball marked this pull request as ready for review May 27, 2024 08:24
@finn-ball
Copy link
Contributor Author

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

@hofbi
Copy link
Contributor

hofbi commented May 27, 2024

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
Copy link
Contributor Author

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:

boost_deps()

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:
nelhage/rules_boost#557

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

@hofbi
Copy link
Contributor

hofbi commented May 27, 2024

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
Copy link
Contributor Author

finn-ball commented May 27, 2024

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
Copy link
Contributor

hofbi commented May 27, 2024

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
Copy link
Contributor Author

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

Copy link
Owner

@mvukov mvukov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice that nelhage/rules_boost#553 is merged.

Anyway, not sure ros is picky about boost version, I am quite some it works with 1.8x used here for a while now.

@@ -1,2 +1,2 @@
ROSMASTER_PATH = 'external/ros_comm/rosmaster'
ROSMASTER_PATH = 'external/_main~non_module_dependencies~ros_comm/rosmaster'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, it's about time to auto-generated this path instead of hard-coding it. The same for the change in third_party/ros/roslaunch/roscore.xml.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to schedule some time for this in the coming days.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea as the name _main~non_module_dependencies will change depending on what they called it in the MODULE file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#18

Done

@udaya2899
Copy link

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
Copy link
Owner

mvukov commented Jun 16, 2024

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

@finn-ball
Copy link
Contributor Author

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 finn-ball force-pushed the finn/bzl-mod branch 2 times, most recently from 6d23557 to 236d4f5 Compare June 17, 2024 08:17
@udaya2899
Copy link

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

@finn-ball
Copy link
Contributor Author

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.

Copy link
Owner

@mvukov mvukov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"boost",
)

python = use_extension("@rules_python//python/extensions:python.bzl", "python")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my other project rules_ros2, I suggest rules_python should be defined as dev dep here and should be set up in the user repo, mvukov/rules_ros2#238 (comment).

I'd say the same applies here. We should not constrain our users which Python they are going to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@mvukov
Copy link
Owner

mvukov commented Jun 30, 2024

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
Copy link
Owner

mvukov commented Jun 30, 2024

Also relevant comment mvukov/rules_ros2#344 (comment)

@hofbi
Copy link
Contributor

hofbi commented Jun 30, 2024

@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
Copy link
Contributor Author

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
Copy link

mering commented Jul 1, 2024

@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
Copy link
Owner

mvukov commented Jul 2, 2024

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
Copy link
Owner

mvukov commented Jul 2, 2024

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

MODULE.bazel Outdated
Comment on lines 18 to 35
"console_bridge",
"roscpp_core",
"rosconsole",
"ros_genmsg",
"ros_gencpp",
"ros_genpy",
"ros_std_msgs",
"ros_comm_msgs",
"ros_comm",
"ros_ros",
"ros_common_msgs",
"ros_actionlib",
"ros_dynamic_reconfigure",
"ros_geometry2",
"orocos_kdl",
"urdfdom_headers",
"tinyxml",
"urdfdom",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the ugly part, but nothing we can do about that. I wish there was a way to import a variable from a repo.

@hofbi
Copy link
Contributor

hofbi commented Jul 3, 2024

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

Copy link
Owner

@mvukov mvukov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@mvukov mvukov merged commit 94aaa46 into mvukov:main Jul 6, 2024
@mvukov mvukov mentioned this pull request Jul 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants