search

moveit / moveit_calibration

Hand-eye calibration tools for robot arms.
BSD 3-Clause "New" or "Revised" License
143 stars 79 forks source link

added missing opencv dep in package.xml #93

Closed JStech closed 3 years ago

JStech commented 3 years ago

This was causing the buildfarm to fail.

JStech commented 3 years ago

Don't you need an exec_depend on libopencv as well?

Yeah, probably, although I admit I don't really understand what's going on here. Without this change, everything worked fine on my machine, in CI, and in the pre-release test, but then it didn't build on the buildfarm. I was using moveit_ros/perception as my reference and it doesn't have any OpenCV dependencies in its package.xml. This change just came from asking for help internally here at PickNik.

JStech commented 3 years ago

OK, I did some more poking around major packages that use OpenCV, and it looks like having build and exec depends on libopencv-dev is the typical practice.

rhaschke commented 3 years ago

Could you provide a link to the failing build farm job please?

JStech commented 3 years ago

https://build.ros.org/view/Nbin_uF64/job/Nbin_uF64__moveit_calibration_plugins__ubuntu_focal_amd64__binary/

rhaschke commented 3 years ago

Our CI pulls in opencv via the MoveIt dependencies (in our docker image). However, it's strange that the prerelease test passed. Do you have a link to that one as well (in case it was run here via GHA)?

JStech commented 3 years ago

I don't have a link to the prerelease test--I ran it locally.

rhaschke commented 3 years ago

Please consider our merge policies: PRs like this with several fixup commits should be squash-merged as a single commit. Otherwise the history becomes rather convoluted: image

rhaschke commented 3 years ago

I found the explanation for the succeeding prerelease test: The prerelease test operates on the full repository (while the buildfarm operates on individual package folders only). Thus, because moveit_calibration_gui depends on cv_bridge, the dependency was pulled in.