Update CMakeLists.txt and package.xml

[matthew-reynolds]

Various fixes and formatting for CMakeLists.txt and package.xml files, including fixes suggested by catkin_lint.

---

Apply consistent format to CMakeLists.txt and package.xml files

The format is largely based on the existing files. The most important change is that this PR corrects the order of commands in the CMakeLists.txt.

I'm not at all attached to the format used in the CMakeLists.txt, happy to gut out the comments or make other stylistic changes as desired. I would mostly just like them to be consistent, as it makes it much easier to identify issues and to maintain.

---

Beyond stylistic changes, the following changes were made:

combined_robot_hw:

• Remove unnecessary boost library dependency

combined_robot_hw_tests:

• Remove unnecessary boost library dependency
• Add missing ${catkin_EXPORTED_TARGETS} dependency
• Gut the catkin_package() and install() commands
• Move all targets into the if(CATKIN_ENABLE_TESTING) block
• Update package description
• Add missing dependencies to package.xml

controller_interface:

• Remove rosunit dependency (See note at bottom)
• Remove unneeded pluginlib dependency

controller_manager:

• Add missing roscpp dependency
• Add missing ${catkin_EXPORTED_TARGETS} dependency
• Add missing exec_depends for python scripts

controller_manager_msgs:

• Add missing exec_depends for python scripts

controller_manager_tests:

• Remove unnecessary boost include_dir dependency
• Add missing hardware_interface and roscpp dependencies
• Add missing ${catkin_EXPORTED_TARGETS} dependency
• Gut the catkin_package() and install() commands
• Move all targets into the if(CATKIN_ENABLE_TESTING) block
• Update package description
• Update dependencies in package.xml

hardware_interface:

• Add missing find_package(Boost REQUIRED) for header-only library dependency ptr_container
• Add missing boost depend in package.xml
• Remove redundant rosconsole dependency (Already included via roscpp)
• Remove unused rostest test_depend
• Remove rosunit dependency (See note at bottom)

joint_limits_interface:

• Remove liburdfdom-dev dependency (See Update CMakeLists.txt and package.xml #404 (comment))

rqt_controller_manager:

• Install python scripts with catkin_install_python() rather than the default cmake install()
• Update package description
• Add missing dependencies

transmission_interface:

• Add missing find_package(Boost REQUIRED) for header-only library dependency lexical_cast
• Add missing boost depend in package.xml
• Remove unused Boost dependencies in tests
• Fix find_package() in tests, to not override previous find_package(catkin [...]). See make rostest build dependency in CMakeLists.txt optional ros/rosdistro#3010 (comment)

---

Notes:

I was unable to find up-to-date documentation on rosunit and the corresponding catkin_add_gtest/catkin_add_gmock. In particular, I don't know if it is necessary to find_package(rosunit) or <test_depend>rosunit</test_depend>. I believe the find_package is unecessary. I think the test_depend is required, but I'm not sure.

For header-only Boost libraries, I used find_package(Boost REQUIRED) and include_directories(${Boost_INCLUDE_DIRS}). I was not able to find any other examples of this use in large ROS packages, so I just wanted to highlight it here in case there's a better approach.

---

TODO:

Couple things I just want to look through before changing this from a draft to a normal PR:

• Make sure add_dependencies(${catkin_EXPORTED_TARGETS}) is used everywhere it's required Done in fa93166
• Make sure roscpp is depended upon directly everywhere that it's required Done in 79b504c
• Investigate the urdf/liburdf-dev situation, and see if things can be simplified Done in 7ca25ab

[bmagyar]

@matthew-reynolds is there anything else you'd add apart from the outstanding review points? (The PR is still in draft mode)

[matthew-reynolds]

Been quite busy the last 2 weeks, my apologies. Other than the outstanding review points, I just want to double check those last couple points in the initial PR message (that we're not missing and roscpp or ${catkin_EXPORTED_TARGETS} dependencies). I'll try to get through this in the next couple days :)

[matthew-reynolds]

While finding missing roscpp & rospy deps, I found a couple additional missing package dependencies (some msgs, pluginlib in some tests, etc). Will do another thorough pass to fix all of those.

Still need to double check we're not missing any ${catkin_EXPORTED_TARGETS} cmake dependencies, and go over the urdf & tinyxml situation again.

[matthew-reynolds]

Follow-up issues all opened. Ready for final review of this PR.

Would you like me to reference #410 in the comment I left in the two _tests CMakeLists.txt?

BTW, came across these while working on this PR: #117 and #157 both look like old issues that have long been resolved. I think they should be closed.

[bmagyar]

Thanks very much for sticking through this long review.
No worries, we are fine without that comment, the issue is open here.
I've closed those two outdated ones, thanks for bringing it to my attention!

[bmagyar]

Apologies for the abrupt "merge branch..." solution, I dared to resolve the conflict using github's interface, please feel free to squash that with any of your commits.

[bmagyar]

Thanks @matthew-reynolds for the great for and @gavanderhoorn for the additional review, I am merging this now!

[mathias-luedtke]

Where did EXCLUDE_FROM_ALL come from?

[matthew-reynolds]

When looking at the targets in all the test packages in https://github.com/ros/ros_comm/tree/melodic-devel/test as an example, I saw they marked targets intended for tests as EXCLUDE_FROM_ALL. Similarly, targets generated by catkin_add_gtest() and catkin_add_gmock() enable the property.

I admit, I don't fully understand why this is required, since the test targets are already wrapped in the if(CATKIN_ENABLE_TESTING). I have the impression it was to cover an edge-case with catkin_make (as opposed to catkin_make_isolated or catkin_tools), something to do with the single CMake invocation over the entire workspace, but instead of properly researching and understanding it, I just used those packages as examples and mimicked.

[mathias-luedtke]

I think you have to add the targets to tests manually:
https://github.com/ros/ros_comm/blob/8d9af410949423342150b79e763348f963e19734/test/test_roscpp/test/src/CMakeLists.txt#L226

[mathias-luedtke]

Or use add_rostest_gtest, which would automate most of the steps.

[matthew-reynolds]

I think you have to add the targets to tests manually

Ah, I think you're right, that makes sense. Since those targets are only depended on by targets in add_rostest, not add_rostest_gtest, they aren't directly or transitively depended on by the tests target, and since I added the EXCLUDE_FROM_ALL property, they aren't part of the all target either. Sounds like this may be the cause of the colcon failure you mentioned.

I will re-add the add_dependencies(tests target_name) for executable targets used in tests. What are your and @bmagyar's thoughts on also adding library targets as dependencies? Back to the ros_comm test package examples, they seem to only mark executables as deps, and rely on libraries being pulled in transitively.

[mathias-luedtke]

rely on libraries being pulled in transitively.

This is sufficient. Add direct dependencies only.