Add build dependency on fmtlib #1930
Conversation
|
This is potentially very painful for us. Can you please elaborate how this is available in Red Hat (e.g., does it suffice to use a devtoolset when compilinng for Red Hat 6 and 7?). |
|
it's in epel. |
If it is installed from EPEL and linked dynamically, this quickly means we need it installed on everyting, not just build servers. |
|
fmt is a pure template library. |
Only a compile time dependency then? That makes it much less painful. It seems like it is available for both Red Hat 6 and 7 in EPEL. Sounds like it will be part of the GCC toolchain come C++20. If my understanding is correct, this is fine from my end. |
|
precisely. |
|
If it is a header-only lib, why do we change the target_link_libraries() call? |
|
because it pulls in include paths from the fmt::fmt dependency. |
|
I assumed target_link_libraries() only added LINK information. What you say seems to imply that it adds COMPILE information as well, is that correct? |
|
yes, fmt uses cmake targets, which is the "modern" way of doing cmake. these are cool because they carry all info. including include paths. the target_link_libraries call adds the include path as well. no need for separate _LIBRARES, _INCLUDE_DIRS etc variables any more. |
True. If we ever decide to take the time to rework OPM's CMake-based build system, then targets and properties is the way to do it. |
|
Thanks for the information. Now I am not against merging this, but I need one more piece of information: What do I need to do on a Ubuntu system to get this? I ask because we must update the build instructions on the website. (I assume |
|
|
CMakeLists.txt
Outdated
| @@ -373,3 +373,5 @@ if (OPM_ENABLE_PYTHON) | |||
| add_definitions(-DEMBEDDED_PYTHON) | |||
| endif() | |||
| endif() | |||
|
|
|||
| target_link_libraries(opmcommon PUBLIC fmt::fmt) | |||
There was a problem hiding this comment.
Looking at the documentation this line could arguably be
target_link_library(opmcommon PRIVATE fmt::fmt-header-only)instead. The above appears to link to a precompiled (.a or .so) version of the fmt library.
There was a problem hiding this comment.
Tried that - did not get it to work - assume the cmake in the binary debian package does not support this target?
There was a problem hiding this comment.
Tried that - did not get it to work.
In that case this (using fmt) may be more untenable. If we do indeed need a precompiled third-party library then it is a much more expensive proposition.
There was a problem hiding this comment.
The debian package supplies a libfmt.a - but that is an implementation detail debian have chosen when packaging, you could just as well dump a bunch of header files in your source folder.
I assume that since debian have opted to supply a libfmt.a in their binary package the corresponding cmake configuration file does not support the fmt::fmt-header-only link target.
But this should not alter the fact that there are no runtime dependencies involved here.
There was a problem hiding this comment.
it's a shared object on epel. anyways, see akva2@d8f0106 (sorry you have to "distribute" across your commits).
tested on rhel and ubuntu.
the fmt-header-only target is fairly recent so we cannot rely on it (even ubuntu 20.04 is on fmt 6.1.2 which does not export it in the config files).
There was a problem hiding this comment.
Thank'll pick that up; but it does look like you have chosen a non optimal starting point for your fixes. I guess if you rebase on top of the tip of this branch I can just merge the changes right in?
There was a problem hiding this comment.
no, it sits on top of two of your branches and changes are across different commits. just make it happen, i don't care about authorship or whatever.
There was a problem hiding this comment.
OK - done; I have added add_definitions(-DFMT_HEADER_ONLY). Thank you
|
jenkins build this please |
|
OK; I see this as roughly ack'ed - I will merge now. All developers need to install fmtlib on their machine. Unfortunately there are some version differences between the fmtlib versions available for different distributions, hopefully this will not create problems. I will merge this now, but will wait a bit with dependent code - so that we can easily pull it out again if it creates problems. |
|
I am struggling, but will try a bit more before I ask for mercy. I think a notice to the mailing list would be appropriate? |
I agree |
|
I managed to figure out things. There were no installation instructions, but a normal cmake-based build process was sufficient. |
In C++20 the library
fmtwhich replacesprintf()and<<based formatting - looks very good.Before C++20 we can use the external library: https://github.com/fmtlib/fmt/ - which is essentially what was included for C++20.
For the ongoing work with improvements in error messages this would be extremely valuable, I therefor suggest that we use
fmtlibuntil we switch to C++20.This PR assumes that fmtlib is installed on your system (it is available as binary package on RH, Ubuntu & Debian) and integrates that in the build system - the cmake integration is quite naive and probably needs some love and attention from someone in the know.