Lua 5.2+ compatbility #9280

mcepl · 2018-11-27T15:33:14Z

This is compatibility layer, which makes the code run both on Lua 5.1 (which is the default for Neovim, and it is what LuaJIT provides) and Lua 5.2+.

As mentioned in #7879 (comment)

jamessan · 2018-11-27T15:51:54Z

Should we have compat modules to contain these types of things? For example, test/compat.lua exports unpack, appropriately defined and other modules use compat.unpack.

Do we need a lint check to avoid adding new uses of unpack which aren't compatible?

bfredl · 2018-11-27T15:53:42Z

What about lua plugin code that is written for nvim+lua5.1 (the officially supported build)? What should we tell the user when such plugins break on this separate nvim+5.2 config? What if someone develops a plugin on nvim+5.2, not knowing the build has been altered, and it break for someone using the official lua5.1/luajit config?

An option is just to make it clearer that distros are expected to use lua 5.1 or luajit. We could could make the buildscripts fail if lua 5.2/5.3 is used. Distros can statically link the correct version with nvim, nvim can't hardly be the only software that expects a given lua 5.x version, given all the incompatibilities.

bfredl · 2018-11-27T15:54:58Z

Though to be less strict, we can still allow external build/generator/test code itself to be run by the distros lua. The issue is with the expectations for people writing runtime plugins.

jamessan · 2018-11-27T15:56:40Z

Lint does fail currently:

Checking /home/travis/build/neovim/neovim/test/functional/api/server_requests_spec.lua 1 warning
    /home/travis/build/neovim/neovim/test/functional/api/server_requests_spec.lua:5:16: accessing undefined field unpack of global table

mcepl · 2018-11-27T16:00:25Z

An option is just to make it clearer that distros are expected to use lua 5.1 or luajit.

I think it is a bad idea. It would make Neovim hard to build for distributions like Fedora which has Lua 5.3 as the default Lua (LuaJIT is available), and not all packages for lua 5.1 are available (and I am neither Fedora user nor maintainer any more).

Look at https://build.opensuse.org/package/show/home:mcepl:neovim/neovim

bfredl · 2018-11-27T16:08:27Z

not all packages for lua 5.1 are available

Again, we could support 5.2/5.3 for the build process, which has the all the dependencies for extra binary lua packages (lpeg etc). But for runtime support only lua-5.1 itself is needed, nothing extra. It would be nice if a plugin developer need not test their code on multiple different configurations.

jamessan · 2018-11-27T16:17:01Z

The unfortunate part of the Lua ecosystem is that the different Lua versions aren't compatible. This forces distributors to choose a Lua version when they need to compile a program.

I'm not sure about Fedora, but I know that Debian's Lua packaging makes it trivial to build modules for multiple Lua versions. However, that doesn't apply to binaries (like vim and nvim). Those need to choose a specific version of Lua, which will then restrict what modules can be used if module upstreams don't target all Lua versions.

This problem already exists with Vim since Vim explicitly supports being built against any of the current Lua versions. I think the number of Lua plugins written for Vim is tiny, though, whereas nvim is specifically treating Lua as a first class citizen. The expectation is that there will be a lot more Lua written for nvim.

If we allow nvim to be built against any Lua version, then plugin authors need to account for that. However, if we don't allow that, then we potentially make it harder for people to use modules they want.

hkupty · 2018-11-27T20:42:47Z

As a plugin developer, one of the things I expect is solid foundations to rely on, such as always being 5.1 on runtime. Otherwise I'd need to have multiple different abstraction layers, which is absolutely suboptimal.

As a fedora user, I solved the issue of having no default lua-5.1 by using supported out-of-the-tree packages to provide me lua-5.1 and luarocks-5.1.

That's a small hurdle, but neovim itself was already packaged correctly, so that was only required for me to write and test plugins.

mcepl · 2018-11-28T10:49:48Z

As a fedora user, I solved the issue of having no default lua-5.1 by using supported out-of-the-tree packages to provide me lua-5.1 and luarocks-5.1.

Except you really cannot have out-of-the-tree packages for the official neovim, right?

bfredl · 2018-11-28T10:52:22Z

Except you really cannot have out-of-the-tree packages for the official neovim, right?

