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

Improve header handling #17

Closed
mgruhler opened this issue Nov 30, 2018 · 3 comments · Fixed by #24
Closed

Improve header handling #17

mgruhler opened this issue Nov 30, 2018 · 3 comments · Fixed by #24
Labels
enhancement request for an enhancement

Comments

@mgruhler
Copy link
Collaborator

mgruhler commented Nov 30, 2018

SOEM expects headers in a flat directory hierarchy (i.e. <headerpath>). In contrast, ROS, by convention, expects headers in a subdirectory which is named after the ROS package (i.e. <pkg>/<headerpath>).

Currently, with the intention of not having to adapt any soem headers, but still somehow stick to the ROS convention, the soem headers are installed to soem/*.h.

This leads to the problem, that any ROS code building against soem runs into problems with finding headers.
A work around to overcome this is shown in the Usage section of the README.md.

This should be cleaned up such that we don't break existing code but without having to adapt any soem sources.

This has been reported multiple time, e.g. in #4 and #8

@jonbinney
Copy link

I spent a little time thinking about this but didn't come up with a clean solution. One way would be to install the include files to CATKIN_GLOBAL_INCLUDE_DESTINATION but that would be ugly because it would put stuff in /opt/ros/<distro>/ directly, and would only work when the package is installed.

Another option would be setting CFG_EXTRAS field in the catkin_package cmake macro to provide an extra cmake config file that adds ${CMAKE_INSTALL_PREFIX}/include/soem to soem_INCLUDE_DIRS automatically. This would also fail in source/devel space though. In that case you could use cmake's ${PROJECT_SOURCE_DIR}/soem i think.

@mgruhler
Copy link
Collaborator Author

mgruhler commented Jul 2, 2019

@jonbinney thanks for your thoughts.

Cluttering CATKIN_GLOBAL_INCLUDE_DESTINATION is not an option, I'd say.

Using CFG_EXTRAS is a good idea, though I'm not sure what would be required to make this work in both install and source/devel space. I don't think it is worthwhile coming up with a solution that only works in one of those cases. This is what we basically have right now :-)
This is the part where I'm not sure again what catkin and CMake is doing where under the hood. So if you have further insights about this, this would be great.

Another thing that I've come up with in the last few days (actually, motivated by your post :-D) is to add a patch file to automatically adapt the soem layout at build time. This might allow us to easily import any new versions without much overhead (i.e. resolving merge conflicts) and we'd only have to adapt any changes that happened in the meantime. But if this is that easy and "stable", I'm not sure...

@mgruhler
Copy link
Collaborator Author

mgruhler commented Jul 5, 2019

@jonbinney I actually took your suggestion and played around a bit with it, see #23.
If you are using this package, it would be great if you could test this.

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

Successfully merging a pull request may close this issue.

2 participants