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

Fix compiling MEX function on Windows #45

Closed
wants to merge 3 commits into from

Conversation

diegoferigo
Copy link
Member

  • Updated FindMatlab.cmake with the latest upstream version
  • Changed external symbol to const. This change has to be tested on Windows.

Fixes #44 and robotology/robotology-superbuild#226.

@diegoferigo
Copy link
Member Author

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 robotology/robotology-superbuild#226. @RAR1989 can you please try to remove the lib entry and check if everything works correctly?

@traversaro
Copy link
Member

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
Copy link
Member

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

As discussed in robotology/robotology-superbuild#226 (comment), 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 robotology/how-to-export-cpp-library#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
Copy link

DanielePucci commented Nov 15, 2019

@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
Copy link
Member

Great!

@traversaro
Copy link
Member

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
Copy link
Member

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

${MEX_VERSION_FILE}
, but for some reason is not defined correctly, perhaps it is incorrectly compiled with the C++ mangling.

@traversaro
Copy link
Member

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

set(MEX_VERSION_FILE "${Matlab_ROOT_DIR}/extern/version/c_mexapi_version.c")
) 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

${MEX_VERSION_FILE}
. 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 robotology/how-to-export-cpp-library#25 and https://github.com/robotology/how-to-export-cpp-library/blob/307f7bd78789cb68aa58c7804a3549ec23679483/CMakeLists.txt#L17 for related info.

@traversaro
Copy link
Member

@traversaro
Copy link
Member

I tested the PR with the modified FindMatlab file available in robotology-dependencies/qpOASES#13 , and indeed now the compilation is working fine. Adding the inline specifier as discussed in #45 (comment) 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
Copy link
Member

As discussed in robotology-dependencies/qpOASES#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
Copy link
Member

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
Copy link
Member Author

Replaced by #54

@diegoferigo diegoferigo closed this Feb 1, 2020
traversaro added a commit to robotology/robotology-superbuild that referenced this pull request Feb 1, 2020
traversaro added a commit to robotology/robotology-superbuild that referenced this pull request Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants