agraef / pd-lua

Lua bindings for Pd, updated for Lua 5.3+
https://agraef.github.io/pd-lua/
GNU General Public License v2.0
47 stars 10 forks source link

preparing for Lua 5.4 #7

Closed claudeha closed 4 years ago

claudeha commented 4 years ago

Lua 5.4 is in preparation and may be released soon.

It has some incompatibilities/changes: https://www.lua.org/work/doc/manual.html#8

claudeha commented 4 years ago

https://github.com/claudeha/pd-lua/commit/e92bb9c32f6a80d725b1d41c71b3283d3ccf5042 will crash if a pdlua compiled with Lua 5.3 is loaded with a Lua 5.4 dll/so because lua_version() will return a number instead of a pointer. I don't see an easy way to prevent this other than linking statically (probably best for Windows) or using versioned library names (as Debian does, for example).

agraef commented 4 years ago

Hmm, maybe we can add some load-time check to prevent this. I'll look into it. Probably there are some more 5.4 compatibility issues I guess?

dvzrv commented 4 years ago

I'm currently trying to rebuild pd-lua on Arch Linux against lua 5.4, running into the issue mentioned by @claudeha

cc -D_FORTIFY_SOURCE=2 -I"/usr/include/pd" -DPD -DVERSION='"0.7.3"' -fPIC -march=x86-64 -mtune=generic -O2 -pipe -fno-plt -I/usr/include/lua -O6 -funroll-loops -fomit-frame-pointer -o pdlua.o -c pdlua.c
pdlua.c: In function ‘pdlua_setup’:
pdlua.c:1655:39: error: incompatible types when initializing type ‘const lua_Number *’ {aka ‘const double *’} using type ‘lua_Number’ {aka ‘double’}
 1655 |     const lua_Number    *luaversion = lua_version (NULL);
      |                                       ^~~~~~~~~~~
make: *** [Makefile:300: pdlua.o] Error 1

I have to patch this to make it build against lua 5.3 (which we provide as lua53). It would be great to fix this for lua 5.4 though:

diff -ruN a/Makefile b/Makefile
--- a/Makefile  2018-03-07 16:42:09.000000000 +0100
+++ b/Makefile  2020-07-09 12:15:56.182015666 +0200
@@ -62,7 +62,7 @@
 LIBS =

 LUA_CFLAGS = -I/usr/include/lua
-LUA_LIBS   = -llua
+LUA_LIBS   = $(shell pkg-config --libs lua-5.3)

 # get library version from meta file
 LIBRARY_VERSION = $(shell sed -n 's|^\#X text [0-9][0-9]* [0-9][0-9]* VERSION \(.*\);|\1|p' $(LIBRARY_META))
diff -ruN a/pdlua.c b/pdlua.c
--- a/pdlua.c   2018-03-07 16:42:09.000000000 +0100
+++ b/pdlua.c   2020-07-09 12:10:25.631400140 +0200
@@ -42,9 +42,9 @@
 #include <unistd.h>
 #endif
 /* we use Lua */
-#include <lua.h>
-#include <lauxlib.h>
-#include <lualib.h>
+#include <lua5.3/lua.h>
+#include <lua5.3/lauxlib.h>
+#include <lua5.3/lualib.h>

 /* we use Pd */
 #include "m_pd.h"
claudeha commented 4 years ago

On 09/07/2020 11:17, David Runge wrote:

I have to patch this to make it build against lua 5.3 (which we provide as lua53).

This can be done without changing the Makefile, by overriding the variables on the make command line.  Something like:


make LUA_CFLAGS=-I/usr/include/lua5.3 LUA_LIBS=$(pkg-config --libs lua-5.3)
dvzrv commented 4 years ago

Ah thanks, valid point! Will check that out

agraef commented 4 years ago

Hi Claude and David! Sorry, I promised to have a look at this back in March, but then all hell broke loose... Fortunately, the semester holidays begin next week, so I'll finally have time to catch up on this.

agraef commented 4 years ago

Ok, looking at https://www.lua.org/manual/5.4/manual.html#8, it seems that lua_version is really the only snag there that might affect us, at least as far as pdlua.c and pd.lua are concerned. And we should be able to handle that with a conditional like #if LUA_VERSION_NUM >= 504, so chances are good that we can maintain backward compatibility with 5.3, even 5.2. Of course, there might still be some 5.3isms in the examples, so we'll have to check them all out.

I've installed lua54 from the AUR now, and will have a go myself.

agraef commented 4 years ago

Ok, that was easy enough, fixed in rev. 3c38858cab7316705fda91d5b8c999153bf3dafc. @claudeha, thanks for the heads up!

@dvzrv, while I was at it, in rev. 450f215a3e5af4e3827e174c052daf8a9a143527 I also fixed the strange install path that we inherited from upstream, so that by default installation goes to $prefix/lib/pd/extra now, as you'd expect. I also put out a new (source-only) release 0.10, so that you can update your Arch package in community and remove that lua-5.3.patch rigmarole. I hope that you're happy now, too. :)

I ran this compiled against Lua 5.4 in purr-data on Arch with a few of my own pd_lua examples and they all worked fine. I didn't check any of the included examples yet, though, so we might still have to reopen this issue (and put out a new release) if any Lua 5.4 incompatibility surfaces there.

agraef commented 4 years ago

Checked a few of the bundled examples with pd-lua 5.4 in vanilla now, these seemed to be fine, too. If anyone wants to thoroughly test all of those, please be my guest. ;-) No, seriously, if anyone discovers any hitches in the examples with Lua 5.4, just submit a separate bug report or add it here, and I'll happily fix them.

dvzrv commented 4 years ago

@agraef thanks, this works for me!

There's only one issue left with the LICENSE/README file for me, but I can hack that out of the Makefile and it's not relevant to this ticket. Thanks for fixing this!

agraef commented 4 years ago

@dvzrv Glad that I could help, and I'm looking forward to upgrade to your latest version in the next update. ;-)

dvzrv commented 4 years ago

That might still take a while. The lua 5.4 rebuild is still in progress. Some maintainers seem to have little time right now.

agraef commented 4 years ago

Ah I see, that's why the new lua 5.4 package is still in staging, I guess. No hurry. For the time being, I've updated pd-lua-git so that people can build a package against lua54 from AUR (using makepkg luaver=54), if they want it early.

agraef commented 4 years ago

There's only one issue left with the LICENSE/README file for me, but I can hack that out of the Makefile and it's not relevant to this ticket.

Yes, I've noticed that in your PKGBUILD. The thing is, if people build and install an external straight from the source, those files are expected to go to the external directory, that's customary in Pd land. So we won't change that in the Makefile, but as you already found a way to work around that, it isn't really an issue anyway.