Install cmake config files for CoolProp library [2144]

[rainman110]

Description of the Change

This PR adds proper cmake config scripts to CoolProp. It installs cmake config files, which enables a user to use the CoolProp library as follows

find_package(CoolProp REQUIRED)

add_executable(CoolPropTest main.cpp)
target_link_libraries(CoolPropTest PRIVATE CoolPropLib)

The user does not need to fiddle with include paths, library names or compiler switches. Everything is stored in the following config files, which are installed by the install target (excerpt from make install):

• Installing: C:/src/3rdparty/CoolProp/install_root/lib/cmake/CoolProp/CoolProp-targets.cmake
• Installing: C:/src/3rdparty/CoolProp/install_root/lib/cmake/CoolProp/CoolProp-targets-release.cmake
• Installing: C:/src/3rdparty/CoolProp/install_root/lib/cmake/CoolProp/CoolProp-config-version.cmake
• Installing: C:/src/3rdparty/CoolProp/install_root/lib/cmake/CoolProp/CoolProp-config.cmake

More information on the specific files and their use can be found here: https://cmake.org/cmake/help/latest/guide/importing-exporting/index.html

Benefits

There are multiple benefits:

• Proper find_package support for coolprop to be used from 3-rd party software
• All specifics, like library name (specified during coolprop build), compile switches, include paths are handled by the config files: i.e. no compile time switches for the user required
• Supports the typical cmake workflow

Possible Drawbacks

The CoolProp libarary target is now fixed to CoolPropLib. From the users perspective, this is a good thing: the user simply needs to link against CoolPropLib, independent on the actually used library name.

The generated library file names can still be renamed in the CoolProp build by the user and are by default still "CoolProp". This is however an internal change that might interfere in the whole build process.

I tested building also the snippets, which worked fine though. I suggest building all interfaces (matlab, octave, java ...) to check, whether the renaming introduces any problems.

Verification Process

I built coolprop both as a "shared" and "static" library and used a small test build to check the functionality.

I attached the check build cmake project here:
CoolPropTest.tar.gz

I did not try to build all language interfaces though.

The check build project can be built like the following:

 cmake -G Ninja .. -DCMAKE_PREFIX_PATH=c:\src\3rdparty\CoolProp\install_root\
 cmake --build .

Note: I suggest adding this test to the CI pipeline.

Applicable Issues

Closes #2144

[CLAassistant]

All committers have signed the CLA.

[ibell]

How does one select which flavor of library (static, dynamic) is used? Same as before?

[ibell]

@jowr do you have an opinion?

This looks pretty sweet to me. I am in favor of improving cmake integration, but I don't want to run the risk that we break people's code if at all possible.

@rainman110 are you interested in taking a more active role in CoolProp maintenance and development?

[rainman110]

@rainman110 are you interested in taking a more active role in CoolProp maintenance and development?

@ibell Thank you for this offer!. I could try to improve the cmake build process... I have found a couple of things that could probably be improved. As I am not actually a user of CoolProp (I currently just try to improve the integration into our own build process), a maintainer role is currently out of my comfort zone 😏

[ibell]

Too bad, any help we can get is greatly appreciated.

[rainman110]

How are we proceeding with this PR? Any plans to merge this?

[rainman110]

@ibell is this PR still in your interest? It is still open and I am not sure whether this contribution is still appreciated . Otherwise i'll close it to not clutter your backlog.

[ibell]

I'm interested; my primary concern is breaking backwards compatibility. If there were a way to make this a totally non-breaking change I would merge it in a heartbeat.

[rainman110]

But is this really breaking backwards compatibility? You can still change the library name, so from the users point of view, nothing changes, right? The only changes are from the coolprop developers point of view, as the cmake target name has changed.

[jowr]

I think this could be super nice to have. I am going to look at this for v6.5 - I hope I can make it this year.

[jowr]

Hi, I have reviewed the changes and they do indeed break compatibility. The problem is that you introduced the new name "CoolPropLib" which is used for the generated libraries. This breaks basically all old wrappers as they rely the name "CoolProp" instead.

Is there any particular reason to make this change? If not, I would suggest to keep the old name. Please let me know what you think.

[rainman110]

Its already quite a while... I definitely know, that I made the rename for a reason, but I don't know anymore, why.

Which wrapper is affected? Note, that the does not change the library name / library file name but only the exported cmake target!

So it only makes a difference, if you somewhere use within cmake target_link_library(somewrapper CoolProp)

[rainman110]

Okay, I found out, why I did this rename. Some of the wrappers, e.g. COOLPROP_VBDOTNET_MODULE already define a target coolprop, see e.g. line https://github.com/CoolProp/CoolProp/blob/master/CMakeLists.txt#L1400.

This target collides with the target "CoolProp" of the library.

Before my changes, the target name was user defined anyhow, which is bad practice imho since this can totally break your cmake scripts.

IMHO, we should use fixed target names, but user definable library file names. We should definitely avoid target name collisions. Of course we can also use namespace prefixes to avoid collisions.

What do you think?

[rainman110]

Sorry to spam the issue. So I reread the cmake spec on add_custom_command. It seems, that is does not create a NEW target but appends the command to the target creation process.

So I misinterpreted the target name collision. In this case, we should be safe renaming CoolPropLib to CoolProp

[rainman110]

It goes on... I tried building the python bindings after I renamed locally CoolPropLib back again to CoolProp using the bool flag COOLPROP_PYTHON_BINARIES.

Then I get a target name collision due to the following cmake code

  add_custom_target(
    CoolProp
    COMMAND python setup.py ${COOLPROP_PYTHON_BINARY_VERSIONS}
    WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/wrappers/Python)

In line 1727 of CMakeLists.txt: https://github.com/CoolProp/CoolProp/blob/master/CMakeLists.txt#L1697

So we either rename all other targets CoolProp (if there are other than the python bindings), or we keep coolproplib

[jowr]

I think the issue here is that we took some unfortunate decisions regarding the design of the CMake file back in 2012. Our idea was that only one target can be built at a time and all targets are called CoolProp. The user switches decide what the CoolProp target actually is.

This is not a good idea, but we were all new to CMake in 2012 and started learning it "on the job".

[rainman110]

Our idea was that only one target can be built at a time and all targets are called CoolProp.

Okay, I see. So what should I do? Rename CoolPropLib to CoolProp and be fine with it?

[jowr]

I am afraid that is the only way to make it backwards-compatible. As I wrote, this is far from ideal, but we would have to make some major changes otherwise.

[dg0yt]

If possible, use a namespace for the modern usage:

find_package(CoolProp REQUIRED)

add_executable(CoolPropTest main.cpp)
target_link_libraries(CoolPropTest PRIVATE CoolProp::CoolProp) # or CoolProp::CoolPropLib

The namespace makes it clear that this is a target to be resolved by cmake, not a namespec to be passed to the linker.
CMake can create better error reports, already during configuration.