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

Reviewers
mat
dbn
andrew_tao173.riddles.org.uk
Group Reviewers
portmgr
Summary

Hi,

This is a port for Lua 5.4.0(Beta)

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

There are a very large number of changes, so older changes are hidden. Show Older Changes

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 ↗(On Diff #45299)

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

Fix silly mistake in previous diff

Makefile
54 ↗(On Diff #45300)

+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 ↗(On Diff #45300)

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 ↗(On Diff #45300)

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 ↗(On Diff #45305)

Please remove all this.

10 ↗(On Diff #45305)

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 ↗(On Diff #45460)

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.

Now that a 5.4.0-beta release has been made, and there are signs that features are actually stabilizing (there have been many changes since work2), perhaps we could pick this up again. The distinfo needs an update of course.

Now that a 5.4.0-beta release has been made, and there are signs that features are actually stabilizing (there have been many changes since work2), perhaps we could pick this up again. The distinfo needs an update of course.

Yes, I'll start on that now. Thanks for the ping.

russ.haley_gmail.com edited the summary of this revision. (Show Details)

Updated for the latest Lua 5.4 Beta release.

  • When the ASSERT option is selected, the extra-patch-assert file fails to apply cleanly. I tried to port it over but I there has been a change to lgc.c and I don't know enough about lgc to apply the patch (Help Andrew?).
  • I tested all the other options and they all compiled.
  • I installed Lua54 beta in the default configuration without issue.

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=199480

Is this patch for arc4random something that we should look at applying?

hmmmm?

> Installing for lua54-5.4.0.b_1

> Checking if lua54 is already installed

> Registering installation for lua54-5.4.0.b_1

pkg-static: duplicate file listing: /usr/local/share/doc/lua54/lua/contents.html, ignoring
pkg-static: duplicate file listing: /usr/local/share/doc/lua54/lua/index.css, ignoring
pkg-static: duplicate file listing: /usr/local/share/doc/lua54/lua/logo.gif, ignoring
pkg-static: duplicate file listing: /usr/local/share/doc/lua54/lua/lua.css, ignoring
pkg-static: duplicate file listing: /usr/local/share/doc/lua54/lua/manual.css, ignoring
pkg-static: duplicate file listing: /usr/local/share/doc/lua54/lua/manual.html, ignoring
pkg-static: duplicate file listing: /usr/local/share/doc/lua54/lua/osi-certified-72x60.png, ignoring
pkg-static: duplicate file listing: /usr/local/share/doc/lua54/lua/readme.html, ignoring
Installing lua54-5.4.0.b_1...

  • When the ASSERT option is selected, the extra-patch-assert file fails to apply cleanly. I tried to port it over but I there has been a change to lgc.c and I don't know enough about lgc to apply the patch (Help Andrew?).

The assert patch no longer needs to touch lgc.c, since that was fixing a bug with a misplaced assert that is now fixed upstream. Only the change to lprefix.h is needed now.

You seem to have duplicated a patch? There should be no patch for lua.c other than the extra-patch-libedit-dl one.

lua54/pkg-plist
13 ↗(On Diff #63394)

These %%PORTDOCS%% lines should not be here, PORTDOCS in the makefile automatically handles adding them to the plist. I think this explains your duplication warnings.

russ.haley_gmail.com updated this revision to Diff 63427.EditedOct 18 2019, 4:18 AM

New patch to address comments, thank you Andrew.

Update: retested the default LIBEDIT_DL configuration and I don't get command line history

russellh@gbsd2 ~/f/p/l/l/w/l/src> ./lua54
Lua 5.4.0  Copyright (C) 1994-2019 Lua.org, PUC-Rio
> print('test')
test
> ^[[A^C⏎

but it works fine with libreadline:

russellh@gbsd2 ~/f/p/l/l/w/l/src> ./lua54
Lua 5.4.0  Copyright (C) 1994-2019 Lua.org, PUC-Rio
> print('test')
test
> print('test')
test

Build output for LIBEDIT_DL is here: https://pastebin.com/UTVrdZVX

Addressed andrews comments. Fixed missing patch for LIBEDIT_DL and tested LIBEDIT_DL option. twice. :-/

Added option for arc4random random seed. I tested running math.random and it looked randomish to me? I don't know what I'm looking for but the flags were present in the build output:

...
--- liolib.o ---
cc  -O2 -Wall -Wextra -DLUA_COMPAT_5_3 -DLUA_USE_POSIX -DLUA_USE_DLOPEN -DLUA_USE_READLINE_DL -isystem /usr/local/include -O2 -pipe  -fPIC -D__BSD_VISIBLE -DLUA_USER_H='<stdlib.h>' -D'lua_makeseed()=cast(unsigned int, arc4random())' -fstack-protector-strong -isystem /usr/local/include  -c liolib.c -o liolib.o
--- lmathlib.o ---
cc  -O2 -Wall -Wextra -DLUA_COMPAT_5_3 -DLUA_USE_POSIX -DLUA_USE_DLOPEN -DLUA_USE_READLINE_DL -isystem /usr/local/include -O2 -pipe  -fPIC -D__BSD_VISIBLE -DLUA_USER_H='<stdlib.h>' -D'lua_makeseed()=cast(unsigned int, arc4random())' -fstack-protector-strong -isystem /usr/local/include  -c lmathlib.c -o lmathlib.o
...

Looks like Roberto snuck some hardcoded -Os options into the makefile, we need to either patch those back out or think of something else to do with them.

Looks like Roberto snuck some hardcoded -Os options into the makefile, we need to either patch those back out or think of something else to do with them.

You should ask about that on the mailing list. -O2 is specified twice, as well as -Os. Which option does the compiler take?

cc -O2 -Wall -Wextra -DLUA_COMPAT_5_3 -DLUA_USE_POSIX -DLUA_USE_DLOPEN -DLUA_USE_READLINE_DL -isystem /usr/local/include -O2 -pipe -fPIC -D__BSD_VISIBLE -DLUA_USER_H='<stdlib.h>' -D'lua_makeseed()=cast(unsigned int, arc4random())' -fstack-protector-strong -isystem /usr/local/include -Os -c lcode.c

Complete build output here: https://pastebin.com/pB2N7D3w

russ.haley_gmail.com updated this revision to Diff 63528.EditedOct 22 2019, 4:13 AM

Patch out targets that added -Os to llex.o, lparser.o, and lcode.o.

Build output here https://pastebin.com/2tLGT9RA

Looks like Roberto snuck some hardcoded -Os options into the makefile, we need to either patch those back out or think of something else to do with them.

You should ask about that on the mailing list. -O2 is specified twice, as well as -Os. Which option does the compiler take?

Generally the last one.

cc -O2 -Wall -Wextra -DLUA_COMPAT_5_3 -DLUA_USE_POSIX -DLUA_USE_DLOPEN -DLUA_USE_READLINE_DL -isystem /usr/local/include -O2 -pipe -fPIC -D__BSD_VISIBLE -DLUA_USER_H='<stdlib.h>' -D'lua_makeseed()=cast(unsigned int, arc4random())' -fstack-protector-strong -isystem /usr/local/include -Os -c lcode.c

That first -O2 needs to be patched out as well - wasn't there something for that in files/patch-src_Makefile before?

Patch out CFLAGS optimization in third party makefile as per handbook:

https://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/dads-cflags.html

../lua54/Makefile
116 ↗(On Diff #63951)

Did the base FreeBSD bug that caused us to require this ever get fixed? I tried following the bug report but lost track of it.

Update to lua 5.4.0-rc1

Update to match recent changes to Lua framework; connect to the build; add the new version to the list of supported versions in default-versions and Uses/lua.mk.

Allowing 5.4 in Uses/lua.mk does have the potential to break builds of ports that specify, e.g., USES=lua:53+ (so that the default version of 5.2 is not in range) and where they use interfaces that change from 5.3 to 5.4. I'm not sure how many cases of this there are (if any); I will do my own test run soon, but an exp-run will likely be needed at some point.

Now that there's an actual release candidate, we can think about pushing this forward.

Test results: adding support for 5.4 causes only two ports (archivers/urbackup-server and x11-toolkits/termit) to switch from lua 5.3 to 5.4 when LUA_DEFAULT is 5.2. (They would not switch if the default were 5.3; both are declared as lua:53+, and the default version is chosen if it is in range, otherwise the latest available version in the range.)

Both of them fail to build as a result.

For archivers/urbackup-server, it's because it has a configure script that looks only for lua53 and older versions.

For x11-toolkits/termit it's because it uses cmake to build, and cmake has no idea that lua54 exists so it also has a hardcoded check for lua53 and older.

In both cases, adding appropriate CONFIGURE_ENV or CMAKE_ARGS *might* fix the issue; I'll have a poke at them.

Update to lua 5.4.0-rc1

Update to match recent changes to Lua framework; connect to the build; add the new version to the list of supported versions in default-versions and Uses/lua.mk.

Allowing 5.4 in Uses/lua.mk does have the potential to break builds of ports that specify, e.g., USES=lua:53+ (so that the default version of 5.2 is not in range) and where they use interfaces that change from 5.3 to 5.4. I'm not sure how many cases of this there are (if any); I will do my own test run soon, but an exp-run will likely be needed at some point.

Now that there's an actual release candidate, we can think about pushing this forward.

I'm not sure what I'm doing wrong but I get the lua54 port files in my ports root directory if I apply this patch.

russellh@gbsd2 ~/f/ports> patch <  ~/freebsd/patches/D14709.diff

Should work with patch -p1

Change the two ports that declared themselves lua:53+ to just lua:53, since they don't seem to respond sanely to attempts to force configuration with 5.4 using environment vars.

(In my documentation patch for the porter's manual, I've added a note about why this style of version specification should be avoided. See https://reviews.freebsd.org/D24430 for details.)

In my opinion, it's a bug in Mk/Uses/lua.mk that USES=lua:53+ chooses lua 5.4 over 5.3

In my opinion, it's a bug in Mk/Uses/lua.mk that USES=lua:53+ chooses lua 5.4 over 5.3

What alternative logic do you suggest?

In my opinion, it's a bug in Mk/Uses/lua.mk that USES=lua:53+ chooses lua 5.4 over 5.3

What alternative logic do you suggest?

Maybe chose the closest from default version that fits the constraints

In my opinion, it's a bug in Mk/Uses/lua.mk that USES=lua:53+ chooses lua 5.4 over 5.3

What alternative logic do you suggest?

Maybe chose the closest from default version that fits the constraints

In other words, choose the lowest version in the range if the default version is below the start of the range, or the highest version if it is above the end?

Should the rule be the same for 53+ as for, say, 53-54 ?

(I should note, I did not change the version selection logic much in my patch for lua.mk; the old lua.mk would choose the highest version in these kinds of cases too.)

I'm honestly not sure that there's a rule that works well for all kinds of ports. The only (unflavored) ports for which lua:53+ really make sense would be ones that are actually written in Lua, rather than embedding Lua into C code (or whatever other language), because the Lua API has breaking changes between every version (while the 5.3 to 5.4 changes are relatively small, there's no guarantee at all about future versions).

The major benefit I can see from your suggested rule is that it would ensure that advancing LUA_DEFAULT would not cause any port to suddenly switch to an older version of Lua, whereas with the current logic, a 53+ port will build with 5.4 once that is added, but revert to 5.3 if LUA_DEFAULT was changed from the current value of 5.2 to 5.3.

The downside would seem to be that ports that really can benefit from a newer Lua version would be stuck on an older one.

In the specific case of the two ports referenced here, I think it's clearly a bug in the ports that they specify 53+ when they cannot actually build with anything other than exactly 5.3.

Others have supported the "closest to default" rule, so I'll put up a separate patch for that.