<!DOCTYPE html>
<html data-lt-installed="true">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body style="padding-bottom: 1px;">
<p>Hi, Sergey,</p>
<p>LGTM, thanks!</p>
<p><br>
</p>
<p>I've also checked locally in Docker using Dockerfile below:</p>
<p><br>
</p>
<p># docker build --tag alpine_test .<br>
# docker run --network="host" -ti alpine_test bash<br>
<br>
FROM alpine:3.16<br>
<br>
RUN apk update && apk upgrade<br>
RUN apk add bash<br>
RUN apk add git<br>
RUN apk add vim<br>
RUN apk add gcc<br>
RUN apk add g++<br>
RUN apk add gdb<br>
RUN apk add make<br>
RUN apk add cmake<br>
RUN apk add linux-headers<br>
RUN apk add libc-dev<br>
<br>
RUN git clone <a class="moz-txt-link-freetext" href="https://github.com/tarantool/luajit">https://github.com/tarantool/luajit</a> && \<br>
cd luajit && \<br>
git checkout skaplun/gh-noticket-fix-alpine-build && \<br>
cmake -S . -B build -DLUAJIT_DISABLE_SYSPROF=ON && \<br>
cmake --build build --parallel && \<br>
cmake --build build --target test --parallel<br>
</p>
<div class="moz-cite-prefix">On 13.12.2024 17:45, Sergey Kaplun
wrote:<br>
</div>
<blockquote type="cite" cite="mid:Z1xIoN0NAZOBSSiV@root">
<pre class="moz-quote-pre" wrap="">Hi, Sergey!
Thanks for your comments!
Fixed them and force-pushed the branch.
On 13.12.24, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Hello again, Sergey,
thanks for the patch!
LGTM with minor comments, see below
On 13.12.2024 15:59, Sergey Kaplun wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Since Alpine uses musl [1] as its C standard library, the build for it
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">s/musl/Musl/
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Replaced typo: s/GLibC/glibc/.</pre>
</blockquote>
Thanks!<br>
<blockquote type="cite" cite="mid:Z1xIoN0NAZOBSSiV@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">distro is "alpine".
[1]:<a class="moz-txt-link-freetext" href="https://wiki.alpinelinux.org/wiki/Musl">https://wiki.alpinelinux.org/wiki/Musl</a>
[2]:<a class="moz-txt-link-freetext" href="https://www.linux.org/docs/man5/os-release.html">https://www.linux.org/docs/man5/os-release.html</a>
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
There is also a documentation about os-release on Freedesktop [1].
I would add this link as well. Feel free to ignore.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Added, thanks!
</pre>
</blockquote>
Thanks!<br>
<blockquote type="cite" cite="mid:Z1xIoN0NAZOBSSiV@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
[1]: <a class="moz-txt-link-freetext" href="https://www.freedesktop.org/software/systemd/man/latest/os-release.html">https://www.freedesktop.org/software/systemd/man/latest/os-release.html</a>
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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]: <a class="moz-txt-link-freetext" href="https://wiki.alpinelinux.org/wiki/Musl">https://wiki.alpinelinux.org/wiki/Musl</a>
| [2]: <a class="moz-txt-link-freetext" href="https://www.linux.org/docs/man5/os-release.html">https://www.linux.org/docs/man5/os-release.html</a>
| [3]: <a class="moz-txt-link-freetext" href="https://www.freedesktop.org/software/systemd/man/latest/os-release.html">https://www.freedesktop.org/software/systemd/man/latest/os-release.html</a>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">---
Branch:<a class="moz-txt-link-freetext" href="https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-fix-alpine-build">https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-fix-alpine-build</a>
PR for tests on master:<a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/pull/10824">https://github.com/tarantool/tarantool/pull/10824</a>
Side note: build locally in Docker for test for Alpine.
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
+ GetLinuxDistro(LINUX_DISTRO)
+ # Alpine uses musl instead of GLibC.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
s/musl/Musl/
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I've used as a reference (don't be confused by the links
themselfs) [1][2], looks like it should be "musl", after all.
</pre>
</blockquote>
okay<br>
<blockquote type="cite" cite="mid:Z1xIoN0NAZOBSSiV@root">
<pre class="moz-quote-pre" wrap="">
[1]: <a class="moz-txt-link-freetext" href="https://wiki.alpinelinux.org/wiki/Musl">https://wiki.alpinelinux.org/wiki/Musl</a>
[2]: <a class="moz-txt-link-freetext" href="https://en.wikipedia.org/wiki/Musl">https://en.wikipedia.org/wiki/Musl</a>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
s/GLibC/glibc/ or s/GLibC/GNU C Library/, see [2].
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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
===================================================================
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
[2]: <a class="moz-txt-link-freetext" href="https://www.gnu.org/software/libc/">https://www.gnu.org/software/libc/</a>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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]:<a class="moz-txt-link-freetext" href="https://sourceware.org/bugzilla/show_bug.cgi?id=16151">https://sourceware.org/bugzilla/show_bug.cgi?id=16151</a>
+ 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)
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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)
===================================================================
</pre>
</blockquote>
Thanks!<br>
<blockquote type="cite" cite="mid:Z1xIoN0NAZOBSSiV@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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()
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>