Kawasaki-Robotics / khi_robot

ROS KHI robot meta-package
http://wiki.ros.org/khi_robot
BSD 3-Clause "New" or "Revised" License
55 stars 28 forks source link

Updates to build scripts and manifests (packaging) #5

Closed gavanderhoorn closed 5 years ago

gavanderhoorn commented 5 years ago

As per subject.

Most of this was triggered by output of catkin_lint -W2 or by running into a non-functioning install space.

gavanderhoorn commented 5 years ago

7f1cbf2 fixes an issue with khi_robot_control that appears to be preventing the ROS buildfarm from building that package successfully (job1 log).

gavanderhoorn commented 5 years ago

An unresolved issue is how to deal with install(..)-ing the main and khi_robot_client targets in khi_robot_control: because of their reliance on the KRNX binary blob, they could pose a problem.

libkrnx-*.so is not installed at the moment, and linking is done against the file inkhi_robot_control/lib, which is located in the source space of the workspace. Copying the .so file to the appropriate location using install(..) is possible, but that doesn't update the RPATH of khi_robot_control, causing a runtime dynamic linker failure.

But without installing main and khi_robot_client, the khi_robot_control package, as released through the ROS buildfarm is useless.

At the moment the driver can only be used from a devel or build space, or a local install space.


It's possible that fiddling with RPATH can fix this, but I haven't tried.

d-nakamichi commented 5 years ago

Thank you for your contribution about ROS build/install. I made this driver only for a local install space, and this is a very helpful PR.

About khi_robot_control, it must need krnx library (not OSS). So I have 3 ideas to solve this problem.

  1. Use a krnx library as a static library .a in khi_robot_control
  2. Make a new ROS package like khi_robot_krnx including *.so and khi_robot_control use it by *_depend
  3. When executing install in khi_robot_control, copy *.so into CATKIN_PACKAGE_LIB_DESTINATION

Do you have any ideas ?

gavanderhoorn commented 5 years ago

About khi_robot_control, it must need krnx library (not OSS). So I have 3 ideas to solve this problem.

1. Use a krnx library as a static library `.a` in `khi_robot_control`

This would certainly work around any issues with dynamic linking.

2. Make a new ROS package like `khi_robot_krnx` including `*.so` and `khi_robot_control` use it by `*_depend`

This could also work, and would make depending on the library from other ROS packages easier.

3. When executing `install` in  `khi_robot_control`, copy `*.so` into `CATKIN_PACKAGE_LIB_DESTINATION`

This last one could be problematic because of the RPATH issue I described above. I'm not an expert though, so perhaps this could be easily fixed. Adding install(..) for the krnx-*.so files was the first thing I did, but then ran into linking problems.

Do you have any ideas ?

the static library approach would probably be the easiest way to work-around any linking issues.

If you could submit a PR adding the static libraries to the repository I'll rebase this PR.

d-nakamichi commented 5 years ago

OK, I'll submit a PR based on idea 1. Thank you.

gavanderhoorn commented 5 years ago

@d-nakamichi: just build-tested this with catkin_make (and catkin_tools): apart from the KRNX library issue this now seems to work.

gavanderhoorn commented 5 years ago

I would actually suggest to fix the KRNX library linking issue in a later PR.

That way we can verify the comment by @k-okada in https://github.com/gavanderhoorn/khi_robot/pull/1#issue-266879413 and not hold up this PR (which fixes an issue that is currently blocking the ROS buildfarm).

d-nakamichi commented 5 years ago

Yes, that's true. I checked the problems are fixed.

gavanderhoorn commented 5 years ago

@d-nakamichi, @k-okada: I've just added 6641def which adds something like what @k-okada suggested in https://github.com/gavanderhoorn/khi_robot/pull/1#issue-266879413.

As the commit message says, I'm not sure this is a proper solution, but it does seem to work for me on the systems that didn't want to run main before.

@k-okada: could you verify this also works for you?

d-nakamichi commented 5 years ago

I accepted the PR. Thank you! @gavanderhoorn

gavanderhoorn commented 5 years ago

@d-nakamichi: is squash-merge the default merging policy for this repository? Provenance of commits and history is lost to a large degree that way.