-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Fix link-type keywords in linked libraries in "PCLConfig.cmake" with CMake >= 3.11 #3341
Conversation
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.
Thanks for implementing a fix.
Just to make it explicit, this fixes things only for CMake >= 3.11. Older versions are still prone to this issue. However, since vcpkg is using a fresh CMake, this indeed can be counted as a proper fix for #2989. Right?
PCLConfig.cmake.in
Outdated
@@ -619,7 +619,7 @@ foreach(component ${PCL_TO_FIND_COMPONENTS}) | |||
INTERFACE_INCLUDE_DIRECTORIES "${PCL_${COMPONENT}_INCLUDE_DIRS}" | |||
INTERFACE_LINK_LIBRARIES "${PCL_${COMPONENT}_LINK_LIBRARIES}" | |||
) | |||
else() | |||
else(CMAKE_VERSION VERSION_LESS 3.11) |
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.
Shouldn't this be elseif
?
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, thanks for noticing and sorry for the noise, fixed in 0801d26, let me know if the rest of the PR seems ok so I can rebase it in the parent commit.
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.
Sorry, wrong button. Please see my inline comment.
Yes, exactly, I update the PR title. Beside vcpkg, for most (even if not all) Windows uses using a recent CMake is fairly straightforward, so it make sense to not worry too much about compatibility with older CMake versions, at least from my point of view. |
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.
Thanks. No need to do anything else, I will squash while merging.
Fix #2989
Related discussions: