[patch] Lua binding: check that module import is correct

[istr]

As of swig 2.0.5 (current svn) that was used to build the lua binding lua/xapian_wrap.cc anyway, the option -nomoduleglobal is supported. It provides a way of creating modules that are compatible with the way lua 5.1 includes modules using require. With lua 5.1 being stable for over 6 years and 5.2 being the last stable version there is not much point for 5.0 backwards compatibility.

See swig tracker item 3394339 (​http://sourceforge.net/tracker/?func=detail&aid=3394339&group_id=1645&atid=301645) for the swig patch. Using this patch improves xapian lua binding compatibility for many current distributions (at least debian, ubuntu).

Please find attached a patch against xapian-bindings-1.2.9 to apply the flag (patches Makefile.in for maintainer mode and xapian_wrap.cc for out of the box compilation).

[istr]

xapian lua binding -nomoduleglobal swig option patch

[Olly Betts]

Needs to go on trunk first.

[Olly Betts]

Hmm, the patch seems to only change generated files - is the actual change required just adding -nomoduleglobal to the swig invocation in lua/Makefile.am?

And is there something I can add to smoketest.lua to ensure that this doesn't regress in the future? Can I check that the unwanted global hasn't been created somehow? Sorry, I'm not very familiar with Lua.

Incidentally, requiring lua 5.1 certainly isn't an issue - that's already the documented as the minimum version supported in the README.

[istr]

You are right, I should have patched Makefile.am not Makefile.in. I just realized that the patch is pointless with a current swig version (svn).

Instead I attached a patch against smoketest.lua that checks for the expected and wanted behaviour of the binding.

[istr]

check that require returns xapian module and sets global xapian

[Olly Betts]

I'm confused at this point. You were suggesting we should use -nomoduleglobal, but the new tests seem to check that the module is set as a global variable. Did you get that test backwards, or I am not following what we're aiming for here?

Also, are you OK with the patch licensing requirements:

​http://trac.xapian.org/browser/trunk/xapian-core/HACKING#L1223

[istr]

Yes, sorry for the confusion. I did not realize a side effect in the swig binding of the -nomoduleglobal option in swig initially.

In fact require in Lua 5.1 does both for the vast majority of modules available:

• return the module as a table
  
• register the module table within the current global environment
  

If you want to use xapian in your own lua module, you need require to return the module table to be able to create a reference within the module's global environment.

Cf.
​http://www.lua.org/manual/5.1/manual.html#2.9 (Environments)
​http://www.lua.org/manual/5.1/manual.html#pdf-require (Require)

The problem with the -nomoduleglobal option is, that some functionality of the swig binding expects to find the module in the current global environment 'xapian' (a bug that is not easy to fix: you need to enhance the userdata and improve the encapsulation of the C/Lua "objects"). So it is best to check that require'xapian' registers the module in the current global environment AND returns the module table.

Life would be much easier here if swig used the modern luaL_register / luaI_openlib method (lauxlib.c since Lua 5.1) and would not use its own registry method meant to be backwards compatible with Lua 5.0.

As for the licencse: the patch is public domain, you may use it at whatever license you want (GPLv2, GPLv3, MIT, whatever)

[Olly Betts]

Thanks for clarifying, that makes sense to me now. I'll sort out getting this applied.

I don't think there's an active maintainer for SWIG's Lua backend at the moment. But if you have any patches for SWIG, I can probably help get them applied. I think there's a good argument for not worrying about compatibility with Lua 5.0 now, especially if it helps support more recent versions better.

[Olly Betts]

OK, I've applied a pending patch to SWIG to add Lua 5.2 support:

​https://sourceforge.net/tracker/index.php?func=detail&aid=3514593&group_id=1645&atid=301645

Then I updated Xapian trunk to use the latest SWIG for bootstrapping, applied your patch, and made some minor tweaks to get it to work with Lua 5.2 (final change in this series is r16489).

It would be nice to fix this in Xapian 1.2.x (the Lua 5.2 support particularly), though I'm reluctant to just do a wholesale update of the SWIG version we use for bootstrapping there, as we've had issues with incompatible SWIG changes before. I'll mark for 1.2.10 for now, to remind us to look at this. We should at least document that 5.2 isn't supported in 1.2.x if we decide not to try to backport this.

[Olly Betts]

OK, fixed for the 1.2 branch by defining some compatibility macros in lua/util.i and running the generated wrapper through perl to fix the things we can't fix with macros. Committed that change in r16546.