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

Put binary "zenoh-bridge-dds" into bin folder while colcon build #134

Merged
merged 2 commits into from
Aug 7, 2023

Conversation

evshary
Copy link
Contributor

@evshary evshary commented May 31, 2023

After running ROS 2 colcon build, the binary (zenoh-bridge-dds) is put under install/zenoh_bridge_dds/lib/ folder, which can not be used directly when we source install/setup.bash
I move the binary to install/zenoh_bridge_dds/bin/ and also rename the binary name to zenoh-bridge-dds (The same name if we build with cargo build).

@JEnoch
Copy link
Member

JEnoch commented Jun 15, 2023

Won't it cause troubles to ROS 2 environment or users to have a binary name (zenoh-bridge-dds) which is different than the ROS 2 package name (zenoh_bridge_dds) ?

@evshary
Copy link
Contributor Author

evshary commented Jun 16, 2023

Won't it cause troubles to ROS 2 environment or users to have a binary name (zenoh-bridge-dds) which is different than the ROS 2 package name (zenoh_bridge_dds) ?

I'm not sure whether there is any rule about the binary name. The reason I want to change the name is that I want to align the binary name in ROS package and Rust cargo build. It takes me some time to find out they are different names.

If you think using zenoh_bridge_dds is better, I'm OK with that change. Maybe from ROS users' perspective, it's more intuitive to use the same name as the package name.

@JEnoch
Copy link
Member

JEnoch commented Jun 19, 2023

We dug into the ROS 2 docs and here is what we found:

Naming conventions (REP 144)

A ROS 2 package name only consist of lowercase alphanumerics and _ separators and start with an alphabetic character. Thus the package name shall remain zenoh_bridge_dds.

There is no rule for a node name, but I would feel confusing that the node name is zenoh-bridge-dds then.
For a regular ROS 2 user, the start-up command would then be:
ros2 run zenoh_bridge_dds zenoh-bridge-dds ...
I think it makes more sense to use the same name for both package and the only node inside this package.

Installation path

In all the examples I saw, the install destination is lib/${PROJECT_NAME} (see https://github.com/ros2/examples/tree/rolling/rclcpp). Actually, it seems the binaries have to be installed in here for ros2 run and ros2 launch commands to find them.
I tried your suggestion to install it in install/zenoh_bridge_dds/bin/. But then the ros2 run zenoh_bridge_dds zenoh-bridge-dds no longer works.


At the end I think the package must comply with what a ROS 2 user would expect. And I assume he will run the bridge as any other ROS 2 node, either via ros2 run, either via a launch file. But probably not calling the binary directly.
What do you think?

@evshary
Copy link
Contributor Author

evshary commented Jun 20, 2023

Oh, I was not aware that you expect users to run zenoh-bridge-dds with ros2 run zenoh_bridge_dds zenoh-bridge-dds. zenoh-bridge-dds is not exactly a regular ROS node, so I didn't think of this possibility. In this scenario, I think it's reasonable to use the same name.
However, even in ROS, there are some binaries that can be executed directly without ros2 run ..., for example, rviz2, rqt, and so on. Although they are put under the path lib, like /opt/ros/<ROS version>/lib/rviz2/rviz2, they also have a copy under bin, like /opt/ros/<ROS version>/bin/rviz2. ROS users can easily run rviz2 directly without a long command.
In my opinion, I agree with you that we should still put it under lib, but maybe having another copy under bin is more user-friendly.


Regarding the binary name, if we decide to put both under lib and bin, there are three choices:

  1. lib: zenoh_bridge_dds, bin: zenoh_bridge_dds
  2. lib: zenoh-bridge-dds, bin: zenoh-bridge-dds
  3. lib: zenoh_bridge_dds, bin: zenoh-bridge-dds

If we keep the binary under lib the same name as the package, we can omit option 2.
Option 3 can make the binary the same name as the original Rust package, but option 1 is consistent with the ROS package.
I don't have a strong preference.

@JEnoch
Copy link
Member

JEnoch commented Jun 26, 2023

I didn't know it was possible to have a binary under both lib and bin.
I think that's a good idea then.

I still think that for ROS users it makes more sense to have the binary name with the exact same name than the ROS package (i.e.: zenoh_bridge_dds). I'm afraid having 2 different names for the same binary (option 3) will bring confusion.

My vote goes for option 1.

@evshary
Copy link
Contributor Author

evshary commented Jun 27, 2023

Ok, I use another install to put the binary in bin folder.
To reduce the code duplicate, I create a macro for the install in CMakeLists.txt.

@Mallets
Copy link
Member

Mallets commented Jul 28, 2023

@evshary is this PR still alive?

@evshary
Copy link
Contributor Author

evshary commented Jul 28, 2023

Yes, I've already updated it. Waiting for @JEnoch to review.

@JEnoch JEnoch merged commit 25230a1 into eclipse-zenoh:master Aug 7, 2023
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.

3 participants