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

Modernise lua-archive #2

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@hadess
Copy link

commented Jun 20, 2016

Lua 5.2 and 5.3 support, as well as porting to libarchive 3.0.

Untested.

@hadess hadess force-pushed the hadess:master branch from b18a41d to ac059c1 Jun 21, 2016

ar.c Outdated
@@ -38,8 +38,12 @@ static int ar_version(lua_State *L) {
}

//////////////////////////////////////////////////////////////////////
#if !defined LUA_VERSION_NUM

This comment has been minimized.

Copy link
@brimworks

brimworks Jun 21, 2016

Owner

Can this #if be refactored into a header (perhaps ar.h?) so we can avoid copying it into ar_entry.c?

This comment has been minimized.

Copy link
@hadess

hadess Jun 21, 2016

Author

Sure.

@@ -9,6 +9,8 @@
PROJECT(lua-archive C)
CMAKE_MINIMUM_REQUIRED (VERSION 2.6)

set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -O0 -g3 -ggdb")

This comment has been minimized.

Copy link
@brimworks

brimworks Jun 21, 2016

Owner

Perhaps this should be reverted since production installs will probably not want this setting? ...or at least comment it out?

This comment has been minimized.

Copy link
@hadess

hadess Jun 21, 2016

Author

Sure, will see what the minimum needed is to have the CFLAGS actually applied.

ar_read.c Outdated
@@ -113,12 +113,16 @@ static int ar_read(lua_State *L) {
lua_getfield(L, 1, "reader"); // {ud}, {fenv}, fn
if ( ! lua_isfunction(L, -1) ) err("MissingArgument: required parameter 'reader' must be a function");
lua_setfield(L, -2, "reader"); // {ud}, {fenv}
lua_setfenv(L, -2); // {ud}
#if LUA_VERSION_NUM > 501

This comment has been minimized.

Copy link
@brimworks

brimworks Jun 21, 2016

Owner

Can this be refactored into ar.h (the new header I mentioned) so it looks something like this instead?

#if LUA_VERSION_NUM <= 501
#    define lua_setuservalue lua_setfenv
#endif

This comment has been minimized.

Copy link
@hadess

hadess Jun 21, 2016

Author

Will do. Note that I'm still working on this as the test suite crashes. There were some other changes in the Lua sandboxing that might be causing this. Still investigating.

hadess added some commits Jun 21, 2016

Add support for Lua 5.1
From http://lua-users.org/wiki/CompatibilityWithLuaFive:
luaL_reg (Lua 5.0) changed its name to luaL_Reg since Lua 5.1.

Apply the recommended fix to support lua 5.1 and prior versions.
Add support for Lua 5.2
Use lua_getuservalue()/lua_setuservalue() instead of the removed
lua_getfenv()/lua_setfenv().
Port to libarchive 3.0
Replace the deprecated functions with their libarchive 3.0 replacements
as listed at:
https://github.com/libarchive/libarchive/wiki/ManPageLibarchiveChanges3
Fix use of unset variable
"wrote" was checked against -1 but was never actually set.
Fix invalid FFlag in tests
When running the tests, libarchive says:
InvalidFFlag: 'archive' is not a known fflag

So remove that one flag from our tests.

@hadess hadess force-pushed the hadess:master branch from ac059c1 to bc0929b Jun 21, 2016

@hadess

This comment has been minimized.

Copy link
Author

commented Jun 21, 2016

Note that I had to disable the "__gc" functions for the code to work and the tests to pass on Lua 5.3.

I don't think that the existing code should regress on 5.1 or older, but I'm still investigating the problems with newer Lua versions.

@tomprince

This comment has been minimized.

Copy link

commented Jul 5, 2016

It at least compiles and loads for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.