conda-forge / qhull-feedstock

A conda-smithy repository for qhull.
BSD 3-Clause "New" or "Revised" License
3 stars 10 forks source link

Regression in 2020.2: Cannot find_package(Qhull) #12

Closed Tobias-Fischer closed 3 years ago

Tobias-Fischer commented 3 years ago

Issue: When building PCL, we noticed a regression in qhull (https://github.com/conda-forge/pcl-feedstock/issues/29):

For some reason, in recent builds that pull in qhull 2020.2, we get

-- Could NOT find Qhull (missing: QHULL_LIBRARIES QHULL_INCLUDE_DIRS) 

whereas previously with qhull 2019.1, we got

-- Found Qhull: optimized;$PREFIX/lib/libqhull_p.so;debug;$PREFIX/lib/libqhull_p.so  
-- QHULL found (include: $PREFIX/include, lib: optimized;$PREFIX/lib/libqhull_p.so;debug;$PREFIX/lib/libqhull_p.so)

So I think there is an issue with the cmake configuration files that are created. It is probably an issue upstream but I am reporting it here as I'm not sure.

/cc @traversaro as CMake guru - can you see anything wrong with the new cmake files in 2020.2? I see some changes when compared to 2019.1 but I don't have sufficient expertise to have an educated opinion whether they're wrong ..

traversaro commented 3 years ago

/cc @traversaro as CMake guru - can you see anything wrong with the new cmake files in 2020.2? I see some changes when compared to 2019.1 but I don't have sufficient expertise to have an educated opinion whether they're wrong ..

The CMake config files are not what changed, as those are ignored by PCL that instead ships its own FindQhull.cmake that takes the precedence over any CMake config file, see https://cmake.org/cmake/help/latest/command/find_package.html#basic-signature-and-module-mode :

If the MODULE option is not specified in the above signature, CMake first searches for the package using Module mode. Then, if the package is not found, it searches again using Config mode.

The main change in qhull is that for some reason the qhull library name changed from libqhull_p (that is the one search for by PCL, see https://github.com/PointCloudLibrary/pcl/blob/pcl-1.11.1/cmake/Modules/FindQhull.cmake#L18 to libqhull_r . I think @GiulioRomualdi had some practice with qhull in the past months (in https://github.com/dic-iit/bipedal-locomotion-framework/pull/51), I will probably check with him if he knows something on this change.

traversaro commented 3 years ago

I think @GiulioRomualdi had some practice with qhull in the past months (in dic-iit/bipedal-locomotion-framework#51), I will probably check with him if he knows something on this change.

From that discussion I found this interesting comment:

This issue is already known in the vcpkg community microsoft/vcpkg#9836 and a PR has been already proposed microsoft/vcpkg#9923. Unfortunately, this PR breaks the compatibility with PCL.

So it may be worth to check how they are dealing with this on vcpkg.

traversaro commented 3 years ago

I am not 100% sure, but it seems that qhull_p is deprecated and qhull_r should be used instead, see: https://github.com/qhull/qhull/blob/2020.2/CMakeLists.txt#L15 . Given that, probably it is worth to try if it is feasible to backport https://github.com/PointCloudLibrary/pcl/pull/4540 .

Other upstream issues:

traversaro commented 3 years ago

This issue is already known in the vcpkg community microsoft/vcpkg#9836 and a PR has been already proposed microsoft/vcpkg#9923. Unfortunately, this PR breaks the compatibility with PCL.

So it may be worth to check how they are dealing with this on vcpkg.

I did not see any vcpkg-specific fix for this, so probably qhull is just silently disabled in vcpkg PCL builds.

Tobias-Fischer commented 3 years ago

Thanks a lot for this detailed digging @traversaro! Backporting https://github.com/PointCloudLibrary/pcl/pull/4540 was quite easy, I changed my PR in https://github.com/conda-forge/pcl-feedstock/pull/30 accordingly.

I'll close this issue - I wasn't aware that PCL is shipping it's own cmake script to find QHull.