Page MenuHomeFreeBSD

lang/lua54 - new port for Lua 5.4.0
Needs ReviewPublic

Authored by russ.haley_gmail.com on Mar 16 2018, 6:28 AM.

Details

Summary

Hi,

This is a port for Lua 5.4.0 work2.

The Lua Manual can be found here:
http://www.lua.org/work/doc/manual.html

Andrew Gierth has added editline and documentation options to the port.

Test Plan

The current expectation on the mailing list is 5.4 should be generally backwards compatible with 5.3. Incompatibilities with 5,3 are listed in the manual:
http://www.lua.org/work/doc/manual.html#8

Build testing:
This was created and tested on Ghost BSD (11.1-p4). I have a clean VM for 11.1 and a TrueOS VM (Some version of CURRENT) that I will test this on. I'll pull out an arm board if I can.

Lua Testing:

  • I ran some heavy garbage collector tests on work1 that chewed up 8gb of memory without issue
  • I would like to request @dbn for a expeditionary run to look at bumping all ports possible to 5.4.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

The so isn't working because there is nothing here to build it; the makefile distributed with Lua does not try to build one and never did, the ports for earlier Lua versions patch the makefile to add it.

Turning on LUA_NILINTABLE is simply not appropriate; at most you could make it a selectable option, off by default.

lua54/Makefile
38 ↗(On Diff #40339)

this should be MYLIBS, not MYLDFLAGS if you're going to do it this way

The so isn't working because there is nothing here to build it; the makefile distributed with Lua does not try to build one and never did, the ports for earlier Lua versions patch the makefile to add it.

Thank you, I didn't remember that I had stripped that out to revise it. I get a few hours here and there for these things.

Turning on LUA_NILINTABLE is simply not appropriate; at most you could make it a selectable option, off by default.

Quite right. I'll make sure it's off for the final port and perhaps figure out how to make it an option.

  • Updated to build shared object.
  • Removed LUA_NILINTABLE. Will turn it into an option. Any other useful options?
  • Fixed to use MYLIBS not MYLDFLAGS for -ledit
russ.haley_gmail.com marked an inline comment as done.Mar 17 2018, 4:10 AM
russ.haley_gmail.com edited the summary of this revision. (Show Details)Mar 17 2018, 4:23 AM
russ.haley_gmail.com edited the test plan for this revision. (Show Details)
russ.haley_gmail.com edited the summary of this revision. (Show Details)
lua54/files/patch-src_Makefile
33 ↗(On Diff #40372)

If you're going to remove -lreadline here, it seems more logical to also remove -DLUA_USE_READLINE and pass that in via MYCFLAGS. This has the advantage that you can make it an option for the port (on by default).

lua54/Makefile
37 ↗(On Diff #40372)

I've been testing this:

OPTIONS_SINGLE= EDIT
OPTIONS_SINGLE_EDIT=EDITNONE LIBEDIT READLINE

OPTIONS_DEFINE= NILINTABLE 
OPTIONS_DEFAULT+= LIBEDIT

EDITNONE_DESC=  No interactive command-line editing
LIBEDIT_DESC=   Use base system libedit
READLINE_DESC=  Use GNU Readline from ports

NILINTABLE_DESC=Experimental (incompatible) nil-in-table feature

LIBEDIT_CPPFLAGS=-DLUA_USE_READLINE -I/usr/include/edit
LIBEDIT_LIBS=-ledit

READLINE_USES=readline
READLINE_CPPFLAGS=-DLUA_USE_READLINE
READLINE_LIBS=-lreadline

NILINTABLE_CPPFLAGS=-DLUA_NILINTABLE

CFLAGS+= -fPIC

WITHOUT_NO_STRICT_ALIASING=yes

MAKE_ARGS+=     CC="${CC}" \
                MYCFLAGS="${CPPFLAGS} ${CFLAGS}" \
                MYLDFLAGS="${LDFLAGS}" \
                MYLIBS="${LIBS}" \

Specific things to note:

  1. Passing CPPFLAGS/CFLAGS/LDFLAGS down via the MY* variables preserves the system default compile options and also any user-supplied ones, whereas your version clobbers those. The following points depend on this, as does the use of OPT_CPPFLAGS and so on.
  1. WITHOUT_NO_STRICT_ALIASING removes the system default -fno-strict-aliasing which is applied at -O2 and up.
  1. USES=readline handles adding -L/usr/local/lib and -I/usr/local/include automatically.
lua54/files/patch-src_Makefile
23 ↗(On Diff #40372)

LDFLAGS already had MYLDFLAGS included in it; don't add it again.

russ.haley_gmail.com updated this revision to Diff 40410.EditedMar 18 2018, 4:31 PM

Updated with improvements from @andrew_tao173.riddles.org.uk :

  • Removed hard coded options
  • Added options for editline/readline library source
  • Added option for experimental NILINTABLE
  • Andrew commented that command names should come from lua.mk. I added a timestamp for tracking. (Updating lua.mk is on my todo list. See D13691)
lua54/files/patch-src_Makefile
33 ↗(On Diff #40410)

-DLUA_USE_READLINE definitely needs to be removed from here now, the EDITNONE option of the port breaks if it's left in. (Sorry, forgot to mention that when I sent the makefile changes.)

russ.haley_gmail.com marked an inline comment as done.Mar 18 2018, 7:13 PM

Corrected src/Makefile patch to remove -DLUA_USE_READLINE as per AG. Compiles with and without interactive mode.

New diff with my suggested version for the work2 release.

NILINTABLE is gone, and I've added a DOCS option and reworked a few minor things.

Makefile
108

oops, typo, should be soname=${PORTNAME}-${LUA_VER}, will fix

Fix silly mistake in previous diff

Makefile
54

+1

russ.haley_gmail.com edited the summary of this revision. (Show Details)Jul 14 2018, 8:06 PM
russ.haley_gmail.com edited the test plan for this revision. (Show Details)Jul 14 2018, 8:10 PM
russ.haley_gmail.com added reviewers: mat, dbn.
russ.haley_gmail.com added a project: Restricted Project.
russ.haley_gmail.com edited the test plan for this revision. (Show Details)
russ.haley_gmail.com added a subscriber: dbn.
Makefile
61

Would it be wrong of me to add a lua-docs shell script to /usr/local/bin that looks for firefox or chromium and opens readme.html?

Makefile
61

It ... doesn't seem to be common style as far as I can see. I'm not keen on the idea.

It ... doesn't seem to be common style as far as I can see. I'm not keen on the idea.

Well it was fun to write anyways. :)

https://pastebin.com/9fRuejBp

Note that work2 is not ready for prime time yet (garbage collector bugs, at least)

russ.haley_gmail.com added a comment.EditedJul 14 2018, 9:38 PM

Note that work2 is not ready for prime time yet (garbage collector bugs, at least)

I didn't see anything on the mailing list that would preclude adding it to the ports collection, or at the very least performing an expeditionary run (if that's possible). @mat, @dbn, I'm keen to get 5.3.5 or preferably 5.4 "build tested" against other packages because I've got a week off work right now with no kids.

Note that work2 is not ready for prime time yet (garbage collector bugs, at least)

I didn't see anything on the mailing list that would preclude adding it to the ports collection, or at the very least performing an expeditionary run (if that's possible). @mat, @dbn, I'm keen to get 5.3.5 or preferably 5.4 "build tested" against other packages because I've got a week off work right now with no kids.

The "uservalue has an incorrect value" thread is the GC bug I know about. Won't affect pure-lua code, but anything that uses uservalues can explode (even just doing require "re" collectgarbage() triggers an invalid access, though it won't crash immediately unless you built with assertions on).

Note that work2 is not ready for prime time yet (garbage collector bugs, at least)

I didn't see anything on the mailing list that would preclude adding it to the ports collection, or at the very least performing an expeditionary run (if that's possible). @mat, @dbn, I'm keen to get 5.3.5 or preferably 5.4 "build tested" against other packages because I've got a week off work right now with no kids.

