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

Implement imported target for FLANN #2861

Merged

Conversation

SunBlack
Copy link
Contributor

Base of documentation and coding style is shipped FindGTest.cmake script of CMake

@SunBlack SunBlack force-pushed the flann_imported_target branch from 2f8ac36 to bfa6a9a Compare February 11, 2019 19:25
@SunBlack
Copy link
Contributor Author

Not sure if we really need -DFLANN_STATIC (just added it again, because I'm not sure)

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.

LGTM. Thank you.

I'm curious to see if this will fix also #2802. I believe it will because we're using find_library regardless of the success of pkg-config and that will ensure that a full path to the library is passed on to the consuming targets. Ping @claudiofantacci to monitor this one.

@SergioRAgostinho
Copy link
Member

Not sure if we really need -DFLANN_STATIC (just added it again, because I'm not sure)

I don't see an explicit use in our source files. But I have the impression this might be needed by FLANN and its dependencies. Actually that's the feeling that it comes out from this PR @jspricke submitted, specifically the addition of the LZ4_STATIC_FLAGS.

@claudiofantacci
Copy link
Contributor

Ping @claudiofantacci to monitor this one.

Yes, I think it does. By a quicklook at the PR, it creates a FLANN target with all the required info, just as the CMake file I was suggesting to use to address the problem (StandardFindModule.cmake from YCM).

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.

👍 Thanks.

I admit I was expecting a requirement to add FindFLANN.cmake to CMAKE_MODULE_PATH, but I suspect PCLConfig.cmake is doing that under the hood. Even better 🙌

Let's discuss squash strategy. I would propose to squash the first three commits and leave the last one separate.

@SunBlack SunBlack force-pushed the flann_imported_target branch from 115d80e to 4081492 Compare March 4, 2019 11:34
@SunBlack
Copy link
Contributor Author

SunBlack commented Mar 4, 2019

Let's discuss squash strategy. I would propose to squash the first three commits and leave the last one separate.

I moved every changed in vfh_recognition/CMakeLists.txt to second commit, too ;).

@SunBlack SunBlack force-pushed the flann_imported_target branch from 4081492 to 7c9d8c5 Compare March 5, 2019 13:21
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.

I moved every changed in vfh_recognition/CMakeLists.txt to second commit, too ;).

Cool thanks 👍

@SergioRAgostinho SergioRAgostinho merged commit 485033d into PointCloudLibrary:master Mar 5, 2019
@SunBlack SunBlack deleted the flann_imported_target branch March 5, 2019 21:22
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.

5 participants