-
Notifications
You must be signed in to change notification settings - Fork 138
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
Adding ign_gazebo_dev package #240
base: foxy
Are you sure you want to change the base?
Conversation
Signed-off-by: Harsh Mahesheka <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some initial feedback and suggestions. I think this is in good shape!
ros_ign_dev/ign_gazebo_dev/cmake/ignition_gazebo_dev-extras.cmake
Outdated
Show resolved
Hide resolved
elseif("$ENV{ROS_DISTRO}" MATCHES "humble") | ||
find_package(ignition-gazebo5 REQUIRED) | ||
set(IGN_GAZEBO_VER ${ignition-gazebo6_VERSION_MAJOR}) | ||
else() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is probably worth allowing an override / fallback for unofficial ROS distributions.
Using a CMake variable like OVERRIDE_ROS_GZ_VERSION
or similar for override or checking ROS_GZ_VERSION
environment variable to fall back to if no known ROS_DISTRO is found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added ign fortress as an override version in ignition_gazebo_dev-extras.cmake and package.xml.
Signed-off-by: Harsh Mahesheka <[email protected]>
Heya @harshmahesheka I noticed this was still in draft mode :o, do we want to move forward with this? |
Definitely. I was waiting for approval on whether the current iteration is correct. Also, this one is just for ign-gazebo( gz-sim now), so should we create a similar folder for all other libraries in gazebo? |
🎉 New feature
Closes #186
Summary
First,package.xml has
<depend condition>
which will make the package depend on the required ign-gazebo version based on Ros distribution and other packages can depend on it.Secondly, ignition_gazebo_dev-extras.cmake under cmake find and set required ign gazebo package so, other packages can simply gofind_package(ign_gazebo_dev REQUIRED)
instead of finding ign-gazebo which will make their packages furthermore compatible with different ROS distros. Currently, I am creating a draft pr for reviews from the community, after that, we can add similar packages for all other ignition libraries.Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
messages.Test it