Use lua-compat-5.3 project instead of carrying own compatibility code

[daurnimator]

https://github.com/keplerproject/lua-compat-5.3

[ThePhD]

I started with lua-compat-5.2 (a few years ago), but it's out-of-the-box build was not suitable for a single header library. It's license is still listed in the documentation and I still credit it, but.

It had a myriad of errors that I had to manually fix for a few different platforms. I would like to update to later versions but each time I'd need to pull the code, scrub the static from the implementation file, add extern "C" around it, and then apply my fixes to it for Android, iOS, and specific Linux builds using glibc, as well as MinGW.

I'm not quite ready to make that effort just yet, but I'll keep it on my TODO list.

[daurnimator]

I started with lua-compat-5.2 (a few years ago), but it's out-of-the-box build was not suitable for a single header library.

lua-compat-5.3 supports a single header mode: infact that's the default. See https://github.com/keplerproject/lua-compat-5.3#c-code

It had a myriad of errors that I had to manually fix for a few different platforms. I would like to update to later versions but each time I'd need to pull the code, scrub the static from the implementation file, add extern "C" around it, and then apply my fixes to it for Android, iOS, and specific Linux builds using glibc, as well as MinGW.

Much of that has been fixed in lua-compat-5.3. e.g. use COMPAT53_API to pick how to export things.

[ThePhD]

My primary problem wasn't export issues: that was just at the start. I'm going to list what I remember running into here and then checking them off 1 by 1 as I find the functions here or figure out proper work arounds to augment the header.

• Some versions of glibc (and MinGW by default on many versions) does not have strerror_s or strerror_r: they are defined as "auxiliary" functions in the C standard. (Fixed: this compat version attempts to use neither and simple uses strerror(s), but this will generate a noisy compiler warning on VC++ I will need to quiet)
• I need luaL_loadbufferx and others that have the mode parameter, even if the compatibility API just ignores the mode parameter altogether. I can probably add those as a post-inclusion patch to the file.
• I do not know how tested this is against luajit-2.1.x-beta{whatever}. A lot of my users build and test and deploy against the beta: does this version carefully step around the fact that the newer version of LuaJIT makes an effort to a handful of 5.2-defined functions?

So far it's not a very long list because most of my qualms are buried in the commit history somewhere: I'm sure if I ran my tests with LuaJIT 2.0.x and LuaJIT 2.1.x-{whatever} I'd remember the rest, but I'm going to need a little time.

[daurnimator]

this will generate a noisy compiler warning on VC++ I will need to quiet

File a bug against lua-compat-5.3?
They might bee able to add a pragma for you

I need luaL_loadbufferx and others that have the mode parameter, even if the compatibility API just ignores the mode parameter altogether.

But luaL_loadbufferx is just luaL_loadbuffer with a mode that matters...
Again though: if you add a feature request to lua-compat-5.3 they should be able to add it for you.

I can probably add those as a post-inclusion patch to the file.

I'd love it if you could not have any patches, but use exact-as-upstream code.

I do not know how tested this is against luajit-2.1.x-beta{whatever}. A lot of my users build and test and deploy against the beta: does this version carefully step around the fact that the newer version of LuaJIT makes an effort to a handful of 5.2-defined functions?

It does :)

[ThePhD]

In the meantime, if you're using sol2, you can define SOL_NO_COMPAT, as described here: http://sol2.readthedocs.io/en/latest/api/compatibility.html

This will allow you to sidestep the current compatibility headers and use anything you yourself include. We do not define anything related to Lua's configuration before including sol2, so you can turn off the compat header and include things yourself if you're so inclined.

Finally, for my API's versions of load (for loading buffers and strings and files) I do pass through whatever loading mode sol::load_mode the user passes through: if someone's on Lua 5.1 they likely won't ever care (hencewhy I don't mind if the parameter just ends up ignored) but on 5.2 and 5.3 they will. So, just having it ignore the mode is why I implemented that patch myself.

