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 option that build with FLANN dynamic linking on Windows #1918

Closed
UnaNancyOwen opened this issue Jun 30, 2017 · 8 comments
Closed

Add option that build with FLANN dynamic linking on Windows #1918

UnaNancyOwen opened this issue Jun 30, 2017 · 8 comments

Comments

@UnaNancyOwen
Copy link
Member

PCL always search static library of FLANN on Windows.
It is good operation as default settings.
But, I think that FLANN search should be introduce an option to search for dynamic libraries like a Boost for Windows users. (PCL_BUILD_WITH_BOOST_DYNAMIC_LINKING_WIN32)
What do you think about this proposal?

  • pcl/pcl_options.cmake#add_new_line
# Build with dynamic linking for FLANN (advanced users)
option(PCL_BUILD_WITH_FLANN_DYNAMIC_LINKING_WIN32 "Build against a dynamically linked FLANN on Win32 platforms." OFF)
mark_as_advanced(PCL_BUILD_WITH_FLANN_DYNAMIC_LINKING_WIN32)
if(NOT PCL_SHARED_LIBS OR ((WIN32 AND NOT MINGW) AND NOT PCL_BUILD_WITH_FLANN_DYNAMIC_LINKING_WIN32))
    set(FLANN_USE_STATIC ON)
endif()
@UnaNancyOwen UnaNancyOwen mentioned this issue Jun 30, 2017
15 tasks
@SergioRAgostinho
Copy link
Member

I think the developer should always have the option to select if its dependencies are static or dynamic, so in that sense I endorse the proposal.

Now the question we should be asking is why weren't we allowing to link against a dynamic flann on Windows? What happens if we simply do this?

if(NOT PCL_SHARED_LIBS)
    set(FLANN_USE_STATIC ON)
endif()

We can wait for you to submit the PR with the proposal.

@UnaNancyOwen
Copy link
Member Author

UnaNancyOwen commented Jun 30, 2017

I don't know the reason why it was implemented this.
However, FLANN is a required library in some major modules of PCL.
I think that it is better to static link FLANN in default settings on Windows.
(Boost is also static link in default settings on Windows.
I think that FLANN is better for the same action as Boost.)

@SergioRAgostinho
Copy link
Member

@UnaNancyOwen Can you try on your windows environment to modify the CMakeLists.txt to removed the check for windows and report on the results?

if(NOT PCL_SHARED_LIBS)
    set(FLANN_USE_STATIC ON)
endif()

However, FLANN is a required library in some major modules of PCL.
I think that it is better to static link FLANN in default settings on Windows.
(Boost is also static link in default settings on Windows.
I think that FLANN is better for the same action as Boost.)

If there's no real known reason at this point, then we should try to see what happens if we just try to mimic the linking behavior of the other OSs.

@UnaNancyOwen
Copy link
Member Author

I sent a pull request (#1919). This change does not affect other OS.
It will same setting as before by default settings on Windows.
FLANN is dynamically linked only when explicitly setting option by user.

@SergioRAgostinho I will confirm that.
It was able to build successfully when I tried it before.

@UnaNancyOwen
Copy link
Member Author

@SergioRAgostinho I tried some tutorials, and confirmed that these tutorials can be built and ran successfully.

@SergioRAgostinho
Copy link
Member

Then we definitely needs to revise things after this release.

@SergioRAgostinho
Copy link
Member

(new pretty labels nice :D)

@SergioRAgostinho
Copy link
Member

Superseeded by #2089.

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

No branches or pull requests

3 participants