[CMake] fix link to eigenpy #2118
Conversation
|
Thanks for helping in improving MoveIt and open source robotics! |
|
thx for this PR, Let me give this a shot real quick EDIT: This PR resolves the issue for me. |
|
The |
fix stack-of-tasks/eigenpy#195 Since moveit#1737, eigenpy is found using find_package, so we can get all required informations to find headers and link to shared objects with a target_link_libraries to the eigenpy::eigenpy imported CMake target.
|
Could you please update your main description and indicate the Ubuntu, ROS, and MoveIt version you are experiencing issues with? I'm also include @wxmerkt, because he is the maintainer for the ROS eigenpy package providing the mentioned variables. If these variables are inconsistent to what |
|
Hey @rhaschke Edit: also from what I understand, in the latest release the system eigenpy is used instead of the catkin eigenpy that @wxmerkt maintains. Therefore I do not think the issue would be fixed by changing that release. |
|
@acxz, thanks for this clarification. $ apt search eigenpy
Sorting... Done
Full Text Search... Done
ros-melodic-eigenpy/bionic,now 2.3.1-1bionic.20200410.133723 amd64 [installed]
Bindings between Numpy and Eigen using Boost.Python
ros-melodic-eigenpy-dbgsym/bionic 2.3.1-1bionic.20200410.133723 amd64
debug symbols for ros-melodic-eigenpyBy the way, #1737 was required due to fkie/catkin_lint#73. From the changes of this PR, I understand that |
|
Ah thank you for clearing that up. Yes I agree with that reasoning. I did try to build moveit-ros-planning-interface with ros-melodic-eigenpy and I still received the same error. |
|
@rhaschke : on Ubuntu LTS and on Debian, eigenpy is available in the robotpkg apt binary repository. So you can consider that this is not only shipped via About the issue with Here is the curent So the path to numpy header is there, in a way sufficient to use If you want to properly package something that depend on eigenpy through its .pc file, I think you need to find numpy and add its include dirs in your CMakeLists.txt. Maybe, we should add One of the issue with this |
|
Or maybe do you expect numpy include dirs and python include dirs to be included in |
|
Hi all, thanks for looking into this. I didn't get a chance yesterday evening to respond to ongoing exchanges on the EigenPy issue. Note, this PR is equivalent to #2010 - however back then the updated EigenPy with CMake targets was not yet released to Kinetic. It now is. Thanks for re-raising this issue; this is an important update for future use and improves the overall situation (and the existing code still works and is backwards compatible). As thus, I think we should merge this PR.
MoveIt correctly builds and links with the currently released EigenPy on Kinetic and Melodic - unless there recently are buildfarm error emails. The EigenPy packages released via the ROS buildfarm are available for Kinetic, Melodic, and Noetic and are released from the upstream repository as a third-party package release. The described error appears on Arch Linux - I have not investigated the ROS packaging for there or why it happens (it's not via the ROS buildfarm). @acxz did this build in the past and start to fail recently on Melodic? Or has it not been released to Melodic before? |
|
As an outside observer, this still appears as an issue with Especially the issue with numpy not being added to the exported include dirs (reported by @nim65s). From everything described until now, it does not seem like it's something which should be fixed here in MoveIt. "All" we do here is |
|
I agree with @wxmerkt, that we could merge this PR as is. This will allow to use the eigenpy package from robotpkg as well - without the need for the patched version from @wxmerkt, which provides However, I expect |
|
Hi @rhaschke and @gavanderhoorn, The PkgConfig and CmakeConfig get generated by https://github.com/jrl-umi3218/jrl-cmakemodules - I am not following all changes there (which frequently get synced into EigenPy). The relevant macros are https://github.com/jrl-umi3218/jrl-cmakemodules/blob/master/package-config.cmake#L108 and the config template is https://github.com/jrl-umi3218/jrl-cmakemodules/blob/master/Config.cmake.in I am also surprised to see NumPy and transitive dependencies aren't included (there was a PR recently to remove NumPy include dirs, jrl-umi3218/jrl-cmakemodules#383, yet I don't know its impact and EigenPy including this change has not been released to Melodic). Can we please clarify where this currently fails? The CI and buildfarm are both fine with the current state? This does appear on Arch Linux - and I have not reproduced or tested any release or packaging there. I want to make sure this gets addressed proper and is not due to the way I've released it directly from upstream. @rhaschke, @gavanderhoorn Do you suggest to add a patch to the release repository to add the NumPy when there is a good chance this is due to https://github.com/jrl-umi3218/jrl-cmakemodules? Or something else? Please advise - happy to provide time and a fix as you suggest. @rhaschke, the recommended and primarily supported way going forward upstream (https://github.com/jrl-umi3218/jrl-cmakemodules) is the CMake targets Edit: @rhaschke, eigenpy as released to the ROS buildfarm does not include a fix or manual export of the INCLUDE_DIRS by myself. This is directly from upstream (https://github.com/jrl-umi3218/jrl-cmakemodules/blob/master/Config.cmake.in). |
|
Thanks for your answers.
@rhaschke : I understand the need for backward compatibility, as I am in charge of some robots on 16.04 / kinetic. But Do you have requirements for older platforms ? Eigenpy and the jrl-cmakemodule are currently not supporting anything older than 16.04. This support can be added, but we would need to know precisely what do we need to support and until when. Otherwise, our CI would still have to check 10.04…
@rhaschke @wxmerkt : do you have any reference for this ? It seems natural for both of you, but it is not for me. I spent painful years to maintain old-style cmake stuff having to include all dependency everywhere, even transitive ones. And I think that's the reason why catkin and the jrl-cmakemodule were even created in the first place, to ease the management of those transitive dependencies. |
|
I found an issue with the Note, this is not an issue with either Kinetic (which has the same patch) or Melodic (which did not need a patch and from which I ported the patches to Noetic). |
|
@nim65s, I guess you are maintainer of eigenpy.
I'm fine to merge this here, but I would like to push you (as well) to either:
Currently, it's obviously in a broken state. |
|
@rhaschke : I totally agree with your points. Actually, I am working towards removing the old-style CMake support, but this should be done carefully to avoid breaking anything. This PR is part of it :) |
This is the crucial point. As the change is backward compatible (by now), let's merge it. |
|
Congrats on getting your first MoveIt pull request merged and improving open source robotics! |
|
Thanks Michael. |
|
I'm still a bit frustrated about not having an answer to this question:
As a Linux distribution maintainer it also "seems natural" to me to include all transitive dependencies. The "old-style" use was fashionable for a long time and replaced Let's just hope changing this does not break anything in terms of include reordering in the longer run, but as long as CI passes, it should be fine. |
|
I'm also a bit frustrated here and would really like to understand that better. pkg-config resolves dependencies transitively. If we put But I never saw any Of course, if we emulate the And if no one would want to keep up with manual dependency chains, who really wants to maintain the CMake code that concatenates those variables ? @v4hn this is one more thing that feel natural for you, but not for me :p In the end, the only natural way for me is the "modern" one. |
|
@nim65s, as far as I understand #2118 (comment), the stuff exported from the eigenpy cmake files is defined by your repo. Neither MoveIt nor catkin cmake read from |
|
@rhaschke This should be handled by https://github.com/jrl-umi3218/jrl-cmakemodules/blob/e715bf761e97dbcd704adecd03c28d8e195e7811/Config.cmake.in#L15-L78 where we search for the full paths for each of the defined pkg-config requirements. These lines were added to address jrl-umi3218/jrl-cmakemodules#346 in order to establish backwards compatibility (particularly for MoveIt as the primary consumer of the traditional CMake way). There is no more catkin-cmake for EigenPy, it's all handled directly by upstream. |
|
I guess, the mentioned code is also used to derive required include dirs and libs for the eigenpy target? If So, my proposal (utilizing |
|
The setting of these variables in the mentioned lines is to allow for variables in the projectConfig.cmake to be set when they are specified with pkg_config_add_dependency downstream - this is for the transition period as there are multiple ways to add dependencies with that cmake-modules. A few lines below that block is the modern CMake part that then includes the exported targets. Yes, your proposal does work and will likely be more robust as it builds on the exported targets. I've just verified it using the following cmake_minimum_required(VERSION 3.5)
find_package(eigenpy REQUIRED)
message(STATUS "eigenpy_INCLUDE_DIRS: ${eigenpy_INCLUDE_DIRS}")
message(STATUS "eigenpy_LIBRARIES: ${eigenpy_LIBRARIES}")
# Extract the properties
get_target_property(eigenpy_target_INCLUDE_DIRS eigenpy::eigenpy INTERFACE_INCLUDE_DIRECTORIES)
get_target_property(eigenpy_target_LIBRARIES eigenpy::eigenpy LINK_LIBRARIES)
message(STATUS "eigenpy_target_INCLUDE_DIRS: ${eigenpy_target_INCLUDE_DIRS}")
message(STATUS "eigenpy_target_LIBRARIES: ${eigenpy_target_LIBRARIES}")
###############################################################################
# Code from https://stackoverflow.com/a/51987470 to print all target properties
# Get all propreties that cmake supports
execute_process(COMMAND cmake --help-property-list OUTPUT_VARIABLE CMAKE_PROPERTY_LIST)
# Convert command output into a CMake list
STRING(REGEX REPLACE ";" "\\\\;" CMAKE_PROPERTY_LIST "${CMAKE_PROPERTY_LIST}")
STRING(REGEX REPLACE "\n" ";" CMAKE_PROPERTY_LIST "${CMAKE_PROPERTY_LIST}")
# Fix https://stackoverflow.com/questions/32197663/how-can-i-remove-the-the-location-property-may-not-be-read-from-target-error-i
list(FILTER CMAKE_PROPERTY_LIST EXCLUDE REGEX "^LOCATION$|^LOCATION_|_LOCATION$")
# For some reason, "TYPE" shows up twice - others might too?
list(REMOVE_DUPLICATES CMAKE_PROPERTY_LIST)
# build whitelist by filtering down from CMAKE_PROPERTY_LIST in case cmake is
# a different version, and one of our hardcoded whitelisted properties
# doesn't exist!
unset(CMAKE_WHITELISTED_PROPERTY_LIST)
foreach(prop ${CMAKE_PROPERTY_LIST})
if(prop MATCHES "^(INTERFACE|[_a-z]|IMPORTED_LIBNAME_|MAP_IMPORTED_CONFIG_)|^(COMPATIBLE_INTERFACE_(BOOL|NUMBER_MAX|NUMBER_MIN|STRING)|EXPORT_NAME|IMPORTED(_GLOBAL|_CONFIGURATIONS|_LIBNAME)?|NAME|TYPE|NO_SYSTEM_FROM_IMPORTED)$")
list(APPEND CMAKE_WHITELISTED_PROPERTY_LIST ${prop})
endif()
endforeach(prop)
function(print_properties)
message ("CMAKE_PROPERTY_LIST = ${CMAKE_PROPERTY_LIST}")
endfunction(print_properties)
function(print_whitelisted_properties)
message ("CMAKE_WHITELISTED_PROPERTY_LIST = ${CMAKE_WHITELISTED_PROPERTY_LIST}")
endfunction(print_whitelisted_properties)
function(print_target_properties tgt)
if(NOT TARGET ${tgt})
message("There is no target named '${tgt}'")
return()
endif()
get_target_property(target_type ${tgt} TYPE)
if(target_type STREQUAL "INTERFACE_LIBRARY")
set(PROP_LIST ${CMAKE_WHITELISTED_PROPERTY_LIST})
else()
set(PROP_LIST ${CMAKE_PROPERTY_LIST})
endif()
foreach (prop ${PROP_LIST})
string(REPLACE "<CONFIG>" "${CMAKE_BUILD_TYPE}" prop ${prop})
# message ("Checking ${prop}")
get_property(propval TARGET ${tgt} PROPERTY ${prop} SET)
if (propval)
get_target_property(propval ${tgt} ${prop})
message ("${tgt} ${prop} = ${propval}")
endif()
endforeach(prop)
endfunction(print_target_properties)
###############################################################################
# Print all set properties
print_target_properties(eigenpy::eigenpy)where the output is as follows (note that the |
|
TL;DR: This is brilliant. But I find it really sad that we would have to do this. If anyone motivates a use case where the current situation would be an issue, I will push for this solution.
@rhaschke In the current situation, yes, that's totally right. The jrl-cmakemodule is mostly a sum of layers after layers of additions of compatibility stuff. It does work pretty well. And wherever it doesn't, we add more compatibility code to make it work how we want it to. This fix a huge ton of issues that we don't even know we would have. Find and link to boost::python privately or publicly on Debian 7 & Fedora 32 for Python 2.6 & 3.4 ? Check. I have no idea of all the features it actually has. This thing is incredible. But… The whole existence of this module (and of catkin), is IMO a design issue of C++ that doesn't come with the right tooling modern languages have (not to mention that we also need Python). CMake came to fix this, but was not mature enough for a long time, so we had to hack stuff around to make it work in most of our use cases. I totally agree with @v4hn in "as long as CI passes, it should be fine". If it ain't broke, don't fix it. Now, each time we add another compatiblity layer for yet another reason… How do we ensure that we're not breaking another one ? We don't. We can't. How could we ? So I'm not joking when I say that if you need anything to work in any different way, we can manage to implement that. And your proposal seems really good for that, I wasn't expecting that much at all, thanks ! And @wxmerkt is taking this really seriously, providing well documented references, facts and experiments, as always, that's motivating ! But what do we actually need, and what do we really want to maintain ? The most standard stuff, as long as it works well enough. I think that the less CMake code I have, the better. This is not a code golf, but the combinatorial on CMake branches is untestable and a nightmare for any maintainer, don't you think ? How could we check all the CMake variables we export ? And to whom this solution would benefit ? Someone compiling an old moveit but with a recent eigenpy, while having CMake >= 3.1 as required by recent eigenpy, but not recent enough to be able to use Your point is that if we provide this feature, we should ensure that it work. Actually, to the best of my knowledge, it does, in all places officially supported by ROS. And certainly a few more. |
|
@nim65s, I can understand your reasoning. If you only recommended the new target-based way, I just should - in the long run - remove the old style, (at least partially) broken |
fix stack-of-tasks/eigenpy#195 Since moveit#1737, eigenpy is found using find_package, so we can get all required informations to find headers and link to shared objects with a target_link_libraries to the eigenpy::eigenpy imported CMake target.
fix stack-of-tasks/eigenpy#195 Since moveit#1737, eigenpy is found using find_package, so we can get all required informations to find headers and link to shared objects with a target_link_libraries to the eigenpy::eigenpy imported CMake target.
fix stack-of-tasks/eigenpy#195 Since moveit#1737, eigenpy is found using find_package, so we can get all required informations to find headers and link to shared objects with a target_link_libraries to the eigenpy::eigenpy imported CMake target.
fix stack-of-tasks/eigenpy#195 Since moveit#1737, eigenpy is found using find_package, so we can get all required informations to find headers and link to shared objects with a target_link_libraries to the eigenpy::eigenpy imported CMake target. (cherry picked from commit 81e64b9)
fix stack-of-tasks/eigenpy#195 Since moveit#1737, eigenpy is found using find_package, so we can get all required informations to find headers and link to shared objects with a target_link_libraries to the eigenpy::eigenpy imported CMake target.
fix stack-of-tasks/eigenpy#195 Since #1737, eigenpy is found using find_package, so we can get all required informations to find headers and link to shared objects with a target_link_libraries to the eigenpy::eigenpy imported CMake target.
Moves the execution callback into its own thread to avoid blocking and actually calls the preempt function in with the cancel callback.
Description
fix stack-of-tasks/eigenpy#195
The issue there is that
eigenpy_INCLUDE_DIRSdoesn't provide numpy include dirs. Moreover, thoseeigenpy_INCLUDE_DIRS&eigenpy_LIBRARIESare backward compatibility artifacts from the pkg-config world and are now considered obsolete in modern CMake.But thanks to #1737, eigenpy is found using
find_package, so we can get all required information to find all the needed headers and link to all the needed libraries with atarget_link_librariesto theeigenpy::eigenpyimported CMake target.Checklist