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

[jsk_topic_tools] Add header file to check jsk_topic_tools' version #1753

Merged
merged 2 commits into from Jul 15, 2022

Conversation

iory
Copy link
Member

@iory iory commented Jul 14, 2022

What is this?

I add version.h for jsk_topic_tools to check version for build.
jsk_topic_tools has base class such as DiagnosticNodelet and ConnectionBasedNodelet.
For example, nodes in jsk_recognition are using those notelets.

This PR is useful to check jsk_topic_tools version.

Related to jsk-ros-pkg/jsk_recognition#2712

@iory iory requested review from k-okada and knorth55 July 14, 2022 10:53
@k-okada k-okada merged commit d1e9474 into jsk-ros-pkg:master Jul 15, 2022
@@ -31,6 +31,21 @@ configure_file(
${CMAKE_CURRENT_SOURCE_DIR}/include/jsk_topic_tools/nodelet_version.h.in
${CATKIN_DEVEL_PREFIX}/${CATKIN_GLOBAL_INCLUDE_DESTINATION}/jsk_topic_tools/nodelet_version.h)

catkin_package_xml()
Copy link
Member

Choose a reason for hiding this comment

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

sorry, I think it safe to put configure_file after catkin_package so that we do not need to use catkin_package_xml, because I suppose catkin_package_xml is not designed to used in User's program. how do you think? @iory

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. That's good for users.
https://github.com/ros/catkin/blob/2612c4ce82a285cac13263576f8a85b8a6d6cd3d/cmake/catkin_package.cmake#L97-L100
catkin_package internally calls catkin_package_xml.

I created a PR #1755

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants