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

catkin_package DEPENDS libs are imported by LOCATION only #1141

Open
devrite opened this issue Apr 15, 2021 · 4 comments · May be fixed by #1142
Open

catkin_package DEPENDS libs are imported by LOCATION only #1141

devrite opened this issue Apr 15, 2021 · 4 comments · May be fixed by #1142

Comments

@devrite
Copy link

devrite commented Apr 15, 2021

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:

catkin_replace_imported_library_targets(_PKG_CONFIG_LIBRARIES ${_PKG_CONFIG_LIBRARIES})

In the case of non windows targets this line is probably being executed:

get_target_property(${lib}_imported_location_${cfg} ${lib} IMPORTED_LOCATION_${cfg})

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
Copy link
Author

devrite commented Apr 16, 2021

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
Copy link
Author

devrite commented Apr 17, 2021

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
Copy link

hgaiser commented Dec 14, 2021

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
Copy link
Author

devrite commented Dec 14, 2021

I think it may be solved in ament with ament_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).

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 a pull request may close this issue.

2 participants