feat: use modern cmake

[wep21]

• update CMakeLists.txt to use modern cmake

[wep21]

@jonbinney Could you review this PR?

[jonbinney]

Is this needed yet? Looking at this page, humble hawksbill only needs cmake 3.14 on macos, and 3.18.4 on debian bullseye, so this would presumably break things for those platforms.

What advantages does this change provide?

[Ryanf55]

Hello,

This is a great improvement. Currently, to consume filters, you have to rely on the old-style CMake variable to get the include directories:

• filters_INCLUDE_DIRS

This is because the filters library does not use ament_export_targets.

It is much preferable for consumers to just have to use find_package and then target_link_libraries to an exported namespace target.

The ROS tutorials now share that ament_export_include_directories is not necessary when using ament_export_targets.
Reference: https://docs.ros.org/en/humble/How-To-Guides/Ament-CMake-Documentation.html#installing

We both have made similar changes improvements across a suite of ROS 2 packages with no reports of bugs.

[Ryanf55]

Instead of repeating this call, consider using a for loop and giving a list of targets to set up the includes for.
This would reduce code duplication.

[wep21]

address here

[Ryanf55]

Can you specify PUBLIC or PRIVATE for the linkage type?

[wep21]

address here

[wep21]

@jonbinney I appreciate it if you could decide whether to merge this PR based on the advantages described by @Ryanf55.

[jonbinney]

@Ryanf55 thank you for reviewing this, and @wep21 thanks for making the changes. This looks good now, merging!

[Ryanf55]

I'm not sure how this works, but I just added the debian testing sources to my machine.
https://docs.ros.org/en/rolling/Installation/Testing.html#debian-testing-repository

After and apt update and apt upgrade, I still have the following version:

ros-rolling-filters/jammy,now 2.1.0-4jammy.20240216.171214 amd64 [installed]
  This library provides a standardized interface for processing data as a sequence of filters.

To use these changes as binaries in grid_map, does there need to be a new release of filters? Or, do we just need to wait more time?

https://index.ros.org/p/filters/github-ros-filters/#rolling

[wep21]

@Ryanf55 I think we have to wait bloom release to use new change.
@jonbinney Could you create a new tag and do bloom-release?

[jonbinney]

Yeaaaaah it needs to be released for other released packages to use it. I'll do that now for rolling. Any pressing need to do it for the other distros right now? If not, i'd like to wait and just make sure it doesn't break anything (it shouldn't, but I am constantly surprised by how minor changes can break things unexpectedly)

[Ryanf55]

Not an issue to let it simmer on rolling. We are doing a CMake upgrade in grid_map, so it will needed on humble, but not urgent.

…

On Mon, Feb 26, 2024, 2:18 PM Jonathan Binney ***@***.***> wrote: Yeaaaaah it needs to be released for other released packages to use it. I'll do that now for rolling. Any pressing need to do it for the other distros right now? If not, i'd like to wait and just make sure it doesn't break anything (it shouldn't, but I am constantly surprised by how minor changes can break things unexpectedly) — Reply to this email directly, view it on GitHub <#70 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF7DFD2SVXUQBG47QGXM6G3YVT3YVAVCNFSM6AAAAABDF3BE36VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRVGMYDGOJTGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[jonbinney]

When building filters and laser_filters together in a rolling-desktop docker container, I'm getting a warning:

Starting >>> laser_filters                         
--- stderr: laser_filters                                                                             
CMake Warning at /ws/install/filters/share/filters/cmake/ament_cmake_export_libraries-extras.cmake:116 (message):
  Package 'filters' exports library 'filters' which couldn't be found                                 
Call Stack (most recent call first):                                                                  
  /ws/install/filters/share/filters/cmake/filtersConfig.cmake:41 (include)                            
  /opt/ros/rolling/share/ament_cmake_auto/cmake/ament_auto_find_build_dependencies.cmake:67 (find_package)
  CMakeLists.txt:9 (ament_auto_find_build_dependencies)
                                                   
                                                                                                      
---                                                                                                   
Finished <<< laser_filters [16.4s]          

Digging deeper now to see if this is some odd quirk of ament_camke_auto, or some leftover cached cmake issue, or something else.

[jonbinney]

(not saying that error is related to this PR, but i want to debug it before releasing)

[jonbinney]

Ah, it looks like this PR adds ament_export_libraries(${PROJECT_NAME}) but there isn't an actual library by that name. Thoughts?

[Ryanf55]

Ah, it looks like this PR adds ament_export_libraries(${PROJECT_NAME}) but there isn't an actual library by that name. Thoughts?

Remove it. The targets are necessary for export are already exported by ament_export_targerts. I can review the PR, or do the work and have you review it.
Sorry for the oversight.

[jonbinney]

No worries - @Ryanf55 could you make the PR? I'll review and merge.