Skip to content

Switch between catkin and ament based on ROS_VERSION#52

Merged
awinkler merged 1 commit intoethz-adrl:masterfrom
mpowelson:patch/remove_catkin
May 30, 2020
Merged

Switch between catkin and ament based on ROS_VERSION#52
awinkler merged 1 commit intoethz-adrl:masterfrom
mpowelson:patch/remove_catkin

Conversation

@mpowelson
Copy link
Copy Markdown
Contributor

This makes it easier to use with ROS 2. Otherwise you have to add a rosdep skip-key on catkin. REP-136 looks to be well before ROS 2 and ament were in the picture.

@awinkler
Copy link
Copy Markdown
Member

awinkler commented May 28, 2020

That tag was needed/recommended for releasing binaries and for the ros build farm. Could you kindly provide a bit more info why you think it is now obsolete. https://www.ros.org/reps/rep-0136.html

@mpowelson
Copy link
Copy Markdown
Contributor Author

Perhaps it isn't. I didn't realize that was required for the build farm. However, when running rosdep on this on a ROS 2 system it complains that it can't find catkin (since it isn't released). HERE is the specific motivator. I assume for releasing this on ROS 2 it would have to be changed to <exec_depend>ament</exec_depend>.

Adding it to the skip-keys isn't that big of a deal, and many people running ROS 2 may already have catkin added for other things. I'm not sure what the recommendation is. Perhaps @gavanderhoorn or @Levi-Armstrong would know.

@wxmerkt
Copy link
Copy Markdown
Contributor

wxmerkt commented May 28, 2020

@mpowelson Is it possible to upgrade to package format 3 and use a condition?

As in:

<exec_depend condition="$ROS_VERSION == 1">catkin</exec_depend>
<exec_depend condition="$ROS_VERSION == 2">ament</exec_depend>

@mpowelson
Copy link
Copy Markdown
Contributor Author

That sounds reasonable to me.

@awinkler
Copy link
Copy Markdown
Member

@mpowelson Is it possible to upgrade to package format 3 and use a condition?

As in:

<exec_depend condition="$ROS_VERSION == 1">catkin</exec_depend>
<exec_depend condition="$ROS_VERSION == 2">ament</exec_depend>

Yes, let's do this. @mpowelson could you kindly try this on your system and update the PR with the necessarily changes. Thanks!

@mpowelson mpowelson force-pushed the patch/remove_catkin branch from f92f6ae to 7318e3e Compare May 29, 2020 14:18
@mpowelson mpowelson changed the title Remove exec_depend on catkin Switch between catkin and ament based on ROS_VERSION May 29, 2020
@mpowelson
Copy link
Copy Markdown
Contributor Author

mpowelson commented May 29, 2020

This works fine. Here is our downstream project ROS 2 CI build using this branch.

@awinkler awinkler merged commit 15fe9b3 into ethz-adrl:master May 30, 2020
@mpowelson mpowelson deleted the patch/remove_catkin branch June 4, 2020 13:21
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.

3 participants