Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Jan 17, 2024
Merged

Conversation

cryptomilk
Copy link
Contributor

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
Copy link
Member

zhaozg commented Jan 14, 2024

@cryptomilk cryptomilk changed the title fix: Use '/etc/os-release' to check for Linux fix: Use os_uname() to check for Linux Jan 14, 2024
@cryptomilk
Copy link
Contributor Author

I've changed it.

@cryptomilk
Copy link
Contributor Author

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

@Bilal2453
Copy link
Contributor

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
Copy link
Contributor Author

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

lib/utils.lua Outdated Show resolved Hide resolved
@cryptomilk
Copy link
Contributor Author

Looks like popen() is not alway available?

@squeek502
Copy link
Member

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.

In build environments like Fedora's mockbuild, there is no OSTYPE
environment variable set. Use a more secure method to detect Linux.
@Bilal2453
Copy link
Contributor

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

Copy link
Member

@squeek502 squeek502 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is a nice improvement to make the tests less dependent on the CI environment!

@squeek502 squeek502 merged commit 08b9790 into luvit:master Jan 17, 2024
13 of 14 checks passed
@cryptomilk
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants