Exporting libnabo's build space: FIX or stop support

[HannesSommer]

In line https://github.com/ethz-asl/libnabo/blob/master/CMakeLists.txt#L196 we export the package libnabo but no targets.

The problem with that is that below we do

set(libnabo_library $<TARGET_FILE:${LIB_NAME}>)

and ultimately pass this generator expression to depender packages through the variable libnabo_LIBRARIES in libnaboConfig.cmake in the build tree.

This leads to the error reported here https://github.com/ethz-asl/libpointmatcher/issues/223#issuecomment-335777486, because the target nabo (${LIB_NAME}) is unknown to the dependers without it being exported and imported, too.

What we have to decide is whether we

1. expand the generator expression as we do for install tree also for the build tree
2. export the target nabo (I never did that; somebody else? might not work on the first shot)
3. stop exporting the build space (usage through catkin doesn't require it)
4. don't use the generator expression at all and hardcode the result, nabo (as done in https://cmake.org/Wiki/CMake/Tutorials/How_to_create_a_ProjectConfig.cmake_file)

History / background of the generator expression: In #57 it was introduced to prevent the warning about reading properties at config time : https://github.com/ethz-asl/libnabo/pull/57/files#diff-af3b638bc2a3e6c650974192a53c7291R162 Using file(GENERATE ... later is the more proper way (thus no warning for the current solution). However it adds quite a complexity for so little.

Any opinion @simonlynen , @stephanemagnenat , @pomerlef ?

I'd opt for 1. or 3., as it seems overkill to export the target as well (2.) and because we generate already for the install tree. I'm hesitating with 4. because I don't know why it wasn't done that way in the first place. Does somebody remember?

[HannesSommer]

PR #76 is a realization of 1. It seems to solve the issue and I'm pretty sure it won't mess up other things.

I've also gave 2. a shot but that seems even more work than expected (the export is easy but the import isn't automatic as I wrongly expected)

[HannesSommer]

Should be done with the merge of #76.

[svenevs]

FWIW this problem still persists. I too was very surprised to see that it somehow libpointmatcher magically found my build directory. I use EasyClangComplete with sublime text, which will internally run cmake when it finds a CMakeLists.txt at the top level of the open project. This is particularly problematic, because now a folder like /private/var/folders/wx/j7610hj10r3_h1xs4xr41bgw0000gn/T/EasyClangComplete/cmake_builds/2b85cf45a0437caf0da423b0ca739b17/ is getting found by libpointmatcher.

I'm willing to try and figure out (3), this seems like the right decision and would fix this for good (right?). However, I'm a little confused -- what is the actual reason for doing export at all? AKA who actually needs the export here / why? Is it the tests or python or something?

[HannesSommer]

Thanks @svenevs , for your input. If it finds that wired folder successfully (i.e. it can build against it later), then this is a/the cmake-export feature IMO. Once you've delete this folder it should no longer be found but something else (also part of the export feature).

That behavior is what we accept by exporting (I personally don't like it that much).

Concerning your question: I guess the only reason to export is to support finding a build space (so the potential to skip the install step).

Probably very few users actually need that. What we could do is add a CMAKE flag with default OFF and make it a condition for exporting. That could be a good compromise but would require some documentation.

I'm willing to try and figure out (3)

Just removing / conditioning the export should be enough: https://github.com/ethz-asl/libnabo/blob/af92330905b7dcc39b8a3c3be6eb5f4c0c89850e/CMakeLists.txt#L200

[svenevs]

Probably very few users actually need that. What we could do is add a CMAKE flag with default OFF and make it a condition for exporting. That could be a good compromise but would require some documentation.

That sounds like a reasonable course of action! I can do that, and include a full list of available CMake options on the README in the compilation section.

Before doing so, though, the real reason I'm playing with this stuff in the first place is to enable libpointmatcher to be able to build libnabo on its own if it is not installed. This relates here with respect to the naming of this option. For example, libpointmatcher and libnabo both have the same variable USE_OPEN_CL stored in the CACHE, which causes issues if a user sets it for libpointmatcher. The solution is simple, the effect for libnabo would be to change it to say LIBNABO_USE_OPENCL.

In light of this, I would like to include in the PR that enables switching on / off the export command renaming of variables for the options.

So to be clear, the would-be resolution here is just renaming options and adding one. The changes that need to happen for parent / child libpointmatcher and libnabo are more significant (about 80% finished, though I can't test OpenCL), and ultimately may not be desired in (either) upstream.

Would this be welcome, or would you rather I just submit a PR adding EXPORT_BUILD_DIR and documentation associated with the existing (and unchanged) options?

Just removing / conditioning the export should be enough

LOL so I had done that, but since I've never really done anything with raw export before (only generated export headers) I felt like more was needed.