Export the right library names

[Ryanf55]

Purpose

Follow up to #70, addressing the problem of exporting the library ${PROJECT_NAME}, when no such library target existed.

Details

I tried continuing to use ament_export_libraries, but it just shouldn't be used.

Exports the bare library names that no longer exist:
ament_export_libraries(${interface_targets} ${plugin_targets})
Generates:
set(_exported_libraries "realtime_circular_buffer;filter_base;filter_chain;mean;params;increment;median;transfer_function")
Error:

CMake Error at /home/ryan/Dev/ros2_ws/src/filters/install/filters/share/filters/cmake/ament_cmake_export_libraries-extras.cmake:48 (message):
  Package 'filters' exports the library 'realtime_circular_buffer' which
  couldn't be found
Call Stack (most recent call first):
  /home/ryan/Dev/ros2_ws/src/filters/install/filters/share/filters/cmake/filtersConfig.cmake:41 (include)
  CMakeLists.txt:7 (find_package)

Ok, so try to export the new names that prepend the project.
This tidbit exports the proper library names, but ament_export_libraries doesn't handle libraries with colons in the names.

unset(_exported_libraries)
list(APPEND _exported_libraries ${interface_targets} ${plugin_targets})
list(TRANSFORM _exported_libraries PREPEND ${PROJECT_NAME}::)
ament_export_libraries(${_exported_libraries})

Generates: set(_exported_library_names "filters::realtime_circular_buffer;filters::filter_base;filters::filter_chain;filters::mean;filters::params;filters::increment;filters::median;filters::transfer_function")

But then has warnings in usage:

CMake Warning at /home/ryan/Dev/ros2_ws/src/filters/install/filters/share/filters/cmake/ament_cmake_export_libraries-extras.cmake:128 (message):
  Package 'filters' exports library 'filters' with LIBRARY_DIRS
  ';realtime_circular_buffer' which couldn't be found
Call Stack (most recent call first):
  /home/ryan/Dev/ros2_ws/src/filters/install/filters/share/filters/cmake/filtersConfig.cmake:41 (include)
  CMakeLists.txt:7 (find_package)

Solution - remove it! It is my impression that ROS does not provide a good transitional mechanism for the old-style variables.

Consumers MUST use either target_link_libraries with filters::realtime_circular_buffer or ament_target_dependencies(filters).

Logs

ryan@B650-970:~/Dev/ros2_ws/src/filters$ git checkout fix-export-libraries 
Branch 'fix-export-libraries' set up to track remote branch 'fix-export-libraries' from 'origin'.
Switched to a new branch 'fix-export-libraries'
ryan@B650-970:~/Dev/ros2_ws/src/filters$ . /opt/ros/rolling/setup.bash
ryan@B650-970:~/Dev/ros2_ws/src/filters$ colcon ubild
usage: colcon [-h] [--log-base LOG_BASE] [--log-level LOG_LEVEL] {build,extension-points,extensions,graph,info,list,metadata,mixin,test,test-result,version-check} ...
colcon: error: argument verb_name: invalid choice: 'ubild' (choose from 'build', 'extension-points', 'extensions', 'graph', 'info', 'list', 'metadata', 'mixin', 'test', 'test-result', 'version-check')
ryan@B650-970:~/Dev/ros2_ws/src/filters$ colcon build
[0.627s] WARNING:colcon.colcon_core.package_selection:Some selected packages are already built in one or more underlay workspaces:
	'filters' is in: /opt/ros/rolling
If a package in a merged underlay workspace is overridden and it installs headers, then all packages in the overlay must sort their include directories by workspace order. Failure to do so may result in build failures or undefined behavior at run time.
If the overridden package is used by another package in any underlay, then the overriding package in the overlay must be API and ABI compatible or undefined behavior at run time may occur.

If you understand the risks and want to override a package anyways, add the following to the command line:
	--allow-overriding filters

This may be promoted to an error in a future release of colcon-override-check.
Starting >>> filters 
Finished <<< filters [8.91s]                     

Summary: 1 package finished [9.37s]

$ cat install/filters/share/filters/cmake/ament_cmake_export_libraries-extras.cmake 
# generated from ament_cmake_export_libraries/cmake/template/ament_cmake_export_libraries.cmake.in

set(_exported_libraries "realtime_circular_buffer;filter_base;filter_chain;mean;params;increment;median;transfer_function")
set(_exported_library_names "")

[jonbinney]

Will filters_LIBRARIES not work anymore? It still seems to be populated when compiling this PR branch; is it expected to be incomplete?

[jonbinney]

For that matter, why link against filter_base? It's a templated hpp only library, so it shouldn't be necessary to link it as a library (or even build it as a library), right?

