search

UniversalRobots / Universal_Robots_Client_Library

A C++ library for accessing the UR interfaces that facilitate the use of UR robotic manipulators by external applications.
Apache License 2.0
117 stars 88 forks source link

Always install package.xml file #78

Closed fmauch closed 3 years ago

fmauch commented 3 years ago

As this package's build type is plain cmake, catkin is not necessarily available at build time. Hence, checking for catkin before installing the package.xml doesn't make any sense as this will never work for the buildfarm builds.

Installig the package.xml file in every case won't hurt in non-ROS environments.

When the package.xml file is not installed, toolings like rospack will not be able to find the package.

@puck-fzi @urmahp opinions on this?

puck-fzi commented 3 years ago

LGTM.

I don't see any problems with always installing. The check for catkin or ament_cmake seemed more like a workaround to see if the system already supports ROS or ROS2. But not necessarily if the lib should be used in that context anyway

fmauch commented 3 years ago

Thanks for looking at this @gavanderhoorn, @puck-fzi

I just don't quite understand why the noetic builds fail with this change, while they succeed with the master branch atm.

gavanderhoorn commented 3 years ago

What about registering the package in the ROS 2 ament/resource index?

fmauch commented 3 years ago

What about registering the package in the ROS 2 ament/resource index?

From what I understand the docs this would be an alternative to installing the resource folder to the package's share location as we currently do.

However, I assume that we should still install the package.xml file, so this won't be a true alternative to this PR, right?

Also, I'd like to keep this repository as independent from ROS/ROS2 as possible (also with as little CMake conditionals as possible.)

Also, we need a solution working for ROS1.

P.S.: I am myself not very familiar to ROS2 (yet), so please correct me if I am saying something stupid about ROS2 :-)

gavanderhoorn commented 3 years ago

From what I understand the docs this would be an alternative to installing the resource folder to the package's share location as we currently do.

I'm not sure I understand. The ament index is basically a registry containing information on which locations should be considered when looking for certain resources. This was introduced to reduce the amount of fs crawling which is the default in ROS 1 (by rospkg fi).

For packages to be 'findable' by ROS 2 tooling, registration in the index is required.

The rationale for doing that with the package in this repository would seem to be similar to what you're doing in this PR (ie: to be able to offer resources which can be located relative to the package, instead of by absolute paths).

However, I assume that we should still install the package.xml file, so this won't be a true alternative to this PR, right?

it's not an alternative to anything :)

It's similar in purpose (see above).

If the client library is purely a CMake project and you don't want to offer resources which should be locatable relative to a "ROS package" (whether ROS 1 or ROS 2), then there's no point in registering. But seeing as you're going to install your manifest, I believe you do want to do that.

Note that "registration" basically comes down to placing a marker file in a certain location (like this).

fmauch commented 3 years ago

Note that "registration" basically comes down to placing a marker file in a certain location (like this).

Thanks for the elaboration.

fmauch commented 3 years ago

After spending some time debugging this, the problem seems that as soon as find_package(catkin QUIET) is removed from the CMakeLists.txt file rosdep in the downstream workspace seems to not recognize this package anymore (if being built with colcon) and instead installs the released binary apt package. Then, there seem to be shadowing issues with the installed binary package...

When changing the builder to catkin_tools the released version doesn't get installed and everything works as expected.

I am unsure how to proceed at this point...

gavanderhoorn commented 3 years ago

After spending some time debugging this, the problem seems that as soon as find_package(catkin QUIET) is removed from the CMakeLists.txt file rosdep in the downstream workspace seems to not recognize this package anymore (if being built with colcon) and instead installs the released binary apt package. Then, there seem to be shadowing issues with the installed binary package...

on systems with ROS 2, that's because the package is not registered with the ament index, which is what rosdep uses there.

Well, it's slightly more complicated then that. The whole rosdep with the resource index etc is "not nice" right now.

See also the lines in the CMake boilerplate I linked earlier.

fmauch commented 3 years ago

To bring this forward, I will set the builder for noetic to catkin tools, as this error seems to only occur when using an underlay workspace that has been built using colcon.

Building against the released package or against the package being in the same workspace or using an underlay workspace built with catkin tools seems to work.