refactor: use __has_include for backward compatibility

[ralwing]

This PR implements changes from #2235

[takayuki5168]

@wep21 Does this PR make sense to you?
P.S. He said this PR seems good!

[yukkysaito]

LGTM
I think it makes sense.

[wep21]

Yes, it makes sense to me, but cppcheck caused this error in galactic when I tried the change.
https://sourceforge.net/p/cppcheck/discussion/development/thread/8596f26bbb/?limit=25
I don't know how to fix the error.

[ralwing]

@wep21 Seems like the issue in cppcheck is far from resolving it, is the inability to use cppcon a blocker?

[kenji-miyake]

@ralwing Thank you for the PR. Since I have several concerns, I'll comment on them.

First of all, as explained in https://github.com/orgs/autowarefoundation/discussions/2622, we'll move to Humble soon.
Are these changes really worth merging now? They'll be removed soon.

And considering the transitioning tasks, I'm concerned about that this PR makes the tasks a little difficult. I'll explain the reason below.

The ROS_DISTRO_ macro is also used for other than include files.
For example, it's used for to_yaml.

src/universe/autoware.universe/common/component_interface_utils/include/component_interface_utils/rclcpp/service_client.hpp
80:#ifdef ROS_DISTRO_GALACTIC
81-    using rosidl_generator_traits::to_yaml;
82-#endif
--
91:#ifdef ROS_DISTRO_GALACTIC
92-    return client_->async_send_request(request, wrapped);
93-#else

Let's think about when we remove the code for ROS distro compatibility.

If there is only the ROS_DISTRO_ macro, the worker only needs to grep ROS_DISTRO_, and then all related parts code blocks are found.

But if ROS_DISTRO_ and __has_include are mixed, the worker has to also grep _has_include, which increases the costs.
And then, it might be difficult to distinguish whether the code is for ROS distro compatibility or not.

@xmfcx What do you think? I'd like to hear your opinion.

[kenji-miyake]