It seems like they're aware of luaL_loadbufferx/luaL_loadfilex not being implemented in their wrapper: I would assume they don't implement it because they don't want to "lie" to users on 5.1 that they can have an exclusively binary-processing function (I believe Lua 5.1 will just handle both text its binary form at all times... but I have no idea if Lua 5.1 can actually produce a purely binary blob in the first place??). I'd have to ask them to be sure, really...

[daurnimator]

You can check if the first byte is \27 to know if it's binary or not. => that feature could be added.

[ThePhD]

If that's the case then I could probably write the function myself and submit it as a patch, then. Thanks for letting me know!

[daurnimator]

Note: by \27 I mean \x1b

It's what lua does internally via LUA_SIGNATURE[0]: http://www.lua.org/source/5.1/ldo.c.html#f_parser http://www.lua.org/source/5.3/ldo.c.html#f_parser

[ThePhD]

Some initial impressions.

• If I define COMPAT53_INCLUDE_SOURCE, it should not be defining it again for me (results in warnings). Basic macro hygiene is to check if something is defined and avoid defining it if it already is (otherwise, undefine it and redefine it).
• If I specify my own COMPAT53_PREFIX, it forcibly undefines COMPAT53_INCLUDE_SOURCE (why?? what does the prefix I select have to do with whether or not the source it includes?). It also automatically externs everything (why can't I just change the name and include the source that way?)
• The header does not define LUAERRGCM
• The compatibility casts a number of integers to doubles (lua_Integer to lua_Number, to use the precise typedefs) implicitly (i.e., passing to functions): this results in conversion warnings about possible loss of data.
• This error comes back, and I cannot define the required macro to get rid of it (because it affects some parts of VC++'s stdlib and other people also may or may not define it in their toplevel code): error C4996: 'strerror': This function or variable may be unsafe. Consider using strerror_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS.

The compilation doesn't get further than that, so I can't start testing for luaL_loadbufferx and friends just yet.

[ThePhD]

As promised, I made a pull request fixing all errors: lunarmodules/lua-compat-5.3#31

If this is accepted, I will transition. Otherwise, I will stay as-is.

[ThePhD]

Some more errors I found while building on other platforms.

• lua_callk((L), (na), (nr), (ctx), cont ## _52) -- this shows up when building on g++ -- it builds fine on VC++, I have no idea what the _52 suffix is for on the macro parameter for there. It's turning my passing of nullptr as an argument to lua_pcallk(lua_state(), static_cast<int>(argcount), static_cast<int>(resultcount), h.stackindex, 0, nullptr) into that, which is oddly bizarre to me.

[daurnimator]

I have no idea what the _52 suffix is for on the macro parameter for there

See https://github.com/keplerproject/lua-compat-5.3/wiki/yieldable_c_functions

[ThePhD]

Final tests should be passing soon enough using the latest of the project plus the fixes I put in the repo: as long as it continues to work as advertised everything should be fine: https://travis-ci.org/ThePhD/sol2/builds/273887242

[ThePhD]

Even if the PR doesn't get accepted, we have the files internally in sol2. I'll track things and keep it as up to date as I can, though I can't make any promises since we don't submodule the code directly and I would be required to manually check the compat headers and pull them in every now and then.

[daurnimator]

Even if the PR doesn't get accepted, we have the files internally in sol2. I'll track things and keep it as up to date as I can, though I can't make any promises since we don't submodule the code directly and I would be required to manually check the compat headers and pull them in every now and then.

FWIW I usually use git subtree to pull compat-5.3 in.
First time:

git subtree add --prefix=vendor/compat-5.3 --squash https://github.com/keplerproject/lua-compat-5.3 v0.5

Then when a new release comes out that I want to upgrade to:

git subtree pull --prefix=vendor/compat-5.3 --squash https://github.com/keplerproject/lua-compat-5.3 v0.6

[ThePhD]

Is there any way I can pull in just the c-api folder with subtree?

[daurnimator]

Is there any way I can pull in just the c-api folder with subtree?

Sort of, by first doing a git subtree split, but it's generally recommended against.

PS, If you use the top level subdirectory vendor, then github doesn't count commits in there towards your repository stats.