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

FLANN_INCLUDE_DIRS is empty from this change #3202 #3218

Closed
UnaNancyOwen opened this issue Jul 10, 2019 · 12 comments · Fixed by #3220
Closed

FLANN_INCLUDE_DIRS is empty from this change #3202 #3218

UnaNancyOwen opened this issue Jul 10, 2019 · 12 comments · Fixed by #3220
Assignees
Labels
Milestone

Comments

@UnaNancyOwen
Copy link
Member

FLANN_INCLUDE_DIRS is remove in this change #3202.
Therefor, cmake/pcl_all_in_one_installer.cmake#L14 is empty.
It means that the copy files of 3rdParty/FLANN fails during installation.

get_filename_component(FLANN_ROOT "${FLANN_INCLUDE_DIRS}" PATH)

There are two solutions to this issue.

  1. Change to FLANN_INCLUDE_DIR from FLANN_INCLUDE_DIRS
# pcl_all_in_one_installer.cmake#L14
- get_filename_component(FLANN_ROOT "${FLANN_INCLUDE_DIRS}" PATH)
+ get_filename_component(FLANN_ROOT "${FLANN_INCLUDE_DIR}" PATH)
  1. Add FLANN_INCLUDE_DIRS in cmake/Modules/FindFLANN.cmake
+ set(FLANN_INCLUDE_DIRS ${FLANN_INCLUDE_DIR})

Which do you think is good?

@UnaNancyOwen
Copy link
Member Author

@taketwo Do you have a reason to removed FLANN_INCLUDE_DIRS in #3202?

@taketwo
Copy link
Member

taketwo commented Jul 12, 2019

Hi, sorry for the delay. Reasons for removal:

  1. New finder script supports both new (config-based) and old (pkgconfig) discovery methods. The new config-based method does not create such variable, so I would have needed to add extra code for that.
  2. Generally, this variable is not needed because the consumers of FLANN only need to link to the imported target, includes will be set up automatically by CMake.

What I did not consider is the fact that this variable is needed for packaging. Unfortunately, none of your solutions are sufficient because of item 1. Could you explain why All-in-One installer needs this variable? As far as I can see, in PCLConfig it's hard-coded anyway:

set(FLANN_ROOT "${PCL_ROOT}/3rdParty/Flann")

@UnaNancyOwen
Copy link
Member Author

Could you explain why All-in-One installer needs this variable?

It is nessesary to get dependencies root path when generating PCL All-in-one Installer.
It need to collect all files (for inclusion in installer) to one directory for generate installer.

# FLANN_INCLUDE_DIRS is C:/Program Files/flann/include
# FLANN_ROOT is C:/Program Files/flann
get_filename_component(FLANN_ROOT "${FLANN_INCLUDE_DIRS}" PATH)

...

# Copy all files in FLANN_ROOT to PCL_ROOT/3rdParty/FLANN
# Generate installer based file contained in PCL_ROOT

@taketwo
Copy link
Member

taketwo commented Jul 12, 2019

Thanks. I will write some CMake code to extract FLANN_ROOT from an imported target. How urgent is this issue for you?

@UnaNancyOwen
Copy link
Member Author

It is necessary before the next release.

@UnaNancyOwen UnaNancyOwen added this to the pcl-1.10.0 milestone Jul 12, 2019
@taketwo taketwo self-assigned this Jul 12, 2019
@UnaNancyOwen
Copy link
Member Author

I will write some CMake code to extract FLANN_ROOT from an imported target.

I think it is not necessary, because the following small changes will work correctory.
It is simple, I will send a pull request soon.

  1. Change to FLANN_INCLUDE_DIR from FLANN_INCLUDE_DIRS
# pcl_all_in_one_installer.cmake#L14
- get_filename_component(FLANN_ROOT "${FLANN_INCLUDE_DIRS}" PATH)
+ get_filename_component(FLANN_ROOT "${FLANN_INCLUDE_DIR}" PATH)

@taketwo
Copy link
Member

taketwo commented Jul 12, 2019

But there is not FLANN_INCLUDE_DIR variable defined if we go this path:

# First try to locate FLANN using modern config
find_package(flann NO_MODULE ${FLANN_FIND_VERSION} QUIET)
if(flann_FOUND)
unset(flann_FOUND)
set(FLANN_FOUND ON)
# Create interface library that effectively becomes an alias for the appropriate (static/dynamic) imported FLANN target
add_library(FLANN::FLANN INTERFACE IMPORTED)
if(FLANN_USE_STATIC)
set_property(TARGET FLANN::FLANN APPEND PROPERTY INTERFACE_LINK_LIBRARIES flann::flann_cpp_s)
else()
set_property(TARGET FLANN::FLANN APPEND PROPERTY INTERFACE_LINK_LIBRARIES flann::flann_cpp)
endif()
return()
endif()

@UnaNancyOwen
Copy link
Member Author

Oops, that's right.
However, This root was not processed in my environment, because flann_FOUND is NO. Why?

@taketwo
Copy link
Member

taketwo commented Jul 12, 2019

Do you have FLANN master installed? If so, did you point CMake to this installation by providing flann_DIR variable?

@UnaNancyOwen
Copy link
Member Author

Is it enable in master? (I'm using 1.9.1.)
I will try with master/HEAD instead of 1.9.1. 👍

@taketwo
Copy link
Member

taketwo commented Jul 12, 2019

Yes, this is a new feature in their master. We had to adapt to it even before a new FLANN version is released because vcpkg is already using the master.

@UnaNancyOwen
Copy link
Member Author

UnaNancyOwen commented Jul 12, 2019

I confirmed that it went through that route with FLANN master/HEAD.
It is necessary to fix FindFLANN.cmake as you say.

I will write some CMake code to extract FLANN_ROOT from an imported target.

Please write this code. Thanks,

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

Successfully merging a pull request may close this issue.

2 participants