Support lua51.

[orbea]

I was trying to work on building RetroArch with a system version of lua as requested in issue libretro/RetroArch#8153.

However this is made impossible by two things.

1. rcheevos only builds with lua52 or lua53.
2. Upstream lua doesn't provide their own shared library or pkgconfig files and this makes checking for the versioned lua libraries or pkgconfig files not portable since every distro reinvents the wheel to accomplish this...

If rcheevos built and worked with any lua version this would be a lot easier to support.

The current build problem is.

deps/rcheevos/src/rcheevos/operand.c: In function ‘rc_evaluate_operand’:
deps/rcheevos/src/rcheevos/operand.c:348:38: error: ‘LUA_OK’ undeclared (first use in this function); did you mean ‘LUA_QL’?
         if (lua_pcall(L, 2, 1, 0) == LUA_OK) {
                                      ^~~~~~
                                      LUA_QL
deps/rcheevos/src/rcheevos/operand.c:348:38: note: each undeclared identifier is reported only once for each function it appears in
make: *** [Makefile:201: obj-unix/release/deps/rcheevos/src/rcheevos/operand.o] Error 1

This feature is apparently not supported by lua51.

[orbea]

Is this the right repo?

[leiradel]

We intend to use Lua 5.3 because it has bit operators, which makes life a lot easier to write functions that test bits in memory.

Do you absolutely have to use an old Lua version?

[orbea]

The only way for me to support a system lua in RetroArch's configure is to check only lua.pc, -llua and lua.h, depending on the distro this could be lua51, lua52, or lua53. Trying to check for specific versions just falls apart due to inconsistency between distros...

The alternative would be to fix upstream lua to provide their own shared libraries and pkgconfig files so it could be consistent to test for specific versions, but I'm not holding my breath on that...

From lua53.

/* thread status */
#define LUA_OK          0
#define LUA_YIELD       1
#define LUA_ERRRUN      2
#define LUA_ERRSYNTAX   3
#define LUA_ERRMEM      4
#define LUA_ERRGCMM     5
#define LUA_ERRERR      6

And in lua51.

/* thread status; 0 is OK */
#define LUA_YIELD       1
#define LUA_ERRRUN      2
#define LUA_ERRSYNTAX   3
#define LUA_ERRMEM      4
#define LUA_ERRERR      5

So this diff seems to at least build, would it be acceptable?

diff --git a/deps/rcheevos/src/rcheevos/operand.c b/deps/rcheevos/src/rcheevos/operand.c
index 1e760daa16..a479abf416 100644
--- a/deps/rcheevos/src/rcheevos/operand.c
+++ b/deps/rcheevos/src/rcheevos/operand.c
@@ -345,7 +345,7 @@ unsigned rc_evaluate_operand(rc_operand_t* self, rc_peek_t peek, void* ud, lua_S
 
         lua_pushlightuserdata(L, &luapeek);
         
-        if (lua_pcall(L, 2, 1, 0) == LUA_OK) {
+        if (lua_pcall(L, 2, 1, 0) == 0) {
           if (lua_isboolean(L, -1)) {
             value = lua_toboolean(L, -1);
           }

[orbea]

@leiradel Any further comments?

[leiradel]

Only that I still don't understand the issue. rcheevos needs Lua 5.3, and the branch where it's integrated into RetroArch even includes Lua 5.3's source code. I don't see why we would want to integrate the system's version of Lua instead of that.

[orbea]

I see two reasons.

1. Its strongly preferred to use system libraries instead of bundled ones with distros and some users.
2. Its helpful for debugging lua bugs or regressions which affect RetroArch.

[leiradel]

Sorry, I still don't understand. RetroArch doesn't include rcheevos yet, so it can use whatever Lua it wants, either integrated as source code or from the system. rcheevos needs Lua 5.3, so why patch it to link against other versions?

[orbea]

I'm not sure who added it, but rcheevos is exposed by ./configure --enable-new_cheevos and it will use lua with --enable-lua.

https://github.com/libretro/RetroArch/tree/0aa9df6acd91212880150554651573d9571f1697/deps/rcheevos

https://github.com/libretro/RetroArch/blob/0aa9df6acd91212880150554651573d9571f1697/Makefile.common#L1678

Honestly I think using lua is causing more problems than its worth, but its not my call. :)

[leiradel]

I'm not sure who added it

I believe it was @meleu , in order to debug the issues that appeared with rcheevos on Android.

Honestly I think using lua is causing more problems than its worth, but its not my call. :)

I know nothing about this other functionality that needs Lua, but I have plenty of experience with it both as a programmer and embedding it into other applications and it's a breeze to work with, but that's also not my call either.

[orbea]

Just for reference one issue is that when built with --enable-lua (Using the builtin lua5.3) and --enable-mpv when libmpv is built against lua RetroArch will immediately crash in the system lua.

Another issue is the mess with upstream lua and shared libraries I was hoping to work around here.

[leiradel]

Hm, maybe mpv doesn't work with 5.3? Is this with latest? I can take a look later.

[orbea]

I'm not sure if it works with 5.3 or not, but its built against 5.1 by default here which works fine. It should occur with the mpv and RetroArch master.

Edit: I asked in the mpv irc channel and was told anything other than lua5.3 or newer...

[leiradel]

$ ./configure --enable-lua --enable-mpv
Checking operating system ... Linux (Ubuntu 18.04.1 LTS 18.04)
(snip)
Checking presence of package mpv ... no
Forced to build with package mpv, but cannot locate. Exiting ...

[orbea]

You need to build mpv with --enable-libmpv-shared and you should have a mpv.pc and libmpv.so files installed.

[leiradel]

I did an apt-get install, but I'll try building it myself. It's this one, right?

[orbea]

Yes, that one.

https://github.com/mpv-player/mpv

If you need any help building it let me know, they use waf as their build system.

[orbea]

Actually I forgot three details, first that --enable-mpv still has this build issue (Hence being disabled by default) since the audio callback was never finished upstream in libmpv.

LD retroarch
/usr/bin/ld: obj-unix/release/cores/libretro-mpv/mpv-libretro.o: in function `libretro_mpv_retro_run':
mpv-libretro.c:(.text+0x88f): undefined reference to `mpv_audio_callback'
collect2: error: ld returned 1 exit status
make: *** [Makefile:196: retroarch] Error 1

Here is a workaround, but there will be no audio when using the mpv core in RetroArch which is not important for testing this.

diff --git a/cores/libretro-mpv/mpv-libretro.c b/cores/libretro-mpv/mpv-libretro.c
index c84ebd2561..15fa6f5b24 100644
--- a/cores/libretro-mpv/mpv-libretro.c
+++ b/cores/libretro-mpv/mpv-libretro.c
@@ -399,6 +399,7 @@ void CORE_PREFIX(retro_reset)(void)
 {
 }
 
+#if 0
 static void audio_callback(double fps)
 {
        /* Obtain len samples to reduce lag. */
@@ -436,6 +437,7 @@ static void audio_callback(double fps)
        }
        while(len > 4);
 }
+#endif
 
 static void retropad_update_input(void)
 {
@@ -557,7 +559,7 @@ void CORE_PREFIX(retro_run)(void)
        retropad_update_input();
 
        /* TODO #2: Implement an audio callback feature in to libmpv */
-       audio_callback(container_fps);
+       /* audio_callback(container_fps); */
 
        process_mpv_events(MPV_EVENT_NONE);
 

Second you need to build RetroArch with --enable-new_cheevos and --enable-lua now to even build with lua.

And third my memory was flawed and the crashes only occur when trying to use the builtin mpv core by loading a video file.

[orbea]

For reference here is the unfinished PR for libmpv to fix the audio.

mpv-player/mpv#5566

[deltabeard]

The master branch of libretro-mpv as I left it seems to have the audio callback functions commented out, so your patch shouldn't be required and should compile fine without any changes to mpv (other than enabling shared libs), unless if you're using the audio-cb-new branch of the repo.

I'm having another crack at fixing this audio callback nonsense in libretro-mpv...