[jonbinney]

I get that it pulls in the libraries that are needed by the filter_base code, but in #70 there was no reason to build the filter_base shared library?

[Ryanf55]

Will filters_LIBRARIES not work anymore? It still seems to be populated when compiling this PR branch; is it expected to be incomplete?

I might have missed something. Where did you see it show up?

THat's just an example library to link against. If you want me to change it to one of the others, I'm happy to adjust.

In #70, filter_base was declared as an interface libary. Not sure what you mean "built", because it's not compiled. It's just declared so that other libraries can link to it.

[jonbinney]

Regarding filters_LIBRARIES: I added debugging to check its value after find_package'ing filters from another library. Just because I was curious if the variable still got filled in.

What I'm getting at is: if some other package has a library (my_library in your example) that uses filter_base.hpp is currently using target_link_libraries(my_library ${filters_LIBRARIES}) to pull in any libraries that filter_base.hpp needs to link against, will my_library still compile after #70 and this PR? Or will compilation break until the authors of my_library update their CMakeLists.txt to use target_link_libraries(my_library PUBLIC filters::filter_base)?

Regarding interface libraries: Clearly my cmake knowledge is rusty - after reading your comment i read up on interface libraries and it makes sense now. Thanks! :-)

[Ryanf55]

It is my understanding that this is a breaking change. That said, it has been years since the official ROS docs recommended the use of these variables. Sadly, it does not seem like they provide any way for users of ament to have a smooth deprecation path.

A few options:

• Accept the break, document it well, give heads up by not merging to humble for a while, and also use the build farm to see if any open source packages are affected by it
• Complain to the ament_cmake maintainers how there is no way to migrate off the old-style variables without a breaking change
• Add some code to filters to set the variable through an -extras.cmake file, except mark the variable as deprecated.

I've had good luck working with the ament_cmake maintainers, but I'm honestly a bit jaded about how poor they do deprecation (or lack thereof).

PS: Good on you for reading up on INTERFACE libraries. Glad you learned something new from this PR.

[jonbinney]

Releasing breaking changes within humble is a no go. I just read through ament/ament_cmake#292 , where I see you've been helping figure out the right deprecation path. As usual, the devil is in the details :-)

I propose the following:
For this PR, fill in both the old style and modern style variables. Basically make sure all dependencies of filter_base end up in filters_LIBRARIES. If I understand correctly, this will let packages which depend on the filters package migrate to modern target based dependencies, without breaking any package. This seems like a strict improvement on the status quo, and should be releasable onto humble, iron, and rolling. Does that sound reasonable to you?

I'll follow ament/ament_cmake#292 , since I do think deprecating the old style is important to avoid confusion. Doing things core packages first and working up through packages that depend on them sounds like the generally right way to go, as well as adding deprecation warnings. In ROS1, at least for deprecating functions within code, the best practice was to add a deprecation warning when releasing into a new ROS distro, and then removing the function in the following ROS distro. This is great for stability for downstream users, but I don't know if it still applies now that new distros happen less often.

[Ryanf55]

Absolutely. I appreciate you taking the time to gain some context here. The deprecation path is absolutely one we want to maintain, and it's more obvious how to do that in C++ code with a [[deprecated]] attribute. I've seen some potential paths forward in CMake to add deprecated messages to variables.

I agree with your assessment and path forward.

[Ryanf55]

In install/filters/share/filters/cmake/ament_cmake_export_dependencies-extras.cmake:89 there is this line:
list(APPEND filters_LIBRARIES "${_libraries}"). I was mistaken. With this PR, the variable is still set.

When I check the value in a consuming package of grid_map_cv, the variable filters_LIBRARIES evaluates to /usr/lib/x86_64-linux-gnu/libconsole_bridge.so.1.0;/opt/ros/rolling/lib/libclass_loader.so;console_bridge::console_bridge;/opt/ros/rolling/lib/librcutils.so;dl;/opt/ros/rolling/lib/librcpputils.so;ament_index_cpp::ament_index_cpp;class_loader::class_loader;rcutils::rcutils;rcpputils::rcpputils;tinyxml2::tinyxml2;/usr/lib/x86_64-linux-gnu/libconsole_bridge.so.1.0;/opt/ros/rolling/lib/libclass_loader.so;console_bridge::console_bridge;/opt/ros/rolling/lib/librcutils.so;dl;/opt/ros/rolling/lib/librcpputils.so;ament_index_cpp::ament_index_cpp;class_loader::class_loader;tinyxml2::tinyxml2;/opt/ros/rolling/lib/librclcpp.so;builtin_interfaces::builtin_interfaces__rosidl_generator_c;builtin_interfaces::builtin_interfaces__rosidl_typesupport_fastrtps_c;builtin_interfaces::builtin_interfaces__rosidl_typesupport_introspection_c;builtin_interfaces::builtin_interfaces__rosidl_typesupport_c;builtin_interfaces::builtin_interfaces__rosidl_generator_cpp;builtin_interfaces::builtin_interfaces__rosidl_typesupport_fastrtps_cpp;builtin_interfaces::builtin_interfaces__rosidl_typesupport_introspection_cpp;builtin_interfaces::builtin_interfaces__rosidl_typesupport_cpp;builtin_interfaces::builtin_interfaces__rosidl_generator_py;libstatistics_collector::libstatistics_collector;rcl::rcl;rcl_interfaces::rcl_interfaces__rosidl_generator_c;rcl_interfaces::rcl_interfaces__rosidl_typesupport_fastrtps_c;rcl_interfaces::rcl_interfaces__rosidl_typesupport_introspection_c;rcl_interfaces::rcl_interfaces__rosidl_typesupport_c;rcl_interfaces::rcl_interfaces__rosidl_generator_cpp;rcl_interfaces::rcl_interfaces__rosidl_typesupport_fastrtps_cpp;rcl_interfaces::rcl_interfaces__rosidl_typesupport_introspection_cpp;rcl_interfaces::rcl_interfaces__rosidl_typesupport_cpp;rcl_interfaces::rcl_interfaces__rosidl_generator_py;rcl_yaml_param_parser::rcl_yaml_param_parser;rcpputils::rcpputils;rcutils::rcutils;rmw::rmw;rosgraph_msgs::rosgraph_msgs__rosidl_generator_c;rosgraph_msgs::rosgraph_msgs__rosidl_typesupport_fastrtps_c;rosgraph_msgs::rosgraph_msgs__rosidl_generator_cpp;rosgraph_msgs::rosgraph_msgs__rosidl_typesupport_fastrtps_cpp;rosgraph_msgs::rosgraph_msgs__rosidl_typesupport_introspection_c;rosgraph_msgs::rosgraph_msgs__rosidl_typesupport_c;rosgraph_msgs::rosgraph_msgs__rosidl_typesupport_introspection_cpp;rosgraph_msgs::rosgraph_msgs__rosidl_typesupport_cpp;rosgraph_msgs::rosgraph_msgs__rosidl_generator_py;rosidl_dynamic_typesupport::rosidl_dynamic_typesupport;rosidl_runtime_c::rosidl_runtime_c;rosidl_runtime_cpp::rosidl_runtime_cpp;rosidl_typesupport_cpp::rosidl_typesupport_cpp;statistics_msgs::statistics_msgs__rosidl_generator_c;statistics_msgs::statistics_msgs__rosidl_typesupport_fastrtps_c;statistics_msgs::statistics_msgs__rosidl_generator_cpp;statistics_msgs::statistics_msgs__rosidl_typesupport_fastrtps_cpp;statistics_msgs::statistics_msgs__rosidl_typesupport_introspection_c;statistics_msgs::statistics_msgs__rosidl_typesupport_c;statistics_msgs::statistics_msgs__rosidl_typesupport_introspection_cpp;statistics_msgs::statistics_msgs__rosidl_typesupport_cpp;statistics_msgs::statistics_msgs__rosidl_generator_py;tracetools::tracetools, so it seems correct.

I will update the README to remove this comment. There is no API break; I propose this is safe for humble without any negative impact.

[jonbinney]

Thanks for your hard work on this!! I've added two small requests to add comments, and then it is ready for merging and releasing to rolling. I agree that this can go to humble as well, although I'll wait a couple weeks to do that release out of an abundance of caution.

[jonbinney]

One small request: could you add one more sentence explaining to users what depending on filters_base does? Something along the lines of "linking to filters_base pulls in all necessary libraries and include directories for targets with classes that extend FilterBase".

[jonbinney]

And also a comment for the add_library(filter_base ...) in the CMakelists.txt for old ROS folks like me who are coming across interface libraries for the first time. Something like "filter_base is a cmake "interface library" which doesn't compile anything, but can be depended on by other targets so that all needed libraries and include directories are pulled in"

[Ryanf55]

Done and done. I reworked it it a bit. Most people probably want to link to one of the pre-written filters.

[jonbinney]

I like the way you worded the comments.

[jonbinney]

LGTM, merging!

[jonbinney]

Started doing the release process, then realized that I'm missing write access to the GBP repo. Created an issue to track the release process here: #72

[wep21]

@Ryanf55 Thank you for modifying my mistake.