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
Thanks!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 its/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 theReplaced typo: s/GLibC/glibc/.
Thanks!distro is "alpine". [1]:https://wiki.alpinelinux.org/wiki/Musl [2]:https://www.linux.org/docs/man5/os-release.htmlThere is also a documentation about os-release on Freedesktop [1]. I would add this link as well. Feel free to ignore.Added, thanks!
okay[1]: https://www.freedesktop.org/software/systemd/man/latest/os-release.htmlThe 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.
Thanks![1]: https://wiki.alpinelinux.org/wiki/Musl [2]: https://en.wikipedia.org/wiki/Musls/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) ===================================================================
+ 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()