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: 6532 bytes --]

Hi, Sergey,

LGTM, thanks!


I've also checked locally in Docker using Dockerfile below:


# docker build --tag alpine_test .
# docker run --network="host"  -ti alpine_test bash

FROM alpine:3.16

RUN apk update && apk upgrade
RUN apk add bash
RUN apk add git
RUN apk add vim
RUN apk add gcc
RUN apk add g++
RUN apk add gdb
RUN apk add make
RUN apk add cmake
RUN apk add linux-headers
RUN apk add libc-dev

RUN git clone https://github.com/tarantool/luajit && \
     cd luajit && \
     git checkout skaplun/gh-noticket-fix-alpine-build && \
     cmake -S . -B build -DLUAJIT_DISABLE_SYSPROF=ON && \
     cmake --build build --parallel && \
     cmake --build build --target test --parallel

On 13.12.2024 17:45, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for your comments!
> Fixed them and force-pushed the branch.
>
> On 13.12.24, Sergey Bronnikov wrote:
>> 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
> Replaced typo: s/GLibC/glibc/.
Thanks!
>
>>> 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.
> Added, thanks!
Thanks!
>>
>> [1]:https://www.freedesktop.org/software/systemd/man/latest/os-release.html
> The new commit message is the following:
>
> | cmake: fix build for Alpine
> |
> | Since Alpine uses musl [1] as its C standard library, the build for it
> | 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][3] 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
> | [3]: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.
>>>
> <snipped>
>
>>> +if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
>>> +  GetLinuxDistro(LINUX_DISTRO)
>>> +  # Alpine uses musl instead of GLibC.
>> s/musl/Musl/
> I've used as a reference (don't be confused by the links
> themselfs) [1][2], looks like it should be "musl", after all.
okay
> [1]:https://wiki.alpinelinux.org/wiki/Musl
> [2]:https://en.wikipedia.org/wiki/Musl
>
>> s/GLibC/glibc/ or s/GLibC/GNU C Library/, see [2].
> Replaced with glibc.
>
> ===================================================================
> diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt
> index adc630f0..c1dbde35 100644
> --- a/test/LuaJIT-tests/CMakeLists.txt
> +++ b/test/LuaJIT-tests/CMakeLists.txt
> @@ -64,7 +64,7 @@ endif()
>   
>   if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
>     GetLinuxDistro(LINUX_DISTRO)
> -  # Alpine uses musl instead of GLibC.
> +  # Alpine uses musl instead of glibc.
>     if(NOT LINUX_DISTRO MATCHES "alpine")
>       GetLibCVersion(LIBC_VERSION)
>       # XXX: <tonumber_scan.lua> uses `strtod()`, old versions of
> ===================================================================
>
>> [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.
> Added the fallback:
> ===================================================================
> diff --git a/test/cmake/GetLinuxDistro.cmake b/test/cmake/GetLinuxDistro.cmake
> index 027368be..2636019c 100644
> --- a/test/cmake/GetLinuxDistro.cmake
> +++ b/test/cmake/GetLinuxDistro.cmake
> @@ -5,13 +5,18 @@ 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)
> +  set(OS_RELEASE_FILE /etc/os-release)
> +  if(NOT EXISTS ${OS_RELEASE_FILE})
> +    set(OS_RELEASE_FILE /usr/lib/os-release)
> +  endif()
> +  file(READ ${OS_RELEASE_FILE} OS_RELEASE)
>     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_FILE)
>     unset(OS_RELEASE)
>     unset(MATCH)
>     unset(CMAKE_MATCH_1)
> ===================================================================
>
Thanks!
>>> +  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: 10669 bytes --]