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

Create xacro Bazel module #350

Draft
wants to merge 22 commits into
base: ros2
Choose a base branch
from
Draft

Create xacro Bazel module #350

wants to merge 22 commits into from

Conversation

mjcarroll
Copy link
Member

No description provided.

Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
@mjcarroll mjcarroll self-assigned this Dec 18, 2024
@mjcarroll
Copy link
Member Author

@rhaschke I talked with @codebot about this addition, and he seemed generally inclined to take it. For context, we use the bazel build system and xacro together currently, but would like to land it in the bazel central registry (https://github.com/bazelbuild/bazel-central-registry) so that people can quickly add xacro to their bazel projects.

This is also currently being done in drake (https://github.com/RobotLocomotion/drake/tree/master/tools/workspace/ros_xacro_internal), so maybe an opportunity to condense to a single set of rules (@jwnimmer-tri)

@mjcarroll
Copy link
Member Author

Note that this is still wip as I haven't gotten the tests running under bazel yet, but wanted to start the conversation.

@jwnimmer-tri
Copy link

Thanks for the ping!

Yes, if xacro were published in the bazel central registry then Drake would probably switch to using that module definition (assuming it works equally well to our current solution).

If I can help out (e.g., if there are questions about how bazel works), just let me know.

@rhaschke
Copy link
Contributor

I never used bazel before. Are there plans to bazalize a large set of ROS2 packages or is this only for xacro?
When including bazel build rules, we should also have a CI test validating them.

Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
@mjcarroll
Copy link
Member Author

Are there plans to bazalize a large set of ROS2 packages or is this only for xacro?

I don't have any plans to do more than xacro at the moment. We mostly want to be able to run it at build time in bazel so that it integrates cleanly.

There is recurring interest in the broader ROS community for bazel and some community supported repos (https://github.com/mvukov/rules_ros, https://github.com/ApexAI/rules_ros) as well as Bazel users in adjacent projects like Drake and Gazebo.

I'm happy to find a reviewer who is familiar and I will add CI as well.

Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
@mjcarroll
Copy link
Member Author

Following up to add CI: #351

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Can we avoid cluttering the root folder with all those bazel files?
Is it possible to move them into a subfolder?

@rhaschke rhaschke mentioned this pull request Dec 20, 2024
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
@mjcarroll
Copy link
Member Author

Can we avoid cluttering the root folder with all those bazel files?

I moved all of the ones possible into a subdirectory. MODULE.bazel and BUILD.bazel files have to appear at the root, much like a CMakeLists.txt, so they aren't possible to move.

The .bazelversion file helps with automatic version tooling like https://github.com/bazelbuild/bazelisk and the CI workflow that I'm using here. It must also appear in the root, but since it's a dotfile, I'm hoping is less of an inconvenience?

Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
@mjcarroll
Copy link
Member Author

@jwnimmer-tri I think I'm pretty happy with the rules and tests that I have created here, mind to take a look from a bazel perspective?

I have also attempted to replace the drake impl here: RobotLocomotion/drake#22341

BUILD.bazel Outdated Show resolved Hide resolved
bazel/defs.bzl Outdated Show resolved Hide resolved
bazel/defs.bzl Outdated Show resolved Hide resolved
bazel/defs.bzl Outdated Show resolved Hide resolved
* Remove src glob
* Add provides
* Trim extension based on string length
* Directly expose xacro_file rule

Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
.github/workflows/bazel.yml Outdated Show resolved Hide resolved
.github/workflows/bazel.yml Show resolved Hide resolved
MODULE.bazel Outdated Show resolved Hide resolved
MODULE.bazel Outdated Show resolved Hide resolved
mjcarroll and others added 4 commits December 20, 2024 12:02
Co-authored-by: Marcel <maleo@google.com>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

I don't yet understand the purpose of open_bazel. Is that to replace $(find pkg) with a search in the XACRO_INPUTS environment variable? If so, why not make it more explicit and introduce a new find directive, e.g. $(find bazel).
In any case, this feature needs to be better documented.

@mjcarroll
Copy link
Member Author

I don't yet understand the purpose of open_bazel. Is that to replace $(find pkg) with a search in the XACRO_INPUTS environment variable?

Since bazel does all builds in a hermetically-sealed workspace (essentially chroot), you can't refer to the files with their absolute filesystem paths. Instead, you have to pre-declare in the build the data files you intend to use and then refer to them via their bazel paths (prefixed with // in same repo or @ across repo).

I wouldn't be opposed to adding a bazel-specific directive, though.

@mbeards
Copy link

mbeards commented Dec 21, 2024

I don't yet understand the purpose of open_bazel. Is that to replace $(find pkg) with a search in the XACRO_INPUTS environment variable?

Since bazel does all builds in a hermetically-sealed workspace (essentially chroot), you can't refer to the files with their absolute filesystem paths. Instead, you have to pre-declare in the build the data files you intend to use and then refer to them via their bazel paths (prefixed with // in same repo or @ across repo).

I wouldn't be opposed to adding a bazel-specific directive, though.

Does the absolute path of the input files end up baked into the output? If not, we could make the invoking rule copy or symlink the inputs into a single directory and invoke xacro on those files, rather than trying to make xacro's internal path resolution support Bazel's semantics.

(I think in the gRPC logic https://github.com/grpc/grpc/pull/27275/files we're doing something ~ similar to handle x-repo Python codegen.)

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