build: make #include <angles/angles.h> internal to cpp file

[felixf4xu]

PR Type

• Improvement

Related Links

Description

before this PR, angles/angles.h is included in a public header file of velodyne_scan_decoder.hpp, so that any project that uses this repo needs to include angles as well.

After this PR, angles/angles.h is moved to .cpp files, so it's internal to this repo. Other project that uses this repo does not need to find and include angles.

This can reduce compile time for both colcon/cmake and gcc.

Another minor change is the

#if defined(ROS_DISTRO_FOXY) || defined(ROS_DISTRO_GALACTIC)
#include <angles/angles.h>  //Galactic
#else
#include <angles/angles/angles.h>  //Humble
#endif

which is not necessary if angles is installed according to https://github.com/ros/rosdistro/blob/master/humble/distribution.yaml#L403

Many other ros2 projects are not using #if/#endif for humble like https://github.com/ros-planning/navigation2/blob/humble/nav2_controller/plugins/pose_progress_checker.cpp#L20

Review Procedure

build this project

Remarks

Pre-Review Checklist for the PR Author

PR Author should check the checkboxes below when creating the PR.

• Assign PR to reviewer

Checklist for the PR Reviewer

Reviewers should check the checkboxes below before approval.

• Commits are properly organized and messages are according to the guideline
• (Optional) Unit tests have been written for new behavior
• PR title describes the changes

Post-Review Checklist for the PR Author

PR Author should check the checkboxes below before merging.

• All open points are addressed and tracked via issues or tickets

CI Checks

• Build and test for PR: Required to pass before the merge.

[knzo25]

@felixf4xu
Thanks for the contribution. It really is better to move the related includes to the source file when possible (just in case, I benchmarked the PR and in both cases it took 2m 22s to build).

My only real comment would be that we still try to support old versions of ROS like galactic to I wrote the suggestions that are less likely to break the code for those versions. 🙏

[felixf4xu]

@knzo25 Thanks for the comment.

As to the code, could you verify again that in humble, the header file path is <angles/angles/angles.h>?

Actually I was building autoware on ros2 humble and I can't find the header file at the path of <angles/angles/angles.h>, and I had to change it to <angles/angles.h>. In that case, there will no difference for humble or foxy.

As an example, the rclcpp.hpp full path is /opt/ros/humble/include/rclcpp/rclcpp/rclcpp.hpp ( 2 rclcpp in the path), but when we include it, we only include <rclcpp/rclcpp.hpp> (1 rclcpp in the path).

In my ubuntu 22, angles.h is at /opt/ros/humble/include/angles/angles/angles.h (2 angles in the path), but in my case, I have to use <angles/angles.h> (1 angles in the name) to compile.

[knzo25]

Hi, sorry for the earlier response.
In the past we had issues with some changes in naming, but in this ocassion it seems it is as you mention.

Summary:
Before your PR, we were doing
#include <angles/angles/angles.h> //Humble
In our code, which is is not standard, as the real path of the headers is

kenzolobos@pcname:/opt/ros/humble/include/angles/angles$ find /opt/ros/humble/include/angles
/opt/ros/humble/include/angles
/opt/ros/humble/include/angles/angles
/opt/ros/humble/include/angles/angles/angles.h

hence, the includes should be:

#include <angles/angles.h>

The reason using our old method still works is because the actual g++ command executed includes the following line
-isystem /opt/ros/humble/include

Which allowed to include headers starting at the general ROS level

In addition to the previous g++ flag, currently the compiler is adding the following:
-isystem /opt/ros/humble/include/angles which enables standard ros includes.

As you can see from the find command above the package angles itself does not contain anything other than the headers, so it may be that in the past the correct compiler flag was not being properly added so the developers at the time fell back to using paths starting at the ros level.

I will be deleting my comments and approving the PR. Again, thanks for the contribution !

Edit: the reason that in the past we could not do the usual includes may be the lack of the include packages

[knzo25]

LGTM !