Fix overlinking of MATLAB bindings to MatlabEngine #840
Conversation
traversaro
changed the title
|
@traversaro could you also add the command you are calling in the first comment? (I suppose it is |
|
Done. |
There was a problem hiding this comment.
I am not an expert, but overall the cmake logic relating to NO_IMPLICIT_LINK_TO_MATLAB_LIBRARIES LGTM.
If you think it'd be safe to have a Matlab bindings/cmake expert's review regarding this, please request another review.
Minor comments regarding documentation.
| # Vendored from https://gitlab.kitware.com/cmake/cmake/-/raw/v3.20.0-rc3/Modules/FindMatlab.cmake | ||
| # With local modification to implement https://gitlab.kitware.com/cmake/cmake/-/issues/21912 |
There was a problem hiding this comment.
We need to document this change in Changelog and also the README so that we remember that we are not using a vanilla FindMatlab.cmake. Also the Matlab versions that we support and this is affecting .
There was a problem hiding this comment.
To be honest, I don't think so. As long as for the users the cmake -DIDYNTREE_USES_MATLAB .. works, I don't think we should document internal details such as which CMake scripts we vendor. Interested advanced user can just look in PR.
There was a problem hiding this comment.
Also the Matlab versions that we support and this is affecting .
This is something that we could do, but to be honest I don't know which MATLAB version we explicitly support (as we don't test them) so I don't know how can we do this in a proper way.
There was a problem hiding this comment.
I was actually thinking about the developers. iDynTree is a project that a few developers we know (including myself) use as a reference for software engineering. In case some other project faces something similar and come looking for answers here. That was the scenario I had in mind.
| Added the ``ENGINE_LIBRARY``, ``DATAARRAY_LIBRARY`` and ``MCC_COMPILER`` | ||
| components. | ||
|
|
||
| .. versionchanged:: 3.14 |
There was a problem hiding this comment.
what do these versionchanged tags imply?
There was a problem hiding this comment.
This is something that is part of the imported CMake file. I think it is used internally by CMake, but I don't have other details.
| if(NOT ${prefix}_NO_IMPLICIT_LINK_TO_MATLAB_LIBRARIES) | ||
| if(Matlab_HAS_CPP_API) | ||
| if(Matlab_ENGINE_LIBRARY) | ||
| target_link_libraries(${${prefix}_NAME} ${Matlab_ENGINE_LIBRARY}) | ||
| endif() | ||
| if(Matlab_DATAARRAY_LIBRARY) | ||
| target_link_libraries(${${prefix}_NAME} ${Matlab_DATAARRAY_LIBRARY}) | ||
| endif() | ||
| endif() | ||
|
|
||
| target_link_libraries(${${prefix}_NAME} ${Matlab_MEX_LIBRARY} ${Matlab_MX_LIBRARY}) |
There was a problem hiding this comment.
This is the logic affecting the first comment in the PR.
This PR fixes the problem described in robotology/robotology-superbuild#653 for iDynTree, by locally implementing the CMake fix proposed in https://gitlab.kitware.com/cmake/cmake/-/issues/21912 .
Before (the library links
libMatlabEngine.soandlibMatlabDataArray.so):After (for MATLAB only
libmexandlibmxare linked):