Why not? nvim with lua 5.1 can load any external .so lua lib that is compiled for lua 5.1.

hkupty · 2018-11-28T11:22:02Z

@mcepl

That's a small hurdle, but neovim itself was already packaged correctly.

I didn't have to install lua-5.1 to have neovim. It was already distributed with Lua correctly embedded (IIRC luajit).

Don't mix my requirement as a plugin developer with neovim install/usage experience.

justinmk · 2018-11-29T01:13:38Z

as @jamessan mentioned there is a lint issue. #9280 (comment)
Then we can merge this.

Should we have compat modules to contain these types of things? For example, test/compat.lua exports unpack, appropriately defined and other modules use compat.unpack.

For simple cases that seems reasonable. And maybe for our use case we won't run into many 5.2/5.3-incompatible situations. For example there's no need for Nvim-Lua code to use setfenv nor os.execute. (So maybe it's reasonable for us to have a goal of Lua 5.2+ forward-compatibility at build-time.)

There is also lua-compat-5.2 or lua-compat-5.3 but those seem risky and may invite more problems than solutions.

mcepl · 2018-11-29T08:31:45Z

Lint does fail currently:

Checking /home/travis/build/neovim/neovim/test/functional/api/server_requests_spec.lua 1 warning
    /home/travis/build/neovim/neovim/test/functional/api/server_requests_spec.lua:5:16: accessing undefined field unpack of global table

I don't understand this. It is exactly the main construct we use, right?

local unpack = table.unpack or unpack

I can understand that it may be problematic for a lint because it always expects to fail on at least one branch of the or expression. However, why lint complains only here not with every other use of this construct?

bfredl · 2018-11-29T09:48:21Z

@mcepl Maybe the linter simply avoids repeating complaints of the same undefined name table.unpack ?

mcepl · 2018-11-29T10:41:39Z

@mcepl Maybe the linter simply avoids repeating complaints of the same undefined name table.unpack ?

What I meant is that linters are not fountains of all wisdom, and in this case I don't see anything particularly wrong with what I did.

bfredl · 2018-11-29T10:46:45Z

I just answered the direct question "why lint complains only here not with every other use of this construct?". I didn't say anything about wisdom, it is just a simple rule to avoid excessive output (but e g gcc will say "further references omitted", so it is clearer). Somewhere it is configured what global names are allowed (nvim already adds some), one would need to add table.unpack there.

bfredl · 2018-11-29T10:51:05Z

The only whitelisted global seems to be vim on line 691 in CMakeLists.txt. Maybe one could turn it to a proper cmake list, or separate file.

mcepl · 2018-11-29T10:58:10Z

@bfredl that wasn't against you. I was reacting more to @justinmk who seemed to indicate (in #9280 (comment)), that this cannot be merged unless the lint issues are resolved.

bfredl · 2018-11-29T11:07:37Z

@justinmk who seemed to indicate (in #9280 (comment)), that this cannot be merged unless the lint issues are resolved.

Well, linter issues have to be resolved, in the sense that the build is not red. If the linter is wrong, the proper resolution is to reconfigure the linter, I don't think anyone is against that.

justinmk · 2018-11-29T22:47:04Z

@mcepl I see that man.lua was modified as well, but it would run using Nvim's builtin Lua, not system Lua--that is, runtime, not build-time. Is Nvim being linked to Lua 5.2? That's a bit surprising since I thought the 5.1/5.2 C APIs were incompatible.

justinmk · 2018-11-29T23:16:50Z

runtime/lua/man.lua

@@ -1,3 +1,5 @@
+require('vim.compat')


Not sure if we want to do this implicitly instead. But let's discuss that later.

justinmk · 2018-11-29T23:19:07Z

cmake/RunTests.cmake

@@ -38,7 +38,10 @@ set(ENV{SYSTEM_NAME} ${SYSTEM_NAME})
 execute_process(
  COMMAND ${BUSTED_PRG} ${TEST_TAG} ${TEST_FILTER} -v -o ${BUSTED_OUTPUT_TYPE}
    --lua=${LUA_PRG} --lazy --helper=${TEST_DIR}/${TEST_TYPE}/preload.lua
-    --lpath=${BUILD_DIR}/?.lua --lpath=?.lua ${TEST_PATH}
+    --lpath=${BUILD_DIR}/?.lua
+    --lpath=${WORKING_DIR}/runtime/lua/?.lua


This is a try at sharing code between tests/runtime/core. Maybe we will do it differently, I will have a followup PR for further discussion, after this one.

Related: #8677 cc @KillTheMule

I don't understand makefiles, so just a thought: Is ? recursive, so it also catches runtime/lua/plugin/init.lua? Also, does the test runner have access to vim.api? If so, great boon to writing tests!

yes it's recursive.

Also, does the test runner have access to vim.api? If so, great boon to writing tests!

vim.api is built-into Nvim so that would not be "shared". The test utilities could spawn nvim to use its features. We probably should do that, to reduce some of the code in */helpers.lua.

justinmk · 2018-11-29T23:23:15Z

@mcepl I pushed to your branch. Could you confirm if this works? Then will merge.

mcepl · 2018-11-30T02:00:40Z

@mcepl Is Nvim being linked to Lua 5.2? That's a bit surprising since I thought the 5.1/5.2 C APIs were incompatible.

Fedora Rawhide package shows this:

matej@stitny: tmp$ rpm -qRp neovim*.rpm
/bin/sh
/bin/sh
/usr/bin/sh
config(neovim) = 0.3.2~git.1543373357.0d1e5ec1b-3.2
gperf
libc.so.6()(64bit)
libc.so.6(GLIBC_2.14)(64bit)
libc.so.6(GLIBC_2.2.5)(64bit)
libc.so.6(GLIBC_2.3)(64bit)
libc.so.6(GLIBC_2.4)(64bit)
libdl.so.2()(64bit)
libjemalloc.so.2()(64bit)
liblua-5.3.so()(64bit)
libm.so.6()(64bit)
libm.so.6(GLIBC_2.2.5)(64bit)
libm.so.6(GLIBC_2.29)(64bit)
libmsgpackc.so.2()(64bit)
libnsl.so.2()(64bit)
libpthread.so.0()(64bit)
libpthread.so.0(GLIBC_2.2.5)(64bit)
librt.so.1()(64bit)
libtermkey.so.1()(64bit)
libunibilium.so.4()(64bit)
libutil.so.1()(64bit)
libutil.so.1(GLIBC_2.2.5)(64bit)
libuv.so.1()(64bit)
libvterm.so.0()(64bit)
python3-nvim
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PartialHardlinkSets) <= 4.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1
rpmlib(TildeInVersions) <= 4.10.0-1
rtld(GNU_HASH)
xsel
matej@stitny: tmp$

And when I pull out a /usr/bin/nvim from that package I get:

matej@stitny: tmp$ ldd nvim
./nvim: /lib64/libm.so.6: version `GLIBC_2.29' not found (required by ./nvim)
	linux-vdso.so.1 (0x00007ffc88315000)
	libuv.so.1 => /usr/lib64/libuv.so.1 (0x00007f529d975000)
	librt.so.1 => /lib64/librt.so.1 (0x00007f529d5a3000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f529d384000)
	libdl.so.2 => /lib64/libdl.so.2 (0x00007f529d180000)
	libnsl.so.2 => /usr/lib64/libnsl.so.2 (0x00007f529cf67000)
	libmsgpackc.so.2 => /usr/lib64/libmsgpackc.so.2 (0x00007f529d96b000)
	libvterm.so.0 => /usr/lib64/libvterm.so.0 (0x00007f529d955000)
	libtermkey.so.1 => /usr/lib64/libtermkey.so.1 (0x00007f529d949000)
	libunibilium.so.4 => /usr/lib64/libunibilium.so.4 (0x00007f529d934000)
	libm.so.6 => /lib64/libm.so.6 (0x00007f529cbd4000)
	libutil.so.1 => /lib64/libutil.so.1 (0x00007f529c9d1000)
	liblua-5.3.so => not found
	libjemalloc.so.2 => /usr/lib64/libjemalloc.so.2 (0x00007f529c712000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f529c352000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f529d7ab000)
	libtirpc.so.3 => /lib64/libtirpc.so.3 (0x00007f529c120000)
	libtinfo.so.6 => /lib64/libtinfo.so.6 (0x00007f529d901000)
	libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007f529d8e7000)
	libgssapi_krb5.so.2 => /usr/lib64/libgssapi_krb5.so.2 (0x00007f529d897000)
	libkrb5.so.3 => /usr/lib64/libkrb5.so.3 (0x00007f529c041000)
	libk5crypto.so.3 => /usr/lib64/libk5crypto.so.3 (0x00007f529d863000)
	libcom_err.so.2 => /lib64/libcom_err.so.2 (0x00007f529d85d000)
	libkrb5support.so.0 => /usr/lib64/libkrb5support.so.0 (0x00007f529d84d000)
	libkeyutils.so.1 => /usr/lib64/libkeyutils.so.1 (0x00007f529d847000)
	libresolv.so.2 => /lib64/libresolv.so.2 (0x00007f529be2a000)
	libselinux.so.1 => /lib64/libselinux.so.1 (0x00007f529bc01000)
	libpcre.so.1 => /usr/lib64/libpcre.so.1 (0x00007f529bb71000)
matej@stitny: tmp$

I guess, the answer is yes. Note however, that I have actually never run binary out of this package myself, so I would have to find some Fedora machine to test it (and aside from openSUSE machines, I have only some CentOS7 ones here).

jamessan · 2018-11-30T02:39:42Z

Is Nvim being linked to Lua 5.2? That's a bit surprising since I thought the 5.1/5.2 C APIs were incompatible.

The different Lua versions aren't explicitly API incompatible. There simply are no guarantees about compatibility, so depending on the APIs being used it is possible for one code base to work in all the Lua versions.

Then there's the matter that different Lua versions definitely are not ABI compatible, so it's not possible to use compiled C modules that were built against different Lua versions.

mcepl · 2018-11-30T08:03:50Z

Then there's the matter that different Lua versions definitely are not ABI compatible, so it's not possible to use compiled C modules that were built against different Lua versions.

Well, in the world of Linux distributions, you are expected to have rebulit all binary modules from its sources, so it shouldn't be a problem.

mcepl · 2018-11-30T14:33:44Z

@mcepl I pushed to your branch. Could you confirm if this works? Then will merge.

It works, but I have a comment.

runtime/lua/vim/compat.lua

justinmk · 2018-11-30T21:26:03Z

bah, QuickBuild is failing, ~~I think because ${WORKING_DIR} is not the right thing to use for --lpath in cmake/RunTests.cmake. Not sure what to use instead.~~

15:54:18,440 WARN  - busted: error: Cannot load helper script: test/functional/preload.lua
15:54:18,440 WARN  - ./test/functional/helpers.lua:1: module 'vim.compat' not found:
15:54:18,440 WARN  - 	no field package.preload['vim.compat']
15:54:18,440 WARN  - 	no file './build/Release/vim/compat.lua'
...
15:54:18,442 INFO  - not ok 1 - test/functional/preload.lua

Update: QuickBuild doesn't use cmake/RunTests.cmake. John added the necessary --lpath to the QuickBuild script.

Make the code run both on Lua 5.1 (which is the default for Neovim, and is what LuaJIT provides) and Lua 5.2+.

ref #9280 Introduce the `vim.compat` module, to help environments with system Lua 5.2+ run the build/tests. Include the module implicitly in all tests. ref #8677 legacy `vim` module: beep buffer command dict eval firstline lastline line list open type window

close #9515 ref #9280

mcepl mentioned this pull request Nov 28, 2018

Function unpack moved in Lua 5.2 and higher to be a method of table. tjdevries/neovim#2

Open

justinmk reviewed Nov 29, 2018

View reviewed changes

justinmk changed the title ~~Function unpack moved in Lua 5.2 and higher to be a method of table.~~ Lua 5.2 compat Nov 29, 2018

justinmk changed the title ~~Lua 5.2 compat~~ Lua 5.2 compatbility Nov 29, 2018

justinmk reviewed Nov 30, 2018

View reviewed changes

runtime/lua/vim/compat.lua Show resolved Hide resolved

justinmk changed the title ~~Lua 5.2 compatbility~~ Lua 5.2+ compatbility Nov 30, 2018

justinmk mentioned this pull request Dec 1, 2018

runtime/lua #9301

Merged

mcepl and others added 3 commits December 20, 2018 11:57

test: Lua 5.2/5.3 compat #9280

e6d0dea

Make the code run both on Lua 5.1 (which is the default for Neovim, and is what LuaJIT provides) and Lua 5.2+.

runtime/lua/vim/compat.lua

221f6ff

ref #9280 Introduce the `vim.compat` module, to help environments with system Lua 5.2+ run the build/tests. Include the module implicitly in all tests. ref #8677 legacy `vim` module: beep buffer command dict eval firstline lastline line list open type window

lint

127b856

justinmk added the lua stdlib label Dec 20, 2018

justinmk added this to the 0.3.2 milestone Dec 20, 2018

justinmk merged commit 87b40f7 into neovim:master Dec 20, 2018

mcepl deleted the lua_53_compatibility branch December 20, 2018 17:37

justinmk pushed a commit that referenced this pull request Jan 17, 2019

test: Lua 5.2/5.3 compat

a8a38f3

close #9515 ref #9280

mcepl mentioned this pull request Oct 28, 2019

luajit/lua 5.1 compatibility? martanne/vis#291

Open

justinmk mentioned this pull request Sep 1, 2020

Patch FindLua to support locating Lua 5.4 #12820

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lua 5.2+ compatbility #9280

Lua 5.2+ compatbility #9280

mcepl commented Nov 27, 2018

jamessan commented Nov 27, 2018

bfredl commented Nov 27, 2018

bfredl commented Nov 27, 2018

jamessan commented Nov 27, 2018

mcepl commented Nov 27, 2018 •

edited

bfredl commented Nov 27, 2018

jamessan commented Nov 27, 2018

hkupty commented Nov 27, 2018

mcepl commented Nov 28, 2018

bfredl commented Nov 28, 2018

hkupty commented Nov 28, 2018

justinmk commented Nov 29, 2018 •

edited

mcepl commented Nov 29, 2018

bfredl commented Nov 29, 2018

mcepl commented Nov 29, 2018

bfredl commented Nov 29, 2018

bfredl commented Nov 29, 2018

mcepl commented Nov 29, 2018

bfredl commented Nov 29, 2018

justinmk commented Nov 29, 2018

justinmk Nov 29, 2018

justinmk Nov 29, 2018

KillTheMule Nov 30, 2018

justinmk Nov 30, 2018

justinmk commented Nov 29, 2018 •

edited

mcepl commented Nov 30, 2018

jamessan commented Nov 30, 2018

mcepl commented Nov 30, 2018

mcepl commented Nov 30, 2018

justinmk commented Nov 30, 2018 •

edited

Lua 5.2+ compatbility #9280

Lua 5.2+ compatbility #9280

Conversation

mcepl commented Nov 27, 2018

jamessan commented Nov 27, 2018

bfredl commented Nov 27, 2018

bfredl commented Nov 27, 2018

jamessan commented Nov 27, 2018

mcepl commented Nov 27, 2018 • edited

bfredl commented Nov 27, 2018

jamessan commented Nov 27, 2018

hkupty commented Nov 27, 2018

mcepl commented Nov 28, 2018

bfredl commented Nov 28, 2018

hkupty commented Nov 28, 2018

justinmk commented Nov 29, 2018 • edited

mcepl commented Nov 29, 2018

bfredl commented Nov 29, 2018

mcepl commented Nov 29, 2018

bfredl commented Nov 29, 2018

bfredl commented Nov 29, 2018

mcepl commented Nov 29, 2018

bfredl commented Nov 29, 2018

justinmk commented Nov 29, 2018

justinmk Nov 29, 2018

Choose a reason for hiding this comment

justinmk Nov 29, 2018

Choose a reason for hiding this comment

KillTheMule Nov 30, 2018

Choose a reason for hiding this comment

justinmk Nov 30, 2018

Choose a reason for hiding this comment

justinmk commented Nov 29, 2018 • edited

mcepl commented Nov 30, 2018

jamessan commented Nov 30, 2018

mcepl commented Nov 30, 2018

mcepl commented Nov 30, 2018

justinmk commented Nov 30, 2018 • edited

mcepl commented Nov 27, 2018 •

edited

justinmk commented Nov 29, 2018 •

edited

justinmk commented Nov 29, 2018 •

edited

justinmk commented Nov 30, 2018 •

edited