(In addition, even though it's a personal preference, I don't feel it's clean if the header name is written twice.)

I know it's a standard way, but I feel we don't need to apply it to temporary ROS distro-related code.

[xmfcx]

I don't mind header file being written twice, because it is still easier than writing a macro variable and the header.

But like @kenji-miyake pointed, when we want to end galactic support, it will be hard to know what to remove.

Example

#ifdef ROS_DISTRO_GALACTIC
#include <tf2_geometry_msgs/tf2_geometry_msgs.h>
#else
#include <tf2_geometry_msgs/tf2_geometry_msgs.hpp>
#endif

turns into:

#if __has_include(<tf2_geometry_msgs/tf2_geometry_msgs.hpp> )
#include <tf2_geometry_msgs/tf2_geometry_msgs.hpp>
#else
#include <tf2_geometry_msgs/tf2_geometry_msgs.h>
#endif

To make this easy to show that we are keeping .h version for backwards compatibility, we can make it:

#if __has_include(<tf2_geometry_msgs/tf2_geometry_msgs.hpp> )
// humble and later
#include <tf2_geometry_msgs/tf2_geometry_msgs.hpp>
#else
// for galactic support
#include <tf2_geometry_msgs/tf2_geometry_msgs.h>
#endif

so that people who wants to deprecate the galactic can search for "for galactic support" to see the references.

But this makes it longer. And if __has_include is used for some other reasons like different os, compatibility for multiple libraries, maybe cuda or not, it gets hard to keep track of the reason what each __has_include stands for.

I agree it helps us remove dependency for a variable (ROS_DISTRO_GALACTIC) but it also makes the macro ambiguous. I think we shouldn't make this change.

[xmfcx]

@ralwing Even if we are to make this change, please suggest and add an identifier keyword comment like //for galactic support so people can search for it while deprecating old parts.

[kenji-miyake]

@xmfcx Thanks for the detailed comment.

And there is another reason.
Since the person who will do the transition tasks would be me, I wouldn't like to merge this and increase my tasks, unless there are any rationale for this.

[codecov]

Codecov Report

Base: 11.15% // Head: 11.08% // Decreases project coverage by -0.06% ⚠️

Coverage data is based on head (c0304e1) compared to base (7829c07).
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2236      +/-   ##
==========================================
- Coverage   11.15%   11.08%   -0.07%     
==========================================
  Files        1200     1200              
  Lines       86184    86963     +779     
  Branches    20787    20882      +95     
==========================================
+ Hits         9615     9644      +29     
- Misses      66443    67184     +741     
- Partials    10126    10135       +9     

Flag	Coverage Δ		*Carryforward flag
differential	13.92% <ø> (?)
total	11.13% <ø> (ø)		Carriedforward from 7829c07

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
...clude/autoware_auto_tf2/tf2_autoware_auto_msgs.hpp	100.00% <ø> (ø)
...ion_common/include/motion_common/motion_common.hpp	0.00% <ø> (ø)
...tion_testing/src/motion_testing/motion_testing.cpp	43.92% <ø> (ø)
...include/tier4_autoware_utils/geometry/geometry.hpp	95.23% <ø> (ø)
...ugin/src/mission_checkpoint/mission_checkpoint.cpp	0.00% <ø> (ø)
...lision_checker_node/obstacle_collision_checker.cpp	7.44% <ø> (ø)
...rsuit/include/pure_pursuit/util/planning_utils.hpp	0.00% <ø> (ø)
...ure_pursuit/include/pure_pursuit/util/tf_utils.hpp	0.00% <ø> (ø)
...ory_follower/src/longitudinal_controller_utils.cpp	83.60% <ø> (ø)
...llower/test/test_longitudinal_controller_utils.cpp	53.87% <ø> (ø)
... and 82 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[ralwing]

If I may @kenji-miyake, in my opinion refactor would be even easier, because You could simply sed all those changes, and also I've refactored code in such a way that they all have the same structure, before there were many different methods of inclusions:

#ifdef ROS_DISTRO_GALACTIC
#include <tf2_eigen/tf2_eigen.h>
#include <tf2_sensor_msgs/tf2_sensor_msgs.h>
#else
#include <tf2_eigen/tf2_eigen.hpp>

#include <tf2_sensor_msgs/tf2_sensor_msgs.hpp>
#endif

vs:

#include <tf2_eigen/tf2_eigen.h>
#include <tf2_sensor_msgs/tf2_sensor_msgs.h>
#else
#include <tf2_eigen/tf2_eigen.hpp>
#include <tf2_sensor_msgs/tf2_sensor_msgs.hpp>
#endif

This slight difference could have complicated the refactor in the future.
But saying that, I've got a compromise, let's meet somewhere in the middle:
instead of my change, I'm offering this:

#if !defined(ROS_DISTRO_GALACTIC) && __has_include ( <tf2_sensor_msgs/tf2_sensor_msgs.hpp> )
#include <tf2_sensor_msgs/tf2_sensor_msgs.hpp>
#else
#include <tf2_sensor_msgs/tf2_sensor_msgs.h>
#endif

Does this satisfy Your needs? If so I'll update this branch.
And also... Thanks a lot for being so polite to me 🙇
Grzegorz

[kenji-miyake]

@ralwing Thank you for your comment. My proposal is, as I wrote first, to wait for the Humble transition. Then we don't need ifdef or __has_include anymore.
Of course, if you have any issues with the ROS_DISTRO_ macro, then I can consider merging this now. In that case, please explain your issues.

First of all, as explained in https://github.com/orgs/autowarefoundation/discussions/2622, we'll move to Humble soon.
Are these changes really worth merging now? They'll be removed soon.

[kenji-miyake]

Also, CI build fails. I don't want to use much time for this now if there is no issue.
https://github.com/autowarefoundation/autoware.universe/actions/runs/3411438565/jobs/5675705119#step:7:1352

[kenji-miyake]

in my opinion refactor would be even easier, because You could simply sed all those changes, and also I've refactored code in such a way that they all have the same structure, before there were many different methods of inclusions:

@ralwing However, for future works, I'd appreciate it if you could explain the following things:

• Exactly what commands/steps did you execute for this PR?
• What commands/steps are you planning to execute to remove all __has_include after we move to Humble?

[ralwing]

ok, i'm closing this.

[kenji-miyake]

@ralwing Anyway, thank you again for the PR and patience! I really appreciate your contribution.