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

Add FLANN 1.9.1 #897

Merged
merged 9 commits into from
Mar 9, 2020
Merged

Add FLANN 1.9.1 #897

merged 9 commits into from
Mar 9, 2020

Conversation

Morwenn
Copy link
Contributor

@Morwenn Morwenn commented Feb 17, 2020

Specify library name and version: flann/1.9.1

I had to package the library to build PCL, and it is notably mentioned in #621. The example to make sure it finds the includes and links correctly is lacking a bit of substance but I couldn't find examples working without loading a whole dataset.

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@conan-center-bot
Copy link
Collaborator

Some configurations of 'flann/1.9.1' failed in build 1 (f37705388d44b074391aa2433236268edd269afc):

@conan-center-bot
Copy link
Collaborator

Some configurations of 'flann/1.9.1' failed in build 2 (0d09899c58128d9ee7ba3eabd9d8b4aae54af7ef):

@conan-center-bot
Copy link
Collaborator

All green in build 3 (9eeb5ad857ad64677437e04af80074544b2d763a)! 😊

recipes/flann/all/conanfile.py Outdated Show resolved Hide resolved
recipes/flann/all/conanfile.py Outdated Show resolved Hide resolved
@conan-center-bot
Copy link
Collaborator

All green in build 4 (4f232db3f99d59b5e0753d0b20fdf5b0e074ddb5)! 😊

int main()
{
flann::Matrix<float> dataset;
flann::Matrix<float> query;
Copy link
Contributor

Choose a reason for hiding this comment

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

These use default constructors of templated classes.
So no code from the packaged library is called and executed.
Maybe the examples at https://github.com/mariusmuja/flann/tree/master/examples are useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They load datasets, which seemed like something you don't always want to ship with your packages because it's an additional 10Mo. Now if it isn't a problem I'm definitely adding that.

Copy link
Contributor

Choose a reason for hiding this comment

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

10MiB is a bit too much, I'm afraid.
What could be done is skip the load, or create a dummy dataset in the same format as the example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try to come up with something when I find some time, maybe tomorrow.

Copy link
Member

Choose a reason for hiding this comment

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

Each folder can consume only 256KB IIRC

Copy link
Contributor

Choose a reason for hiding this comment

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

the point is just to verify example compiles and links correctly.
so, an actual example isn't needed. it should be enough to just call some API function (e.g. flann_get_distance_type, or any other you prefer). you may also call the function that will fail, it's okay (it just shouldn't crash or hang at least).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently only the C bindings allow to test whether linking works easily since flann_cpp.cpp only includes flann.hpp. Therefore I had the recipe build the C bindings along with the rest, I guess that it's pretty harmless to have them too.

@planetmarshall
Copy link
Contributor

We have flann and pcl recipes - through we used the FLANN master as we needed c++17 support, which in turn introduced an LZ4 dependency, so we also added flann-lib/flann#400 to the package. Feel free to email me if you run into any issues.

@Morwenn
Copy link
Contributor Author

Morwenn commented Feb 18, 2020

Thanks for proposing. We are using a stripped-down version of the FLANN recipe here which seems enough for PCL but considering that there haven't been official releases since 2016 it might be worth introducing date-based releases. I don't even know whether the project is still actively maintained anymore.

@conan-center-bot
Copy link
Collaborator

All green in build 5 (81aa86d03b1f89d43ac86092abc6d07eaa63dced)! 😊

SSE4
SSE4 previously requested changes Feb 20, 2020
Copy link
Contributor

@SSE4 SSE4 left a comment

Choose a reason for hiding this comment

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

call some API in test package pls (see comments)

@conan-center-bot
Copy link
Collaborator

Some configurations of 'flann/1.9.1' failed in build 6 (05dfb57219152d676958fb90816ffdaa5c8d73c1):

@conan-center-bot
Copy link
Collaborator

Some configurations of 'flann/1.9.1' failed in build 7 (60c8994474ce5a86e601af30cd38068d09ca261c):

@Morwenn
Copy link
Contributor Author

Morwenn commented Feb 22, 2020

Is there a way to generically handle OpenMP in Conan?

@conan-center-bot
Copy link
Collaborator

All green in build 8 (53ac2f45086d27660cfbc5689f38793a295028b7)! 😊

@danimtb danimtb requested a review from SSE4 March 3, 2020 15:12
@conan-center-bot
Copy link
Collaborator

All green in build 9 (53ac2f45086d27660cfbc5689f38793a295028b7)! 😊

@danimtb danimtb self-assigned this Mar 6, 2020
@danimtb danimtb merged commit f33e115 into conan-io:master Mar 9, 2020
@Morwenn Morwenn deleted the flann branch March 9, 2020 12:38
@Morwenn
Copy link
Contributor Author

Morwenn commented Mar 9, 2020

@planetmarshall Feel free to improve the recipe in any way, I only got a very basic recipe in.

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

Successfully merging this pull request may close these issues.

7 participants