fix: include order for cpplint

[wep21]

fix include order to pass cpplint linter test

[wxmerkt]

Thanks for the PR - I am aware of an issue with octomap on Jammy but haven't had the time to fix it yet. I could get back to this in ~1.5 weeks

[wep21]

@clalancette @nuclearsandwich could you also review this? This PR fixes regression test failure on rolling.
@wxmerkt I found out that build with liboctomap-dev was successful, while build with ros-rolling-octomap was failed.
I guess the header path was different. By the way, why do you release the ros2 package of octomap instead of registering rosdep key of liboctomap-dev?

[clalancette]

@clalancette @nuclearsandwich could you also review this?

This looks generally fine to me, and is inline with what we had to do elsewhere when we updated the version of cpplint in use.

[nuclearsandwich]

Personally I'd usually request / recommend that the PR description be edited to describe the cpplint warnings that the change is resolving but as this isn't my project to review I'll abstain from "binding" requests.

As far as the changes themselves I don't feel qualified to evaluate them without more information.

[clalancette]

By the way, why do you release the ros2 package of octomap instead of registering rosdep key of liboctomap-dev?

I missed this last time. It's a great question; someone in the past must have wanted it when it wasn't available, or too old a version. That said, we really should be relying on the rosdep key if at all possible. I'd be for that change, though we'd have to check what depends on octomap and if they'd be happy to use the rosdep key instead.

[wxmerkt]

Hi @wep21 and all,
In the past we had to release octomap into different distros as the liboctomap-dev versions differed across some of the supported operating system combinations (per REP-3 or REP-2000). I don't think this is the case anymore.

From a quick check of available packages, I am in favour of removing ros-rolling-octomap and adding a rosdep key to the respective distribution versions.

[wxmerkt]

I looked into the underlying failure more closely and it's due to a common issue with finding OctoMap, as it only sets CMake variables with upper-case OCTOMAP by default:

find_package(octomap REQUIRED) will NOT set up the CMake variables correctly
find_package(OCTOMAP REQUIRED) will.

As thus, the ament-auto-find-and-configure in the modified ROS2 CMakeLists fails. The reason non-rolling releases are fine is that I added patches to those releases to change the libdir from lib/$arch to lib and it incidentally gets configured correctly. Similarly, the reason it works with liboctomap-dev is that the paths incidentally are correct - not that the CMake configures correctly.

My current plan of action - independent from changing the rosdep key - is to modify Octomap's config.cmake to also set the octomap_INCLUDE_DIRS etc. variables with lower-case octomap.

[nuclearsandwich]

My current plan of action - independent from changing the rosdep key - is to modify Octomap's config.cmake to also set the octomap_INCLUDE_DIRS etc. variables with lower-case octomap.

This would be great @wxmerkt. Please @ me directly if you'd like reviews.

[wxmerkt]

This has now been merged upstream (OctoMap/octomap#369), but not yet released (and not sure if/when the [Ubuntu/RHEL/etc.] distro maintainers would make a new release either?).

As a current solution, we can add target_include_directories(... PUBLIC ${OCTOMAP_INCLUDE_DIRECTORIES}) to the ROS2 branch in octomap_ros and octomap_mapping as well.

[wep21]

@wxmerkt I added workaround at cfe4783.

[wxmerkt]

Thank you very much for the swift fix @wep21, and for the review and feedback @nuclearsandwich @clalancette

[wxmerkt]

New releases are out:

• Foxy: octomap_ros: 0.4.3-1 in 'foxy/distribution.yaml' [bloom] ros/rosdistro#33184
• Galactic: octomap_ros: 0.4.3-1 in 'galactic/distribution.yaml' [bloom] ros/rosdistro#33188
• Rolling: octomap_ros: 0.4.3-1 in 'rolling/distribution.yaml' [bloom] ros/rosdistro#33189
• Humble: octomap_ros: 0.4.3-1 in 'humble/distribution.yaml' [bloom] ros/rosdistro#33190