Incompatibility with add_subdirectory

[cstratopoulos]

This relates to an issue mentioned in #7: the coinor-cmake subprojects are incompatible with add_subdirectory from a parent project.

The main problem is the use of CMAKE_[SOURCE|BINARY]_DIR rather than CMAKE_CURRENT_[SOURCE|BINARY]_DIR. I have made some attempts to fix this myself when focused just on using libClp with libOsiClp; the changes are in this patch file:
https://pastebin.com/BzmFd0wM
which you can try with git apply. But basically:

• the top level Clp/CMakeLists.txt needs the _CURRENT_ infix added
• in subprojects such as Clp/Clp, Clp/CoinUtils, instances of, e.g., ${CMAKE_SOURCE_DIR}/CoinUtils need to be replaced with just ${CMAKE_CURRENT_SOURCE_DIR}.

I suspect there may be remaining issues with coin-macros but I'm not sure.

I can't seem to verify if this patch works or not however. When running on Visual Studio 2017 15.9 and also on the Windows Subsystem for Linux, cmake encounters a segmentation fault in the CoinUtils directory. I have been able to isolate this to the point where add_library(libCoinUtils ...) is called, but I can't seem to figure out why it happens.

[ycollet]

Sorry, I was absorbed by other tasks on the project.
I will try to fix this ASAP.

[ycollet]

I pushed some changes.
I still need to perform thorough tests but everything seems to work on Cbc and Clp.
Just check from time to time if some fixes has not been pushed to fix some errors ...

[cstratopoulos]

@ycollet thanks for your work on this! Later today I will test locally as well and let you know if anything comes up

[cstratopoulos]

It seems that everything is set correctly in terms of using _CURRENT_ flavors of all variables where appropriate.

I am still experiencing a bizarre issue where CMake segfaults when entering the subdirectory for CoinUtils, during the call to add_library. Through some convoluted workarounds I was able to prevent this issue only to have it happen again when entering Osi/Osi.

However I think this is much more likely to be an issue with CMake rather than with your CMakeLists.txt scripts. I will see if I can reduce it to a minimal failing example and open a bug with CMake.

[ycollet]

That's odd ...
On which Linux distribution are you ?
Have you tried to compile a version of cmake yourself ?

[cstratopoulos]

I am on Ubuntu 16.04 operating in the Windows Substem for Linux on a Windows 10 machine. When invoking CMake through Windows an issue shows up in the same place as well, although a segfault is not reported.

I will indeed try compiling a later version of CMake to see if the issue persists.

[cstratopoulos]

Today I did some experimenting with continuous integration builds which run on a project at my work.

I was able to determine that the CMake segfault issue appears to be local to my machine as that step went fine on the cloud build machine.

However the build failed I believe due to an issue with the way include paths are set. I am experimenting with a fix on my fork https://github.com/cstratopoulos/coinor-cmake/tree/feature/target_link and will report back with results.

I believe the issue is with target property transitivity. I notice, for example, that in subdirectories such as Clp/Clp, Clp/Osi, etc. there is some duplication.

For example in Clp/Clp/CMakeLists.txt, we have the macro which adds the include directory ${CMAKE_BINARY_DIR}/CoinUtils/include as an include directory of libClp.

Are you familiar with the propagation of properties through the target_link_libraries command? To give an example with some simplified code snippets, we know that CoinUtils is a dependency of most (all?) of the other COIN-OR projects. Suppose the following appears in Clp/CoinUtils/CMakeLists.txt

add_library(libCoinUtils STATIC ${CoinUtils_SRC_LIST})

target_include_directories(libCoinUtils PUBLIC
    ${CoinUtils_DIR}/src
    ${CMAKE_BINARY_DIR}/CoinUtils/include
)

Next there is Clp which depends on CoinUtils:

add_library(libClp STATIC ${Clp_SRC_LIST})
target_include_directories(libClp PUBLIC
    ${Clp_DIR}/src
    ${CMAKE_BINARY_DIR}/Clp/include
)

target_link_libraries(libClp PUBLIC libCoinUtils)

note that we did not have to specify any of the include directories for CoinUtils; these are automatically propagated since they are public properties of the libCoinUtils targets. Moreover, building libClp.a will automatically link it against libCoinUtils.a.

Similarly in Clp/Osi/CMakeLists.txt, since Osi depends on CoinUtils, we can have something like

add_library(libOsi STATIC ${Osi_SRC_LIST})
target_include_directories(libOsi PUBLIC
    ${Osi_DIR}/src/Osi
    ${CMAKE_BINARY_DIR}/Osi/include
)

target_link_libraries(libOsi PUBLIC libCoinUtils)

again we don't have to manually specify any paths related to CoinUtils, and the linking is automatic.

And finally for OsiClp which naturally depends on Osi and Clp, we can just do

add_library(libOsiClp STATIC ${Clp_DIR}/src/OsiClp/OsiClpSolverInterface.cpp)

target_include_directories(libOsiClp PUBLIC ${Clp_DIR}/src/OsiClp)

target_link_libraries(libOsiClp PUBLIC Osi Clp CoinUtils)

this removes the need for

include_directories(${Clp_DIR}/src)
include_directories(${Clp_DIR}/src/OsiClp)
include_directories(${Osi_DIR}/src)
include_directories(${Osi_DIR}/src/Osi)
  target_include_directories(${TARGET} BEFORE PRIVATE ${CMAKE_BINARY_DIR}/Clp/include)
  target_include_directories(${TARGET} BEFORE PRIVATE ${CMAKE_BINARY_DIR}/CoinUtils/include)
  target_include_directories(${TARGET} BEFORE PRIVATE ${CMAKE_BINARY_DIR}/Osi/include)

With all this done, the CMakeLists.txt for my file may have a line such as add_subdirectory(Clp) where Clp is a folder constructed by cloning your repository and moving the Clp folder as per the README.

This will make the targets for CoinUtils, Osi, Clp, and OsiClp visible in my project. At which point I just have to write

target_link_libraries(MyTarget PUBLIC OsiClp)

then my target will automatically be linked against libClp.a, libCoinUtils.a, libOsi.a, libOsiClp.a (which already happens with the changes you've made), as well as having its include paths set appropriately to be able to find files like CoinPragma.hpp ClpSimplex.hpp, etc.

I realize this is another large-scope change, and I apologize for not being able to contribute further other than the sketch I've laid out above and the changes in my fork. The fork shows a quick workaround for what I hoped to get the build succeeding as quickly as possible, but taking the modern CMake, targets-based approach may have some pretty dramatic impacts in terms of being able to reduce code duplication and ease maintenance of the CMakeLists.txt files.

[cstratopoulos]

Sorry just a quick update:

• the project appears to build correctly with the changes on my feature/target_link branch (modulo an issue with an ARM build which I'll have to look into)
• I also moved the definition of the CLP_BUILD_EXAMPLES option into the root Clp/CMakeLists.txt and used it to guard all invocations of add_test. Otherwise, add_subdirectory(Clp) causes an invocation of ctest for my project to run all the Clp test cases/examples.