The "uservalue has an incorrect value" thread is the GC bug I know about. Won't affect pure-lua code, but anything that uses uservalues can explode (even just doing require "re" collectgarbage() triggers an invalid access, though it won't crash immediately unless you built with assertions on).

I may be mistaken, but the experimental (not expeditionary!) run only build packages so I'm hoping we can get the build output and see what needs upgrading. I see now that I can request an experimantal run on Bugzilla so I'll submit one today.

I may be mistaken, but the experimental (not expeditionary!) run only build packages so I'm hoping we can get the build output and see what needs upgrading. I see now that I can request an experimantal run on Bugzilla so I'll submit one today.

I know of some outstanding (possibly unreported) issues with ports: devel/lua-lpeg sets LUA_32BITS which makes it useless against the default lua build, and also fails to install re.lua

Is it worth adding options for enabling assertions and apicheck in the port? I use apicheck enough that it would be helpful, but my usage is possibly atypical.

Added ASSERT and APICHECK options.

Conform port Makefile to conventions specified in ports manual and run portlint (1 warning remains, for EDITNONE not needing any implementation)

Remove -O2 from src/Makefile in accordance with porting guidelines; -O should be provided system-wide.

Conform src/Makefile's use of $(AR) to respect $(ARFLAGS)

Ensure liblua.so is linked to libm without risking linking it to undesired additional libs.

mat added a comment.Jul 16 2018, 7:37 AM

Could you start this by doing a svn cp lua53 lua54 and then only uploading the diff from the actual changes from 5.3 ?

Makefile
3–5

Please remove all this.

10

Remove.

This is now a diff against the result of 'svn cp lua53 lua54' as requested by mat.

Also includes mat's other requested changes.

mat added a comment.Jul 16 2018, 2:07 PM

Could you use devel/arcanist, or at least generate a diff with full context like it does, with svn diff -x -U9999 or git diff -U9999.

Remove use of ${PORTNAME} in line with discussions over on the 5.3.5 patch.

