Problems with the shipping of a development version of FLANN.

[SergioRAgostinho]

Hey vcpkg team. I'm one of the maintainers for the Point Cloud Library project. We've been using vcpkg in our CI to meet our dependencies for a while now. Recently, we've hit a problem we can't seem to figure out how to patch, because it's vcpkg specific. It pertains the FLANN library.

1. The latest official Flann version, 1.9.1 ships its own lz4.
2. Our windows CI started having issues because you decided to use the development version instead of waiting for an official release. Installation of pcl from within vcpkg is patched to work with your own changes e.g., you strip out our custom FindFLANN.cmake because the flann development version already exports a configuration file.
3. When compiling the flann (dev version) target withing vcpkg, finding lz4 is now required. Before it wasn't. There's no version increment we can look for in the flann config file you export, meaning we have no way of distinguishing between the official version and your custom one, at configuration time. This means we currently can't patch things easily from our side.

Ultimately users who want to compile PCL from source, using vcpkg for the latest dependencies are pretty much in a tough spot at this point. I came here seeking advice on how to proceed, assuming I might have overlooked something.

Ping @UnaNancyOwen , @claudiofantacci .

[JackBoosY]

@SergioRAgostinho Thanks for reporting this issue!
Since the official version released by flann is too old (v1.9.1 08/05/2016), we upgrade it with the latest version (commit ID: 1d04523268c388dabf1c0865d69e1b638c8c7d9d).

And using this latest version build in vcpkg should be successful.

If you wish to use version 1.9.1, please revert vcpkg to Commit ID 18b029a and rebuild flann.

Thanks.

[cenit]

@SergioRAgostinho I will also take a look at your problem, since I want to integrate PCL in a project soon.
Are you sure the problem was done by that commit and not subsequent one, which reverted to 1.9.1 (now on master) in #6931?
To be honest, I can't understand why it was even reverted, maybe @JackBoosY can tell us better what's going on

edit: sorry, it was not reverted, he just used 1.9.1 tag to label the dev version... which is maybe an error, it would have been better to use the day tag

edit2: also I built PCL with my commit and didn't experience any problem, didn't try with the most recent one by @JackBoosY, but CI should have caught regression since we also have PCL here??

[JackBoosY]

@cenit As I said:

Since the official version released by flann is too old (v1.9.1 08/05/2016), we upgrade it with the latest version (commit ID: 1d04523268c388dabf1c0865d69e1b638c8c7d9d).

And the version tag should NOT be 1.9.1, which is my fault. We need to mark this dev version with a date.

The only problem is the reason why the author failed to build.

[SergioRAgostinho]

Thank you for your replies. I'll try to address all the comments.

If you wish to use version 1.9.1, please revert vcpkg to Commit ID 18b029a and rebuild flann.

For our ci issue, this is a valid workaround. I still believe we need to think ahead of simply sticking to a specific commit. I assume our users will want to keep vcpkg somewhat updated and it would be bad to force them to stick to a specific old version of vcpkg just because of PCL.

Are you sure the problem was done by that commit and not subsequent one, which reverted to 1.9.1 (now on master) in #6931?

I am. We are printing the port list at the very end of the dependency installation phase and it is showing jan2019. See https://dev.azure.com/PointCloudLibrary/pcl/_build/results?buildId=5194 , click on the task "Install dependencies", scroll the log all the way down.

flann:x86-windows                                  jan2019          Fast Library for Approximate Nearest Neighbors

This also means I haven't been able to verify if we still have problems with your current master. I simply checked the subsequent commits to the flann port and from briefly skimming through the diffs, I believe the issue should still be there.

edit2: also I built PCL with my commit and didn't experience any problem, didn't try with the most recent one by @JackBoosY, but CI should have caught regression since we also have PCL here??

To help you replicate our steps, this is what we do in our CI.

1. Clone the latest master from our github page
2. We currently only need to install the following dependencies

   vcpkg.exe install eigen3 flann gtest qhull
   

   However, this is because boost comes preinstalled in azure pipelines. You will need to install some extra boost modules. In *nix platforms, these usually are filesystem, date_time, iostreams, system. Depending on the commit you checkout, you might need more or less modules. We're currently migrating towards c++14, so threads was already removed and date_time will follow.
