-
Notifications
You must be signed in to change notification settings - Fork 0
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
No absolute paths for dependants #3
base: master
Are you sure you want to change the base?
Conversation
This reverts commit f24142c.
@@ -72,9 +72,9 @@ target_include_directories(${PROJECT_NAME} | |||
${EIGEN3_INCLUDE_DIRS} | |||
) | |||
if (WITH_PCL) | |||
target_include_directories(${PROJECT_NAME} PUBLIC ${PCL_INCLUDE_DIRS}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but this would be wrong. client projects need to include pcl include dirs if they want to make use of pcl.hpp
shipped with this project.
same with libs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the description of the PR, looks like it still produces the correct includes. Thing is we just cannot use absolute paths to where the includes "were" during install if it should be relocatable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where do you see the correct pcl dirs being included? did you try with pcl being in a different prefix than this library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea its not in there, but if it contains a path which does not exist anymore, it errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes no doubt, but I'm not sure what the fix is.
shifting the inclusion and linking of transitive dependencies onto all client projects can't be the way to go. |
# Have to come last as they modify some variable used above. | ||
include(CMakeFindDependencyMacro) | ||
find_dependency(Eigen3 3.3.4 REQUIRED) | ||
find_dependency(PCL 1.11 REQUIRED COMPONENTS common io) | ||
# link the deps so that client projects get them also | ||
target_link_libraries(SickLMS5xx::SickLMS5xx INTERFACE Eigen3::Eigen pcl_common pcl_io) | ||
target_include_directories(SickLMS5xx::SickLMS5xx INTERFACE $<INSTALL_INTERFACE:include>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it make sense to make the PCL links private as you did above, but the generated SickLMS5xxConfig.cmake should still have the find_dependency()
calls to resolve this stuff at configure time, does including them here lead to any hardcoded paths???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i had thought that gets executed when you call find_package(Sick), so there should not be any install paths in there.
By no means sure this is the correct approach.. Cmake documentation on this is rather confusing because there appear to be many different ways of achieving the same thing.
Result in
deps/lib/cmake/SickLMS5xx/SickLMS5xxTargets.cmake
:dislike that all the vtk thingies are listed there instead of only the top-level dependencies. That appears to be a result of this line in the
CMakeLists.txt
: