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

Provide catkin package.xml per ROS REP 136 #409

Merged

Conversation

c-andy-martin
Copy link
Contributor

@c-andy-martin c-andy-martin commented Jun 27, 2019

This provides the least intrusive, but most automated and correct
method for releasing non-catkin packages through the ROS
infrastructure.

These changes allow me to use catkin tools to build fcl, and to
have other catkin packages depend on fcl directly.


This change is Reviewable

package.xml Outdated
<name>fcl</name>
<version>0.6.0</version>
<description>FCL: the Flexible Collision Library</description>
<maintainer email="[email protected]">Jeongseok Lee</maintainer>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not currently maintaining FCL actively. I guess @sherm1 or @SeanCurtis-TRI would be more make sense to be the maintainer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I changed it to both of them.

@sherm1
Copy link
Member

sherm1 commented Jun 27, 2019

We are creating an [email protected] address so that various people in TRI's geometry group can serve as maintainers. Can you list the maintainer as
<maintainer email="[email protected]">TRI Geometry Team</maintainer>
?

@jmirabel
Copy link

Note that the way this is setup, the version number must
be maintained in two places. I'm not sure how to resolve that.

A few lines in CMake are sufficient to parse package.xml and extract the version:

FILE(READ package.xml PACKAGE_XML)
STRING(REGEX MATCH "<version>[0-9]+\\.[0-9]+\\.[0-9]+</version>" DIRTY_VERSION_STRING ${PACKAGE_XML})

STRING(REGEX REPLACE "^<version>([0-9]+)\\.[0-9]+\\.[0-9]+</version>" "\\1" major_vers "${DIRTY_VERSION_STRING}")
STRING(REGEX REPLACE "^<version>[0-9]+\\.([0-9])+\\.[0-9]+</version>" "\\1" minor_vers "${DIRTY_VERSION_STRING}")
STRING(REGEX REPLACE "^<version>[0-9]+\\.[0-9]+\\.([0-9]+)</version>" "\\1" patch_vers "${DIRTY_VERSION_STRING}")

Note if CMake >= 3.9, you can catch groups in regular expression using CMAKE_MATCH_<n>

@c-andy-martin
Copy link
Contributor Author

We are creating an [email protected] address so that various people in TRI's geometry group can serve as maintainers. Can you list the maintainer as
<maintainer email="[email protected]">TRI Geometry Team</maintainer>
?

Done

@c-andy-martin
Copy link
Contributor Author

Note that the way this is setup, the version number must
be maintained in two places. I'm not sure how to resolve that.

A few lines in CMake are sufficient to parse package.xml and extract the version:

FILE(READ package.xml PACKAGE_XML)
STRING(REGEX MATCH "<version>[0-9]+\\.[0-9]+\\.[0-9]+</version>" DIRTY_VERSION_STRING ${PACKAGE_XML})

STRING(REGEX REPLACE "^<version>([0-9]+)\\.[0-9]+\\.[0-9]+</version>" "\\1" major_vers "${DIRTY_VERSION_STRING}")
STRING(REGEX REPLACE "^<version>[0-9]+\\.([0-9])+\\.[0-9]+</version>" "\\1" minor_vers "${DIRTY_VERSION_STRING}")
STRING(REGEX REPLACE "^<version>[0-9]+\\.[0-9]+\\.([0-9]+)</version>" "\\1" patch_vers "${DIRTY_VERSION_STRING}")

Note if CMake >= 3.9, you can catch groups in regular expression using CMAKE_MATCH_<n>

Excellent! Done.

@c-andy-martin
Copy link
Contributor Author

I have addressed the issues I think. Thanks to TRI for supporting FCL!

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Thanks, Andy. This looks good to me but I don't know much about this build system. Can we get a quick review by a more-knowledgeable person? @j-rivero @scpeters, or @jamiesnape maybe?
:lgtm:

Reviewed 1 of 2 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SeanCurtis-TRI and @sherm1)

Copy link
Contributor

@jamiesnape jamiesnape left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @c-andy-martin, @SeanCurtis-TRI, and @sherm1)


CMakeModules/FCLVersion.cmake, line 2 at r2 (raw file):

# set the version in a way CMake can use
FILE(READ package.xml PACKAGE_XML)

Use lowercase for FILE and STRING.

@c-andy-martin
Copy link
Contributor Author

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @c-andy-martin, @SeanCurtis-TRI, and @sherm1)

CMakeModules/FCLVersion.cmake, line 2 at r2 (raw file):

# set the version in a way CMake can use
FILE(READ package.xml PACKAGE_XML)

Use lowercase for FILE and STRING.

Done

Copy link
Contributor Author

@c-andy-martin c-andy-martin left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @jamiesnape, @SeanCurtis-TRI, and @sherm1)


CMakeModules/FCLVersion.cmake, line 2 at r2 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Use lowercase for FILE and STRING.

Done.

Copy link
Contributor

@jamiesnape jamiesnape left a comment

Choose a reason for hiding this comment

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

The one thing I don't like about this is that when we switch to CMake 3 it is nice to be able to use the VERSION argument to project(). I guess we can live without that, though.

:lgtm:

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @c-andy-martin, @SeanCurtis-TRI, and @sherm1)


CMakeLists.txt, line 291 at r3 (raw file):

# Install catkin package.xml
install(FILES package.xml DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/fcl)
"${CMAKE_INSTALL_DATAROOTDIR}/${PROJECT_NAME}"

This provides the least intrusive, but most automated and correct
method for releasing non-catkin packages through the ROS
infrastructure.

Instead of specifying the FCL version in CMakeModules/FCLVersion.cmake,
that module reads the version from package.xml. This provides a single
point of maintenance for the version number.
@c-andy-martin
Copy link
Contributor Author


CMakeLists.txt, line 291 at r3 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…
"${CMAKE_INSTALL_DATAROOTDIR}/${PROJECT_NAME}"

Done.

Copy link
Contributor

@jamiesnape jamiesnape left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 2 files at r2, 1 of 1 files at r3, 1 of 1 files at r4.
Dismissed @jslee02 from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@SeanCurtis-TRI SeanCurtis-TRI merged commit 8cc285f into flexible-collision-library:master Jul 16, 2019
@c-andy-martin c-andy-martin deleted the catkinize branch July 17, 2019 19:28
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.

6 participants