3. Configure and build pcl. You don't need to enable anything in particular. At some point, this error message should pop

Creating library D:/a/build/lib/pcl_kdtree.lib and object D:/a/build/lib/pcl_kdtree.exp
kdtree_flann.obj : error LNK2001: unresolved external symbol __imp__LZ4_resetStreamHC [D:\a\build\kdtree\pcl_kdtree.vcxproj]
kdtree_flann.obj : error LNK2001: unresolved external symbol __imp__LZ4_setStreamDecode [D:\a\build\kdtree\pcl_kdtree.vcxproj]
kdtree_flann.obj : error LNK2001: unresolved external symbol __imp__LZ4_decompress_safe_continue [D:\a\build\kdtree\pcl_kdtree.vcxproj]
kdtree_flann.obj : error LNK2001: unresolved external symbol __imp__LZ4_decompress_safe [D:\a\build\kdtree\pcl_kdtree.vcxproj]
kdtree_flann.obj : error LNK2001: unresolved external symbol __imp__LZ4_compress_HC_continue [D:\a\build\kdtree\pcl_kdtree.vcxproj]
D:\a\build\bin\pcl_kdtree.dll : fatal error LNK1120: 5 unresolved externals [D:\a\build\kdtree\pcl_kdtree.vcxproj]

Worth noting, that this is running on the CI and it is a clean machine every time we run. The error is popping because flann (master) now needs linking against an external lz4. However since our custom FindFLANN.cmake is complying with the latest official version, we don't look for or link with the newest lz4 dependency. Hence, the missing symbols.

This being said, yesterday @taketwo brought up the fact that since the development version now uses a config file, we can use that. Here's the solution draft he started working on PointCloudLibrary/pcl#3202. I.e. it is likely that we will be able to patch things from our side after all and this issue can be closed.

[taketwo]

We ended up going that path and now support both config- and pkgconfig-based detection of FLANN (PointCloudLibrary/pcl#3202), so this can be closed.

I would like to ask vcpkg maintainers to consider in future that PCL relies on vcpkg in CI. Thus when you introduce breaking changes (like it happened in #6294), it breaks our CI. Which is okay. But it would be great if besides from adding a patch to your portfile, you also ping us about the problem so we can take measures in timely fashion. Thanks!

[cenit]

My bad sorry. I forgot to ping upstream about the patch. Good that you solved your problems :)

[SergioRAgostinho]

Just adding extra clues to help you in case there is further interest in trying to reproduce the original issue. This is what we pass at configuration stage

cmake $(Build.SourcesDirectory) -G"%GENERATOR%" -DCMAKE_TOOLCHAIN_FILE=%VCPKG_ROOT%\scripts\buildsystems\vcpkg.cmake -DVCPKG_APPLOCAL_DEPS=ON -DPCL_BUILD_WITH_BOOST_DYNAMIC_LINKING_WIN32=ON -DPCL_BUILD_WITH_FLANN_DYNAMIC_LINKING_WIN32=ON -DPCL_BUILD_WITH_QHULL_DYNAMIC_LINKING_WIN32=ON -DBUILD_global_tests=ON -DBUILD_tools=OFF -DBUILD_surface_on_nurbs=ON

I'm not sure if it is relevant but we're linking dynamically against flann. This was the last commit before we merged the fix. PointCloudLibrary/pcl@d91f2af

[Neumann-A]

I wonder how this
-DPCL_BUILD_WITH_FLANN_DYNAMIC_LINKING_WIN32=ON
interacts with
vcpkg_check_linkage(ONLY_STATIC_LIBRARY)
in the portfile of flann. Build might work but could have strange runtime effects (don't know if something in the internals of PCL changes due to that). Should probably set the value dependent on FLANN_STATIC within the CMakeLists.txt instead

[Cheney-W]

Is this issue resolved?

[Cheney-W]

Please feel free to reopen this issue if you still encounter it. Thank you!