andrew_tao173.riddles.org.uk retitled this revision from Lua 5.4.0 to lang/lua54 - new port for Lua 5.4.0.Jul 18 2018, 2:21 PM
andrew_tao173.riddles.org.uk set the repository for this revision to rP FreeBSD ports repository.

Change OPTIONS_DEFAULT+= to just = in accordance with nitpicking on the 5.3.5 patch.

mat added inline comments.Jul 18 2018, 3:25 PM
Makefile
4–5

This should probably be:

DISTVERSION= 5.4.0-work2

Use DISTVERSION. This corrects the package version too (now 5.4.0.w2)

Sync with 5.3.5 work.

This adds the dynamic-libedit patch (which is an extra patch here, rather than unconditionally patching lua.c as in 5.3.5, because unlike 5.3.5 we don't otherwise have any need to patch that file). Other changes are just cosmetic.

Changes in line with 5.3.5 patch:

  • move DOCSDIR to lua54/lua to free up lua54/${PORTNAME} for module ports
  • change doc install to avoid complaints from check-plist

So are we done here? I have nothing more to add.

So are we done here? I have nothing more to add.

I'm happy with it.

dbn added a subscriber: jbeich.Jul 28 2018, 5:14 AM

I cannot comment on the specifics of lua (i.e. do these changes work), I've never used lua and haven't tested it. However, I am happy with the above changes with the following comments:

  1. I assume the indentation is correct, despite it not rendering correctly in Chrome
  2. Please try to upstream all patches

@jbeich: your thoughts?

And, who will commit this change?

In D14709#349808, @dbn wrote:

I cannot comment on the specifics of lua (i.e. do these changes work), I've never used lua and haven't tested it. However, I am happy with the above changes with the following comments:

  1. I assume the indentation is correct, despite it not rendering correctly in Chrome
  2. Please try to upstream all patches

    @jbeich: your thoughts?

    And, who will commit this change?

Hi @dbn, Lua 5.4 is still in a beta phase so my preference is to hold this port until the Lua 5.4 official release, then update and push it to the repository. @andrew_tao173.riddles.org.uk has already indicated his preference to hold the port until the Lua release (unless something has changed?)

Hi @dbn, Lua 5.4 is still in a beta phase so my preference is to hold this port until the Lua 5.4 official release, then update and push it to the repository. @andrew_tao173.riddles.org.uk has already indicated his preference to hold the port until the Lua release (unless something has changed?)

I didn't say to hold it until release, I said -work2 was "not ready for prime time" or some such. But in the absence of any statement from the Lua devs about the likely timing of a -work3 or a final 5.4.0, I see these options:

  1. make the port available anyway as is (it doesn't seem unusual to have ports for non-final releases of languages, see e.g. tcl87)
  2. add a patch to stop it enabling the generational GC by default in lua.c (generational GC has known crash/corruption bugs in -work2) and make it available
  3. combine options 1 and 2 by making the patch an option
  4. sit on it until there's a (hopefully more stable) -work3 or final release, but we have no idea of the timeline for this

I'm not all that keen on option 1, but any of options 1-3 might be useful from a Lua development point of view.

Another factor to consider is updating Uses/lua.mk (which doesn't recognize 5.4 yet). I have been testing the idea of giving USES=lua a flavors option for lua modules to use and it seems to work well - I could put that work up as a separate revision for review?

Hi @dbn, Lua 5.4 is still in a beta phase so my preference is to hold this port until the Lua 5.4 official release, then update and push it to the repository. @andrew_tao173.riddles.org.uk has already indicated his preference to hold the port until the Lua release (unless something has changed?)

I didn't say to hold it until release, I said -work2 was "not ready for prime time" or some such. But in the absence of any statement from the Lua devs about the likely timing of a -work3 or a final 5.4.0, I see these options:

  1. make the port available anyway as is (it doesn't seem unusual to have ports for non-final releases of languages, see e.g. tcl87)
  2. add a patch to stop it enabling the generational GC by default in lua.c (generational GC has known crash/corruption bugs in -work2) and make it available
  3. combine options 1 and 2 by making the patch an option
  4. sit on it until there's a (hopefully more stable) -work3 or final release, but we have no idea of the timeline for this

    I'm not all that keen on option 1, but any of options 1-3 might be useful from a Lua development point of view.

    Another factor to consider is updating Uses/lua.mk (which doesn't recognize 5.4 yet). I have been testing the idea of giving USES=lua a flavors option for lua modules to use and it seems to work well - I could put that work up as a separate revision for review?

I had started a review for updating Uses/lua.mk last year (https://reviews.freebsd.org/D13691 - feel free to re-use this or start a new one). This review is when I was informed about the previous exp-run and why Lua was still at 5.2. My plan had been to follow the Lua releases and try to get everything updated to 5.3, but I have nothing against your suggestions. Option 2 would be my preference; it seems like the best "bang for the buck".

I'm keen to see your flavors work!

Track changes requested by mat for the 5.3 port.