Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: simplify Rolling support #854

Merged
merged 1 commit into from
May 5, 2022
Merged

Conversation

kenji-miyake
Copy link
Contributor

@kenji-miyake kenji-miyake commented May 5, 2022

Description

As a follow-up of #849, I replaced the custom-defined macros with ROS_DISTRO macros.

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@esteve
Copy link
Contributor

esteve commented May 5, 2022

@kenji-miyake thanks for simplifying this code. Regarding these blocks:

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

do it the other way around, because otherwise we'd have to add an #ifdef for Rolling, Humble, etc. so instead:

#ifdef ROS_DISTRO_FOXY
#include <tf2_geometry_msgs/tf2_geometry_msgs.h>
#else
#include <tf2_geometry_msgs/tf2_geometry_msgs.hpp>

a different option is to keep the code as is, but instead of adding USE_TF2_GEOMETRY_MSGS_DEPRECATED_HEADER to the compile definitions in the CMakeLists.txt files, add it to autoware_cmake() when compiling for anything lower than Galactic. What do you think?

@esteve
Copy link
Contributor

esteve commented May 5, 2022

@kenji-miyake forget what I just wrote, for some reason I thought we supported Foxy and that the special case for Galaxy was the same as for Rolling and Humble.

@kenji-miyake kenji-miyake requested review from wep21 and esteve May 5, 2022 14:52
@kenji-miyake kenji-miyake marked this pull request as ready for review May 5, 2022 14:52
@esteve
Copy link
Contributor

esteve commented May 5, 2022

@kenji-miyake is it documented anywhere that we no longer support Foxy? Otherwise this code would fail when compiling on Foxy.

@kenji-miyake
Copy link
Contributor Author

@esteve Although there is no background written, the supported distros are here.
https://autowarefoundation.github.io/autoware-documentation/main/installation/#ros-version

@kenji-miyake
Copy link
Contributor Author

kenji-miyake commented May 5, 2022

@kenji-miyake
Copy link
Contributor Author

After Humble, I think we should support LTS distros at least until the next LTS is released. (If there is no special reason like Foxy.)

And regarding Humble, we have to plan a migration strategy.
Here is the issue: #268

@esteve
Copy link
Contributor

esteve commented May 5, 2022

@kenji-miyake thanks for the links, in that case the changes look good to me.

@kenji-miyake kenji-miyake enabled auto-merge (squash) May 5, 2022 15:49
@kenji-miyake kenji-miyake merged commit 4a08c20 into main May 5, 2022
@kenji-miyake kenji-miyake deleted the simplify-rolling-support branch May 5, 2022 15:53
kminoda pushed a commit to kminoda/autoware.universe that referenced this pull request May 19, 2022
boyali referenced this pull request in boyali/autoware.universe Sep 28, 2022
boyali referenced this pull request in boyali/autoware.universe Oct 3, 2022
boyali referenced this pull request in boyali/autoware.universe Oct 3, 2022
boyali referenced this pull request in boyali/autoware.universe Oct 19, 2022
satoshi-ota pushed a commit to satoshi-ota/autoware.universe that referenced this pull request Oct 3, 2023
…date_start_goal_planner_v0.10.1_2

Feat/sugahara/update start goal planner v0.10.1 2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants