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

console_bridgeConfig.cmake will not reroot its include directories in a crosscompilation build. #44

Closed
mightyCelu opened this issue Apr 27, 2017 · 6 comments

Comments

@mightyCelu
Copy link

When building ROS in a cross-compiled build, console_bridgeConfig.cmake will set console_bridge_INCLUDE_DIRS to its installation include directory.
This will cause e.g. /usr/include to be added to the INCLUDE_DIRECTORIES, if the requiring package does not manually prefix the directories with the CMAKE_FIND_ROOT_PATH. As a result, builds might fail, e.g. because the hosts math.h is included which likely contains invalid ASM for the target system.
I am not sure about the convention in this regard, but looking at e.g. FindBoost.cmake they use find_path which returns the prefixed path.
I applied this patch in my case which fixes the issue:

diff --git a/console_bridge-config.cmake.in b/console_bridge-config.cmake.in
index 9d0763f..22ac756 100644
--- a/console_bridge-config.cmake.in
+++ b/console_bridge-config.cmake.in
@@ -3,7 +3,9 @@ if (@PKG_NAME@_CONFIG_INCLUDED)
 endif()
 set(@PKG_NAME@_CONFIG_INCLUDED TRUE)
 
-set(@PKG_NAME@_INCLUDE_DIRS "@CMAKE_INSTALL_FULL_INCLUDEDIR@")
+find_path(@PKG_NAME@_INCLDE_DIRS
+  NAMES console_bridge/console.h
+  HINTS @CMAKE_INSTALL_FULL_INCLUDE_DIR@)
 
 foreach(lib @PKG_LIBRARIES@)
   set(onelib "${lib}-NOTFOUND")
@traversaro
Copy link
Contributor

traversaro commented Apr 27, 2017

I think the proper way of solving this problem (avoid hard-coding installation paths in CMake configuration files) is to make the urdfdom CMake configuration files relocatable.
This solves this problems and has other advantages (for example simplifying installation on Windows or in vcpkg).

Related CMake docs: https://cmake.org/cmake/help/v3.8/manual/cmake-packages.7.html#creating-relocatable-packages .
Related Gazebo issues/discussion :

@mightyCelu
Copy link
Author

mightyCelu commented Apr 28, 2017

This might also fix the problem and be the better solution, however it will require more work. For now it might still be a good idea to include the fix I posted since it changes only one CMake call.

@traversaro
Copy link
Contributor

For now it might still be a good idea to include the fix I posted since it changes only one CMake call.

Note the only one CMake call that it changes has subtle implications: the find_path CMake command has a complex logic for determine its search path, and the location indicated by the HINTS come after several location specified in CMake cache variables and in environmental variables. Those can be modified if the NO_CMAKE_PATH and the NO_CMAKE_ENVIRONMENT_PATH are passed to the function call, but I don't know if then everything works in your case, as, as you noted, @CMAKE_INSTALL_FULL_INCLUDE_DIR@ does not contain the actual directory of the headers.

traversaro added a commit to traversaro/console_bridge that referenced this issue Apr 29, 2017
@traversaro
Copy link
Contributor

@mightyCelu PR #45 would solve your problem?

@mightyCelu
Copy link
Author

mightyCelu commented May 3, 2017

I am not sure about the configure_file semantics but it looks to me like the change to line 6 resp. 8 in the template will not change my problem, it will still contain the install-path which is not prefixed by an optional CMAKE_FIND_ROOT_PATH.

edit: I am mistaken, after reading the configure_package_config_file it looks to me like this should work.

@scpeters
Copy link
Contributor

scpeters commented Aug 3, 2017

fixed by #45

@scpeters scpeters closed this as completed Aug 3, 2017
lsolanka pushed a commit to ros-hunter/console_bridge that referenced this issue Dec 26, 2017
See discussion in ros#44

(cherry picked from commit 15eb3ea)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants