search

ros2 / ros2cli

ROS 2 command line interface tools
Apache License 2.0
173 stars 159 forks source link

ros2 run and ros2 pkg executables do not look for runtime executables in standard CMake GNUInstallDirs location #845

Closed Ryanf55 closed 1 year ago

Ryanf55 commented 1 year ago

Bug report

Required Info:

Steps to reproduce issue

Create a ROS package, and perform the install like so using modern CMake with GNUInstallDirs

include(GNUInstallDirs)
install(
  TARGETS ${PROJECT_NAME} ${PROJECT_NAME}_node
  EXPORT ${PROJECT_NAME}Export
  FILE_SET HEADERS
  INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
)
ament_export_targets(${PROJECT_NAME}Export HAS_LIBRARY_TARGET)
ament_export_dependencies(
  foobar
)

install(DIRECTORY launch/
  DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/launch
)

ament_package()

Expected behavior

A call to ros2 executables list PROJECT_NAME should show PROJECT_NAME_node above which resides in install/PROJECT_NAME/bin/PROJECT_NAME_node

Actual behavior

A call to ros2 executables list PROJECT_NAME does not show a node

Additional information

According to the GNU directory standard: The directory for installing executable programs that users can run. This should normally be /usr/local/bin. ROS does not look here for nodes.

My ROS answers post from a while ago

CMake GnuInstallDirs Deep CMake for library authors slide 107

The actual cause for ros2 pkg executables is here: https://github.com/ros2/ros2cli/blob/4523cc5a1a8bb6800dafa62f9f6379023ea7ede3/ros2pkg/ros2pkg/api/__init__.py#L44 The cause for people using the wrong directory is ros2 pkg create also puts them in the wrong place: https://github.com/ros2/ros2cli/blob/4523cc5a1a8bb6800dafa62f9f6379023ea7ede3/ros2pkg/ros2pkg/resource/cmake/CMakeLists.txt.em#L90 Here is the relevant screenshot from docs:

image

clalancette commented 1 year ago

For better or worse, this is by choice and by design.

First, let me address the use of the lib directory. In all reality, according to FHS that should be libexec; we made a mistake when we originally did this work, and we've never fixed it (it would also probably be somewhat painful to fix). So even though I'll say lib below to match what is there, conceptually it should be libexec.

Second, we made these decisions back in 2017 or 2018 on where to store the binaries. So my memory is somewhat fuzzy. But as I recall, there is a very major reason for our current setup:

Because ROS 2 is highly federated, it is very possible (and maybe even common) for multiple packages to create the exact same named binary. In our current lib setup, this works fine; binaries are effectively namespaced by their package name.

If we were to put them all in bin, then we could easily run into collisions. In an isolated build (the default when building from source on Linux or macOS), this wouldn't matter; every package would install them to install/<pkgname>/bin directory, and they would avoid each other. Unfortunately, that would not be the case for merged installs (which we use on Windows), nor on our debian packages (where all packages are installed to /opt/ros/<distroname>.

@ros2/team that's my (5 year old) recollection of why we made this decision; is there anything else I'm missing?

In order to make any changes here, we'd have to come up with an alternative way to avoid that particular problem.

Ryanf55 commented 1 year ago

I'd be happy to entertain the idea of installing runtime program binaries (nodes) to libexec and using defaults for the rest of the install command. I have a working patch to ros2cli that allows it to find and run nodes placed in libexec/PACKAGE_NAME without any behavioral change to other packages that are currently using bin.

This could be more clear to developers writing the CMake code than the current method.

sloretz commented 1 year ago

that's my (5 year old) recollection of why we made this decision; is there anything else I'm missing?

I remember the same thing.

According to the GNU directory standard: The directory for installing executable programs that users can run. This should normally be /usr/local/bin. ROS does not look here for nodes.

IIRC part of the discussion was a decision that the executables aren't meant to be run directly by the user. They are meant to be run by another program like ros2 run ... or ros2 launch ....

First, let me address the use of the lib directory. In all reality, according to FHS that should be libexec;

It looks like both are allowed:

Some previous versions of this document did not support /usr/libexec, despite it being standard practice in a number of environments. [[26]](https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s07.html#ftn.idm236091914528) To accomodate this restriction, it became common practice to use /usr/lib instead. Either practice is now acceptable, but each application must choose one way or the other to organize itself.
clalancette commented 1 year ago

I'd be happy to entertain the idea of installing runtime program binaries (nodes) to libexec and using defaults for the rest of the install command

We can't exactly do that, anyway. Due to another problem with overlays, we also need to install headers to include/<pkgname>/<pkgname>: see https://github.com/ros2/ros2/issues/1150 .

Ryanf55 commented 1 year ago

Fair enough. What about constraining any changes to allow installing nodes in libexec/PACKAGE_NAME.

clalancette commented 1 year ago

Fair enough. What about constraining any changes to allow installing nodes in libexec/PACKAGE_NAME.

Given what @sloretz posted above, I'm not really inclined to make the change at this time. That is, it doesn't seem any more "correct" than lib and would result in a bunch of churn.

Ryanf55 commented 1 year ago

It also doesn't help the ament_cmake docs tell you to install your nodes in the wrong path.

https://docs.ros.org/en/humble/How-To-Guides/Ament-CMake-Documentation.html?highlight=ament_cmake#building-a-library

It says to set the runtime destination for binaries to lib instead of lib/${PROJECT_NAME}, meanwhile the ROS tutorial's ros2 pkg create --build-type ament_cmake --node-name my_node my_package CLI generates the following from the example. 

install(TARGETS my_node
  DESTINATION lib/${PROJECT_NAME})

Turtlesim, used in the tutorials, has this code, where it install the targets directly instead of using the export set.

install(TARGETS turtlesim_node turtle_teleop_key draw_square mimic
  DESTINATION lib/${PROJECT_NAME})
clalancette commented 1 year ago

It also doesn't help the ament_cmake docs tell you to install your nodes in the wrong path.

https://docs.ros.org/en/humble/How-To-Guides/Ament-CMake-Documentation.html?highlight=ament_cmake#building-a-library

Oof, yeah, good call. That should really be corrected; I'll open a PR to do that.

Turtlesim, used in the tutorials, has this code, where it install the targets directly instead of using the export set.

Can you explain more about what you think the problem there is? As far as I can tell that is installing things like we expect.

Ryanf55 commented 1 year ago

I can do the work, no worries. I'll tag you when it's fixed, there's much cleaner ways such as using an export set to do the install. The following recommendations for includes in colcon are not obeyed in the ament_cmake tutorial. https://colcon.readthedocs.io/en/released/user/overriding-packages.html#install-headers-to-a-unique-include-directory