Fixes Builds where the C++ runtime library does not have std::put_time() #2439
Conversation
…e(). The original checks are not sufficient. For instance when building on Ubuntu14 with the Clang-3.4 compiler. This likely fixes other builds as well.
jlsantiago0
changed the title
| # NOTE: Should we set -std or use the current compiler configuration. | ||
| # It seems that the top level build does not track the compiler | ||
| # in a consistent manner. So Maybe we need this? | ||
| set(CMAKE_REQUIRED_FLAGS "-std=c++11") |
There was a problem hiding this comment.
Using particular C++ standard is quite a complicated thing; not sure what these "required" flags do, but whether C++11 standard is required to compile the C++ library, it's also checked elsewhere. This check upon confirmation should add its brick to requiring C++11 for compiling SRT library, but this is only one of the pieces and should be checked toghether with the others, for example, to check conflict with --disable-c++11 flag.
There was a problem hiding this comment.
CXX_STANDARD and CXX_STANDARD_REQUIRED is available to underlying try_compile(), which is used by check_cxx_source_compiles(), but is not available before CMake v3.8. This project has a minimum required CMake v2.8.x. So I couldnt use it. Otherwise I would have.
The CMAKE_REQUIRED_FLAGS are used by check_cxx_source_compiles() when trying to compile and is also of a course a local variable in the function. So it will not bleed over to the parent scope. You can use this method for instance to try and deduce what flags are needed to compile a bit of code. check_cxx_source_compiles() will use the current state of the CMAKE_CXX_FLAGS as well as any prior compiler configurations in addition to the CMAKE_REQUIRED_* , but since in the top level sometimes these are added to target configurations, I could not rely on them being set in the top level toolchain configuration because the toolchain configuration is not always consistently configured globally, but some of the configuration is only added at target configuration. So this at least determines if this function is callable when -std=c++11 is set. Someone could add more conditions before calling add_definitions(-DHAVE_CXX_STD_PUT_TIME=1) in the top level project should that become necessary.
This resolves some build breakage and should not break anything since the current source file that uses that definition is only built when ENABLE_CXX11 is set.
| elseif (LINUX AND HAVE_LIBATOMIC AND HAVE_LIBATOMIC_COMPILES) | ||
| # This is a workaround for some older Linux Toolchains. | ||
| if (srt_libspec_static) | ||
| target_link_libraries(${TARGET_srt}_static PUBLIC atomic) | ||
| endif() |
There was a problem hiding this comment.
@jlsantiago0
Please confirm if this is the intended change.
P.S. Sorry, it's been quite some time since you opened the PR.
There was a problem hiding this comment.
Couldn't all these conditions be consolidated so that this target_link_libraries call happens only once?
There was a problem hiding this comment.
@jlsantiago0 Please confirm if this is the intended change.
P.S. Sorry, it's been quite some time since you opened the PR.
Yes I was fixing the build with Ubuntu14 and the Clang compiler that installed with apt get install clang . That compiler uses the GCC Standard C++ library with partial support for C++11. The __STDC_VERSION__ is set to 201112 so C++11 is indicated, but it does not seem to be complete. It seems to have all of TR1 and most of C++11, but it does not have std::puttime(). So this checks for it programatically. BTW. This also fixes the builds on some of the pre 10.9 MacOS SDKs which are in a similar state.
Also that toolchain on Ubuntu14 requires linking of the atomic library when compiling statically. So it is safe to link in the atomic library if the tests show that it is linkable when compiling statically. Because I was unable to figure out a way to test whether it was required or not in every situation. But def needed for the Clang toolchain anyway on this platform.
There was a problem hiding this comment.
IRT @ethouris questions. I was trying to minimize the changes to implement this check. I assume that the target_link_libraries() stuff could be consolidated. But It would require some restructuring I think anyway. And was not related to this particular issue i was trying to solve.
There was a problem hiding this comment.
I mean, it looks kinda weird once you merge them. It's something like:
if (HAVE_LIBATOMIC and srt_libspec_static)
if (HAVE_LIBATOMIC_COMPILES_STATIC OR (LINUX AND HAVE_LIBATOMIC_COMPILES))
target_link_libraries...
endif()
endif()
Meaning, you add libatomic to the static SRT library target (though not to shared one - in which case it might be expected to be added indirectly?) in case when you have statically compiled-in atomic library or if you have also the shared one but on Linux (although not for, for example, Android, for which the first of the condition branches was done). What exactly should then happen in case when you have a shared atomic library and you request to compile the static library? Does HAVE_LIBATOMIC_COMPILES imply HAVE_LIBATOMIC_COMPILES_STATIC? Can adding this library be in specific situations superfluous?
There was a problem hiding this comment.
Yeah i found that on some versions of linux, LIBATOMIC Cannot be linked to a static application, but it can be linked with a static SRT library. The HAVE_LIBATOMIC_COMPILES_STATIC is set if we could link LIBATOMIC with an application built with -static. The application itself is static. If that works, then we can unconiditionally link in libatomic here. The other check (LINUX AND HAVE_LIBATOMIC_COMPILES) means that we are on LINUX and LIBATOMIC can be compiled and linked into an application. It just may or may not be able to link into a static application. This usually means that the host does not have a libatomic.a, but only libatomic.so or that there is some unexpected dependency on libatomic.a that I couldnt figure out. This can happen using the libatomic with clang when the sanitizers are enabled for instance. And in that case it is safe to let libatomic be a link dependency.
Most of this has been worked out now with the newer toolchains. It is just a bit painful for GCC<6 and Clang<4.2ish i think or if you are using the sanitizers. I tend to run the SRT library through the sanitizer builds on both GCC and Clang so I find weird things like this.
In the case I was specifically fixing here: It was Ubuntu14, gcc (Ubuntu 4.8.4-2ubuntu1~14.04.4) 4.8.4, and clang version 3.4.2 (tags/RELEASE_34/dot2-final)
In particular, this C++ runtime has most of C++11, except it does not have std::put_time() and when building with the Clang toolchain on this platform we need to specify a link dependency for libatomic for the SRT static library. It does not have a static version of libatomic.a, so the static application build fails, but the static srt library is usable as long as we specify the libatomic link dependency. I have had similar issues with other older platforms as well. I dont remember all of them since it has been while since i looked at this issue.
maxsharabayko
added
Type: Maintenance
The original checks are not sufficient. For instance when building on Ubuntu14 with the Clang-3.4 compiler. This likely fixes other builds as well with older C++11 runtimes. For instance MacOS SDK <10.9 also does not support this function, but does support c++11.
SRT now builds with GCC-4.8.4 and Clang-3.4 under Ubuntu14.