ros / catkin

A CMake-based build system that is used to build all packages in ROS.
http://wiki.ros.org/catkin
BSD 3-Clause "New" or "Revised" License
321 stars 280 forks source link

catkin_package DEPENDS libs are imported by LOCATION only #1141

Open devrite opened 3 years ago

devrite commented 3 years ago

External system libraries are exported via catkin_package DEPENDS to dependees via IMPORT_LOCATION only.

For example an external cmake project installed via an debian package with an cmake auto generated export config of the following form:

add_library(external SHARED ...)
#will generate libexternal.so pointing to .so.1
#with .so.1 pointing to .so.1.2.3
set_target_properties(external PROPERTIES VERSION 1.2.3 SOVERSION 1)

#Do cmake like auto export with commands like these
install(TARGETS external EXPORT external-export)
install(EXPORT external-export ..

#will generate some external-export.cmake external-export-release.cmake
#with maybe a line like this:
set_target_properties(external PROPERTIES
  IMPORTED_LOCATION_RELEASE "${_IMPORT_PREFIX}/lib/libexternal.so.1.2.3"
  IMPORTED_SONAME_RELEASE "libexternal.so.1"
  )

Let's say the ros package 'external_ros' forwards external via depends. This package is backwards compatible and a newer version maybe installed than a ros package was linked against at build time. The package itself faces no problem as it was correctly linked and will accept any newer package providing the .so.1 file.

But any package that now depends on external_ros as build dependency will now receive the libexternal.so.1.2.3 in their list although it is not present nor "installable". If the SONAME were queried if present the issue would not arise.

I think this happens when this is being called for external targets: https://github.com/ros/catkin/blob/a9672d78ec483c3a991a051c365da329fceaa9f2/cmake/catkin_package.cmake#L258

In the case of non windows targets this line is probably being executed: https://github.com/ros/catkin/blob/a9672d78ec483c3a991a051c365da329fceaa9f2/cmake/catkin_libraries.cmake#L160

I do not know if there are any side effects for other platforms like windows otherwise I would have committed a pr. Also I have not checked if this affects ament too, but I guess it probably might be an issue there as well. Without a fix I can only recommend for custom libs using a fixed lib version or just the short soversion.

devrite commented 3 years ago

The merge request #1142 should not affect the WIN32 path and I tested it on melodic which indeed switches to the SONAME if available. For me it looks like using the find_library mechanism is the better solution here but maybe someone knows another way to achieve the same.

BR,

Markus

PS: If you guys need an working example project (with an external library project) I can upload an archive to this issue or somewhere else.

devrite commented 3 years ago

Just as a remark for ament: just checked that depending on the external project type being imported it will do the same.

If you private link the library and export it via ament_export_libraries you will get the same hard coded library version problem as above. If you public link it will be exported as target dependency meaning the target package must execute it's own find package to have this target available. To avoid this, if your external package is compatible, you can exploit ament_export_dependency and it will be searched automatically. This requires that the target can find it. In my example I must set a hint in which case it fails without setting it globally. Cmake though offers find_dependency though which could be exploited.

So when I have time I will try fix the same in ament (or somebody else). Maybe we can also make an add on which uses find_dependency similar to ament export dependencies as this is the intended way of cmake anyway.

hgaiser commented 2 years ago

I think I'm running into this same underlying issue, but the result is a bit different. My (catkin) package depends on PCL, which in turn depends on VTK. VTK has recently (afaik) moved away from the legacy use of cmake vars for include dirs and libraries (meaning: ${VTK_INCLUDE_DIRS} is now empty). Instead, users are supposed to link to the target VTK::CommonCore for example. This will set the right include dirs by using that target and will link against the right libraries. PCL exposes these targets correctly (meaning: ${PCL_LIBRARIES} contains VTK::CommonCore).

The problem starts if you create a package that uses PCL (or probably also if you create one that uses VTK directly, though I haven't tested this). Catkin will not store the target in ${package_name}Config.cmake, but instead the absolute path to the libraries that the target uses. By not using the target, the include dirs are not set and including PCL fails with unknown VTK includes.

devrite commented 2 years ago

I think it may be solved in ament withament_export_dependencies (for some) packages with modern cmake, but I have not checked. In the case I have it is still an old style dependency.

But yeah if you go recursively over all libs/deps, that is what you end up with and is part of the same problem. I would have to recheck both catkin and ament as it has been a while since I traced down what both of them do with such targets.

I probably think that, since ROS1 will be EOL, if these issues really persist, we probably should fix them in ament (first). For my part, since the lib is in our control, we can work around it for catkin (and ament).