Fixed Python binding tests on Windows #259
Conversation
...to be able to run e.g. the python binding tests using MSBuild without using generator expressions.
The pre-check target has become obsolete since there STP now uses a single binary directory on Windows, thus eliminating the need to copy stp.dll to directories containing executable files.
...since the Windows loader searches the directory of an executable for dynamic libraries it needs to load.
bindings/python/stp/CMakeLists.txt
Outdated
| if(WIN32) | ||
| set(LIBSTP_PATH ${CMAKE_INSTALL_PREFIX}/bin/stp.dll) | ||
| else() | ||
| set(LIBSTP_PATH ${CMAKE_INSTALL_PREFIX}/lib/libstp.so) |
There was a problem hiding this comment.
You should fix this before committing to master. This will break on Darwin where the suffix is .dylib, not .so.
CMakeLists.txt
Outdated
| @@ -385,8 +386,32 @@ message(STATUS "All defines at startup: ${COMPILE_DEFINES}") | |||
| # ----------------------------------------------------------------------------- | |||
| # Setup library build path (this is **not** the library install path) | |||
| # ----------------------------------------------------------------------------- | |||
| set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${PROJECT_BINARY_DIR}/lib) | |||
| set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${PROJECT_BINARY_DIR}/lib) | |||
| if(NOT WIN32) | |||
There was a problem hiding this comment.
Your branch condition is wrong. This is not windows specific. It depends on whether or not the CMake generator is single or multi-configuration. For example it is perfectly possible to use Ninja on Windows which is a single configuration generator. Instead your condition should be.
if (DEFINED CMAKE_CONFIGURATION_TYPES)
# Multi-configuration generator (e.g. XCode, Visual Studio)
else()
# Single configuration (e.g. Ninja, Unix Makefiles)
endif()
|
I'm fine with this. I have to say, I kind of ran out of steam regarding the Windows build. It would be really nice if it just built on AppVeyor, so we got green. Can you please either disable the tests or fix them? I'm fine with disabling. The problem is, I don't want to break AppVeyor, and if it's red, I don't know if it's red because the tests don't work, or because it doesn't even build. Thanks! |
|
I actually want to improve the code at this point. I won't be able to do that if it's red all the time and it's sending me 20 emails a day that it failed to build :( I know tests are important, but I think my sanity & making sure the application builds at all is even more important :) |
|
I can totally understand that. At this point, I’d actually be happy if the tests failed on my machine, too, but they still pass, which makes debugging whatever happens differently on AppVeyor pretty hard. So let’s disable the tests for now, and I’ll try to fix them in a branch. |
|
The fail on TravisCI was due to CMS, not STP. It should now be fixed. |
See issue #247 - the generator-expression-based approach didn't work with CMake versions shipped with Debian. With this PR, binaries are placed in a single, configuration-independent output directory on Windows, so that the python binding tests work without using generator expressions.
The PR also contains a small bugfix for installing STP on Windows: the .dll file is now placed in the installation's bin/ directory. Previously, only the .lib file had been copied to the installation's lib/ directory.