luvit / luv

Bare libuv bindings for lua
Apache License 2.0
831 stars 187 forks source link

fix: Use os_uname() to check for Linux #686

Closed cryptomilk closed 10 months ago

cryptomilk commented 10 months ago

In build environments like Fedora's mockbuild, there is no OSTYPE environment variable set. Use a more secure method to detect Linux. Details:

man os-release https://www.man7.org/linux/man-pages/man5/os-release.5.html

zhaozg commented 10 months ago

I think should use https://github.com/luvit/luv/blob/master/docs.md#uvos_uname

cryptomilk commented 10 months ago

I've changed it.

cryptomilk commented 10 months ago

The minimum supported libuv doesn't provide os_uname(). What now, fall back to /etc/os-release?

Bilal2453 commented 10 months ago

I don't think simply checking if /etc/os-release exists is as reliable either! As far as I know, it is a systemd thing that may not exists on non-systemd distribution. The most reliable way I know of for detecting if the OS is Linux based (works on Android environments as well) is by checking uname --kernel-name (uname -s), this will require an io.popen most likely, roughly something like this:

local popen_handle = io.popen('uname -s')
local uname_os = assert(popen_handle:read('*a'))
popen_handle:close()
isLinux = uname_os:lower() == 'linux'

I would also like to point out that in the current code it seems to me like isLinux is set to nil instead of false if the OS is not Windows but also isn't Linux (for example, when the OS is openbsd and jit isn't available).

cryptomilk commented 10 months ago

I've updated it to use uname -s.

/etc/os-release is a freedesktop standard, it also has been added to Python 3.10, see https://docs.python.org/3/library/platform.html#platform.freedesktop_os_release

cryptomilk commented 10 months ago

Looks like popen() is not alway available?

squeek502 commented 10 months ago

Looks like popen is dependent on LUA_USE_POSIX being defined, which we currently don't do. Adding

IF(NOT WIN32)
  ADD_DEFINITIONS(-DLUA_USE_POSIX)
ENDIF(NOT WIN32)

to lua.cmake is probably a good idea in general and should fix the error.

Bilal2453 commented 10 months ago

@squeek502 I think the CI tests need to be manually re-triggered?

cryptomilk commented 10 months ago

I've tested it with fedora mockbuild and the patch works fine.