pantor / ruckig

Motion Generation for Robots and Machines. Real-time. Jerk-constrained. Time-optimal.
https://ruckig.com
MIT License
734 stars 166 forks source link

Fix BUILD_PYTHON_MODULE option on Windows/MSVC when BUILD_SHARED_LIBS is ON #18

Closed traversaro closed 3 years ago

traversaro commented 3 years ago

When building the Python bindings on Windows, the python_ruckig target generate three files: a .exp, a .lib and a .dll .The .dll has a special name that avoids its collision with the ruckig.dll shared library, but if OUTPUT_NAME is simply set to ruckig, then the ruckig.lib of the python_ruckig target has the same name of the ruckig.lib of the ruckig target, resulting in compilation errors such as:

C:\src\ruckig\src\velocity-step2.cpp(49,156): warning C4458: declaration of 'aMax' hides class member [C:\src\ruckig\build\ruckig.vcxproj]
C:\src\ruckig\include\ruckig/velocity.hpp(43,12): message : see declaration of 'ruckig::VelocityStep2::aMax' [C:\src\ruckig\build\ruckig.vcxproj]
C:\src\ruckig\src\velocity-step2.cpp(49,156): warning C4458: declaration of 'aMin' hides class member [C:\src\ruckig\build\ruckig.vcxproj]
C:\src\ruckig\include\ruckig/velocity.hpp(43,18): message : see declaration of 'ruckig::VelocityStep2::aMin' [C:\src\ruckig\build\ruckig.vcxproj]
C:\src\ruckig\src\velocity-step2.cpp(49,156): warning C4458: declaration of 'jMax' hides class member [C:\src\ruckig\build\ruckig.vcxproj]
C:\src\ruckig\include\ruckig/velocity.hpp(43,24): message : see declaration of 'ruckig::VelocityStep2::jMax' [C:\src\ruckig\build\ruckig.vcxproj]
  Generating Code...
  Auto build dll exports
     Creating library C:/src/ruckig/build/Release/ruckig.lib and object C:/src/ruckig/build/Release/ruckig.exp
  ruckig.vcxproj -> C:\src\ruckig\build\Release\ruckig.dll
  Building Custom Rule C:/src/ruckig/CMakeLists.txt
  python.cpp
LINK : fatal error LNK1149: output filename matches input filename 'C:\src\ruckig\build\Release\ruckig.lib' [C:\src\ruckig\build\python_ruckig.vcxproj]

This PR avoids the problem by just setting RUNTIME_OUTPUT_NAME (that just change the names of the .dll) when MSVC is used.

pantor commented 3 years ago

Thanks @traversaro! There was a bug in the CI pipeline, so I didn't catch this error. Thanks for fixing!

traversaro commented 3 years ago

Hi @ruckig, actually I think that the change I proposed is problematic, as it results in the library being called python_ruckig.cp39-win_amd64.pyd . The correct variable to set is LIBRARY_OUTPUT_NAME, but perhaps a more robust change is just to have the following logic, without any MSVC-specific code:

set_target_properties(python_ruckig PROPERTIES OUTPUT_NAME ruckig)
# To avoid conflicts in import libraries names in MSVC
set_target_properties(python_ruckig PROPERTIES ARCHIVE_OUTPUT_NAME python_ruckig)
pantor commented 3 years ago

Now it seems to work fine :+1:

traversaro commented 3 years ago

Great, thanks! cc @ahoarau