search

ros-geographic-info / geographic_info

ROS packages for geographic information
http://ros.org/wiki/geographic_info
60 stars 61 forks source link

[Ros2] clean cmake and package.xml for geographic_msgs #23

Closed mikaelarguedas closed 4 years ago

mikaelarguedas commented 6 years ago

I came across this when trying to use mavros with ROS 2.

This is mostly simplifying the CMake logic for the message package.

@matt-attack as the original contributor of the ROS2 port, can you confirm that this still works for your use case?

bmagyar commented 4 years ago

The changes in general look good to me but I don't currently have a working setup to test them in. @matt-attack ?

matt-attack commented 4 years ago

I'm currently without a working setup as well, but it looks okay to me.

Not sure if anything changed with the migration to Colcon that would cause this to break on anything newer than Bouncy though.

bmagyar commented 4 years ago

@SteveMacenski FYI for the ROS2 releases?

SteveMacenski commented 4 years ago

The only major differences with my port and this one is the extra cmake flags, which we could certainly add if someone felt strongly about it. @mikaelarguedas do you think elements of this are still necessary?

SteveMacenski commented 4 years ago

Closing from merging #28 & no activity on flag necessity

mikaelarguedas commented 4 years ago

Sorry for the late reply I was away for a while. Glad to see this has been picked up :tada:, didnt expect it after 18months ^^

@SteveMacenski I have a diverging history on this repo, did the ros2 branch get force pushed removing all original ROS2 port commits? Regarding the flags, these are the standard ROS2 flags used by all core packages (and nav2 packages as well). I don't feel strongly about it, it just seemed to be common practice to use them. I'll open a separate PR to add them back for consistency

SteveMacenski commented 4 years ago

Nothing was force pushed (to my knowledge) but happy to assist

mikaelarguedas commented 4 years ago

Nothing was force pushed (to my knowledge) but happy to assist

The mysteries of github, two commits porting the messages to ROS2 were successfully merged while conflicting o_0 https://github.com/ros-geographic-info/geographic_info/commit/5f7af445496992767fa09a26e59a6124b8417c39 https://github.com/ros-geographic-info/geographic_info/commit/b981940213ffd17ac6121cd36ed938e0b5095af0

Anyway thanks for taking this over and the express reviews on the follow-up PRs !