Solver CMake improvements #1402
Conversation
|
Should solve an issue @pkesseli encountered building with Cygwin. Maybe @marek-trtik could review too? |
There was a problem hiding this comment.
Looks good to me.
However, one question. Both minisat2 and glucose are put under if-else statements. Also, they cannot come together, i.e. either one is installed or another. In the original CMakeLists.txt both were installed always. Is this conditional installation really desired?
|
Yes, I think it's more desirable. It's not possible to use glucose + minisat at the same time. Therefore, we should choose one during configuration, and then only download the one we selected. |
|
Sounds good and correct! Thanks. |
|
Any news on this? |
There was a problem hiding this comment.
Works now. For the record, I had to use the following configuration: cmake -H. -Bbuild -DCMAKE_CXX_COMPILER=i686-w64-mingw32-g++ -DCMAKE_C_COMPILER=i686-w64-mingw32-gcc -DCMAKE_LEGACY_CYGWIN_WIN32=1 -DCMAKE_CXX_FLAGS='-Wno-overflow -Wno-return-type -D__STDC_FORMAT_MACROS'. Most importantly, __STDC_FORMAT_MACROS needs to be defined in order to avoid errors about a missing symbol PRIi64.
|
@pkesseli Could you add this information to the CMake wiki so it doesn't get lost. @reuk Is it worth adding it to the |
|
Yes, those flags should probably go in the readme, but we should also fix the build so that warnings don't have to be manually suppressed. It's worth noting that this PR is superseded by #1426, which should handle flag pass-through in a more robust way. @pkesseli could you try building the branch from #1426, but with the same flags as above? If that branch also works, I'll close this PR because I think the other solution is much better. |
|
Documented it briefly in the Wiki. #1426 is already merged. |
Pass compiler and flags through to the solver libraries when configuring, and ensure they use C++11.