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

Modernize some CMake macros (Increases minimum required CMake version to 3.5) #3044

Merged
merged 6 commits into from
May 3, 2019

Conversation

SunBlack
Copy link
Contributor

@SunBlack SunBlack commented Apr 29, 2019

This is a preparation for another change, which I didn't got ready today. But I think it's already enough for a PR.

//Edit: Typical error source: Using to modern CMake commands. PCL now requires CMake 3.5, because cmake_parse_arguments was new in CMake core with CMake 3.5. Wanted already to use PARSE_ARGV N, but this was introduced in 3.7, so had to use old way.
//Edit2: We could keep CMake 3.1 compatibility by calling include(CMakeParseArguments) before. As far as I see we used cmake_parse_arguments already before this changes, but we didn't included module, so CMake script was already broken for 3.4 and below xD

@SunBlack SunBlack force-pushed the modernize_cmake_macros branch from 6ba659d to 75a4342 Compare April 29, 2019 19:01
@SunBlack SunBlack changed the title Modernize some Cmake macros Modernize some CMake macros (Increases minimum required CMake version to 3.5) Apr 29, 2019
@SergioRAgostinho
Copy link
Member

Don't forget to update these documentations files
https://github.com/PointCloudLibrary/pcl/pull/2605/files

@SergioRAgostinho SergioRAgostinho added this to the pcl-1.10.0 milestone May 3, 2019
Copy link
Member

@SergioRAgostinho SergioRAgostinho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When all is reviewed, I propose to squash the "fixup" commit df5b630 onto the original commit which generated that particular line.

@SunBlack
Copy link
Contributor Author

SunBlack commented May 3, 2019

How you want to squash with master? The fixup fixes something which is already merged into master.

@SergioRAgostinho
Copy link
Member

Ignore what I said then 😅 I reviewed the PR per commit and thought it was something added on some previous commit.

@taketwo taketwo merged commit fd964d2 into PointCloudLibrary:master May 3, 2019
@SunBlack SunBlack deleted the modernize_cmake_macros branch May 3, 2019 14:16
@SergioRAgostinho
Copy link
Member

SergioRAgostinho commented May 6, 2019

Heads up: I'm unable to successfully configure CMake on my OSX environment after this PR. Need to figure out what is different between me and the CI.

Here's a really shortened version of the error log

CMake Error at cmake/pcl_targets.cmake:297 (install):
  install TARGETS given no RUNTIME DESTINATION for executable target
  "pcl_fs_face_detector".
Call Stack (most recent call first):
  apps/CMakeLists.txt:70 (PCL_ADD_EXECUTABLE)


CMake Error at cmake/pcl_targets.cmake:297 (install):
  install TARGETS given no RUNTIME DESTINATION for executable target
  "pcl_manual_registration".
Call Stack (most recent call first):
  apps/CMakeLists.txt:80 (PCL_ADD_EXECUTABLE)

It's being very clear about the reason. I'll dig into it to better understand what changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants