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

Remove broken support for external metslib. #5959

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

larshg
Copy link
Contributor

@larshg larshg commented Feb 11, 2024

Fixes #4744
Fixes #605
Fixes #4792

@mvieth
Copy link
Member

mvieth commented Feb 19, 2024

My assessment of the situation:
If we wanted to support external/system metslib, we would still have to keep the metslib shipped with PCL, because not many package managers offer it: https://repology.org/project/metslib/information (vcpkg, homebrew, and apt for Debian/Ubuntu are missing for example).
We would have to make sure (in PCLConfig.cmake/hv_go.h) that downstream projects find the metslib includes, either the ones from PCL or the external/system installed.
We would also have to adapt the code in hv_go.h and hv_go.hpp to work with both the metslib shipped with PCL, and versions of metslib that may be externally available (e.g. from metslib's latest master branch). This would concern at least the call to setApplyAndEvaluate in hv_go.hpp.
In fact, the simulated annealing in the metslib in the PCL repo has been modified, such that the movit requires additional member functions (apply_and_evaluate, evaluate, apply, unapply). I am not sure what effects these modifications have. I think there is not really a guarantee that GlobalHypothesesVerification works correctly with any metslib except the one in the PCL repo. There is currently no unit test for GlobalHypothesesVerification.
PCL uses the BSD license, as does the metslib shipped with PCL ( e4d6501 ). Normally however, metslib uses a GPL or CPL license. I am no expert in licensing, but it sounds like this could be a problem if someone relies on PCL's BSD licensing.
I am okay with removing the (broken) support for external metslib, but we should probably wrap the metslib code in an additional pcl namespace, to avoid conflicts if a system metslib is available and used (then merge after PCL 1.14.1 is released because of possible ABI breaks).

@larshg
Copy link
Contributor Author

larshg commented Feb 27, 2024

I tried installing metslib from source and build recognition, but it is incompatible with more than just the setApplyAndEvaluate, ie there are errors with the following:

https://github.com/coin-or/metslib/blob/956478a9c9f1aee01097ff000c6cd13d9f9cc81a/metslib/simulated-annealing.hh#L136-L145

fx. std::variate_generator never made it to the standard afaik. So it seems to me that it would require updates to support the "system" installed version.

Effectively the system used version of metslib haven't been available since 2015, when 4fdd12b was commited. So I don't think many use it - and the few who would try, would just end up with compilations errors.

Regarding licensing, I think it should be up to the user if they install metslib from source, they should already have to conform to their license and since PCL is lesser restrictive, its fine.

@jiapei100
Copy link

Worked for me .... Thank you ...

@larshg larshg force-pushed the Removemetslibsystemsupport branch from 1c1e801 to 0f3da4c Compare May 8, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants