switch to ament, replace todos with ardent#3
Conversation
mikaelarguedas
left a comment
There was a problem hiding this comment.
one question about warning users when mixing versions/distros otherwise LGTM
CMakeLists.txt
Outdated
| set(shell "sh") | ||
| else() | ||
| set(shells "bat") | ||
| set(shell "bat") |
There was a problem hiding this comment.
I know the catkin and ament API are different but it would be great to have the same variable names in both branches to keep diff at a minimum
There was a problem hiding this comment.
I can rename the variable in the ROS 1 branch but the diff is irrelevant since each distro needs its own branch anyway.
There was a problem hiding this comment.
I can rename the variable in the ROS 1 branch but the diff is irrelevant since each distro needs its own branch anyway.
Agreed we will never merge these branches together. It just seem easier to compare what's different in ROS1 and ROS 2 if we use the same variable names but that's not a blocker for this PR to be merged
| ROS_DISTRO = environ.get("ROS_DISTRO_OVERRIDE", "TODO") | ||
| }@ | ||
| if [ -n "$ROS_DISTRO" -a "$ROS_DISTRO" != "@(ROS_DISTRO)" ]; then | ||
| echo "ROS_DISTRO was set to '$ROS_DISTRO' before. Please make sure that the environment does not mix paths from different distributions." |
There was a problem hiding this comment.
do we want to keep that behavior for people unvoluntarily mixing workspaces?
There was a problem hiding this comment.
The ament API doesn't have the feature for .em templates. I could move the ROS distro name in to the CMake file and convert the env hooks into .in templates: 55af69a
79fe107 to
55af69a
Compare
| <buildtool_depend>ament_cmake_core</buildtool_depend> | ||
|
|
||
| <export> | ||
| <build_type>ament_cmake</build_type> |
There was a problem hiding this comment.
Is use of the ament_cmake build type possible when depending only on ament_cmake_core?
There was a problem hiding this comment.
Yes, the build type is only relevant to the build tool - it is not related to the build system / CMake part.
After it has been reviewed and merged and the name of the next ROS 2 release (B turtle) has been picked a branch for it can be created.