brimworks / lua-zip

Lua binding to libzip.
80 stars 25 forks source link

Lua 5.3 support ? #8

Closed MisterDA closed 5 years ago

MisterDA commented 8 years ago

Hello again ! Lua 5.3 is the default with my Linux distribution, and there have been changes and depreciations between Lua 5.2 and Lua 5.3 . The lib will compile but trigger an error because luaL_checkint has been depreciated in favour of lua_checkinteger. Perhaps a little wrapper will do the trick :

diff --git a/lua_zip.c b/lua_zip.c
index 5d1d22e..5e2ac07 100644
--- a/lua_zip.c
+++ b/lua_zip.c
@@ -17,6 +17,10 @@
 #define luaL_register(L,_,funcs) luaL_setfuncs((L),funcs,0)
 #endif

+#if LUA_VERSION_NUM > 502
+#define luaL_checkint(L, arg) luaL_checkinteger((L), (arg))
+#endif
+
 #define ARCHIVE_MT      "zip{archive}"
 #define ARCHIVE_FILE_MT "zip{archive.file}"
 #define WEAK_MT         "zip{weak}"

But now I'm getting an error when I run the tests, and I can't figure it out.

$ ./test.lua tests 
ok 1 - Circular reference is error: Circular reference of zip sources is not allowed
lua: ./test.lua:417: No such file
stack traceback:
    [C]: in function 'assert'
    ./test.lua:417: in function 'test_open_close'
    ./test.lua:41: in function 'main'
    ./test.lua:442: in main chunk
    [C]: in ?
brimworks commented 8 years ago

Your wrapper looks good to me.

This is the line that is failing:

local ar = assert(zip.open("test_open_close.zip", zip.CREATE))

Maybe the zip.CREATE flag isn't getting through and it is trying to do an open() with the expectation of reading an existing archive? Specifically, I would double check that zip.CREATE is a number and that this line contains that flag:

https://github.com/brimworks/lua-zip/blob/master/lua_zip.c#L90

Thanks, -Brian

MisterDA commented 8 years ago

luasocket#124 gives more informations about Lua 5.3 compatibility.

#ifndef luaL_checkint
#define luaL_checkint(L, n) ((int)luaL_checkinteger(L,n))
#endif

or

#ifndef LUA_COMPAT_APIINTCASTS
#define LUA_COMPAT_APIINTCASTS
#endif
#include <lauxlib.h>

or

defines = { "LUA_COMPAT_APIINTCASTS" },  -- in the rockspec
markuman commented 8 years ago

My fork from @MisterDA (which is a fork from @brimworks ) seems to work with Lua 5.3.2 Only the mtime check is failing. Dunno if it's a serious problem or not. What do you think?

    $ lua test.lua brimworks/
    ok 1 - Circular reference is error: Circular reference of zip sources is not allowed
    ok 2 - No such file matches 'No such file'
    ok 3 - test_open_close was successful
    ok 4 - 2 == 2
    ok 5 - 2 == 2
    ok 6 - 2 == 2
    ok 7 - No such file matches 'No such file'
    ok 8 - 2 == 2
    ok 9 - 2 == 2
    ok 10 - [one
    two
    three
    ] == [one
    two
    three
    ]
    ok 11 - [one
    two
    three
    ] == [one
    two
    three
    ]
    # Expected [mtime] to be '1296450278', but got '1296417878.0'
    not ok 12 - TEST.TXT stat
    # Expected [mtime] to be '1296450278', but got '1296417878.0'
    not ok 13 - index 2 stat
    ok 14 - test/text.txt == 'test/text.txt'
    ok 15 - No comment is set.  TODO: test w/ real comment
    ok 16 - zip.FL_UNCHANGED works
    ok 17 - test fun == 'test fun'
    ok 18 - No comment set. TODO: test w/ real comment
    ok 19 - Invalid argument == 'Invalid argument'
    ok 20 - add_dir returns 1
    ok 21 - Archive contains one entry: 1
    ok 22 - dir exists
    ok 23 - name_locate returns nil if FL_NODIR flag is passed
    ok 24 - Archive contains one entry: 1
    ok 25 - Contents == 'Contents'
    ok 26 - Archive contains one entry: 1
    ok 27 - Replacement == 'Replacement'
    ok 28 - Rename non-existant file error=No such file 'DNE'
    ok 29 - Archive contains one entry: 1
    ok 30 - Contents == 'Contents'
    ok 31 - Delete non-existant file error=No such file 'DNE'
    ok 32 - Archive contains one entry: 1
    ok 33 - Archive contains one entry: 1
    ok 34 - two == 'two'
    ok 35 - Archive contains one entry: 1
    ok 36 - /usr/bin/env == '/usr/bin/env'
brimworks commented 8 years ago

The mtime test is broken if ran in a timezone other than PST8PDT. I didn't realize when I wrote that test that the zip file mtimes are encoded as a date rather than an offset from the epoch.

MisterDA commented 8 years ago

