Add missing test dependency on xacro in four_wheel_steering_controller

[mateus-amarante]

No description provided.

[matthew-reynolds]

Good catch! Can you also add xacro to the test find_package in the CMakeLists?

ros_controllers/four_wheel_steering_controller/CMakeLists.txt

Lines 43 to 44 in 6653d90

	if(CATKIN_ENABLE_TESTING)
	find_package(catkin REQUIRED COMPONENTS rosgraph_msgs rostest std_srvs controller_manager tf)

And perhaps also do the same to the CMakeLists for diff_drive_, effort_, and joint_trajectory_controllers.

Those CMakeLists are in desperate need of cleanup, it fell off of my plate a while back... But certainly adding those missing deps would help in the meantime.

[mateus-amarante]

Good catch! Can you also add xacro to the test find_package in the CMakeLists?

Why is xacro required in CMakeLists.txt? It is only required to load the robot_description in:

ros_controllers/four_wheel_steering_controller/test/launch/four_wheel_steering_common.launch

Lines 6 to 7 in 6653d90

	<param name="robot_description"
	command="$(find xacro)/xacro '$(find four_wheel_steering_controller)/test/urdf/four_wheel_steering.urdf.xacro'" />

So, it is an execution dependency of the tests. The package do not demand headers, libraries or cmake modules from xacro.

[mateus-amarante]

Those CMakeLists are in desperate need of cleanup, it fell off of my plate a while back... But certainly adding those missing deps would help in the meantime.

I can help with that. I just need to understand what you mean with a cleanup. I have already noticed some omitted transitive dependencies (for example, a package includes headers from hardware_interface, but it does not have it defined as a dependency, but no error is raised because it has another dependency like controller_interface depending on it) and some "over-definition" of dependencies (some depend that are not necessary exec_depend, some unneeded "find_packaged" packages and so on).

I'm working on some scripts to facilitate isolated builds and tests, so I can catch this type of problems easily. Currently, only this package and effort_controller (already fixed in #506) were causing errors in the isolated builds.

[bmagyar]

I think catkin_lint complains about package.xml dependencies not appearing in find_package calls. I'm not sure about test-depends but I don't think that needs to be find_package-d.

I'm working on some scripts to facilitate isolated builds and tests, so I can catch this type of problems easily.

Even though colcon build is the newer tool, I found using catkin build in ROS Noetic better, no need to fiddle with the 200 explicit parameters to get a reasonable output from build failures.

[matthew-reynolds]

Why is xacro required in CMakeLists.txt? It is only required to load the robot_description in:

Whoops, you're absolutely correct. I saw that it was in the ackermann_steering_controller and joint_trajectory_controller test find_packages, but not in the other 4, and didn't turn my brain on enough to realize the correct solution is removing it from those 2 rather than adding it to the other 4. My bad, you are correct.

I just need to understand what you mean with a cleanup

Basically the same as ros-controls/ros_control#404. A few goals:

• Going through all the source and test files to update the dependencies, exactly as you described (Reduce deps where possible, add missing/transitive deps, etc)
• Apply some consistent format to CMakeLists and package.xml (This isn't that critical but is kinda just nice to have consistency. I suggest we use the same format as the linked PR, above)
• Fix the double find_package we've got going on for all our tests
  • find_package(catkin ...) overwrites the catkin_* variables, so technically there's risk here since we could lose the components we found in the first find_package(catkin).
  • Some solutions are to either manually include each of the additional test depends (We do this in ros_control in a few places) or to backup the catkin_* vars before the find_package and append them to the new values afterwards.
  • In my opinion, the correct solution here is to split all the tests into their own packages which are not part of the metapackage (But still in the same repo), since the rosinstall generator and similar tools do a bad job of excluding test_depends when generating installations. So currently, a lot of unnecessary dependencies (In the worst case: Gazebo) get pulled in.

Last time I worked on this I just did it all manually. Having some scripts would be great.

I also foolishly was trying to fix everything at once rather than going package-by-package, plus I was tackling the test split at the same time, so the project grew and never got done right. I think we should attack this one package at a time, and leave the test split discussion for afterwards (So use one of the other two solutions to fix the double find_package). Let's open a metaissue with a checklist for each package that needs to be updated, so it can be tackled bit-by-bit.

[bmagyar]

Thanks for this, the test for ros-main is a known issue with the industrial CI atm, hopefully will "fix itself" soon.