Re: [Tarantool-patches] [PATCH luajit] cmake: fix build for Alpine

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 4164 bytes --]

Hello again, Sergey,

thanks for the patch!

LGTM with minor comments, see below

On 13.12.2024 15:59, Sergey Kaplun wrote:
> Since Alpine uses musl [1] as its C standard library, the build for it
s/musl/Musl/
> failed after the commit af0f59da76292f30ff75be1ab01458d47b226995 ("test:
> fix LuaJIT-tests for old libc version"), since `GetLibCVersion()` raises
> an error. This patch adds the check of the ID in the /etc/os-release [2]
> of the Linux distribution and avoids setting the GLibC version if the
> distro is "alpine".
>
> [1]:https://wiki.alpinelinux.org/wiki/Musl
> [2]:https://www.linux.org/docs/man5/os-release.html

There is also a documentation about os-release on Freedesktop [1].

I would add this link as well. Feel free to ignore.


[1]: https://www.freedesktop.org/software/systemd/man/latest/os-release.html

> ---
>
> Branch:https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-fix-alpine-build
> PR for tests on master:https://github.com/tarantool/tarantool/pull/10824
> Side note: build locally in Docker for test for Alpine.
>
>   test/CMakeLists.txt              |  1 +
>   test/LuaJIT-tests/CMakeLists.txt | 18 +++++++++++-------
>   test/cmake/GetLinuxDistro.cmake  | 18 ++++++++++++++++++
>   3 files changed, 30 insertions(+), 7 deletions(-)
>   create mode 100644 test/cmake/GetLinuxDistro.cmake
>
> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> index be02d2e4..86de9ed1 100644
> --- a/test/CMakeLists.txt
> +++ b/test/CMakeLists.txt
> @@ -74,6 +74,7 @@ separate_arguments(LUAJIT_TEST_COMMAND)
>   set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
>   include(AddTestLib)
>   include(GetLibCVersion)
> +include(GetLinuxDistro)
>   include(LibRealPath)
>   
>   # CTEST_FLAGS is used by CMake targets in test suites.
> diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt
> index 0df3f4ea..adc630f0 100644
> --- a/test/LuaJIT-tests/CMakeLists.txt
> +++ b/test/LuaJIT-tests/CMakeLists.txt
> @@ -62,13 +62,17 @@ if(CMAKE_C_FLAGS MATCHES "-march=skylake-avx512")
>     list(APPEND LUAJIT_TEST_TAGS_EXTRA +avx512)
>   endif()
>   
> -if(NOT CMAKE_SYSTEM_NAME STREQUAL "Darwin")
> -  GetLibCVersion(LIBC_VERSION)
> -  # XXX: <tonumber_scan.lua> uses `strtod()`, old versions of
> -  # which have the bug [1] for "0x1p-2075" parsing. Add the skip
> -  # check for it.
> -  # [1]:https://sourceware.org/bugzilla/show_bug.cgi?id=16151
> -  list(APPEND LUAJIT_TEST_TAGS_EXTRA +libc=${LIBC_VERSION})
> +if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
> +  GetLinuxDistro(LINUX_DISTRO)
> +  # Alpine uses musl instead of GLibC.

s/musl/Musl/

s/GLibC/glibc/ or s/GLibC/GNU C Library/, see [2].

[2]: https://www.gnu.org/software/libc/

> +  if(NOT LINUX_DISTRO MATCHES "alpine")
> +    GetLibCVersion(LIBC_VERSION)
> +    # XXX: <tonumber_scan.lua> uses `strtod()`, old versions of
> +    # which have the bug [1] for "0x1p-2075" parsing. Add the skip
> +    # check for it.
> +    # [1]:https://sourceware.org/bugzilla/show_bug.cgi?id=16151
> +    list(APPEND LUAJIT_TEST_TAGS_EXTRA +libc=${LIBC_VERSION})
> +  endif()
>   endif()
>   
>   if(LUAJIT_ENABLE_TABLE_BUMP)
> diff --git a/test/cmake/GetLinuxDistro.cmake b/test/cmake/GetLinuxDistro.cmake
> new file mode 100644
> index 00000000..027368be
> --- /dev/null
> +++ b/test/cmake/GetLinuxDistro.cmake
> @@ -0,0 +1,18 @@
> +# Determine the Linux distro id and return it in the `output`
> +# variable. Seehttps://www.linux.org/docs/man5/os-release.html
> +# for details.
> +macro(GetLinuxDistro output)
> +  if(NOT CMAKE_SYSTEM_NAME STREQUAL "Linux")
> +    message(FATAL_ERROR "GetLinuxDistro macro must be used only on Linux")
> +  endif()
> +  file(READ /etc/os-release OS_RELEASE)

Fallback to /usr/lib/os-release as suggested in doc:

 > Applications should check for the former, and exclusively use its 
data if it exists, and only fall back to /usr/lib/os-release if it is 
missing.

> +  string(REGEX MATCH "ID=([0-9a-z._-]+)" MATCH ${OS_RELEASE})
> +  if(MATCH)
> +    set(${output} ${CMAKE_MATCH_1})
> +  else()
> +    set(${output} linux)
> +  endif()
> +  unset(OS_RELEASE)
> +  unset(MATCH)
> +  unset(CMAKE_MATCH_1)
> +endmacro()

[-- Attachment #2: Type: text/html, Size: 6152 bytes --]