The issue with mtime was reported here: #6

For Lua 5.1 compatibility, there is a set of macros in luaconf.h. You can use LUA_COMPAT_5_1 to be sure that your lib will always be compatible with future versions of lua, and remove your workarounds with your macros and aliases :

diff --git a/lua_zip.c b/lua_zip.c
index 5d1d22e..1b41a6d 100644
--- a/lua_zip.c
+++ b/lua_zip.c
@@ -1,3 +1,5 @@
+#define LUA_COMPAT_5_1
+
 #include <lauxlib.h>
 #include <lua.h>
 #include <zip.h>
@@ -5,18 +7,6 @@
 #include <errno.h>
 #include <string.h>

-#if LUA_VERSION_NUM > 501
-#define lua_objlen lua_rawlen
-#endif
-
-#if LUA_VERSION_NUM > 501
-#define lua_equal(L, idx1, idx2) lua_compare((L), (idx1), (idx2), LUA_OPEQ)
-#endif
-
-#if LUA_VERSION_NUM > 501
-#define luaL_register(L,_,funcs) luaL_setfuncs((L),funcs,0)
-#endif
-
 #define ARCHIVE_MT      "zip{archive}"
 #define ARCHIVE_FILE_MT "zip{archive.file}"
 #define WEAK_MT         "zip{weak}"

I've successfully build lua-zip and tested it with Lua 5.3.2 using this define. You can also check my branch lua51-compat. It only has this commit.

MisterDA commented 8 years ago

And by the way, lauxlib.h already includes lua.h so you could drop your #include <lua.h> statement without harm.

diff --git a/lua_zip.c b/lua_zip.c
index 1b41a6d..6720f8d 100644
--- a/lua_zip.c
+++ b/lua_zip.c
@@ -1,7 +1,6 @@
 #define LUA_COMPAT_5_1

 #include <lauxlib.h>
-#include <lua.h>
 #include <zip.h>
 #include <assert.h>
 #include <errno.h>
brimworks commented 8 years ago

Unfortunately, Lua 5.2 doesn't appear to have that macro:

http://www.lua.org/source/5.2/luaconf.h.html

...but this looks like a great strategy for supporting Lua 5.3.

MisterDA commented 8 years ago

I think you still can get remove your of your compatibility macros and the #include <lua.h>. If you add LUA_COMPAT_5_1 it will be compatible with Lua 5.3, if you add LUA_COMPAT_ALL it will be compatible with Lua 5.2. Both macros won't interfere. luaconf.h: 5.3, 5.2, 5.1 (search for LUA_COMPAT)

diff --git a/lua_zip.c b/lua_zip.c
index 5d1d22e..ecc5d4c 100644
--- a/lua_zip.c
+++ b/lua_zip.c
@@ -1,22 +1,12 @@
+// Lua 5.3 -> Lua 5.1 compatibility
+#define LUA_COMPAT_5_1
+// Lua 5.2 -> Lua 5.1 compatibility
+#define LUA_COMPAT_ALL
+
 #include <lauxlib.h>
-#include <lua.h>
 #include <zip.h>
 #include <assert.h>
 #include <errno.h>
 #include <string.h>

-#if LUA_VERSION_NUM > 501
-#define lua_objlen lua_rawlen
-#endif
-
-#if LUA_VERSION_NUM > 501
-#define lua_equal(L, idx1, idx2) lua_compare((L), (idx1), (idx2), LUA_OPEQ)
-#endif
-
-#if LUA_VERSION_NUM > 501
-#define luaL_register(L,_,funcs) luaL_setfuncs((L),funcs,0)
-#endif
-
 #define ARCHIVE_MT      "zip{archive}"
 #define ARCHIVE_FILE_MT "zip{archive.file}"
 #define WEAK_MT         "zip{weak}"
MisterDA commented 8 years ago

Oh and it seems a good idea to use @markuman's 98474c4 too.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index b1de5d4..4334cf3 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -23,7 +23,7 @@ CMAKE_MINIMUM_REQUIRED (VERSION 2.6)
 # / Find libzip

 # Find lua
-  FIND_PACKAGE(Lua51 REQUIRED)
+  FIND_PACKAGE(Lua REQUIRED)
 # / Find lua

 # Define how to build zip.so:
Neustradamus commented 5 years ago

Any news?

MisterDA commented 5 years ago

Take a look at https://github.com/MisterDA/lua-zip. I have merged features from forks, and somehow improved compatibility, I hope.

brimworks commented 5 years ago

@MisterDA is it okay if I pull your commits? ...or if you file a Pull Request, that would be great. I just don't want to "steal" your work without consent.

Thanks, -Brian

MisterDA commented 5 years ago

@brimworks Hey, it’s free software, after all I’d by very happy to create a PR. There are also commits code from other forks.

brimworks commented 5 years ago

Much thanks to @MisterDA for the contributions! This has now been published as version 0.1-0 on luarocks: https://luarocks.org/modules/brimworks/lua-zip/0.1-0