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.

Sign up for GitHub

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

Jump to bottom

No absolute paths for dependants #3

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

No absolute paths for dependants #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
2b38125
No target link or include
ahoereth Jun 8, 2023
fc6dc46
No deps
ahoereth Jun 8, 2023
a434abe
Do not source Targets.cmake for relocatability.
ahoereth Jun 8, 2023
8a538d6
Remove whole dependency block as it wants abs paths.
ahoereth Jun 8, 2023
bec613c
Private link libraries.
ahoereth Jun 8, 2023
d5ced81
private include
ahoereth Jun 8, 2023
40c6b24
public libs
ahoereth Jun 8, 2023
3fe58dc
target link dir public
ahoereth Jun 8, 2023
f24142c
private libs
ahoereth Jun 8, 2023
f657446
Revert "private libs"
ahoereth Jun 8, 2023
95a8929
Targets.cmake handles dependencies.
ahoereth Jun 8, 2023
a715655
Targets.cmake handles dependencies.
ahoereth Jun 8, 2023
4548c78
remove link dir
ahoereth Jun 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,9 @@ target_include_directories(${PROJECT_NAME}
${EIGEN3_INCLUDE_DIRS}
)
if (WITH_PCL)
target_include_directories(${PROJECT_NAME} PUBLIC ${PCL_INCLUDE_DIRS})
Copy link

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

Copy link
Member Author

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.

Copy link

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?

Copy link
Member Author

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.

Copy link

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.

target_include_directories(${PROJECT_NAME} PRIVATE ${PCL_INCLUDE_DIRS})
target_compile_definitions(${PROJECT_NAME} PRIVATE ${PCL_DEFINITIONS} WITH_PCL)
target_link_directories(${PROJECT_NAME} PUBLIC ${PCL_LIBRARY_DIRS})
target_link_directories(${PROJECT_NAME} PRIVATE ${PCL_LIBRARY_DIRS})
endif()

# make eigen, pcl an threads transitive dependencies of dependent projects
Expand Down
7 changes: 0 additions & 7 deletions cmake/SickLMS5xxConfig.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,3 @@ if(NOT TARGET SickLMS5xx::SickLMS5xx)
include("${CMAKE_CURRENT_LIST_DIR}/SickLMS5xxTargets.cmake")
endif()

# 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>)
Comment on lines -23 to -29
Copy link

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???

Copy link

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.