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

Switch from qos_event.hpp to event_handler.hpp #2111

Merged
merged 14 commits into from
Apr 18, 2023

Conversation

sjahr
Copy link
Contributor

@sjahr sjahr commented Apr 14, 2023

Description

Address error happening here #2020 (review)

  Starting >>> moveit_resources_prbt_ikfast_manipulator_plugin
  --- stderr: moveit_ros_occupancy_map_monitor
  In file included from /home/runner/work/moveit2/moveit2/.work/target_ws/src/moveit2/moveit_ros/occupancy_map_monitor/src/occupancy_map_server.cpp:48:
  /opt/ros/rolling/include/rclcpp/rclcpp/qos_event.hpp:18:2: error: This header is obsolete, please include rclcpp/event_handler.hpp instead [-Werror,-W#warnings]
  #warning This header is obsolete, please include rclcpp/event_handler.hpp instead
   ^
  1 error generated.
  gmake[2]: *** [CMakeFiles/moveit_ros_occupancy_map_server.dir/build.make:76: CMakeFiles/moveit_ros_occupancy_map_server.dir/src/occupancy_map_server.cpp.o] Error 1
  gmake[1]: *** [CMakeFiles/Makefile2:204: CMakeFiles/moveit_ros_occupancy_map_server.dir/all] Error 2
  gmake: *** [Makefile:146: all] Error 2
  ---
  Failed   <<< moveit_ros_occupancy_map_monitor [38.5s, exited with code 2]
  Aborted  <<< moveit_resources_prbt_ikfast_manipulator_plugin [22.5s]

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@sjahr sjahr requested a review from JafarAbdi April 14, 2023 07:48
@sjahr
Copy link
Contributor Author

sjahr commented Apr 14, 2023

Waiting until ros-rolling-rclcpp version 20.0.0 is available https://github.com/ros2/rclcpp/tags

@Shobuj-Paul
Copy link
Contributor

Shobuj-Paul commented Apr 14, 2023

Resolves #2113

Copy link
Member

@JafarAbdi JafarAbdi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will break main on Humble, we need to check for the distro and include the header accordingly

@AndyZe
Copy link
Member

AndyZe commented Apr 16, 2023

I think this will break main on Humble, we need to check for the distro and include the header accordingly

@JafarAbdi we have a dedicated Humble branch now, though. I think it would be OK to break main on Humble.

@codecov
Copy link

codecov bot commented Apr 16, 2023

Codecov Report

Patch coverage has no change and project coverage change: +50.84 🎉

Comparison is base (c5d1ec9) 0.00% compared to head (7ff9dfc) 50.84%.

❗ Current head 7ff9dfc differs from pull request most recent head 63fb5b6. Consider uploading reports for the commit 63fb5b6 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##           main    #2111       +/-   ##
=========================================
+ Coverage      0   50.84%   +50.84%     
=========================================
  Files         0      391      +391     
  Lines         0    32167    +32167     
=========================================
+ Hits          0    16351    +16351     
- Misses        0    15816    +15816     
Impacted Files Coverage Δ
...or/src/current_state_monitor_middleware_handle.cpp 88.89% <ø> (ø)

... and 390 files with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JafarAbdi
Copy link
Member

I think this will break main on Humble, we need to check for the distro and include the header accordingly

@JafarAbdi we have a dedicated Humble branch now, though. I think it would be OK to break main on Humble.

That branch is stable and used for binary releases.

Rolling is very unstable (at least that was my experience) to be used by a general user. IMHO, we should always aim to have support for rolling + latest LTS version on main (for people who are ok with building from source to get the latest features)

Copy link
Contributor

@ChrisThrasher ChrisThrasher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solution feels like overkill in comparison to disabling -W#warning but it's a valid solution nonetheless. Do we plan on removing this extra infrastructure as soon as it's no longer needed? Do we know how soon Humble will get access to this header it needs?

moveit_common/cmake/moveit_package.cmake Outdated Show resolved Hide resolved
@@ -1,10 +1,10 @@
add_executable(cancel_action cancel_action.cpp)
ament_target_dependencies(cancel_action ${THIS_PACKAGE_INCLUDE_DEPENDS})
target_link_libraries(cancel_action ${LIBRARIES})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this previously dereferencing a variable that didn't exist?

Copy link
Contributor Author

@sjahr sjahr Apr 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable exists but it should not be necessary to link against the target list in this variable

Co-authored-by: Chris Thrasher <chrisjthrasher@gmail.com>
@sjahr sjahr requested review from AndyZe and JafarAbdi April 18, 2023 11:16
@AndyZe AndyZe enabled auto-merge (squash) April 18, 2023 14:11
@AndyZe AndyZe merged commit 42d69ed into moveit:main Apr 18, 2023
@tylerjw tylerjw deleted the pr-address_clang_tidy branch April 18, 2023 15:36
@@ -41,6 +41,20 @@ macro(moveit_package)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)

if(NOT DEFINED ENV{ROS_DISTRO})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: this breaks the moveit2_packages: https://github.com/moveit/moveit2_packages/actions/runs/4749763095/jobs/8437320692

The better way would be to check the rclcpp version where this change was introduced.

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.

None yet

6 participants