<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Hi! Thanks for the patch.<div class=""><br class=""></div><div class="">Looks good to me.</div><div class=""><div class=""><br class="Apple-interchange-newline">
</div>
<div><br class=""><blockquote type="cite" class=""><div class="">6 дек. 2019 г., в 7:50, Alexander Tikhonov <<a href="mailto:avtikhon@tarantool.org" class="">avtikhon@tarantool.org</a>> написал(а):</div><br class="Apple-interchange-newline"><div class="">
<div class="">Alexander,<br class=""><br class="">Thanks for the review, updated commit message as you suggested.<br class=""><br class=""><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;" class="">
Четверг, 5 декабря 2019, 4:49 +03:00 от Alexander Turenko <<a href="mailto:alexander.turenko@tarantool.org" class="">alexander.turenko@tarantool.org</a>>:<br class=""><br class=""><div id="" class=""><div class="js-helper js-readmsg-msg"><div class=""><div id="style_15755105661900185799_BODY" class="">LGTM.<br class=""><br class="">
I'm okay with the approach I proposed, of course :) My review would be<br class="">
insufficient.<br class=""><br class="">
Serge, can you do the second review?<br class=""><br class="">
WBR, Alexander Turenko.<br class=""><br class="">
> build: fix unit tests build with lrt<br class=""><br class="">
Nit: librt or -ltr<br class=""><br class="">
Or just 'on CentOS 6'. All those details about librt are described in<br class="">
the commit message below, while the header would be better if it will<br class="">
give us an idea why the commit was made.<br class=""></div></div></div></div></blockquote>Corrected commit message added 'on CentOS 6' add '-lrt', (use of 'librt' exceeded the limit of the commit message length)<br class=""><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;" class=""><div id="" class=""><div class="js-helper js-readmsg-msg"><div class=""><div id="style_15755105661900185799_BODY" class=""><br class="">
> After the commit 77fa45bd05f8cdd4c0f9bad85185ef5b61528d49<br class="">
> ('lua: add <a href="http://fiber.top" class="">fiber.top</a>() listing fiber cpu consumption')<br class="">
> the unit tests builds failed like:<br class="">
> <br class="">
> /opt/rh/devtoolset-6/root/usr/libexec/gcc/x86_64-redhat-linux/6.3.1/ld:<br class="">
> ../../src/lib/core/libcore.a(fiber.c.o): undefined reference to symbol<br class="">
> 'clock_gettime@@GLIBC_2.2.5'<br class="">
> //lib64/librt.so.1: error adding symbols: DSO missing from command line<br class="">
> collect2: error: ld returned 1 exit status<br class="">
> test/unit/CMakeFiles/cbus.test.dir/build.make:108: recipe for target<br class="">
> 'test/unit/cbus.test' failed<br class="">
> make[2]: *** [test/unit/cbus.test] Error 1<br class="">
> <br class="">
> Found that <a href="http://fiber.cc" class="">fiber.cc</a> is using now clock_gettime(), which requires -lrt<br class="">
> with glibc. To fix it added librt dependency for core library for glibc.<br class="">
> Due to glibc requires for -lrt for clock_gettime() only for some<br class="">
> versions, check 'man clock_gettime.2':<br class="">
> 'Link with -lrt (only for glibc versions before 2.17).'<br class="">
> the check whether is able to use clock_gettime() w/o librt library is<br class="">
> added.<br class="">
> <br class="">
> Close #4639<br class="">
> ---<br class="">
> <br class="">
> Github: <a href="https://github.com/tarantool/tarantool/tree/avtikhon/gh-4639-lrt-suggested-full-ci" target="_blank" class="">https://github.com/tarantool/tarantool/tree/avtikhon/gh-4639-lrt-suggested-full-ci</a><br class="">
> Issue: <a href="https://github.com/tarantool/tarantool/issues/4639" target="_blank" class="">https://github.com/tarantool/tarantool/issues/4639</a><br class="">
> <br class="">
> CMakeLists.txt | 6 ++++++<br class="">
> src/lib/core/CMakeLists.txt | 8 ++++++++<br class="">
> 2 files changed, 14 insertions(+)<br class="">
> <br class="">
> diff --git a/CMakeLists.txt b/CMakeLists.txt<br class="">
> index 7d6c0846e..ca968eb8d 100644<br class="">
> --- a/CMakeLists.txt<br class="">
> +++ b/CMakeLists.txt<br class="">
> @@ -116,6 +116,12 @@ else ()<br class="">
> set(HAVE_CLOCK_GETTIME ${HAVE_CLOCK_GETTIME_DECL})<br class="">
> endif ()<br class="">
> set(CMAKE_REQUIRED_LIBRARIES "")<br class="">
> +# According to `man 2 clockgettime` the glibc wrapper requires<br class="">
> +# -lrt on glibc versions before 2.17. We need to check whether<br class="">
> +# the function is available without -lrt to set this linker option<br class="">
> +# conditionally.<br class="">
> +check_function_exists(clock_gettime HAVE_CLOCK_GETTIME_WITHOUT_RT)<br class="">
> +<br class="">
> check_symbol_exists(__get_cpuid cpuid.h HAVE_CPUID)<br class="">
> <br class="">
> # Checks for libev<br class="">
> diff --git a/src/lib/core/CMakeLists.txt b/src/lib/core/CMakeLists.txt<br class="">
> index e60b5199e..8623aa0de 100644<br class="">
> --- a/src/lib/core/CMakeLists.txt<br class="">
> +++ b/src/lib/core/CMakeLists.txt<br class="">
> @@ -46,3 +46,11 @@ target_link_libraries(core salad small uri decNumber ${LIBEV_LIBRARIES}<br class="">
> if (ENABLE_BACKTRACE AND NOT TARGET_OS_DARWIN)<br class="">
> target_link_libraries(core gcc_s ${UNWIND_LIBRARIES})<br class="">
> endif()<br class="">
> +<br class="">
> +# Since <a href="http://fiber.top" class="">fiber.top</a>() introduction, <a href="http://fiber.cc" class="">fiber.cc</a>, which is part of core<br class="">
> +# library, depends on clock_gettime() syscall, so we should set<br class="">
> +# -lrt when it is appropriate. See a comment for<br class="">
> +# HAVE_CLOCK_GETTIME_WITHOUT_RT in ${REPO}/CMakeLists.txt.<br class="">
> +if ("${HAVE_CLOCK_GETTIME}" AND NOT "${HAVE_CLOCK_GETTIME_WITHOUT_RT}")<br class="">
> + target_link_libraries(core rt)<br class="">
> +endif()<br class="">
> -- <br class="">
> 2.17.1<br class="">
> <br class=""></div></div></div></div></blockquote>
<br class="">
<br class="">-- <br class="">Alexander Tikhonov<br class=""></div>
</div></blockquote><br class=""></div><div><br class=""></div><div><div>--</div><div>Serge Petrenko</div><div><a href="mailto:sergepetrenko@tarantool.org" class="">sergepetrenko@tarantool.org</a></div><div class=""><br class=""></div></div><br class=""></div></body></html>