search

robotology / blockfactory

A tiny framework to wrap algorithms for dataflow programming
https://robotology.github.io/blockfactory
GNU Lesser General Public License v2.1
40 stars 16 forks source link

Fix compiling MEX function on Windows #45

Closed diegoferigo closed 4 years ago

diegoferigo commented 5 years ago

Fixes #44 and https://github.com/robotology/robotology-superbuild/issues/226.

diegoferigo commented 5 years ago

Then, on @RAR1989 setup, adding to PATH the <plugin-install-prefix>/blockfactory/bin and <plugin-install-prefix>/blockfactory/lib was necessary.

I am not really sure about the lib folder because it should be handled by blockfactory after setting the BLOCKFACTORY_PLUGIN_PATH. The missing bin folder, though, was the source of the errors described in https://github.com/robotology/robotology-superbuild/issues/226. @RAR1989 can you please try to remove the lib entry and check if everything works correctly?

traversaro commented 4 years ago

This error is now blocking the Windows CI for robotology-superbuild, see https://github.com/robotology/robotology-superbuild/pull/296/checks?check_run_id=303624209 . Unfortunately I don't have Windows with Matlab, so I can't test directly this PR, @DanielePucci can we get someone with Windows + Matlab from the lab to test this fix so that we can merge the PR? Thanks!

traversaro commented 4 years ago

Changed external symbol to const. This change has to be tested on Windows.

As discussed in https://github.com/robotology/robotology-superbuild/issues/226#issuecomment-510073518, unfortunately CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS is not able to export automatically static and global data variables, so those need to be manually exported (see https://github.com/robotology/how-to-export-cpp-library/issues/41), so those need to be manually exported, for example using generate_export_header. An alternative is to move the variable in the header and declare it as inline.

DanielePucci commented 4 years ago

@traversaro there should be Matlab+Windows on the "Monster" (btw, we should find a mapping between acronyms and official IIT names). So @diegoferigo was telling me that he could test the PR next week on it

traversaro commented 4 years ago

Great!

traversaro commented 4 years ago

With Matlab 2019b, this PR is now failing with the following error: 

LINK : error LNK2001: unresolved external symbol mexfilerequiredapiversion [C:\src\robotology-superbuild\build\robotology\BlockFactory\sources\Simulink\Simulink.vcxproj]
C:/src/robotology-superbuild/build/robotology/BlockFactory/lib/Debug/BlockFactoryd.lib : fatal error LNK1120: 1 unresolved externals [C:\src\robotology-superbuild\build\robotology\BlockFactory\sources\Simulink\Simulink.vcxproj]

Indeed, this make sense as mexfilerequiredapiversion is a symbol that is explictly requested to be part of the Mex funtion in the CMake code, and we do not define in our mex function.

traversaro commented 4 years ago

With Matlab 2019b, this PR is now failing with the following error:

LINK : error LNK2001: unresolved external symbol mexfilerequiredapiversion [C:\src\robotology-superbuild\build\robotology\BlockFactory\sources\Simulink\Simulink.vcxproj]
C:/src/robotology-superbuild/build/robotology/BlockFactory/lib/Debug/BlockFactoryd.lib : fatal error LNK1120: 1 unresolved externals [C:\src\robotology-superbuild\build\robotology\BlockFactory\sources\Simulink\Simulink.vcxproj]

Indeed, this make sense as mexfilerequiredapiversion is a symbol that is explictly requested to be part of the Mex funtion in the CMake code, and we do not define in our mex function.

This is not true. The function should be defined by https://github.com/robotology/blockfactory/blob/724c934e8af81316a40290e625836dfd8f522b84/cmake/FindMatlab.cmake#L1015 , but for some reason is not defined correctly, perhaps it is incorrectly compiled with the C++ mangling.

traversaro commented 4 years ago

Ok, that was fun to debug. : ) First of all I wanted to check why the symbol mexfilerequiredapiversion was not there, so I went where the object files for each compilation unit were stored, i.e. in <build>\sources\Simulink\Simulink.dir\Release. There I had only three .obj files:

There was no c_mexapi_version.obj, that was supposed to be the output of the compilation of the Matlab-provided c_mexapi_version.c file (see https://github.com/robotology/blockfactory/blob/724c934e8af81316a40290e625836dfd8f522b84/cmake/FindMatlab.cmake#L980) that is the one the Matlab provided file that defines mexfilerequiredapiversion.

For some reason, the c_mexapi_version.c file was ignored and not compiled even if it was added to the BlockFactory Mex library in https://github.com/robotology/blockfactory/blob/724c934e8af81316a40290e625836dfd8f522b84/cmake/FindMatlab.cmake#L1015 . What was the reason of that? Simply that the BlockFactory CMake project was configured in https://github.com/robotology/blockfactory/blob/v0.8/CMakeLists.txt#L6 as just be a project for compiling C++, so it silently ignores any C file. Adding C to the LANGUAGES option of the project CMake function call solved the problem.

This is not the first problem that I have with CMake projects that only decleare CXX as a project language, as a lot of Find***.cmake scripts actually try to compile C files for checking some characteristics of the library of the build environment, so in general I think it is wiser to always configure CMake projects with C and CXX as enabled languages, see https://github.com/robotology/how-to-export-cpp-library/issues/25 and https://github.com/robotology/how-to-export-cpp-library/blob/307f7bd78789cb68aa58c7804a3549ec23679483/CMakeLists.txt#L17 for related info.

traversaro commented 4 years ago

Related CMake upstream issues and MR:

traversaro commented 4 years ago

I tested the PR with the modified FindMatlab file available in https://github.com/robotology-dependencies/qpOASES/pull/13 , and indeed now the compilation is working fine. Adding the inline specifier as discussed in https://github.com/robotology/blockfactory/pull/45#issuecomment-554275195 works fine as well, but actually do to do we will need to require C++17, as inline variables are only supported since C++17, see https://en.cppreference.com/w/cpp/language/inline .

traversaro commented 4 years ago

As discussed in https://github.com/robotology-dependencies/qpOASES/pull/13 , I suggest to change this PR to use the FindMatlab that will be shipped with CMake 3.16.3, see https://gitlab.kitware.com/cmake/cmake/blob/e6c5bed2aa4ec729dd9d91c056c0147a1f5cd16e/Modules/FindMatlab.cmake .

traversaro commented 4 years ago

CMake 3.16.3 has been released with the fixes of FindMatlab.cmake useful to fix all the Windows linking problems, see https://discourse.cmake.org/t/cmake-3-16-3-available-for-download/530 .

diegoferigo commented 4 years ago

Replaced by #54